Forum Discussion

Altera_Forum's avatar
Altera_Forum
Icon for Honored Contributor rankHonored Contributor
16 years ago

Faulty State Machine

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;

5 Replies

  • Altera_Forum's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    16 years ago

    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's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    16 years ago

    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's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    16 years ago

    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's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    16 years ago

    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's avatar
    Altera_Forum
    Icon for Honored Contributor rankHonored Contributor
    16 years ago

    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