vhdl - How to Rewrite FSM not to use Latches

Question

I have an FSM and it works. The synthesizer, however, complains that there are latches for "acc_x", "acc_y", and "data_out" and I understand why and why it is bad. I have no idea, however, how to rewrite the FSM so the state-part goes to the clocked process. Any ideas where to start from? Here is the code of the FSM:

---

library IEEE;

use IEEE.std_logic_1164.all;
use IEEE.numeric_std.all;

entity storage is
    port
    (
        clk_in                           : in  std_logic;
        reset                            : in  std_logic;
        element_in                       : in  std_logic;
        data_in                          : in  signed(11 downto 0);
        addr                             : in  unsigned(9 downto 0);
        add                              : in  std_logic; -- add = '1' means add to RAM
                                                          -- add = '0' means write to RAM
        dump                             : in  std_logic;
        element_out                      : out std_logic;
        data_out                         : out signed(31 downto 0)
    );
end storage;

architecture rtl of storage is
    component bram is
    port
    (
        clk                              : in  std_logic;
        we                               : in  std_logic;
        en                               : in  std_logic;
        addr                             : in  unsigned(9 downto 0);
        di                               : in  signed(31 downto 0);
        do                               : out signed(31 downto 0)
    );
    end component bram;

    type state is (st_startwait, st_add, st_write);

    signal current_state                 : state := st_startwait;
    signal next_state                    : state := st_startwait;

    signal we                            : std_logic;
    signal en                            : std_logic;
    signal di                            : signed(31 downto 0);
    signal do                            : signed(31 downto 0);

    signal acc_x                         : signed(31 downto 0);
    signal acc_y                         : signed(31 downto 0);
begin
    ram : bram port map
    (
        clk  => clk_in,
        we   => we,
        en   => en,
        addr => addr,
        di   => di,
        do   => do  
    );

    process(clk_in)
    begin
        if rising_edge(clk_in) then
            if (reset = '1') then
                current_state           <= st_startwait;
            else
                current_state           <= next_state;
            end if;
        end if;
    end process;

    process(current_state, element_in, add, dump, data_in, do, acc_x, acc_y)
    begin
        element_out                     <= '0';

        en                              <= '1';
        we                              <= '0';

        di                              <= (others => '0');

        case current_state is
            when st_startwait =>          
                if (element_in = '1') then
                    acc_x               <= resize(data_in, acc_x'length);

                    next_state          <= st_add;
                else
                    next_state          <= st_startwait;
                end if;
            when st_add =>
                if (add = '1') then
                    acc_y               <= acc_x + do;
                else
                    acc_y               <= acc_x;
                end if;

                next_state              <= st_write;
            when st_write =>      
                if (dump = '1') then
                    data_out            <= acc_y;
                    element_out         <= '1';
                else
                    di                  <= acc_y;
                    we                  <= '1';
                end if;

                next_state              <= st_startwait;
        end case;
    end process;  
end rtl;

---

Answer 1

This is personal preference, but I think most people on here will agree with me on this one... do not use two processes to control your state machine. The whole previous_state next_state thing is total garbage in my opinion. It's really confusing and it tends to make latches - SURPRISE - You found that out. Try rewriting your state machine with a single clocked process and only one state machine signal.

Here's my attempt at rewriting your state machine. Note that I'm not sure the functionality that I have below will work for you. Simulate it to make sure it behaves the way you expect. For example the signal en is always tied to '1', not sure if you want that...

process (clk_in)
begin
  if rising_edge(clk_in) then
    element_out <= '0';
    en <= '1';                      -- this is set to 1 always?
    we <= '0';

    di <= (others => '0');

    case state is

      when st_startwait =>
        if (element_in = '1') then
          acc_x <= resize(data_in, acc_x'length);
          state <= st_add;
        end if;

      when st_add =>
        if (add = '1') then
          acc_y <= acc_x + do;
        else
          acc_y <= acc_x;
        end if;
        state <= st_write;

      when st_write =>
        if (dump = '1') then
          data_out    <= acc_y;
          element_out <= '1';
        else
          di <= acc_y;
          we <= '1';
        end if;
        state <= st_startwait;
    end case;
  end if;
end process;