VHDL: Default values in a Finite State Machine

Question

I am trying to make a finite state machine that switches states based on serial input. I need some explanation regarding how my code is executed. I read in a textbook that the section in the process that I have marked "DEFAULT VALUES" is where I should put my default values. However, my signals seem to take on these values whenever I switch states. For example, I set state_next to idle as the default value. Doing this has caused the FSM to continue to jump to idle from other states for no reason.

My other question is clarification in how the whole process for the FSM is executed. When I move from one state to another, is the section before the case statement (the part marked DEFAULT VALUES) supposed to be executed? Or is it only executed if I move from some later state back to idle? When is the DEFAULT VALUES section supposed to be executed?

My code is shown below. Please refer to the "Next-State Logic" section.

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

entity delay_incrementor is
     generic ( delay_ports : natural := 3;
               width_ports : natural := 3
                );
    Port ( clk,reset: in STD_LOGIC;
              update : in STD_LOGIC;
              in_data : in  STD_LOGIC_VECTOR (7 downto 0);
              led : out STD_LOGIC_VECTOR (2 downto 0);
              out_data : out  STD_LOGIC_VECTOR (7 downto 0);
              d_big,d_mini,d_opo : inout  STD_LOGIC_VECTOR (25 downto 0);
              w_big,w_mini,w_opo : inout STD_LOGIC_VECTOR (25 downto 0));
end delay_incrementor;

architecture fsm_arch of delay_incrementor is
    type state_type is (idle,channel,d_or_w,delay_channel,delay_channel_inc,width_channel,width_channel_inc);
    type delay_file_type is array (delay_ports-1 downto 0) of std_logic_vector (25 downto 0);
    type width_file_type is array(width_ports-1 downto 0) of std_logic_vector (25 downto 0);
    signal d_reg,d_next,d_succ: delay_file_type;
    signal w_reg,w_next,w_succ: width_file_type;
    signal state_reg,state_next: state_type;
    signal which_channel,which_channel_next: natural;
begin
--------------------------------------
--State Register
--------------------------------------
process(clk,reset)
begin
if reset='1' then
    state_reg <= idle;
    d_reg <= (others => (others => '0'));
    w_reg <= (others => (others => '0'));
    which_channel <= 0;
