timed state machines VHDL

[sahko123]

I'm struggling to get timed state machines working in reality, simulations seem to be ok though.

The basic premise is that there is a list of cases for a state variable and the conditions are checked after a few clock cycles decided by a separate timer/counter which is reset when the next state is triggered when simulating it everything works flawlessly but in fabric (on a max V dev kit) the logic falls apart and doesnt continue through all the states the way it should. sometimes it goes back a state where it shouldnt be possible to.

I believe this is something timing related so could anyone suggest a better way of going about doing this? For context this is for a multislope ADC.

[ejeffrey]

Is your synthesis tool giving you any warnings about clocks or timing?  Any inferred latches? Are you using wildcard sensitivity lists on all of your combinatoric logic?  Are you doing comparison against X values?  Are your external inputs stable?

If you follow the rules you shouldn't get simulation synthesis mismatch and the tools should generally warm you about problematic usage but unfortunately vendor IPs have so many meaningless warnings that it can be hard to notice ones that matter.

[SiliconWizard]

Maybe post your code?

[sahko123]

Here is a good chunk of the code excluding most declarations.



architecture behavioral of converter is


signal RP_COUNT:signed(23 downto 0):=(others=>'0');--runup counter
signal count_stage12:unsigned(7 downto 0):=(others=>'0');--10k rundown
signal count_stage34:unsigned(7 downto 0):=(others=>'0');--80k pos(4MSB) 640k neg(4LSB)
signal count_stage5:unsigned(7 downto 0):=(others=>'0');--5.12M pos
signal state:std_logic_vector(3 downto 0):=(others=>'0');
signal timer:std_logic_vector(3 downto 0):=(others=>'0');
signal timer_reset:std_logic:='0';
signal comp_hold:std_logic:='0';--flag for comparator input


component counter is
port(
count_out:out std_logic_vector(3 downto 0);
RST,CLK:in std_logic:='0'
);
end component;
begin

state_output_0<=not state(0);
state_output_1<=not state(1);
CLK_pass<=CLK;
time_counter:counter port map(count_out=>timer,CLK=>CLK,RST=>timer_reset);

