State machine going into unknown state

Discussion in 'VHDL' started by axr0284, Jul 7, 2008.

  1. axr0284

    axr0284 Guest

    Hey guys,
    I have a weird problem with a spartan 3E FPGA. I don't know if you
    have ever seen this.
    I have a simple state machine to read data from an ADC. The code is
    below:

    <CODE>
    --
    -- Sequential part of state machine
    --
    seq_state_machine: process(reset, clk_120, nextState,
    next_write_flag, next_data_valid_internal,
    next_cs_zero_l, next_cs_one_l, next_rd_l, next_conv_l,
    next_adc_zero_data_A_one, next_adc_zero_data_A_two,
    next_adc_one_data_A_one, next_adc_one_data_A_two,
    next_gamma_sign, next_reset_skip_count, next_busy_flag)
    begin
    if (clk_120'event and clk_120 = '1') then
    if (reset='1') then -- Synchronous active low reset
    currentState <= IDLE;

    internal_cs_zero_l <= '1';
    internal_cs_one_l <= '1';
    internal_rd_l <= '1';
    internal_conv_l <= '1';
    internal_adc_zero_data_A_one <= (others => '0');
    internal_adc_zero_data_A_two <= (others => '0');
    internal_adc_one_data_A_one <= (others => '0');
    internal_adc_one_data_A_two <= (others => '0');
    internal_gamma_sign <= '0';
    reset_skip_count <= '1';
    busy_flag <= '0';
    write_flag <= '0';
    data_valid_internal <= '0';

    else
    currentState <= nextState;

    internal_cs_zero_l <= next_cs_zero_l;
    internal_cs_one_l <= next_cs_one_l;
    internal_rd_l <= next_rd_l;
    internal_conv_l <= next_conv_l;
    internal_adc_zero_data_A_one <= next_adc_zero_data_A_one;
    internal_adc_zero_data_A_two <= next_adc_zero_data_A_two;
    internal_adc_one_data_A_one <= next_adc_one_data_A_one;
    internal_adc_one_data_A_two <= next_adc_one_data_A_two;
    internal_gamma_sign <= next_gamma_sign;
    reset_skip_count <= next_reset_skip_count;
    busy_flag <= next_busy_flag;
    write_flag <= next_write_flag;
    data_valid_internal <= next_data_valid_internal;

    end if;
    end if;
    end process seq_state_machine;

    --
    -- Combinational part of state machine
    --
    comb_state_machine: process(currentState, adc_busy_zero,
    adc_busy_one, adc_data, data_valid_internal,
    internal_cs_zero_l, internal_cs_one_l, internal_rd_l,
    internal_conv_l, gamma_sign,
    internal_adc_zero_data_A_one, internal_adc_zero_data_A_two,
    internal_adc_one_data_A_one, internal_adc_one_data_A_two,
    internal_gamma_sign, reset_skip_count, busy_flag, skip_count,
    write_flag, convert_count)
    begin

    nextState <= currentState;
    next_cs_zero_l <= internal_cs_zero_l;
    next_cs_one_l <= internal_cs_one_l;
    next_rd_l <= internal_rd_l;
    next_conv_l <= internal_conv_l;
    next_adc_zero_data_A_one <= internal_adc_zero_data_A_one;
    next_adc_zero_data_A_two <= internal_adc_zero_data_A_two;
    next_adc_one_data_A_one <= internal_adc_one_data_A_one;
    next_adc_one_data_A_two <= internal_adc_one_data_A_two;
    next_gamma_sign <= internal_gamma_sign;
    next_reset_skip_count <= reset_skip_count;
    next_busy_flag <= busy_flag;
    next_write_flag <= write_flag;
    next_data_valid_internal <= data_valid_internal;

    case currentState is
    when IDLE =>
    next_write_flag <= '0';
    next_conv_l <= '1';
    next_cs_zero_l <= '1';
    next_cs_one_l <= '1';
    next_rd_l <= '1';
    next_reset_skip_count <= '1';
    next_busy_flag <= '0';
    next_data_valid_internal <= '0';
    nextState <= START_THE_CONVERSION_ZERO;

    when START_THE_CONVERSION_ZERO =>
    if(convert_count = b"0110") then -- assert convert for
    50 ns
    next_conv_l <= '1';
    nextState <= WAIT_FOR_BUSY_HIGH;--
    STOP_THE_CONVERSION_ZERO;--
    else
    next_conv_l <= '0';
    nextState <= START_THE_CONVERSION_ZERO;
    end if;

    when WAIT_FOR_BUSY_HIGH =>
    if(adc_busy_zero = '1' AND adc_busy_one = '1') then --
    Wait for busy signal to go high
    nextState <= WAIT_FOR_BUSY_LOW;

    else
    nextState <= WAIT_FOR_BUSY_HIGH;

    end if;

    when WAIT_FOR_BUSY_LOW =>
    if(adc_busy_zero = '0' AND adc_busy_one = '0') then --
    Wait for busy signal to go high
    next_cs_zero_l <= '0';
    next_rd_l <= '0';
    next_reset_skip_count <= '0';
    nextState <= READ_DATA_ZERO; -- Read the data

    else

    nextState <= WAIT_FOR_BUSY_LOW;

    end if;

    when READ_DATA_ZERO =>
    next_reset_skip_count <= '0';
    if(skip_count = b"0001100") then
    next_adc_zero_data_A_one <= adc_data; -- Store the data A1
    nextState <= READ_DATA_ZERO; -- Read the data

    elsif(skip_count = b"0001111") then
    next_rd_l <= '1';
    nextState <= READ_DATA_ZERO; -- Read the data

    elsif(skip_count = b"0010011") then
    next_rd_l <= '0';
    nextState <= READ_DATA_ZERO; -- Read the data

    elsif(skip_count = b"0010111") then
    next_adc_zero_data_A_two <= adc_data; -- Store the
    data A1
    nextState <= READ_DATA_ZERO; -- Read the data

    elsif(skip_count = b"0100011") then -- 2 cycles later
    deassert chip select
    next_rd_l <= '1';
    next_cs_zero_l <= '1';
    nextState <= SWITCH_TO_ADC_ONE; -- Read the data
    else
    nextState <= READ_DATA_ZERO; -- Read the data
    end if;

    when SWITCH_TO_ADC_ONE =>
    if(skip_count = b"0101011") then
    next_cs_one_l <= '0';
    next_rd_l <= '0';
    next_reset_skip_count <= '1';
    nextState <= READ_DATA_ONE; -- Read the
    data

    else
    nextState <= SWITCH_TO_ADC_ONE; -- Read the
    data

    end if;

    when READ_DATA_ONE =>
    next_reset_skip_count <= '0';
    if(skip_count = b"0001100") then
    next_adc_one_data_A_one <= adc_data; -- Store the
    data A1
    nextState <= READ_DATA_ONE; -- Read the data

    elsif(skip_count = b"0001111") then
    next_rd_l <= '1';
    nextState <= READ_DATA_ONE; -- Read the data

    elsif(skip_count = b"0010011") then
    next_rd_l <= '0';
    nextState <= READ_DATA_ONE; -- Read the data

    elsif(skip_count = b"0010111") then
    next_adc_zero_data_A_two <= adc_data; -- Store the
    data A1
    next_gamma_sign <= gamma_sign;
    nextState <= READ_DATA_ONE; -- Read the data

    elsif(skip_count = b"100001") then -- Skip 60 cycles
    next_cs_one_l <= '1';
    next_rd_l <= '1';
    next_write_flag <= '1';
    next_data_valid_internal <= '1';
    nextState <= READ_DATA_ONE; -- Read the data

    elsif(skip_count = b"1011101") then -- Skip 60 cycles
    next_reset_skip_count <= '1';
    nextState <= IDLE; -- Read the data

    else
    nextState <= READ_DATA_ONE; -- Read the data
    end if;

    when OTHERS =>
    nextState <= IDLE; -- Read the data

    end case;
    end process comb_state_machine;
    </CODE>

    In the state machine if I don't have
    when OTHERS =>
    nextState <= IDLE;

    the state machine CONSISTENTLY goes into an unknown state and locks
    up. That does not seem right to me. I am wondering if you guys can
    take a look at the code and tell me if there is a bug because I can't
    find any. Thanks,
    Amish
    axr0284, Jul 7, 2008
    #1
    1. Advertising

  2. axr0284

    KJ Guest

    On Jul 7, 10:18 am, axr0284 <> wrote:
    > Hey guys,
    >  I have a weird problem with a spartan 3E FPGA. I don't know if you
    > have ever seen this.
    > I have a simple state machine to read data from an ADC. The code is
    > below:
    >
    > <SNIP CODE>


    > In the state machine if I don't have
    > when OTHERS =>
    >         nextState <= IDLE;
    >
    > the state machine CONSISTENTLY goes into an unknown state and locks
    > up. That does not seem right to me.


    It's not right...there are no 'unknown' states in real hardware, there
    are no 'unknown' states in simulation either when the state variable
    is an enumerated type. What you most likely mean is YOU don't know
    what state things are in. (Hint: Take a look at the state signals,
    they are not unknowns).

    I'm assuming you've simulated the design, if not do that first and get
    it working.

    If you're talking about real hardware, then most likely the state
    machine is one hot encoded and the 'unknown' state is occurring
    because more than one state bit is getting set. Then because of the
    'when others' taking you back to the idle state you can recover. But
    one hot encoded states with more than one bit set is the design issue
    that needs fixing.

    Assuming the sim works, but real hardware doesnt' then the design
    problem you have that causes more than one state bit to get set is
    failing timing analysis. In this particular case, the timing problem
    is most likely because the inputs to your state machine have not been
    synchronized to the clock. Based on the list-o-signals in your
    sensitivity list any one of the following signals could be the
    culprit:

    currentState, adc_busy_zero, adc_busy_one, adc_data,
    data_valid_internal, internal_cs_zero_l, internal_cs_one_l,
    internal_rd_l, internal_conv_l, gamma_sign,
    internal_adc_zero_data_A_one, internal_adc_zero_data_A_two,
    internal_adc_one_data_A_one, internal_adc_one_data_A_two,
    internal_gamma_sign, reset_skip_count, busy_flag, skip_count,
    write_flag, convert_count

    If any of these are not outputs of a flip flop that is clocked by your
    signal 'clk_120' then you'll get a flaky design that fails in ways
    that you're seeing as well as in other 'mysterious' manners.

    The other possibility is that you are using a signal inside that
    combinatorial process that is not in the sensitivity list. This would
    cause a mismatch between sim and reality. To detect if this is the
    case, peruse your synthesis report looking for something like
    "incomplete sensitivity list". To avoid ever having to have this
    particular problem ever bite you, then do away with the two process
    style and use a single clocked process.

    > I am wondering if you guys can
    > take a look at the code and tell me if there is a bug because I can't
    > find any. Thanks,


    No thanks, look at my suggestions and then go back and look at your
    own code.

    Kevin Jennings
    KJ, Jul 7, 2008
    #2
    1. Advertising

  3. axr0284

    axr0284 Guest

    Thanks for the answer, I will make sure to check the code carefully.
    Now that you mention it, I never synchronize the asynchronous signals.
    I'll try that out. Thanks.

    > then do away with the two process
    > style and use a single clocked process.
    >


    Could you elaborate why a single process is better. Thanks,
    axr0284, Jul 7, 2008
    #3
  4. axr0284

    axr0284 Guest

    On Jul 7, 11:04 am, KJ <> wrote:
    > On Jul 7, 10:18 am, axr0284 <> wrote:
    >
    > > Hey guys,
    > >  I have a weird problem with a spartan 3E FPGA. I don't know if you
    > > have ever seen this.
    > > I have a simple state machine to read data from an ADC. The code is
    > > below:

    >
    > > <SNIP CODE>
    > > In the state machine if I don't have
    > > when OTHERS =>
    > >         nextState <= IDLE;

    >
    > > the state machine CONSISTENTLY goes into an unknown state and locks
    > > up. That does not seem right to me.

    >
    > It's not right...there are no 'unknown' states in real hardware, there
    > are no 'unknown' states in simulation either when the state variable
    > is an enumerated type.  What you most likely mean is YOU don't know
    > what state things are in.  (Hint:  Take a look at the state signals,
    > they are not unknowns).
    >
    > I'm assuming you've simulated the design, if not do that first and get
    > it working.
    >
    > If you're talking about real hardware, then most likely the state
    > machine is one hot encoded and the 'unknown' state is occurring
    > because more than one state bit is getting set.  Then because of the
    > 'when others' taking you back to the idle state you can recover.  But
    > one hot encoded states with more than one bit set is the design issue
    > that needs fixing.
    >
    > Assuming the sim works, but real hardware doesnt' then the design
    > problem you have that causes more than one state bit to get set is
    > failing timing analysis.  In this particular case, the timing problem
    > is most likely because the inputs to your state machine have not been
    > synchronized to the clock.  Based on the list-o-signals in your
    > sensitivity list any one of the following signals could be the
    > culprit:
    >
    > currentState, adc_busy_zero, adc_busy_one, adc_data,
    > data_valid_internal, internal_cs_zero_l, internal_cs_one_l,
    > internal_rd_l, internal_conv_l, gamma_sign,
    > internal_adc_zero_data_A_one, internal_adc_zero_data_A_two,
    > internal_adc_one_data_A_one, internal_adc_one_data_A_two,
    > internal_gamma_sign, reset_skip_count, busy_flag, skip_count,
    > write_flag, convert_count
    >
    > If any of these are not outputs of a flip flop that is clocked by your
    > signal 'clk_120' then you'll get a flaky design that fails in ways
    > that you're seeing as well as in other 'mysterious' manners.
    >
    > The other possibility is that you are using a signal inside that
    > combinatorial process that is not in the sensitivity list.  This would
    > cause a mismatch between sim and reality.  To detect if this is the
    > case, peruse your synthesis report looking for something like
    > "incomplete sensitivity list".  To avoid ever having to have this
    > particular problem ever bite you, then do away with the two process
    > style and use a single clocked process.
    >
    > > I am wondering if you guys can
    > > take a look at the code and tell me if there is a bug because I can't
    > > find any. Thanks,

    >
    > No thanks, look at my suggestions and then go back and look at your
    > own code.
    >
    > Kevin Jennings


    You were correct. The asynchronous signals were causing the issue. I
    added double buffering on all my asynchronous lines and it seems to
    have solved the problem. Thanks a lot for the help.
    Amish
    axr0284, Jul 7, 2008
    #4
  5. axr0284

    KJ Guest

    On Jul 7, 1:25 pm, axr0284 <> wrote:
    >
    > Could you elaborate why a single process is better. Thanks,


    1. Less typing.
    2. Less chance for creating a design error because of an incomplete
    sensitivity list.

    Whenever you have a combinatorial (i.e. unclocked) process, you can
    unintentionally create the following problems that then need to be
    debugged in simulation or (even worse) by perusing synthesis
    warnings. Neither of these problems occur if you simply use a single
    clocked process for your state machine:

    Prob 1: Forget to assign a signal under all conditions. Typically one
    would handle this with a "Next_State <= Current_State" assignment at
    the begining of the process and then go into your 'real' logic. That
    way, there will always be some assignment made whenver the process
    triggers which will prevent generating latches which are a big no-no
    in FPGA/CPLDs. The "Next_State <= Current_State" statement is also
    'extra' typing, in that it is simply not needed if you have a single
    clocked process.

    Prob 2: Forget to put a signal into the sensitivity list of the
    process. The simulator won't care, but the synthesis tool will create
    different logic since it will assume that you just forgot to put the
    signal in and create logic as if you had put it into the sensitivity
    list. Thus your simulation and 'real world' designs will do different
    things.

    Ex:

    process(a)
    begin
    x1 <= a and b;
    end process;

    process(a,b)
    begin
    x2 <= a and b;
    end process;

    x3 <= a and b;

    a <= '0', '1' after 10 ns;
    b <= '0', '1' after 20 ns;

    The first above process will not change the value of 'x' when 'b'
    changes unless it also changes at the exact same time as 'a'.
    Simulate the above and check it out, noticing that x2 and x3 will do
    the same thing, but x1 will not change at t=20ns. Synthesis of x1, x2
    and x3 however will produce the exact same logic for all three. While
    the synthesis tool's actions might be what you had really intended to
    do anyway, it is bad practice to have a simulation model that
    functionally differs from reality.

    Now ask yourself if those are the types of problems you want to spend
    your time debugging or not.

    KJ
    KJ, Jul 7, 2008
    #5
  6. axr0284

    Andy Peters Guest

    On Jul 7, 10:25 am, axr0284 <> wrote:
    > Thanks for the answer, I will make sure to check the code carefully.
    > Now that you mention it, I never synchronize the asynchronous signals.
    > I'll try that out. Thanks.
    >
    > > then do away with the two process
    > > style and use a single clocked process.

    >
    > Could you elaborate why a single process is better. Thanks,


    Because you avoid potential combinatorial loops when you forget to
    assign to a signal in a state.

    Also, your synchronous process should NOT have every signal that's on
    the RHS of an assignment in the sensitivity list. When using a
    synchronous reset, the ONLY signal that should be on the list is the
    clock.

    -a
    Andy Peters, Jul 8, 2008
    #6
  7. axr0284 wrote:

    > Could you elaborate why a single process is better.


    It would eliminate half of the questions
    to this newsgroup, for one thing.

    -- Mike Treseler
    Mike Treseler, Jul 8, 2008
    #7
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. David Lamb
    Replies:
    1
    Views:
    659
  2. Weng Tianxiang
    Replies:
    7
    Views:
    1,082
    Mike Treseler
    Nov 25, 2003
  3. Unknown Action by the machine

    , Feb 19, 2007, in forum: ASP .Net Web Services
    Replies:
    0
    Views:
    96
  4. fenster
    Replies:
    3
    Views:
    1,151
    jeppe
    Dec 23, 2011
  5. Vincent Arnoux
    Replies:
    1
    Views:
    236
    Arnaud Bergeron
    Aug 11, 2006
Loading...

Share This Page