Faulty State Machine

[Altera_Forum]

Hi there,  

 There is a bug in the following state machine that sometimes causes the state transits directly from standby to locked (or some undefined state and stays there) when the enable signal goes high, without going through detecting state. I think it's timing related bug, because for some compiles it seems to be working ok, but not quite sure on how to improve the code. Right now I am compiling a version that takes the n_pur out of the equation of the second process.  Any suggestions?  Appreciated.  Hua     -- asd fsm sync process process (n_pur, clk) begin if n_pur='0' then asd_cs <= standby; lock <= '0'; rate <= "00000"; rate_tmp1 <= "00000"; asd_pass_reg <= 0; elsif rising_edge (clk) then asd_cs <= asd_ns; lock <= lock_tmp; rate <= rate_tmp; rate_tmp1 <= rate_tmp; asd_pass_reg <= asd_pass; end if; end process; -- asd fsm state transition process (n_pur, asd_cs, enable, s2_rate, rate_tmp1, current_state, last_detected_rate, asd_pass_reg) begin if n_pur='0' then lock_tmp <= '0'; state_tmp <= '0'; rate_tmp <= "00000"; counter_fsm_en <= '0'; asd_pass <= 0; asd_ns <= standby; else case asd_cs is when standby => lock_tmp <= '0'; state_tmp <= '0'; rate_tmp <= rate_tmp1; counter_fsm_en <= '0'; asd_pass <= 0; --last_detected_rate <="00000"; --if enable = '1' and current_state = standby then if enable = '1' then asd_ns <= detecting; else asd_ns <= standby; end if; when detecting => lock_tmp <= '0'; state_tmp <= '1'; --rate_tmp <= "00000"; rate_tmp <= rate_tmp1; counter_fsm_en <= '1'; if s2_rate = last_detected_rate and s2_rate /= "00000" and output='1' then asd_pass <= asd_pass_reg+1; else asd_pass <= 0; end if; if enable = '0' then asd_ns <= standby; elsif asd_pass >= asd_max_pass-1 then asd_ns <= locked; else asd_ns <= detecting; end if; when locked => lock_tmp <= '1'; state_tmp <= '0'; rate_tmp <= s2_rate; counter_fsm_en <= '0'; asd_pass <= 0; if enable = '0' then asd_ns <= standby; else asd_ns <= locked; end if; when others => lock_tmp <= lock_tmp; state_tmp <= '0'; rate_tmp <= rate_tmp1; counter_fsm_en <= '0'; asd_pass <= 0; asd_ns <= standby; end case; end if; end process;

[Altera_Forum]

I know many tell you to use two or three processes for fsm but I only trust using one single clocked process. This avoids the clutter of code and combinatorial issues. 

 Try this change(not tested obviously)    process (n_pur, clk) begin if n_pur='0' then asd_cs <= standby; lock <= '0'; rate <= "00000"; rate_tmp1 <= "00000"; asd_pass_reg <= 0; lock_tmp <= '0'; state_tmp <= '0'; rate_tmp <= "00000"; counter_fsm_en <= '0'; asd_pass <= 0; elsif rising_edge (clk) then lock <= lock_tmp; rate <= rate_tmp; rate_tmp1 <= rate_tmp; asd_pass_reg <= asd_pass; case asd_cs is when standby => lock_tmp <= '0'; state_tmp <= '0'; rate_tmp <= rate_tmp1; counter_fsm_en <= '0'; asd_pass <= 0; if enable = '1' then asd_cs <= detecting; end if; when detecting => lock_tmp <= '0'; state_tmp <= '1'; rate_tmp <= rate_tmp1; counter_fsm_en <= '1'; if s2_rate = last_detected_rate and s2_rate /= "00000" and output='1' then asd_pass <= asd_pass_reg+1; else asd_pass <= 0; end if; if enable = '0' then asd_cs <= standby; elsif asd_pass >= asd_max_pass-1 then asd_cs <= locked; end if; when locked => lock_tmp <= '1'; state_tmp <= '0'; rate_tmp <= s2_rate; counter_fsm_en <= '0'; asd_pass <= 0; if enable = '0' then asd_cs <= standby; end if; when others => null; end case; end if; end process;

[Altera_Forum]

Personally, I also prefer single process state machines. They provide in a compact form what you have to achieve with additional temorary variables in much more text in the above used style. 

 The reason for failure is possibly in this line elsif asd_pass >= asd_max_pass-1 then where you are using the temporary variable in the comparison but you should use the registered one.  More generally, asynchronous input signal are always a rich source of possibly failing code. Any external input signal to the state machine, e.g. enable must be known synchronous to clk or has to be synchronized additionally. Also the reset should be synchronously released.

[Altera_Forum]

I used to use 2 process ones as well, but now I much prefer a single self contained process with the state_type enumeration of all states declared inside the process and a variable to represent "next" state.

[Altera_Forum]

Also, if you have a signal coming into the chip that you are using it in a synchronized state machine you absolutely have to be sure to synchronize it, especially at high speed clock rates and if that signal changes often or has fast edge rates. You can do that using a shift register type of approach or one like this article shows: http://www.edn.com/articles/pdfs/edn/20000106/01d24651.pdf . Altera and Xilinx both have whitepapers on metastability that you might want to read if you are unfamiliar with this

[Altera_Forum]

Thank you all for your replies. I will definitely consider to use single-process approach of coding state machines from now on, even just for the sake of reducing the portition of asynchronous design. 

 For this particular problem, FvM and alt1000 were right on the point. The enable signal was coming from a different clock domain and I forgot to sync it in :o . Now I had fixed it and the problem went away. Lesson learned. :D