--control registers
process(address,CLK) begin
   --if rising_edge(clk) then
   case address is
      when "0000" => data<=std_logic_vector(count_stage12);
      when "0001" => data<=std_logic_vector(count_stage34);
      when "0010" => data<=std_logic_vector(count_stage5);
      when "0011" => data<=std_logic_vector(RP_COUNT(7 downto 0));
      when "0100" => data<=std_logic_vector(RP_COUNT(15 downto );
      when "0101" => data<=std_logic_vector(RP_COUNT(23 downto 16));
      when "0110" => data<="01010101";
      when "0111" => data(3 downto 0)<=timer;data(7 downto 4)<=(others=>'0');
      when "1000" => data(3 downto 0)<=state;data(7 downto 4)<=(others=>'0');
      --when others => data<=(others=>'Z');
      when others => data<="01010101";
      
   end case;
   --end if;
end process;

--conversion process



process(CLK) begin

case state is

-----------------------------------------------initial state
when "0000" =>
SW_sample<='0';
SW10K1<='0';
SW10K2<='0';
SW80K3<='0';
SW640K4<='0';
SW5120K5<='0';
timer_reset<='0';
if CONV='1' then --if conversion triggered
state<="0001";
timer_reset<='1';
end if;
-----------------------------------------------pause before conversion
when "0001"=>
timer_reset<='0';
RP_COUNT<=(others=>'0');
count_stage12<=(others=>'0');
count_stage34<=(others=>'0');
count_stage5<=(others=>'0');
SW_short<='1';
if timer="0101" then
state<="0010";
timer_reset<='1';
end if;
-----------------------------------------------t1 sample
when "0010"=>
SW_short<='0';
timer_reset<='0';
SW_sample<='1';
comp_hold<=comp;
if timer="0111" then
timer_reset<='1';
state<="0011";
end if;
-----------------------------------------------runup
when "0011"=>
timer_reset<='0';

if comp_hold='0' then
SW10K1<='1';--positive ramp
SW10K2<='0';
else
SW10K1<='0';--negative ramp
SW10K2<='1';
end if;

if timer="1010" then --if timer=10
if comp_hold='1' then
RP_COUNT<=RP_COUNT+1;
comp_hold<=comp;
else
RP_COUNT<=RP_COUNT-1;
comp_hold<=comp;
end if;
timer_reset<='1';
state<="0100";
end if;
-----------------------------------------------recharge
when "0100"=> --recharge
timer_reset<='0';
SW10K1<='0';
SW10K2<='0';
if timer="0011" then
timer_reset<='1';
if CONV='1' then
state<="0011";
else
state<="0101";
end if;
end if;
-----------------------------------------------rundown 10k pos
when "0101"=>
SW10K2<='0';
if CLK<='1' then
comp_hold<=comp;
end if;

if comp_hold=not comp then
SW10K1<='1';--positive ramp
count_stage12<=count_stage12+1;
else
state<="0110";
end if;

[nctnico]

You need to make your logic synchronous. Asynchronous logic doesn't work very well (usually not at all) in an FPGA.

Undo this comment:
--if rising_edge(clk) then

 and make all your other logic synchronous as well if there is an 'if then' clause

[SiliconWizard]

Your first process is essentially a MUX, and doesn't necessarily need to be synchronous. Depends on your goal. Making it asynchronous will make it run at a lower Fmax, but making it synchronous will add one clock delay. Depends on what you want to achieve here. I guess you commented out the 'if rising_edge(clk) then' due to this latency not being desirable here?

The real problem stands with your second process, the FSM. It is clockless (asynchronous), and changes its own state based on signals other than clocks.
Such as 'if timer="0101" then state<="0010";'

Not the way a FSM should be done. It won't work properly. Change states using a single clock. For testing against a "counter", you can still use this kind of above 'if', but add a 'if rising_edge(clk) then' at the beginning of the FSM process.

Of course, I suspect you did that because you had difficulties dealing with the 1-clock latency introduced when clocking your processes.
That's really something you need to wrap your head around, as it is absolutely essential for digital design.

[ejeffrey]

What they said.  Go back and look at your synthesis logs and you should see warning messages about inferring latches.  On an FPGA/CPLD you should 99.9% of the time regard that as a hard error.

The cause is specifically lines that look like:

when "0111" => data(3 downto 0)<=timer;data(7 downto 4)<=(others=>'0');

This creates a circular logic loop (data depends on data), without an intervening clocked register.  That gets converted to a latch, and is likely to cause synthesis / simulation mismatch.

[sahko123]

as long as having the whole thing clocked delays everything by a clock cycle there is no issue.

[tggzzz]

as long as having the whole thing clocked delays everything by a clock cycle there is no issue.

You should look up and understand the differences between Moore FSMs and Mealey FSMs.

[sahko123]

Ive got the thing working now. I just have a question.

If for example a state machine is within a clocked process (if rising_edge(clk) then.....etc) if the state machine switches states does that happen within the same clock cycle? or does the case statement switchover within the same clock cycle?

[ejeffrey]

Ive got the thing working now. I just have a question.

If for example a state machine is within a clocked process (if rising_edge(clk) then.....etc) if the state machine switches states does that happen within the same clock cycle? or does the case statement switchover within the same clock cycle?

All of the assignment statements within the if rising_edge block take effect on each clock cycle.

[SiliconWizard]

Ive got the thing working now. I just have a question.

If for example a state machine is within a clocked process (if rising_edge(clk) then.....etc) if the state machine switches states does that happen within the same clock cycle? or does the case statement switchover within the same clock cycle?

That's something pretty fundamental you need to master when dealing with sequential logic.

Anything done on a clock edge will obviously not be effective instantly, electronics is not magic. What you are guaranteed (as long as timing closure is met, which timing analysis will tell you) is that any signal change scheduled on a clock edge will be done and stable on the next corresponding edge.

So, that's sequential and everything happens in a sequence. Otherwise clocking would not have any effect, would it?

[rstofer]

You might get some help from this tutorial:

https://vhdlwhiz.com/finite-state-machine/

Note the default value assignments around line 45 of the final code.  Every output must be defined under every condition of if-else, state, etc.  Otherwise a latch has to hold the previous value of every output and this is problematic.

By having default values, at each state we only need to worry about signals that may change, not every signal being controlled.  This get to be a really big deal when state machines get large.

The final value of a signal is determined by the last expression evaluated (reading top to bottom).  So, some signal is arbitrarily assigned a default (but logically chosen) value and if the output is to be different than the default, it is changed inside the case statements.

You may find VHDL Whiz to be a particularly helpful site.  The free stuff is excellent and the paid stuff has proven to be worth every penny.

[hamster_nz]

Did you look at your synthesis messages?

(I've adjusted the layout of the code a bit, and removed the comments)

Code: [Select]

process(address, CLK) begin
   case address is
      when "0000" => data<=std_logic_vector(count_stage12);
      when "0001" => data<=std_logic_vector(count_stage34);
      when "0010" => data<=std_logic_vector(count_stage5);
      when "0011" => data<=std_logic_vector(RP_COUNT(7 downto 0));
      when "0100" => data<=std_logic_vector(RP_COUNT(15 downto 8));
      when "0101" => data<=std_logic_vector(RP_COUNT(23 downto 16));
      when "0110" => data<="01010101";
      when "0111" => data(3 downto 0)<=timer;
                               data(7 downto 4)<=(others=>'0');
      when "1000"  => data(3 downto 0)<=state;
                                data(7 downto 4)<=(others=>'0');
      when others  =>  data<="01010101";
     
   end case;
end process;

You are missing "timer", 'data', "count_stage12", "count_stage34", "count_stage5" and "RP_COUNT" from your sensitivity list. This will definitely cause a mismatch between simulation and the implemented design.

There will have been a warning about it., along with others.

[langwadt]

You might get some help from this tutorial:

https://vhdlwhiz.com/finite-state-machine/

Note the default value assignments around line 45 of the final code.  Every output must be defined under every condition of if-else, state, etc.  Otherwise a latch has to hold the previous value of every output and this is problematic.

By having default values, at each state we only need to worry about signals that may change, not every signal being controlled.  This get to be a really big deal when state machines get large.

The final value of a signal is determined by the last expression evaluated (reading top to bottom).  So, some signal is arbitrarily assigned a default (but logically chosen) value and if the output is to be different than the default, it is changed inside the case statements.

I don't see where you think you get a latch without defaults, defaults are often convenient but you could get the same by writing out the signals that change where they change.

default unless assigned something else is effectively the same as don't change unless assigned something else

[SiliconWizard]

You don't get unwanted 'latches' if everything happens within a clocked process, whether you change states or not. You get registers.

[hamster_nz]

Hi sahko123! I've reformatted the code into something that I can analyze, and added my comments on lines with "----":

Code: [Select]

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;

entity converter is
    Port (
        clk            : in  STD_LOGIC;
        address        : in  std_logic_vector(3 downto 0);
        data           : out std_logic_vector(7 downto 0);
        CLK_pass       : out STD_LOGIC;
        state_output_0 : out STD_LOGIC;
        state_output_1 : out STD_LOGIC;
        comp           : in  STD_LOGIC;
       
        CONV           : in  STD_LOGIC;
        SW_sample      : out STD_LOGIC;
        SW_short       : out STD_LOGIC;
        SW10K1         : out STD_LOGIC;
        SW10K2         : out STD_LOGIC;
        SW80K3         : out STD_LOGIC;
        SW640K4        : out STD_LOGIC;
        SW5120K5       : out STD_LOGIC
    );
end converter;

architecture behavioral of converter is
    signal RP_COUNT      : signed(23 downto 0)          := (others=>'0');--runup counter
    signal count_stage12 : unsigned(7 downto 0)         := (others=>'0');--10k rundown
    signal count_stage34 : unsigned(7 downto 0)         := (others=>'0');--80k pos(4MSB) 640k neg(4LSB)
    signal count_stage5  : unsigned(7 downto 0)         := (others=>'0');--5.12M pos
   
---- 'state' would be better as an enumerated type
----    type t_state is (s_reset, s_armed, s_counting, s_whatever...);     
----    signal state : t_state := s_reset;
    signal state         : std_logic_vector(3 downto 0) := (others=>'0');
    signal timer         : std_logic_vector(3 downto 0) := (others=>'0');
    signal timer_reset   : std_logic                    := '0';
    signal comp_hold     : std_logic                    := '0';--flag for comparator input

    component counter is
    port(
        count_out : out std_logic_vector(3 downto 0);
        RST       : in  std_logic:='0';
        CLK       : in  std_logic:='0'
    );
    end component;

begin

   state_output_0 <= not state(0);
   state_output_1 <= not state(1);
   
---- This is a bad away to get clocks to an I/O pin - better to use a DDR output with the data pins connected to '1' and '0'
   CLK_pass       <=CLK;

---- You would most likely be better off having this logic internal to this process.
time_counter:counter port map (
        count_out => timer,
        CLK       => CLK,
        RST       => timer_reset
    );

--control registers
process(address,CLK) begin
----  Missing lots off the sensitivity list
   --if rising_edge(clk) then
   case address is
      when "0000" => data<=std_logic_vector(count_stage12);
      when "0001" => data<=std_logic_vector(count_stage34);
      when "0010" => data<=std_logic_vector(count_stage5);
      when "0011" => data<=std_logic_vector(RP_COUNT(7 downto 0));
      when "0100" => data<=std_logic_vector(RP_COUNT(15 downto 8));
      when "0101" => data<=std_logic_vector(RP_COUNT(23 downto 16));
      when "0110" => data<="01010101";
      when "0111" => data(3 downto 0)<=timer;data(7 downto 4)<=(others=>'0');
      when "1000" => data(3 downto 0)<=state;data(7 downto 4)<=(others=>'0');
      --when others => data<=(others=>'Z');
      when others => data<="01010101";
     
   end case;
   --end if;
end process;

--conversion process

process(CLK) begin
----  This should be a clocked process
    case state is
         -----------------------------------------------initial state
         when "0000" =>
             SW_sample<='0';
             SW10K1<='0';
             SW10K2<='0';
             SW80K3<='0';
             SW640K4<='0';
             SW5120K5<='0';
             timer_reset<='0';
             if CONV='1' then --if conversion triggered
                 state<="0001";
                 timer_reset<='1';
             end if;
         -----------------------------------------------pause before conversion
         when "0001"=>
             timer_reset<='0';
             RP_COUNT<=(others=>'0');
             count_stage12<=(others=>'0');
             count_stage34<=(others=>'0');
             count_stage5<=(others=>'0');
             SW_short<='1';
             if timer="0101" then
                 state<="0010";
                 timer_reset<='1';
             end if;
         -----------------------------------------------t1 sample
         when "0010"=>
             SW_short<='0';
             timer_reset<='0';
             SW_sample<='1';
             comp_hold<=comp;
             if timer="0111" then
                 timer_reset<='1';
                 state<="0011";
             end if;
         -----------------------------------------------runup
         when "0011"=>
             timer_reset<='0';

             if comp_hold='0' then
             SW10K1<='1';--positive ramp
             SW10K2<='0';
             else
             SW10K1<='0';--negative ramp
             SW10K2<='1';
             end if;

             if timer="1010" then --if timer=10
                 if comp_hold='1' then
                     RP_COUNT<=RP_COUNT+1;
                     comp_hold<=comp;
                 else
                     RP_COUNT<=RP_COUNT-1;
                     comp_hold<=comp;
                  end if;
                 timer_reset<='1';
                 state<="0100";
             end if;
         -----------------------------------------------recharge
         when "0100"=> --recharge
             timer_reset<='0';
             SW10K1<='0';
             SW10K2<='0';
             if timer="0011" then
                 timer_reset<='1';
                 if CONV='1' then
                     state<="0011";
                 else
                     state<="0101";
                 end if;
             end if;
         -----------------------------------------------rundown 10k pos
         when "0101"=>
             SW10K2<='0';
             if CLK<='1' then
                 comp_hold<=comp;
             end if;
             
             if comp_hold=not comp then
                 SW10K1<='1';--positive ramp
                 count_stage12<=count_stage12+1;
             else
                 state<="0110";
             end if;
    end case;
end process;

end behavioral;


[sahko123]

Ive got the thing "working" now but it has some weirdness in terms of working consistently it seems like im having timing issues somewhere or may be that im changing and reading the same variable at the same clock edge. are there ways to delay an if statement to allow for some setup time?

im also getting a lot of warnings in the timing analyser which may either be due to actual issues or me not setting it up or something

[langwadt]

Ive got the thing "working" now but it has some weirdness in terms of working consistently it seems like im having timing issues somewhere or may be that im changing and reading the same variable at the same clock edge. are there ways to delay an if statement to allow for some setup time?

you need to take a step a back and realize that you are describing hardware is not writing software, that is important

you logic will constantly be "reacting" to what signals are right _before_ the clock edge and the signals will change _after_ the clock edge

[tggzzz]

Ive got the thing "working" now but it has some weirdness in terms of working consistently it seems like im having timing issues somewhere or may be that im changing and reading the same variable at the same clock edge. are there ways to delay an if statement to allow for some setup time?

im also getting a lot of warnings in the timing analyser which may either be due to actual issues or me not setting it up or something

Have a look at wikipedia to understand the differences between Moore and Mealy FSMs. Moore FSMs are preferable, unless there is a good reason for using Mealy.

Unless there is a requirement otherwise, you should use the "2 Process" Design Pattern, e.g. https://vhdlwhiz.com/n-process-state-machine/

Benefits:

• encourages "good thinking" leading to a good design
• is easy for you to translate the design into an implementation
• the implementation is easily modified when (not if) that becomes necessary (see point 5)
• is easy for the tools to synthesise into LUTs and registers
• is easy to see where any timing problems are occurring (see point 3)

And do realise you are writing wires and gates, not CPU instructions. Everything happens everywhere, all at once - except where you instruct the tools to control the chaos by using clocks/registers.

[sahko123]

Ive got the thing "working" now but it has some weirdness in terms of working consistently it seems like im having timing issues somewhere or may be that im changing and reading the same variable at the same clock edge. are there ways to delay an if statement to allow for some setup time?

you need to take a step a back and realize that you are describing hardware is not writing software, that is important

you logic will constantly be "reacting" to what signals are right _before_ the clock edge and the signals will change _after_ the clock edge

The issue i believe is that the states of signals change on the clock edge and are read on that clock edge

[langwadt]

Ive got the thing "working" now but it has some weirdness in terms of working consistently it seems like im having timing issues somewhere or may be that im changing and reading the same variable at the same clock edge. are there ways to delay an if statement to allow for some setup time?

you need to take a step a back and realize that you are describing hardware is not writing software, that is important

you logic will constantly be "reacting" to what signals are right _before_ the clock edge and the signals will change _after_ the clock edge

The issue i believe is that the states of signals change on the clock edge and are read on that clock edge

with everything that is clocked, signals changes _after_ the edge based on what the signals where _before_ the edge

[sahko123]

so should i bring my timer instance into the process? because atm i have it as a component with clk and rst input and a 4 bit output. So its potential thatsetup and hold stuff isnt setup correctly because the software doesnt know it needs to.

The timer is basically just a counter to allow for states to last up to 16 clock cycles

[tggzzz]

Ive got the thing "working" now but it has some weirdness in terms of working consistently it seems like im having timing issues somewhere or may be that im changing and reading the same variable at the same clock edge. are there ways to delay an if statement to allow for some setup time?

you need to take a step a back and realize that you are describing hardware is not writing software, that is important

you logic will constantly be "reacting" to what signals are right _before_ the clock edge and the signals will change _after_ the clock edge

The issue i believe is that the states of signals change on the clock edge and are read on that clock edge

Your design must obey the flip flops setup and hold times. If it does not then the flip flops might enter a metastable state. That would be reported as a timing violation. That is standard stuff that began to be understood in 1974, and was common knowledge 40 years ago.

To avoid that, all inputs that do not change synchronously do the the clock (external inputs or inputs from a different on-chip clock domain) must pass through a synchroniser before entering the FSM. It will also be necessary to add timing rules that tell the synthesis and DRC tools that the inputs are not synchronous.

Read you design tool's application notes to understand how to instantiate a synchroniser and define the timing rules.

[hamster_nz]

FWIW... there's two parts to the problem caused by asynchronous input signals:

1. If your design uses an unsynchronized signal in multiple places, there is a timing delay as signals propagate along the 'wire' in the FPGA. The logic that is electrically closer to the pin may see a different signal to logic further away. This causes lots of failures - e.g. if your clock rate is 100MHz, and the routing has a connection 2ns away from the pin and another 7ns from the pin, then there is a 50% chance that any transition will result in a logical inconsistency (i.e. an error).  You can usually recreate these 'random' failures with a few transitions of the input signal.

The solution to this level of issues is a basic flip-flop on any signal before it is used elsewhere in the design.

2. Metastability. If the setup and hold for a flipflop is violated (due to a data signal edge occurring very close to a clock edge) then the FF may go into a 'metastable' state, where it doesn't cleanly switch states. The chances of this happening for a random transition is very small but not zero - one in a million or one in a billion small. Are very, very unlikely to see metastability with a manual pushbutton.

The solution to this is to have two or more flipflops in a row. If the first flipflop goes metastable, hopefully it will snap out of it in time for the second flipflop to get a clean signal. The more FFs you have, the less chance you have of failure.

The faster the async input signal changes, and the faster the design's clock rates are, the more FFs you need to ensure the chances of failure approach zero.