State Machine using Case getting unexpected result

Question

I am trying to write a very simple state machine that implements a combinational lock.

The code is: Switch1 -> Switch2 -> Switch3 -> Switch4

I realize that it is Switch 7, 6, 5, 4 accordingly in the code.

If it is not done in that order then it gives the error(incorrect) state.

The problem I am having is that even though state is state_start (as I see it on the LEDs) it will not change to state_1_right and instead will just pump out the error_state. I know it does go into that if statement because I changed the else to state <= "00001010"; and it displays that.

What am I doing wrong? I do not see any error in my logic (unless there is some weird switch bounce).

Here is the code I am trying now:

entity CombinationLockFSM is
    Port(
        Switches: in std_logic_vector(7 downto 0);
        LEDs: out std_logic_vector(7 downto 0)
    );
end CombinationLockFSM;

architecture Behavioral of CombinationLockFSM is
    constant state_start: std_logic_vector(7 downto 0) := "10000000";
    constant state_1_right: std_logic_vector(7 downto 0) := "01000000";
    constant state_2_right: std_logic_vector(7 downto 0) := "00100000";
    constant state_3_right: std_logic_vector(7 downto 0) := "00010000";
    constant state_error: std_logic_vector(7 downto 0) := "00001111";

    signal state: std_logic_vector(7 downto 0) := (others => '0');
begin

    LEDs <= state;

    process(Switches)
    begin
        case Switches is
            when "00000000" => 
                state <= state_start;
            when "10000000" => 
                if state = state_start then
                    state <= state_1_right;
                else
                    state <= state_error;
                end if;
            when "11000000" => 
                if state = state_1_right then
                    state <= state_2_right;
                else
                    state <= state_error;
                end if;
            when "11100000" => 
                if state = state_2_right then
                    state <= state_3_right;
                else
                    state <= state_error;
                end if;
            when "11110000" => 
                if state = state_3_right then
                    state <= "11110000";
                else
                    state <= state_error;
                end if;


            when others =>
                state <= state_error;
        end case;

    end process;

end Behavioral;

Thank you to Brian Drummond for finding the error in my logic and suggestion of a clock. I had to add some extra logic in the if statements since the clock cycles through the case block rapidly and the state could stay the same.

Here is the updated code that solves the issue:

entity CombinationLockFSM is
    Port(
        mclk: in std_logic;
        sw: in std_logic_vector(7 downto 0);
        Led: out std_logic_vector(7 downto 0)
    );
end CombinationLockFSM;

architecture Behavioral of CombinationLockFSM is
    constant state_start: std_logic_vector(7 downto 0) := "10000000";
    constant state_1_right: std_logic_vector(7 downto 0) := "01000000";
    constant state_2_right: std_logic_vector(7 downto 0) := "00100000";
    constant state_3_right: std_logic_vector(7 downto 0) := "00010000";
    constant state_4_right: std_logic_vector(7 downto 0) := "11110000";
    constant state_error: std_logic_vector(7 downto 0) := "00001111";

    signal state: std_logic_vector(7 downto 0) := (others => '0');
begin

    Led <= state;

    process(mclk)
    begin
        if rising_edge(mclk) then
            case sw is
                when "00000000" => 
                    state <= state_start;
                when "10000000" => 
                    if state = state_start or state = state_1_right then
                        state <= state_1_right;
                    else
                        state <= state_error;
                    end if;
                when "11000000" => 
                    if state = state_1_right or state = state_2_right then
                        state <= state_2_right;
                    else
                        state <= state_error;
                    end if;
                when "11100000" => 
                    if state = state_2_right or state = state_3_right then
                        state <= state_3_right;
                    else
                        state <= state_error;
                    end if;
                when "11110000" => 
                    if state = state_3_right or state = state_4_right then
                        state <= state_4_right;
                    else
                        state <= state_error;
                    end if;


                when others =>
                    state <= state_error;
            end case;
        end if;

    end process;

end Behavioral;

Answer 1

The other response is correct about needing a clock.

But ignore the two-process example he linked to : search in the usual places for "VHDL single process state machine" for a better solution.

http://www.openhdl.com/vhdl/664-vhdl-tip-single-process-vhdl-state-machine-design.html for one.

Answer 2

Perhaps you need a clock to sync the state changes.

Could be switch bounce, but if the first was bouncing it seems like it would go back to start_state not state_error I think. Are you using physical switches?

I reprint example code from here http://esd.cs.ucr.edu/labs/tutorial/fsm.vhd for example of syncing to clock for the state changes.

-----------------------------------------------------
-- VHDL FSM (Finite State Machine) modeling
-- (ESD book Figure 2.7)
-- by Weijun Zhang, 04/2001
--
-- FSM model consists of two concurrent processes
-- state_reg and comb_logic
-- we use case statement to describe the state 
-- transistion. All the inputs and signals are
-- put into the process sensitive list.  
-----------------------------------------------------

library ieee ;
use ieee.std_logic_1164.all;

-----------------------------------------------------

entity seq_design is
port(   a:      in std_logic;
    clock:      in std_logic;
    reset:      in std_logic;
    x:      out std_logic
);
end seq_design;

-----------------------------------------------------

architecture FSM of seq_design is

    -- define the states of FSM model

    type state_type is (S0, S1, S2, S3);
    signal next_state, current_state: state_type;

begin

    -- cocurrent process#1: state registers
    state_reg: process(clock, reset)
    begin

    if (reset='1') then
            current_state <= S0;
    elsif (clock'event and clock='1') then
        current_state <= next_state;
    end if;

    end process;                          

    -- cocurrent process#2: combinational logic
    comb_logic: process(current_state, a)
    begin

    -- use case statement to show the 
    -- state transistion

    case current_state is

        when S0 =>  x <= '0';
            if a='0' then
                next_state <= S0;
            elsif a ='1' then
                next_state <= S1;
            end if;

        when S1 =>  x <= '0';
            if a='0' then 
                next_state <= S1;
            elsif a='1' then 
                next_state <= S2;
            end if;

        when S2 =>  x <= '0';
            if a='0' then
                next_state <= S2;
            elsif a='1' then
                next_state <= S3;
            end if;

        when S3 =>  x <= '1';
            if a='0' then 
                next_state <= S3;
            elsif a='1' then 
                next_state <= S0;
            end if;

        when others =>
            x <= '0';
            next_state <= S0;

    end case;

    end process;

end FSM;

-----------------------------------------------------