Verilog FSM not working on FPGA

[Altera_Forum]

Hi, 

 Please accept my appologies if I have posted this is the incorrect place, but im new to this and this post seems kind of relevant.   I am trying to create a format for my FSM,   To test this I wrote a simple clock_divider.   This verilog works perfect in RTL simulation but when tested on an FPGA (Cyclone III) the generated clock is very random.   I have attached the "count" register to LED's and every second clock some lights go dim - makes me think they may be flashing on an off extreemly quickly.   Ive tried everything i can think off, even my uni lecturer is stumped. Thanks in advance for your help.   Paul   module Clock_Gen(rst_n,   clk_out,  clk_in,  count);   //Input Ports input rst_n; input clk_in; //**************************************************** //Output Ports output [3:0] count; output clk_out; //**************************************************** //Output Registers reg clk_out; //**************************************************** //Internal Registers reg [3:0] count; //**************************************************** //State Definition parameter S0 = 4'd0,  S1 = 4'd1,  S2 = 4'd2,  S3 = 4'd3,  S4 = 4'd4,  S5 = 4'd5,  S6 = 4'd6;   //**************************************************** //Internal state variables reg [3:0] state; reg [3:0] next_state; //**************************************************** //Set initial reg values //**************************************************** //State changes only at positive edge of clock (synchronous) always @(posedge clk_in)  if (!rst_n)  state <= S0;   else  state <= next_state; //State Change   //**************************************************** //State actions always @(state) begin  case(state)  S0: begin  clk_out = 1'b1;  count = 4'b0000;  end  S1: clk_out = 1'd1;  S2: begin  count <= (count + 1'b1);  clk_out = 1'b1;  end  S3: begin  clk_out = 1'b0;  count = 4'b0000;  end  S4: clk_out = 1'b0;  S5: begin  clk_out = 1'b0;  count <= (count + 1'b1);  end  endcase end //**************************************************** //State machine using case statement always @(state) begin  case(state)  S0: next_state = S1;  S1: begin  if (count == 14)  next_state = S3;  else  next_state = S2;  end  S2: next_state = S1;  S3: next_state = S4;  S4: begin  if (count == 14)  next_state = S0;  else  next_state = S5;  end  S5: next_state = S4;  endcase end  endmodule

[Altera_Forum]

I guess, you got a lot of serious Quartus synthesis warnings with your code. If so, you probably shouldn't ignore them. 

 You e.g. have a counter in an combinational always block. This can't be expected to ever result in reliable operation. always @(state) begin .. count <= (count + 1'b1); .. end  A counter may count depending on state machine states and other conditions, but it must be placed in an edge sensitive always block.  P.S:  If the present code seems to give correct results in functional (RTL) simulation, this doesn't mean anything.

[Altera_Forum]

Cheers for your reply,  

 I do get many warnings 337 in fact.   I've changed the always @ (state) to always @ (negedge clk_in) now - all works fine!   Thanks

[Altera_Forum]

Before using a clock divider created this way, check the cautions for ripple clocks at http://www.alteraforum.com/forum/showthread.php?t=2388.

[Altera_Forum]

Hi Paul, 

 even if all works fine now this is not the way I would suggest. Your FSM consists of three parts: State register assignment, state actions, and next_state calculation. This is perfect. However, the next_state calculation lacks two items. The first three lines should be:  always @(state or count or whatever appears on the right hand side of an expression in that block) begin  case(state) next_state = state; ...  The always construct also can be written this way always @(*) in Verilog 2001. Quartus supports that.

[Altera_Forum]

Thanks for your reply Harald. 

 I am unsure about your comment however. Would this not mean that the state would be changed whenever a reg was changed?   I want the FSM to change state synchronously with the clk.   Thanks for your help   Paul

[Altera_Forum]

Hi Paul, 

 the state of the FSM can only change on the rising clock edge since you clearly define:   always @(posedge clk_in) if (!rst_n) state <= S0;  else state <= next_state;   This is the one and only assignment for the state register. What I mentioned is the calculation of next_state. This, in fact, is the input to the state register, and this is combinational logic depending on the input ports and the state. So, the always construct must be triggered every time one of these signals changes. The first statement assigns the current state to next_state. Depending on this, the case switch is evaluated and a new value for next_state is calculated.

[Altera_Forum]

Thans Harald, 

 I understand what you mean now. I am currently triggering the calculation of next_state at every posedge of clock, ready for the state register to be updated on the negedge of the clock. This does seem to work correclty when synthsised on FPGA.   Would it be better to do this combinationally?   Also if possible could you explain further as to why I cannot write...   //State actions always @(state) begin case(state) S0: begin clk_out = 1'b1; count = 4'b0000; end S1: clk_out = 1'd1; S2: begin count <= (count + 1'b1); clk_out = 1'b1; end   The count does not work correctly, but my understanding of the Verilog is that, whenever state changes value, the construct would be run once. This should update the count register once. And so why is this not possible?   Thanks so much for your help in this matter   Paul     Below is the current working code with all the constructs executed on a clk edge:   module Clock_Gen(rst_n,   clk_out,  clk_in,  count);   //Input Ports input rst_n; input clk_in; //**************************************************** //Output Ports output [3:0] count; output clk_out; //**************************************************** //Output Registers reg clk_out; //**************************************************** //Internal Registers reg [40:0] count; //**************************************************** //State Definition parameter S0 = 4'd0,  S1 = 4'd1,  S2 = 4'd2,  S3 = 4'd3,  S4 = 4'd4,  S5 = 4'd5,  S6 = 4'd6;   //**************************************************** //Internal state variables reg [3:0] state; reg [3:0] next_state; //**************************************************** //Set initial reg values //**************************************************** //State changes only at positive edge of clock (synchronous) always @(negedge clk_in)  if (!rst_n)  state <= S0;   else  state <= next_state; //State Change   //**************************************************** //State actions always @(posedge clk_in) begin  case(state)  S0: begin  clk_out = 1'b1;  count = 25000000;  end  S1: clk_out = 1'd1;  S2: begin  count <= (count - 1);  clk_out = 1'b1;  end  S3: begin  clk_out = 1'b0;  count = 25000000;  end  S4: clk_out = 1'b0;  S5: begin  clk_out = 1'b0;  count <= (count - 1);  end  endcase end //**************************************************** //State machine using case statement always @(posedge clk_in) begin  case(state)  S0: next_state = S1;  S1: begin  if (count == 0)  next_state = S3;  else  next_state = S2;  end  S2: next_state = S1;  S3: next_state = S4;  S4: begin  if (count == 0)  next_state = S0;  else  next_state = S5;  end  S5: next_state = S4;  endcase end  endmodule

[Altera_Forum]

--- Quote Start ---  always @(state or count or whatever appears on the right hand side of an expression in that block)  --- Quote End ---    This suggestion can be basically ignored in synthesis. Although Quartus gives warnings on conditions missing in the always block, the synthesized code won't be different at all. In simulation, the missing trigger in always block most likely causes different behaviour.

[Altera_Forum]

Hi FvM, 

 I totally agree to that. Nevertheless, I still hope that the young folks learn to simulate, simulate, and simulate again before starting Quartus the first time.  Hi Paul,  sure, it works when you trigger the next_state calculation on the falling edge of the clock but this is not the way a FSM is meant to be. Maybe some day you will design an FSM running at *really* high speed and then there is not enough time for this additional register. Concerning the count issue, please clarify what you mean when you say that is does not work. Simulation or real world? To be honest, I have no clue what Quartus systhesizes out of this source. I would suggest that you have count and the clk_out be triggered on a clock edge.