From: Richard on
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
From: Frank Buss on
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, fb(a)frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de
From: backhus on
On 6 Jul., 01:18, Richard <Richar...(a)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           : 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

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 = something, which is no legal
assignment. It's either := or <= .

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 = '1' then
state := READ0;
read_en <= '0';
register <= (otheres => '0');
elsif rising_edge(clk) then
-- calculate branches
case state is
when READ0 => state := READ1;
when READ1 => state := READ2;
when READ2 => state := READ3;
when READ3 => state := READ4;
when READ4 => state := DONE;
when DONE => state := READ0;
end case;
-- set outputs here
read_en <= '0'; -- default output assignment
case state is
when READ0 => null;
when READ1 => read_en <= 1;
register(7 downto 0 <=
byte_in;
when READ2 => read_en <= 1;
register(15 downto 8) <=
byte_in;
when READ3 => read_en <= 1;
register(23 downto 16) <=
byte_in;
when READ4 => read_en <= 1;
register(31 downto 24) <=
byte_in;
when DONE => 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 = '0'. Saves some
code lines.

See what works for you.

Have a nice synthesis
Eilert