Hi all,

I just came across a - what I think - must be a quite standard 
programming idiom for implementing an FSM. Essentially, I read
a byte value byte_in sequentially, and assign it to the appropriate
portions of signal register. As you can see, the read_en signal
must be HIGH in the state before the data in byte_in can be read.
Unfortunately, my FSM looks messy and I wonder if I could compress
the states a bit.

signal read_en           : std_logic;
signal byte_in           : std_logic_vector(7 downto 0);
signal register          : std_logic_vector(31 downto 0);

process (clk, reset)
    begin
         if (clk'event and clk = '1') then
           case state is
                when READ0 =>
                  read_en <= 1;
                  state = READ1;
                when READ1 =>
                  read_en <= 1;
                  register(7 downto 0) <= byte_in;
                  state = READ2;
                when READ2 =>
                  read_en <= 1;
                  register(15 downto 8) <= byte_in;
                  state = READ3;
                when READ3 =>
                  read_en <= 1;
                  register(23 downto 16) <= byte_in;
                  state = READ4;
                when READ4 =>
                  register(31 downto 24) <= byte_in;
                  state = DONE;
           end case;
end process;

Many thanks for your comments,
Rich

Richard wrote:

> I just came across a - what I think - must be a quite standard 
> programming idiom for implementing an FSM. Essentially, I read
> a byte value byte_in sequentially, and assign it to the appropriate
> portions of signal register. As you can see, the read_en signal
> must be HIGH in the state before the data in byte_in can be read.
> Unfortunately, my FSM looks messy and I wonder if I could compress
> the states a bit.

I would use a counter and shifting:

    signal counter: natural range 0 to 4 := 0;

    process(clk)
    begin
        if rising_edge(clk) then
		case state is
			when start =>
				read_en <= '1';
				counter <= 4;
				state <= readByte;
			when readByte =>
				if counter > 0 then
					register <= byte_in & register(31 downto 8);
					counter <= counter - 1;
				else
					read_en <= '0';
					state <= start;
				end if;
		end case;
        end if;
    end process;

If you don't do other things in your state machine, you even don't need a
state machine, just the counter, but this can lead to difficult to maintain
code, if you later want to add features.

-- 
Frank Buss, f...@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

On 6 Jul., 01:18, Richard
<Richar...@hotmail.com> wrote:
> Hi all,
>
> I just came across a - what I think - must be a quite standard
> programming idiom for implementing an FSM. Essentially, I read
> a byte value byte_in sequentially, and assign it to the appropriate
> portions of signal register. As you can see, the read_en signal
> must be HIGH in the state before the data in byte_in can be read.
> Unfortunately, my FSM looks messy and I wonder if I could compress
> the states a bit.
>
> signal read_en =A0 =A0 =A0 =A0 =A0 : std_logic;
> signal byte_in =A0 =A0 =A0 =A0 =A0 : std_logic_vector(7 downto 0);
> signal register =A0 =A0 =A0 =A0 =A0: std_logic_vector(31 downto 0);
>
> process (clk, reset)
> =A0 =A0 begin
> =A0 =A0 =A0 =A0 =A0if (clk'event and clk =3D '1') then
> =A0 =A0 =A0 =A0 =A0 =A0case state is
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 when READ0 =3D>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 read_en <=3D 1;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 state =3D READ1;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 when READ1 =3D>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 read_en <=3D 1;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 register(7 downto 0) <=3D byte_in;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 state =3D READ2;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 when READ2 =3D>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 read_en <=3D 1;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 register(15 downto 8) <=3D byte_in;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 state =3D READ3;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 when READ3 =3D>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 read_en <=3D 1;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 register(23 downto 16) <=3D byte_in;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 state =3D READ4;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 when READ4 =3D>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 register(31 downto 24) <=3D byte_in;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 state =3D DONE;
> =A0 =A0 =A0 =A0 =A0 =A0end case;
> end process;
>
> Many thanks for your comments,
> Rich

Hi Rich,
first, you have missed to implement the reset branch to set the
initial state at Pon.
The reset signal is in your sensitivity list already.
Then, you have no default assignments for read_en and register(n+7
downto n), so read_en will always be active.
Yo set it in the first state, without assigning the data in value
(probably to couteract pipelining effects), but you are not
deactivating in the last read,
And also, once you reached DONE happens ...what?
That state is not covered.
Besides, you wrote just state =3D something, which is no legal
assignment. It's either :=3D or <=3D .

For a cleaner implementation you may implement the state as a variable
and define the type in the process too.

signal read_en           : std_logic;
signal byte_in           : std_logic_vector(7 downto 0);
signal register          : std_logic_vector(31 downto 0);

process (clk, reset)
  type state_type : (READ0, READ1,READ2,READ3,DONE);
  variable state : state_type;
    begin
         if reset =3D '1' then
               state :=3D READ0;
               read_en <=3D '0';
               register <=3D  (otheres =3D> '0');
         elsif rising_edge(clk) then
           -- calculate branches
           case state is
                when READ0 =3D>   state :=3D READ1;
                when READ1 =3D>   state :=3D READ2;
                when READ2 =3D>   state :=3D READ3;
                when READ3 =3D>   state :=3D READ4;
                when READ4 =3D>   state :=3D DONE;
                when DONE  =3D>    state :=3D READ0;
           end case;
           -- set outputs here
            read_en <=3D '0';  -- default output assignment
           case state is
                when READ0 =3D>  null;
                when READ1 =3D>  read_en <=3D 1;
                                           register(7 downto 0 <=3D
byte_in;
                when READ2 =3D>  read_en <=3D 1;
                                           register(15 downto 8) <=3D
byte_in;
                when READ3 =3D>  read_en <=3D 1;
                                           register(23 downto 16) <=3D
byte_in;
                when READ4 =3D>  read_en <=3D 1;
                                           register(31 downto 24) <=3D
byte_in;
                when DONE  =3D>   null;
           end case;
end process;

Here the state valu changes imediately after an active cloch edge and
the outputs are sett according to the new states.
If the condition still applies that the read enable has to be set one
state before the assignment of the data_in value, you can easily
rearrange the output assignments.
Keep in mind that the reset conditions must met the output values for
the initial state.

You may also decide to use '1' as the default value for read_en. then
you have just to cover the few cases where read_en =3D '0'. Saves some
code lines.

See what works for you.

Have a nice synthesis
  Eilert