Weird behaviour of finite state machine in VHDL

Question

So I've recently started learning VHDL as part of a practicum at the university. This time, our task was to create a moore-machine on which you can set the time in a certain way and use it as a countdown, triggering an alarm as soon as it reaches 0.

The starting state is "idle", if you press "keySet_in" you can set the minutes by pressing "keyUp_in" to increase it or "keyDown_in" to decrease it. If you press any other key, the countdown will go to "start" and start with the default value of 1 Minute. There are also LEDs to controll the the decimal points on the clock and the highlighting of the digits. Those are not important tho and were working as far as I could tell.

From the minute selection you can reach the first/second-digit of the second counter by pressing "keyRight_in". You can increase/decrease it in the same way as with minutes. Once you have reached the lesser value digit you will go to start if you "keyRight_in" again. You can always go back with keyLeft_in unless you are at minutes, in that case it does nothing.

If you press any key during countdown, the countdown will cancel and reset to idle with the previous value. If it reaches 0, the alarm will play until you press a button and then you will go back to idle.

Now we tried implementing my code on an FPGA today and there was some very weird misbehaviour which neither me nor the teachers could explain even tho we rewrote the entire code and checked it bit for bit. Now those teachers are also just students so I imagine that their knowledge is not perfect and maybe someone here might know more:

The kind of weird behaviour we had was for example (from my memory, might not be 100% accurate) that most buttons would not work. When trying to increase/decrease the minutes, it would just toggle between 9 and 8. When trying to increase/decrease higher digit seconds, it would always take 2 steps at once (from 9 to 7 to 5, ...). Most of the other buttons didnt even react, you could for example not go back with leftKey_in. The initial value was 1 minute tho. We tried 3 different FPGA's as well.

Since then I have thought of 2 (unlikely) options of solving the problems: 1. adding a sensitivty list to process P1 2. adding ' state <= idle; ' as initial value (included in the code)

So I know that this is quite a wall of text, so in case anyones still reading, heres the code: pastebin link (had problems using the code-thingy of stackoverflow, so I used pastebin)

library IEEE;
    use IEEE.STD_LOGIC_1164.ALL;
    use IEEE.NUMERIC_STD.ALL;
    use work.TIMER_PKG.ALL;

entity CONTROLLER is
    port (
        -- Clocks and Reset
        sysclk_in   : in std_logic;
        clk10hz_in  : in std_logic;
        reset_in    : in std_logic;




        -- Alu
        alu_instruction_out : out ALU_INSTRUCTION_TYPE;
        timer_is_zero_in : in std_logic;

        -- Key Controls
        keyUp_in    : in std_logic;
        keyDown_in  : in std_logic;
        keyLeft_in  : in std_logic;
        keyRight_in : in std_logic;
        keySet_in   : in std_logic;

        -- 7 Segment Display
        disp_highlight_out : out std_logic_vector(3 downto 0);
        disp_dots_out      : out std_logic_vector(3 downto 0);

        -- Alarm
        alarm_out   : out std_logic
    );
end CONTROLLER;

architecture Behavioral of CONTROLLER is

type fsm_type is ( IDLE, START, WAIT10HZ, CNTDWN, TIMER_END, RESTORE,
                                                SEL_MIN, INC_MIN, DEC_MIN,
                                                SEL_SEC10, INC_sec10, DEC_SEC10,
                                                SEL_SEC, INC_SEC, DEC_SEC);

signal state, next_state : fsm_type;
state <= idle;

begin

P1:process is begin
wait until rising_edge(sysclk_in);
state <= next_state;
end process;