elsif (clk='1' and clk'event) then
    state_reg <= state_next;
    d_reg <= d_next;
    w_reg <= w_next;
    which_channel <= which_channel_next;
end if;
end process;
--------------------------------------
--Next-State Logic/Output Logic
--------------------------------------
process(state_reg,in_data,d_reg,w_reg,which_channel)
begin
    state_next <= idle; --DEFAULT VALUES
    d_succ <= d_reg;
    w_succ <= w_reg;
    which_channel_next <= 0;
    case state_reg is
        when idle =>
            if in_data = "01100011" then --"c"
                state_next <= channel;
                which_channel_next <= 0;
            end if;
        when channel =>
            if (48 <= unsigned(in_data)) and (unsigned(in_data)<= 57) then
                which_channel_next <= (to_integer(unsigned(in_data))-48);
                state_next <= d_or_w;
            elsif in_data = "00100011" then --"#"
                state_next <= idle;
                which_channel_next <= which_channel;
            end if;
        when d_or_w =>
            if in_data = "01100100" then --"d"
                state_next <= delay_channel;
            elsif in_data = "01110111" then --"w"
                state_next <= width_channel;
            elsif in_data = "00100011" then --"#"
                state_next <= idle;
            end if;
        when delay_channel =>
            if in_data = "01101001" then --"i"
                state_next <= delay_channel_inc;
            elsif in_data = "00100011" then --"#"
                state_next <= idle;
            end if;
        when delay_channel_inc =>
            if in_data = "01110101" then --"u"
                d_succ(which_channel) <= std_logic_vector(unsigned(d_reg(which_channel))+250);
            elsif in_data = "01100100" then --"d"
                d_succ(which_channel) <= std_logic_vector(unsigned(d_reg(which_channel))-250);
            else
                d_succ(which_channel) <= d_reg(which_channel);
            end if;
            if in_data = "00100011" then --"#"
                state_next <= idle;
            end if;
        when width_channel =>
            if in_data = "01101001" then --"i"
                state_next <= width_channel_inc;
            elsif in_data = "00100011" then --"#"
                state_next <= idle;
            end if;
        when width_channel_inc =>
            if in_data = "01110101" then --"u"
                w_succ(which_channel) <= std_logic_vector(unsigned(w_reg(which_channel))+250);
            elsif in_data = "01100100" then --"d"
                w_succ(which_channel) <= std_logic_vector(unsigned(w_reg(which_channel))-250);
            else
                w_succ(which_channel) <= w_reg(which_channel);
            end if;
            if in_data = "00100011" then --"#"
                state_next <= idle;
            end if;
    end case;
end process;
process(update,d_reg,w_reg,reset)
begin
if reset='1' then
    d_next <= (others => (others =>'0'));
    w_next <= (others => (others =>'0'));
elsif (update'event and update='1') then
    d_next <= d_succ;
    w_next <= w_succ;
else
    d_next <= d_reg;
    w_next <= w_reg;
end if;
end process;
--------------------------------------
--Output Logic
--------------------------------------
d_big <= d_reg(0);
d_mini <= d_reg(1);
d_opo <= d_reg(2);
w_big <= w_reg(0);
w_mini <= w_reg(1);
w_opo <= w_reg(2);
end fsm_arch;

Your code example is incomplete without declarations. – Jul 21 '15 at 19:25 — , Jul 21 '15 at 19:25

score 2 · Accepted Answer · 2015-07-24T10:02:09.060

Here's an alternative version in single process style.

As you surmised, the "default values" were resetting things including State any time you did not explicitly set values : this is probably unwanted, and I have made some transitions to idle explicit (in *_channel_inc) instead. I've guessed a bit at the semantics here : what happens if a character is present on InData for more than one cycle, or if a different character comes in? but the logic should be easy to change.

A few notes:

Any time you write x <= std_logic_vector(unsigned(y)+250); something is probably the wrong type. This means you're fighting the type system instead of using it. I've made d_reg etc arrays of Unsigned, and banished the type conversions to the outputs, out of the program flow. X <= Y + 250; is clearer and simpler.
Those ports should be Out, not InOut - and I would negotiate to make them Unsigned, further simplifying and clarifying the design.
Spaces do not consume gates.
One advantages of the single process style is that there are fewer signals simply used to interconnect processes, updated at difficult-to-determine times. Here, "update" is used on the same clock edge as everything else.
Magic numbers... if in_data = "01101001" then --"i" versus if in_data = to_slv('i') then . The latter is easier to read and MUCH easier to write (and get correct). If you don't like the to_slv helper function (which IS synthesisable), at least use named constants with names reflecting the characters instead. This also makes it easy to tell that this code is processing ASCII (sorry, Latin-1).
EDIT : to make the SM less cluttered, I have locally overloaded = to allow direct comparison between a slv and a character : it's a good idea keep this kind of trick localised to where it is needed. These functions could even be declared in the process itself.
Prefer rising_edge(clk) to the obsolete (clk='1' and clk'event) style.

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

entity delay_increment is
    generic ( delay_ports : natural := 3;
              width_ports : natural := 3
                );
    Port ( clk,reset: in STD_LOGIC;
              update : in STD_LOGIC;
              in_data : in  STD_LOGIC_VECTOR (7 downto 0);
              led : out STD_LOGIC_VECTOR (2 downto 0);
              out_data : out  STD_LOGIC_VECTOR (7 downto 0);
              d_big,d_mini,d_opo : out STD_LOGIC_VECTOR (25 downto 0);
              w_big,w_mini,w_opo : out STD_LOGIC_VECTOR (25 downto 0));
end delay_increment;

architecture fsm_arch of delay_increment is
    type state_type is (idle,channel,d_or_w,delay_channel,delay_channel_inc,width_channel,width_channel_inc);
    type delay_file_type is array (delay_ports-1 downto 0) of unsigned (25 downto 0);
    type width_file_type is array(width_ports-1 downto 0) of unsigned (25 downto 0);
    signal d_reg, d_succ: delay_file_type;
    signal w_reg, w_succ: width_file_type;
    signal state_reg : state_type;
    signal which_channel : natural;
    function to_slv(C : Character) return STD_LOGIC_VECTOR is
    begin
       return STD_LOGIC_VECTOR(to_unsigned(Character'pos(c),8));
    end to_slv;
    function "=" (A : STD_LOGIC_VECTOR(7 downto 0); B : Character) 
       return boolean is
    begin
       return (A = to_slv(B));
    end function "+";

begin
--------------------------------------
--State Machine
--------------------------------------
process(clk,reset)
begin
if reset='1' then
    state_reg <= idle;
    d_reg <= (others => (others => '0'));
    w_reg <= (others => (others => '0'));
    which_channel <= 0;
elsif rising_edge(clk) then
    -- default actions ... update if asked
    if update ='1' then
       d_reg <= d_succ;
       w_reg <= w_succ;
    end if;
    case state_reg is
        when idle =>
            if in_data = 'c' then 
                state_reg <= channel;
                which_channel <= 0;
            end if;
        when channel =>
            if (Character'pos('0') <= unsigned(in_data)) and (unsigned(in_data)<= Character'pos('9')) then
                which_channel <= (to_integer(unsigned(in_data)) - Character'pos('0'));
                state_reg <= d_or_w;
            elsif in_data = '#' then 
                state_reg <= idle;
                which_channel <= which_channel;
            end if;
        when d_or_w =>
            if in_data = 'd' then 
                state_reg <= delay_channel;
            elsif in_data = 'w' then 
                state_reg <= width_channel;
            elsif in_data = '#' then 
                state_reg <= idle;
            end if;
        when delay_channel =>
            if in_data = 'i' then 
                state_reg <= delay_channel_inc;
            elsif in_data = '#' then 
                state_reg <= idle;
            end if;
        when delay_channel_inc =>
            if in_data = 'u' then 
                d_succ(which_channel) <= d_reg(which_channel) + 250;
                state_reg <= idle;
            elsif in_data = 'd' then 
                d_succ(which_channel) <= d_reg(which_channel) - 250;
                state_reg <= idle;
            else
                d_succ(which_channel) <= d_reg(which_channel); -- wait for any of 'u', 'd', '#'
            end if;
            if in_data = '#' then 
                state_reg <= idle;
            end if;
        when width_channel =>
            if in_data = 'i' then 
                state_reg <= width_channel_inc;
            elsif in_data = '#' then 
                state_reg <= idle;
            end if;
        when width_channel_inc =>
            if in_data = 'u' then 
                w_succ(which_channel) <= w_reg(which_channel) + 250;
                state_reg <= idle;
            elsif in_data = 'd' then 
                w_succ(which_channel) <= w_reg(which_channel) - 250;
                state_reg <= idle;
            else
                w_succ(which_channel) <= w_reg(which_channel); -- wait for any of 'u', 'd', '#'
            end if;
            if in_data = '#' then 
                state_reg <= idle;
            end if;
    end case;
end if;
end process;

--------------------------------------
--Output Logic
--------------------------------------
d_big  <= std_logic_vector(d_reg(0));
d_mini <= std_logic_vector(d_reg(1));
d_opo  <= std_logic_vector(d_reg(2));
w_big  <= std_logic_vector(w_reg(0));
w_mini <= std_logic_vector(w_reg(1));
w_opo  <= std_logic_vector(w_reg(2));
end fsm_arch;

Is there a problem with trying to do too much in one rising clock edge? What if I were to add 20 more states into the process? — Eugene Wu, Jul 24 '15 at 15:33
(1) Depends. At some point, doing too much will consume more FPGA resources (gates, etc) than you have... However note that a state machine is only in one state at a time so what it can do on any clock edge is limited. (2) The process will work; however it might be too difficult to understand, maintain and debug. I recommend not letting state machines grow to the point where you can't understand them. I have seen some SMs with many states that had a simple structure (several chains of simple states) and were easy to maintain and understand - so won't suggest any firm rules. — , Jul 24 '15 at 20:37

Paebbels · Answer 2 · 2015-07-21T21:05:46.920

A process is evaluated everytime when one of the listed signals has changed. Therefore this list is called 'sensitivity list'.

There are 2 types of processes:
- sequential ones (with a clock signal) and
- combinatorical (just plain logic).

The first type needs only the clock signal in the sensitivity list, the latter one needs every right handside signal, otherwise simulation will show other results than real hardware.

So everytime an input signal changes (<signal>'event = true) the process is evaluated from begin ... end process.

So, regarding your DEFAULT section, every signal gets a default value and it's impossible to produce latches with this coding style. Usually state_next is not set to idle. It's set to state_reg

free translated: stay in the current state until told otherwise

Your code:

...
elsif (update'event and update='1') then
  d_next <= d_succ;
  w_next <= w_succ;
else
  d_next <= d_reg;
  w_next <= w_reg;
end if;

can not be synthesised. I assume that update is no real clock signal, so it shouldn't be used in a rising_edge expression. Secondly, when is the else condition true?

Solution: You need an enable signal for your register.

Addendum:

process(clk,reset)
begin
  if reset='1' then
    state_reg <= idle;
    d_reg <= (others => (others => '0'));
    w_reg <= (others => (others => '0'));
    which_channel <= 0;
  elsif (clk='1' and clk'event) then
    state_reg <= state_next;
    which_channel <= which_channel_next;
    if update = '1' then
      d_reg <= d_next;
      w_reg <= w_next;
     end if;
  end if;
end process;

Thanks, that clears things up. Regarding that segment of my code, I was trying to use `update` as an enable signal. From what I have tested so far, it seems that it somewhat works, and that the else condition is true whenever the update signal is not changing. How could I update `d_next` and `w_next` without such an update signal? — Eugene Wu, Jul 21 '15 at 20:30
@eugenewu An enable signal is embedded into the then branch of a rising_edge condition. You can delete your last process. — Paebbels, Jul 21 '15 at 21:07

VHDL: Default values in a Finite State Machine

2 Answers2