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;

• geometrikal: How do you have the LEDs connected? Long shot, but perhaps everything is inverted, so e.g. if the LEDs say 10000000 it is actually 01111111
• MLM: @geometrikal LEDs is fine and even if it was backwards the error is 4 lights in a row so you can't really mess it up.

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.

• MLM: I added a clock and put the rising_edge statement around my case but there is no change.
• geometrikal: wouldn't you need Switches in its own process statement, otherwise it would clock straight through to error state.
• user16324: @MLM : (1) and is "clk" the only thing in the sensitivity list? it should be. (2) Bug in your logic : having got to State 1 with the correct switch pattern, you need to stay there until the switches change. Given that clue it's an easy change to the "if" expression. Same applies to other states...
• MLM: @BrianDrummond I tried it with just the clk in the sensitivity list and there is no change. You do stay on state_1 until you mess up or go to state_2. Although I cannot even get to state_1. I am not even sure I need a clock...
• user16324: Simulate it. Best way to see what it's really doing... There are several free simulators out there including ghdl.

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;
-----------------------------------------------------

• MLM: I am using physical switches on the board. I will look into adding a sync clock but I am unsure why you would need to in this situation.
• MLM: I added a clock and put the rising_edge statement around my case but there is no change.