P2:process(sysclk_in, reset_in, keyup_in, keydown_in, keyleft_in, keyright_in, keyset_in,
                                timer_is_zero_in) is begin
        next_state <= state;
        case state is
                when IDLE =>
                        if (keyright_in or keyup_in or keydown_in or keyleft_in) = '1' then
                                next_state <= start;
                        elsif keyset_in = '1' then
                                next_state <= sel_min;
                        end if;

                when Start =>
                        next_state <= wait10hz;

                when wait10hz =>
                        if clk10hz_in = '1' then
                        next_state <= cntdwn;
                        end if;

                when cntdwn =>
                        if timer_is_zero_in = '1' then
                        next_state <= timer_end;
                        elsif (keyright_in or keyup_in or keydown_in or keyleft_in or keyset_in) ='1' then
                        next_state <= restore;
                        end if;

                when restore =>
                        next_state <= start;

                when timer_end =>
                if (keyright_in or keyup_in or keydown_in or keyleft_in or keyset_in) ='1' then
                        next_state <= idle;
                        end if;

                when sel_min =>
                if keyup_in = '1' then
                        next_state <= inc_min;
                elsif keydown_in ='1' then
                        next_state <= dec_min;
                elsif keyright_in ='1' then
                        next_state <= sel_sec10;
                end if;

                when sel_sec10 =>
                        if keyleft_in = '1' then
                        next_state <= sel_min;
                        elsif keyright_in ='1' then
                        next_state <= sel_sec;
                        elsif keyup_in = '1' then
                        next_state <= inc_sec10;
                        elsif keydown_in = '1' then
                        next_state <= dec_sec10;
                        end if;

                when sel_sec=>
                        if keyleft_in ='1' then
                        next_state <= sel_sec10;
                        elsif keyright_in ='1' then
                        next_state <= start;
                        elsif keydown_in ='1' then
                        next_state <= dec_sec;
                        elsif keyup_in ='1' then
                        next_state <= inc_sec;
                        end if;

                when inc_min =>
                next_state <= sel_min;

                when dec_min =>
                next_state <= sel_min;

                when inc_sec10 =>
                next_state <= sel_sec10;

                when dec_sec10 =>
                next_state <= sel_sec10;

                when inc_sec =>
                next_state <= sel_sec;

                when dec_sec =>
                next_state <= sel_sec;

                when others => null;


        end case;

        if  reset_in = '1' then
        next_state <= idle;
        end if;

                        disp_highlight_out <= "1111";
                  disp_dots_out <= "0101";
                  alarm_out <= '0';
                  alu_instruction_out <= alu_none;



        case state is
                when start =>
                alu_instruction_out <= alu_store;
                disp_highlight_out <= "1111";
                disp_dots_out <= "0101";

                when idle =>
                alu_instruction_out <= alu_none;
                disp_highlight_out <= "1111";
                disp_dots_out <= "0101";
                alarm_out <= '0';

                when wait10hz =>
                alu_instruction_out <= alu_none;
                disp_highlight_out <= "1111";
                disp_dots_out <= "0101";


                when cntdwn =>
                alu_instruction_out <= alu_dec_timer;
                disp_highlight_out <= "1111";
                disp_dots_out <= "0101";

                when timer_end =>
                alu_instruction_out <= alu_store;
                disp_highlight_out <= "1111";
                disp_dots_out <= "0101";
                alarm_out <= '1';

                when sel_min =>
                alu_instruction_out <= alu_none;
                disp_highlight_out <= "1000";
                disp_dots_out <= "0101";

                when inc_min =>
                alu_instruction_out <= alu_single_inc_min;
                disp_highlight_out <= "1000";
                disp_dots_out <= "0101";

                when dec_min =>
                alu_instruction_out <= alu_single_dec_min;
                disp_highlight_out <= "1000";
                disp_dots_out <= "0101";

                when sel_sec10 =>
                alu_instruction_out <= alu_none;
                disp_highlight_out <= "0100";
                disp_dots_out <= "0101";

                when dec_sec10 =>
                alu_instruction_out <= alu_single_dec_sec10;
                disp_highlight_out <= "0100";
                disp_dots_out <= "0101";               

                when inc_sec10 =>
                alu_instruction_out <= alu_single_inc_sec10;
                disp_highlight_out <= "0100";
                disp_dots_out <= "0101";

                when dec_sec =>
                alu_instruction_out <= alu_single_dec_sec;
                disp_highlight_out <= "0010";
                disp_dots_out <= "0101";

                when inc_sec =>
                alu_instruction_out <= alu_single_inc_sec;
                disp_highlight_out <= "0010";
                disp_dots_out <= "0101";

                when sel_sec =>
                alu_instruction_out <= alu_none;
                disp_highlight_out <= "0010";
                disp_dots_out <= "0101";
                when others =>
                alu_instruction_out <= alu_none;


                end case;
                end process;



end Behavioral;

Hope someone can help! Greets

Sessoms

Yet another 2-process state machine. Who's still teaching them, and more importantly, why? – Intrastate

Not sure what board you're using but I imagine that a key press probably asserts a level that is maintained (many clock cycles) while the button is depressed. Looking at you code I think you expect to transition states on a single key press event which means you probably want to add some edge-detect logic to your key_press signals. – Jenellejenesia

In addition to Ciano's comment about edge detection vs level detection, you might also be suffering from switch bouncing (which will cause multiple edges in short succession). You typically have to implement some sort of debouncing to get consistent performance from physical push buttons. – Decency

I probably should have mentioned that we have been provided a "mask" which includes a debouncer and clockmanagement. Thats why the problem appears to come from the part of the code which I wrote, as other people have managed to make it work with the same mask. – Sessoms

@BrianDrummond - I found a number of VHDL guides (eg. this one) that split state machine implementations into a process for the combinatorial logic, and a process for the clock/reset. – Descender

Alright, thanks for all the help. At the next practicum date I'll try out the FPGA with risingedges instead of level detection. This will only be in 2 weeks since I dont own any programmable hardware myself. – Sessoms

@BrianDrummond - You might be interested in a question I just asked about problems with two-process state machines. It's how I learnt it too, so I'm really curious about this aversion to it I keep seeing. – Descender

Answer 1

while it is possible to implement 2-process state-machine it is more difficult to read and manage.

however, if you use 2-processes then: - 1st process is clocked (as is your P1) - 2nd process has ALL used inputs in the sensitivity list. Your P2 has NO "clk_10hz_in" and NO "state" signal in the sensitivity list. BUT you have "sysclk_in", that is not used as input in the combinatorial P2, in the sensitivity list.

--> this might lead to unexpected behavior in case of different signals changing levels at (almost) the same time (e.g. sysclk_in and other signals).

Rosannarosanne