Latch has unsafe behaviour: Ports D and ENA on the latch are fed by the same signal?

[digi001]

I cannot figure out why Altera Quartus is giving me warning for this state machine design?

module command_processor2(open_command,close_command,clk,immed_open,close,fault,reset_n,solenoid,sgto,led);

input open_command;
input close_command;

input clk;
input fault;
input reset_n;

output close;
output immed_open;
output solenoid;
output sgto;
output led;

reg [23:0]count;
reg [7:0]count2;
reg [3:0]state_reg,state_next;
reg close;
reg immed_open;
reg solenoid;
reg sgto;
reg led;
reg fault_flag=1'b0;

localparam s0=4'b0000,s1=4'b0001,s2=4'b0010,s3=4'b0011,s4=4'b0100,s5=4'b0101,s6=4'b0110,s7=4'b0111,s8=4'b1000,s9=4'b1001;

initial
begin
state_reg = s9;
count = 1500000;
count2 = 20;
end


	//state register
	always@(posedge clk)
		if(reset_n == 0)
			state_reg <= s9;
		else
			state_reg <= state_next;
	
	

	// Output Logic and Determine the next state
	always @ (*) 
		case (state_reg)
			s0:
				begin
				if ((open_command == 1'b1) || (fault == 1'b1 && fault_flag == 1'b0))
						state_next = s1;
				else if (close_command == 1'b1)
						state_next = s3;
				else 
						state_next = s0;
				end
			s1:
				begin
				sgto = 1'b1;
				close = 1'b0;
				fault_flag = 1'b1;
				led = 1'b0;
				state_next = s2;
				end
			s2:
				begin
				sgto = 1'b0;
				state_next = s0;
				end
			s3:
				begin
				close = 1'b1;
				fault_flag = 1'b0;
				state_next = s4;	
				end
			s4:
				begin
				count = count - 1;
				if (fault == 1'b1)
				state_next = s7;
				else if (count == 0 && fault == 1'b0)
				state_next = s5;
				else
				state_next = s4;
				end
			s5:
				begin
				solenoid = 1'b1;
				led = 1'b1;
				count = 1500000;
				state_next = s6;
				end
			s6:
				begin
				solenoid = 1'b0;
				state_next = s0;
				end
			s7:
				begin
				if (count2 == 0)
				state_next = s8;				
				else
					begin
					count2 = count2 - 1;
					state_next = s7;
					end
				
				immed_open = 1'b1;
				close = 1'b0;
				fault_flag = 1'b1;
				count = 1500000;			
				end		
			s8:
				begin
				immed_open = 1'b0;
				count2 = 20;
				state_next = s0;
				end
			s9:
				begin
				close = 1'b0;
				fault_flag = 1'b0;
				sgto = 1'b0;
				solenoid = 1'b0;
				immed_open = 1'b0;
				count = 1500000;
				led = 1'b0;
				state_next = s0;
				end
			endcase
	
	

endmodule

 

[rberek]

You have generated many latches in this design, by incompletely specifying the state of several registers. Registers such as led and solenoid are not initialized, and are described in only a few states. This incomplete specification leads to generation of latches, which undesireable for most designs.

Hence the warning regarding latches.

r.b.

 

[digi001]

So are you saying that every state should define the output of every register, whether 1 or 0?

 

[rberek]

Yes, or you can list the default states immediately after the always @(*) line

            always (*) begin

               led = 0;
               solenoid = 0;
                    ....
               case (state_reg)

If the register is not named in the state, it will take the default value listed at the top. It makes the state descriptions cleaner but it is a matter of personal preference. At any rate, the synthesizer must always be able to define a value for a register in every state.

r.b.

 

[digi001]

I was having issues with this code working correctly. So it is possible If i explicitly state every output variable for every state, my code will run correctly? Or is this just good code practice?

 

[mrflibble]

As rberek pointed out, several of your problems are solved by putting the default values at the start of the alway block. Looks like most of those defaults are 0...

Another thing you can do is use state encoded outputs for at least some of the signals.

If for example you intend the "sgto" output to be high only during the S1 state, and low for all other states ... then you can use the corresponding bit in your 1-hot "state_reg" to directly serve as this "sgto" output. As in, you ditch the "reg sgto;" , and you introduce "assign sgto = state_reg[1];".

Furthermore you can change the S0 ... S1 parameters and the case statement like so to get a one-hot FSM.

---

Code Verilog - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
module command_processor2(open_command,close_command,clk,immed_open,close,fault,reset_n,solenoid,sgto,led);
 
input open_command;
input close_command;
 
input clk;
input fault;
input reset_n;
 
output close;
output immed_open;
output solenoid;
output sgto;
output led;
 
 
localparam S0=0, S1=1, S2=2, S3=3, S4=4, S5=5, S6=6, S7=7, S8=8, S9=9; // note the difference
reg  [9:0]state_reg,state_next; // wider regs to fit the one-hot states
 
reg [23:0]count;
reg  [7:0]count2;
reg       sgto;
reg       close;
reg       immed_open;
reg       solenoid;
reg       led;
reg       fault_flag=1'b0;
 
// You could use state encoded outputs with for example "sgto".
// If you intend sgto to be high during state "S1", and low during
// all other states, you could do the following:
// assign sgto = state[S1];
// Obviously you'd have to remove the "reg sgto;" up there.
 
initial begin
    state_reg     = 0;
    state_reg[S9] = 1'b1;
    count         = 1500000;
    count2        = 20;
end
 
 
//state register
always @(posedge clk)
    if (reset_n == 0) begin
        state_reg     <= 0;
        state_reg[S9] <= 1'b1;
    end else
        state_reg     <= state_next;
    
    
 
    // Output Logic and Determine the next state
    always @(*) begin
            // Note that the default 1-hot next state is all zeroes.
            // The active next state bit is set below in the respective cases.
            state_next = 0;
            sgto       = 1'b0;
            close      = 1'b0;
            fault_flag = fault_flag;
            led = 1'b0;
            // etc etc, you get the idea
 
        case (1'b1) // synthesis parallel_case
            state_reg[S0]:
                begin
                if ((open_command == 1'b1) || (fault == 1'b1 && fault_flag == 1'b0))
                  state_next[S1] = 1'b1;
                else if (close_command == 1'b1)
                  state_next[S3] = 1'b1;
                else 
                  state_next[S0] = 1'b1;
                end
            state_reg[S1]:
                begin
                sgto = 1'b1;
                fault_flag = 1'b1;
                state_next[S2] = 1'b1;
                end
            state_reg[S2]:
                begin
                state_next[S0] = 1'b1;
                end
            state_reg[S3]:
                begin
                close = 1'b1;
                fault_flag = 1'b0;
                state_next[S4] = 1'b1;  
            end
            state_reg[S4]:
                begin
                count = count - 1;
                if (fault == 1'b1)
                state_next[S7] = 1'b1;
                else if (count == 0 && fault == 1'b0)
                state_next[S5] = 1'b1;
                else
                state_next[S4] = 1'b1;
                end
            state_reg[S5]:
                begin
                solenoid = 1'b1;
                led = 1'b1;
                count = 1500000;
                state_next[S6] = 1'b1;
                end
            state_reg[S6]:
                begin
                solenoid = 1'b0;
                led = 1'b1;
                state_next[S0] = 1'b1;
                end
            state_reg[S7]:
                begin
                if (count2 == 0)
                  state_next[S8] = 1'b1;
                else begin
                   count2 = count2 - 1;
                   state_next[S7] = 1'b1;
                end
                
                led = 1'b1;
                immed_open = 1'b1;
                close = 1'b0;
                fault_flag = 1'b1;
                count = 1500000;            
                end     
            state_reg[S8]:
                begin
                led = 1'b1;
                immed_open = 1'b0;
                count2 = 20;
                state_next[S0] = 1'b1;
                end
            state_reg[S9]:
                begin
                close = 1'b0;
                fault_flag = 1'b0;
                sgto = 1'b0;
                solenoid = 1'b0;
                immed_open = 1'b0;
                count = 1500000;
                led = 1'b0;
                state_next[S0] = 1'b1;
                end
        endcase
    end
 
endmodule // command_processor2

---

On S0 through S4 I tried to show the concept of the default values at the start, and only the differences in the case statement. After that I got tired of it, so left most of it unchanged. This is more so you get the idea...

Also I am not saying you should one-hot as opposed to your current binary encoded states. Just pointing out that should you use one-hot that you can use quite a few of the state bits directly as outputs. So the extra FFs you spend for the wider state_reg etc, you get back for not having an explicit "sgto" register etc.

The "parallel_case" at the case statement makes sure you get MUXes instead of a priority encoder. Read: makes sure you don't get slow logic.

As for your original code, if you keep that style FSM, you'll have to add a "default: ;" to your case statement. As in a "do nothing" default statement. Combined with putting all the default values for your registers at the top of your always block you should get rid of those pesky latches.

Hope that helps somewhat.

 

[digi001]

Hey thanks to all! I got rid of the latches and changed a few other things and the design works great now.

I also found this document to be quite useful.

**broken link removed**