Take the 2-minute tour ×
Electrical Engineering Stack Exchange is a question and answer site for electronics and electrical engineering professionals, students, and enthusiasts. It's 100% free, no registration required.

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;
share|improve this question
    
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 –  geometrikal Nov 24 '12 at 22:27
    
@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. –  MLM Nov 25 '12 at 0:24
add comment

2 Answers

up vote 2 down vote accepted

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.

share|improve this answer
    
I added a clock and put the rising_edge statement around my case but there is no change. –  MLM Nov 24 '12 at 21:07
    
wouldn't you need Switches in its own process statement, otherwise it would clock straight through to error state. –  geometrikal Nov 24 '12 at 22:24
1  
@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... –  Brian Drummond Nov 24 '12 at 23:01
    
@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... –  MLM Nov 25 '12 at 0:55
2  
Simulate it. Best way to see what it's really doing... There are several free simulators out there including ghdl. –  Brian Drummond Nov 25 '12 at 10:36
show 6 more comments

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;

-----------------------------------------------------
share|improve this answer
    
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 Nov 24 '12 at 20:52
    
I added a clock and put the rising_edge statement around my case but there is no change. –  MLM Nov 24 '12 at 21:08
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.