VHDL question: Why is my FSM Oscillating/Unstable?

[AlessandroAU]

I have the written the following FSM in VHDL.

It behaves very strangely, I cannot get it to stay in the "S_IDLE" state.

I have FSM_state attached to an 7seg LED so I can see the behavior. When IO_FLAG = 0 it should stay in state S_IDLE but I can see that instead it rapidly oscillates between states 1 and 2.

If change the S_IDLE assignment (highlighted) to S_RESET it will 'skip' S_RESET and then stay on S_IDLE! It seems as if it is passing through the first "if ( IO_FLAGD = '1')" block even though the flag is not asserted.

[kfnight]

Use the debugging power of your simulator to see exactly what is going on with all the signals.

[nctnico]

Assuming this is implemented in an FPGA: There is no clock signal. In an FPGA you'll have paths with different delays causing an FSM like yours to misbehave because the FSM state signals don't arrive at the same time. The easiest (and preferred) way to use an FPGA is with a clock signal.

[SiliconWizard]

You're showing only part of your FSM.  There is likely a second, clocked, process just to advance the FSM (from state_next to state). I think this is the "Moore" style of FSM. I don't particularly care for this style and currently see no specific justification for using it in VHDL with modern tools (if anyone has a good rationale, I'll be glad to read it). I have seen it used quite often by older people (so this is still likely to be the preferred style of many older teachers).

If properly used, this style is not problematic per se, though.
Not knowing anything about the IO_FLAGD signal, I'd venture this signal is asynchronous to the clock used to advance the FSM. This is likely your problem here, and a very common trap for young players (and some older as well!) If this is the case, you'll need to resynchronize the IO_FLAGD to the FSM's clock domain before using it this way as a flag.

[AlessandroAU]

Sorry, yes, there is a 'trivial' clocked part that just advances the state and handles reset.

How would you make this clocked? Just remove the other items from the sensitivity list and make it process(clk_in) ?

@SiliconWizard, you are right, FLAGD is an external signal connected to an IO. How do I resynchronize it? Read it in a process(clk) and assign to an another internal signal?

Sorry for the dumb questions, I'm trying to self learn all this.

[SiliconWizard]

@SiliconWizard, you are right, FLAGD is an external signal connected to an IO.

Yes, the kind of symptoms you're noticing is typical of metastability problems. It can lead to seemingly completely erratic behavior, and no matter what amount of debugging you're going to do, if you're not aware of this clock domains crossing problem, you can waste hours, days or even weeks. A slight change in your code may lead to a completely different behavior (due to a different mapping and PAR), yet still incorrect, further puzzling you and making it intractable.

How do I resynchronize it? Read it in a process(clk) and assign to an another internal signal?

Exactly. Something like:

Code: [Select]

process (Reset, Clock)
begin
if Reset = '1' then
IO_FLAGD_s <= '0';
elsif rising_edge(Clock) then
IO_FLAGD_s <= IO_FLAGD;
end if;
end process;

Some people like to cascade more flip-flops in the chain to further lower the probability of metastability issues. In many cases, the above kind of process is enough, especially on FPGAs.

Look up articles on metastability. To get you started: https://asic-design-verification.blogspot.com/2009/03/understanding-clock-domain-crossing.html
It's a very fundamental concept to grasp.

[AlessandroAU]

YES! That is exactly the behavior I was seeing, I changed something completely irrelevant somewhere else and it effected the FSM behavior! I thought I was going crazy. I already wasted to 2 days trying to figure it out 

Thank you, it's all starting to make sense.

One more question, so what I am trying to do is interface with a FX2LP USB chip in order to stream some data from an ADC (with the FPGA in the middle). The fx2lp is running in 'slave fifo' mode, where it acts like a dumb data bus.

So actually, the FLAGD should be synchronous I think? Perhaps the propagation delay from the IO to the FSM logic area is too high? If I look at the reference design it doesn't appear that they do any re-synchronization.

Here it is:

----------------------------------------------------------------------------------
-- Engineer:Rahul Kumar
--
-- Design Name:  FX2LP-FPGA interface (loopback)
-- Module Name: 
-- Project Name:
-- Target Devices:
-- Tool versions:
-- Description:
--
-- Dependencies:
--
-- Revision:
-- Revision 0.01 - File Created
-- Additional Comments:
--
----------------------------------------------------------------------------------
library IEEE;
use IEEE.STD_LOGIC_1164.ALL;     
use IEEE.STD_LOGIC_ARITH.ALL;     
use IEEE.STD_LOGIC_UNSIGNED.ALL;
library UNISIM;
use UNISIM.vcomponents.all;

entity fx2lp_slaveFIFO2b_streamIN_fpga_top is
  Port ( 
     reset_n_out   : out STD_LOGIC;                         --used for TB
    fdata     : inout  STD_LOGIC_VECTOR(7 downto 0);  --  FIFO data lines.
    faddr     : out STD_LOGIC_VECTOR(1 downto 0);     --  FIFO select lines
    slrd      : out STD_LOGIC;                        -- Read control line
    slwr      : out STD_LOGIC;                        -- Write control line
   
    flagd     : in  STD_LOGIC;                        --EP6 full flag
    flaga     : in  STD_LOGIC;                        --EP2 empty flag
    clk       : in  STD_LOGIC;                        --Interface Clock
    sloe      : out STD_LOGIC;                        --Slave Output Enable control
    clk_out   : out STD_LOGIC
    --pkt_end   : out STD_LOGIC;
       --  done      : out STD_LOGIC;
         --sync      : in std_logic
    
  );
end fx2lp_slaveFIFO2b_streamIN_fpga_top;

architecture fx2lp_slaveFIFO2b_streamIN_fpga_top_arch of fx2lp_slaveFIFO2b_streamIN_fpga_top is

component clk_wiz_v3_6
   port(   
         CLK_IN1           : in     std_logic;         
         CLK_OUT1          : out    std_logic;
         CLK_OUT2          : out    std_logic;
         CLK_OUT3          : out    std_logic;
         CLK_OUT4          : out    std_logic;
         RESET             : in     std_logic;
         LOCKED            : out    std_logic
      );
end component;


--streamIN fsm signal
type stream_in_state is (stram_in_idle, stream_in_write);
signal current_stream_in_state, next_stream_in_state : stream_in_state;

signal slrd_n, slwr_n, sloe_n,slrd_d_n : std_logic;

signal CLK_OUT_0, clk_out_90, clk_out_180, CLK_OUT_270 : std_logic;
signal reset_n : std_logic;
signal lock : std_logic;

signal data_out : std_logic_vector(7 downto 0);
signal done_d   : std_logic;
signal wait_s   : std_logic_vector(3 downto 0);

begin --architecture begining

oddr_y : ODDR2                                               -- clk out buffer
   port map
   (
    D0    => '1',
    D1    => '0',
    CE    => '1',
    C0   => clk_out_180, 
    C1   => (not clk_out_180),
    R     => '0',
    S     => '0',
    Q     => clk_out
   );
   
      
pll : clk_wiz_v3_6                                        -- PLL
   port map(
    CLK_IN1         => clk,
    clk_out1        => clk_out_0,      
    clk_out2        => clk_out_90,
    clk_out3        => clk_out_180,
    CLK_OUT4    => clk_out_270,
    RESET           => '0',
    LOCKED          => lock
   );


--for TB
reset_n_out <= reset_n;


--output signal asignment
reset_n <= lock;   
slwr  <= slwr_n;
slrd  <= slrd_n;
sloe  <= sloe_n;
faddr <= "10";
--pkt_end <= '1';
--done <= done_d;

fdata <= data_out;

process(clk_out_0, reset_n) begin
   if(reset_n = '0')then
      done_d <= '0';
        elsif(clk_out_0'event and clk_out_0 = '1')then
      if(wait_s = "1010")then
                   done_d <= '1';
      end if;   
        end if;
end process;

process(clk_out_0, reset_n) begin
   if(reset_n = '0')then
            wait_s <= "0000";
        elsif(clk_out_0'event and clk_out_0 = '1')then
      if(wait_s < "1010")then
                   wait_s <= wait_s + '1';
      end if;   
        end if;
end process;


--write control signal generation
process(current_stream_in_state, flagd)begin
   if((current_stream_in_state = stream_in_write) and (flagd = '1'))then
      slwr_n <= '0';
   else
      slwr_n <= '1';
   end if;
end process;

--streamIN mode state machine
streamIN_mode_fsm_f : process(clk_out_0, reset_n) begin
   if(reset_n = '0')then
            current_stream_in_state <= stram_in_idle;
        elsif(clk_out_0'event and clk_out_0 = '1')then
                current_stream_in_state <= next_stream_in_state;
        end if;
end process;

--LoopBack mode state machine combo
streamIN_mode_fsm : process(flaga, flagd, current_stream_in_state) begin
   next_stream_in_state <= current_stream_in_state;
   case current_stream_in_state is
      when stram_in_idle =>
         if(flagd = '1')then
            next_stream_in_state <= stream_in_write;
         else
            next_stream_in_state <= stram_in_idle;
         end if;

      when stream_in_write =>
         if(flagd = '0')then
            next_stream_in_state <= stram_in_idle;
         else
            next_stream_in_state <= stream_in_write;
         end if;
            
      when others =>
         next_stream_in_state <= stram_in_idle;
      end case;
end process;

--data generator counter
process(clk_out_0, reset_n) begin
   if(reset_n = '0')then
            data_out <= "00000000";
        elsif(clk_out_0'event and clk_out_0 = '1')then
      if(slwr_n = '0')then
                    data_out <= data_out + '1';
      end if;   
        end if;
end process;

end architecture;

[NorthGuy]

As ntcnico said, your combinatorial part of the code produces lots of latches, which are completely asynchronous. This may produce loops. Also, combinatorial logic is glithcy and may produce unpredicable behaviour, especially at higher complexity.

The prototype code is rather simple and tries to avoid latches by making sure that the assignment to a variable always happen (such as if you assign something in "if", you must assign it in "else"). Such approaches eliminates self-dependence in the combinatorial code.

The best way to deal with this is to avoid combinatorial processes altogether and do everything on the rising edge of the clock. This synchronizes all the logic by registering the variables.

[Sal Ammoniac]

I don't particularly care for this style and currently see no specific justification for using it in VHDL with modern tools (if anyone has a good rationale, I'll be glad to read it). I have seen it used quite often by older people (so this is still likely to be the preferred style of many older teachers).

For all of us old geezers out here, can you post an example of the newer style preferred by younger people?

[rstofer]

I don't particularly care for this style and currently see no specific justification for using it in VHDL with modern tools (if anyone has a good rationale, I'll be glad to read it). I have seen it used quite often by older people (so this is still likely to be the preferred style of many older teachers).

For all of us old geezers out here, can you post an example of the newer style preferred by younger people?

Just take the two process FSM (one clocked to stuff next_state into current_state) and one combinatorial where the actual work is done and rewrite it as one large clocked process.

Really, it doesn't matter if the combinatorial process is glitchy as long as everything settles out before the next clock because other processes aren't going to trigger off the output until the next clock anyway.  Even inside the large clocked process approach, signals still transition (and bounce around) following a clock transition.  As long as things settle out before the next clock, no harm, no foul.

Either way, all signals must be defined at all times.  Sometimes they are given default values at the top of the process and other times they are defined for every conceivable path through the logic.  Clearly, as the complexity of the FSM grows, defaults are easier to deal with.

[emece67]

.

[ehughes]

Where is your simulation test bench???  What is it telling you?

[nctnico]

It might be the FSM works correctly in a simulator. The behaviour the OP experiences is typical when you have paths with different delays inside an FPGA. As a rule of thumb a process should only have a clock (and maybe an asynchronous reset) in it's signal sensitivity list when dealing with an FPGA. Anything else is likely to result in latches. If you want to have asynchronous logic then latches can be avoided by making sure each 'if' statement has a (final) 'else' statement and each 'when' statement has a default. Both should assign all signals affected.

[SiliconWizard]

I don't particularly care for this style and currently see no specific justification for using it in VHDL with modern tools (if anyone has a good rationale, I'll be glad to read it). I have seen it used quite often by older people (so this is still likely to be the preferred style of many older teachers).

For all of us old geezers out here, can you post an example of the newer style preferred by younger people?

Just take the two process FSM (one clocked to stuff next_state into current_state) and one combinatorial where the actual work is done and rewrite it as one large clocked process.

Really, it doesn't matter if the combinatorial process is glitchy as long as everything settles out before the next clock because other processes aren't going to trigger off the output until the next clock anyway.  Even inside the large clocked process approach, signals still transition (and bounce around) following a clock transition.  As long as things settle out before the next clock, no harm, no foul.

Exactly and exactly. This FSM style is quite common as I said above, although I personally don't like it and see more negative points than positive, but that's just my opinion. I tend to favor clocked processes only, which overall look more readable to me, although "clockless" processes are certainly possible. There is no inherent problem if you use them wisely. Timing analysis will normally catch potential issues as long as it's in the same clock domain and remains inside the FPGA. (Also, don't underestimate modern synthesis tools. They do a lot more than translating your code "literally". Trying to "hand optimize" code on a very low level, based on assumptions of what the synthesis will spit out, is often fruitless. Some vendors release guides for writing RTL for their specific tools, with sometimes rather different approaches.)

Again, most issues with delays between paths inside an FPGA, as long as they are in the same clock domain, will be caught by timing analysis. One thing the OP didn't state either is whether their tool reported timing violations, so that would obviously be a first step.

But in the OP's case, there is an external signal. Even though it can be considered on the same clock domain, which the OP only made clear later on (seems reasonable for a slave FIFO, but remains to be checked in the FX2 manual), its inherent characteristics are completely out of the control of the timing analysis of the FPGA's tools and may have excessive delay/skew, hence the possible need of resynchronizing it IMO.

I strongly recommend using test benches as well, but it's usually not possible to catch this kind of issues with simulation.

[asmi]

I see two possibilities:
1. OP did not declare any clocks and therefore timing engine can't check if there are any timing violations. Always create clocks! Constraining your design is crucial if you need to communicate to any outside peripherals otherwise they might not behave as they should. There are cases where static constraints won't work and you'll have to ignore them, but these are pretty rare occasions and you'd better know what are you doing in these cases.
2. FSM depends on input(s) which have invalid voltage level(s) and this is what causes inconsistent behavior.

[asmi]

Exactly and exactly. This FSM style is quite common as I said above, although I personally don't like it and see more negative points than positive, but that's just my opinion.

Can you please list any negative points (besides "I don't like that approach" of course)?

Again, most issues with delays between paths inside an FPGA, as long as they are in the same clock domain, will be caught by timing analysis. One thing the OP didn't state either is whether their tool reported timing violations, so that would obviously be a first step.

Timing engine won't catch anything unless you properly constrain your design. At the very least all input clocks should be declared - this will catch all internal timing issues, though without IO delays it tend to cause placer to place logic around input clock/PLL, and so paths to/from IO tiles are not going to be optimal. At least this is the way Vivado engine works.

But in the OP's case, there is an external signal. Even though it can be considered on the same clock domain, which the OP only made clear later on (seems reasonable for a slave FIFO, but remains to be checked in the FX2 manual), its inherent characteristics are completely out of the control of the timing analysis of the FPGA's tools and may have excessive delay/skew, hence the possible need of resynchronizing it IMO.

If input signal is asynchronous to any clocks that exists in the system (a push button is a good example), the best approach is to just "double-flop" it. In Vivado there are a bunch of xpm_cdc_* primitives for all kinds of CDC issues.
But if it's synchronous to any clock in the system, the best way is to bring this clock into FPGA and create input constraints relative to this clock. This will ensure that data will be captured properly regardless of any PVT variations as long as constraints are accurate and reflect the actual system behavior. Like I said in the post above, there are situations when static capture is not possible, but these are high-speed interfaces (faster than approximately 400 MHz for A7/S7), and there are other methods to achieve capture at these speeds.

I strongly recommend using test benches as well, but it's usually not possible to catch this kind of issues with simulation.

This is exactly what timing simulation is for. Post-implementation timing simulation is going to show you exactly how FPGA is going to behave, provided that your test bench accurately reflects what's happening on the boundary of the chip. I'm yet to see a case when post-P&R timing sim would work but hardware would not.

[hamster_nz]

YES! That is exactly the behavior I was seeing, I changed something completely irrelevant somewhere else and it effected the FSM behavior! I thought I was going crazy. I already wasted to 2 days trying to figure it out 

Thank you, it's all starting to make sense.

One more question, so what I am trying to do is interface with a FX2LP USB chip in order to stream some data from an ADC (with the FPGA in the middle). The fx2lp is running in 'slave fifo' mode, where it acts like a dumb data bus.

So actually, the FLAGD should be synchronous I think? Perhaps the propagation delay from the IO to the FSM logic area is too high? If I look at the reference design it doesn't appear that they do any re-synchronization.

....

Correct me if I am wrong, but are you not also doing something funny with the clocking for clk_out?

It looks as if you are using the 180 degree clock on the ODDR to generate an output that is 180 degrees out of phase with the system's internal clock.

Have you set up your timing constraints correctly for this? Otherwise the output pins may think they have nearly the whole cycle to become stable, but the external clock raises half way though.

I suggest you try to design such that all the external signals are registered in the IOB blocks - to do this have no asynchronous logic between the last register and the output pins. You can add the IOB constraint to the pin definitions too.

Also flip the '1' and '0' on the data inputs to the ODDR, and then use the clk_0 not clk_180, so it is using the same clock as the data pins.

This should ensure that all the outputs (both data and falling edge of the clk_out) get changed on the rising edge of the system's internal clock cycle (including the falling edge on the clk_out signal) and then half way through the cycle the clk_out signal will rise.

This should give you the setup and hold times for the external devices, without getting too bogged down with timing constraints.

[AlessandroAU]

@hamster_nz (PS your website is fantastic!)

The code with the ODDR is not actually my code.

It's taken directly from the application note of the manufacturer.

https://www.cypress.com/documentation/application-notes/an61345-designing-ez-usb-fx2lp-slave-fifo-interface

Apparently, the purpose of the 180 phase shift is to make it easier to meet the setup and hold times https://community.cypress.com/thread/42082. From the timing diagrams I've seen they use the phase shifted clock.
I'll try your suggestion with the ODDR.

Their reference source is exactly as I posted with nothing fancy happening in the constraints file. I am only trying to run this at 20Mhz on a S6!. I've tried a few different VHDL examples for this chip interface I've found on the web but nothing seems to work properly

All I want is to expand on (the reference code) slightly so I have an independently clocked FIFO where I can clock in data on one side and have it streamed over USB transparently on the other, but this is kicking my butt. /rant.

I tried to rewrite my code as a true Moore style FSM and made sure no unintentional latching was happening. It seemed to work for a bit, but then randomly my FX2 chip would stop asserting its FLAGD signal  and my FSM would get stuck. Not sure if this is my fault for violating the bus timing or there is something else going on :\.

Moreover, I found some inconsistencies in the FX2 documentation between the official datasheet and the technical reference manual. One document shows that I need to toggle the SLWR (slave write) and the other calls for keeping it held low. (img attached)

Sorry for the rant, and thanks for the suggestions everyone. 

[legacy]

Moreover, I found some inconsistencies in the FX2 documentation between the official datasheet and the technical reference manual

I did a lot of documentation in the past, and I know how it works: too often guys re-use an old document as a template and changing here and there for describing a new product. This usually works,  but somethines "oops" ... and we have inconsistencies.

I suggest you to contact Cypress, and to tell them directly. This will have the benefit of an errata in the next pdf.

[NorthGuy]

All I want is to expand on (the reference code) slightly so I have an independently clocked FIFO where I can clock in data on one side and have it streamed over USB transparently on the other, but this is kicking my butt. /rant.

Use BRAM blocks for dual-clock FIFO. This will give you FIFO and clock domain crossing at the sane time. In S6 (unlike S7), BRAM doesn't have built-in FIFO configuration, but it is very easy to build a FIFO from BRAM blocks. If you don't want to do it manually, I'm sure there must be a "Wizard" to generate FIFOs.

[asmi]

I think this whole approach of downloading random code from the Internet and putting it into real hardware hoping it will somehow work is just wrong. I always treat such pieces of code as a reference so that if datasheet has some blind spots - I can look at the code to hopefully clear these up. So I suggest OP to actually read the code provided and understand how it works, instead of making random changes in the hope that it fixes something. Hope is a very bad engineering practice.