How do I correctly implement a Finite-State Machine into VHDL without taking in multiple inputs from Basysy3 FPGA

Question

I am new to VHDL and I am attempting to implement the following state machine into VHDL (state diagram provided below). When I press a button on my Basys3 FPGA board( P input) the output is a random state. I suspect this is because the clock is going through many cycles during a single press so more than 1 input is being taken in from a single press but I am unsure. Is there anything I can do to fix this. I want to be able to press button P and the states change one at a time.

library IEEE;
USE ieee.std_logic_1164.all;

ENTITY trasher is
PORT (
      clock :   IN STD_LOGIC;
      P     :   IN STD_LOGIC;
      reset :   IN STD_LOGIC;
      LED3, LED1,LED2,LED0     :    OUT STD_LOGIC);
END ENTITY;

-- Architecture definition for the SimpleFSM entity
Architecture RTL of trasher is
TYPE State_type IS (A, B, C, D);  -- Define the states
    SIGNAL State : State_Type;    -- Create a signal that uses 
                                  -- the different states

BEGIN 
  PROCESS (clock, reset) 
  BEGIN 
    IF (reset = '1') THEN            -- upon reset, set the state to A
        State <= A;
 
    ELSIF rising_edge(clock) THEN    -- if there is a rising edge of the
                             -- clock, then do the stuff below
 
        -- The CASE statement checks the value of the State variable,
        -- and based on the value and any other control signals, changes
        -- to a new state.
        CASE State IS
            -- If the current state is A and P is set to 1, then the
            -- next state is B
            WHEN A => 
                IF P='1' THEN 
                    State <= B; 
                END IF; 
            -- If the current state is B and P is set to 1, then the
            -- next state is C
            WHEN B => 
                IF P='1' THEN 
                    State <= C; 
                END IF; 
            -- If the current state is C and P is set to 1, then the
            -- next state is D
            WHEN C => 
                IF P='1' THEN 
                    State <= D; 
                END IF; 
            -- If the current state is D and P is set to 1, then the
            -- next state is B.
            -- If the current state is D and P is set to 0, then the
            -- next state is A.
            WHEN D=> 
                IF P='1' THEN 
                    State <= B; 
                ELSE 
                    State <= A; 
                END IF; 
            WHEN others =>
                State <= A;
        END CASE; 
    END IF; 
  END PROCESS;
  
  -- Decode the current state to create the output
  -- if the current state is D, R is 1 otherwise R is 0

  LED0 <= '1' WHEN State=A ELSE '0';
  LED1 <= '1' WHEN State=B ELSE '0';
  LED2 <= '1' WHEN State=C ELSE '0';
  LED3 <= '1' WHEN State=D ELSE '0';
END rtl;

Answer 1

Do not use directly the input from your press-button. What you need to feed your state machine is the output of a rising edge detector of P, not P itself.

Moreover P is not synchronous with your master clock and there is thus a risk of meta-stability. Last but not least, if it bounces, you will get several value changes instead of just one. To solve the meta-stability issue you need a re-synchronizer, which is just a shift register. And you can also use it to generate an intermediate signal that is asserted high during only one clock period when the button is pressed, that is, the rising edge detector you need for your state machine. Example with 3-stages:

signal sync: std_ulogic_vector(0 to 2);
signal button_pressed: std_ulogic;
...
process(clock, reset)
begin
  if reset = '1' then
    sync <= (others => '0');
  elsif rising_edge(clock) then
    sync <= P & sync(0 to 1);
  end if;
end process;
    
button_pressed <= sync(1) and (not sync(2));

Stages 1 and 2 of sync are safe to use because they have already been resynchronized (assuming 2 stages are enough for your target technology and mean time between failures; read something about meta-stability, maybe, if you don't understand this).

When the button is pressed, ones are shifted in sync. After two clock periods sync = "110" so button_pressed is asserted high. One clock period later sync = "111" and button_pressed is de-asserted. button_pressed is thus a one-clock-period-only indicator that the button was pressed. You can use it as an input of your state machine.

The second problem comes from the way press-buttons work. If your prototyping board does not already debounce its press-buttons it can be that, when the button is pressed, your P input oscillates several times between 0 and 1 before stabilizing to 1. Same when the button is released. As this is sometimes not the case do some tests before implementing a debouncer. For instance, count the number of times button_pressed is asserted high and send this to your LEDs:

signal cnt: u_unsigned(3 downto 0);
...
process(clock, reset)
begin
  if reset = '1' then
    cnt <= (others => '0');
  elsif rising_edge(clock) then
    cnt <= cnt + button_pressed;
  end if;
end process;

LED0 <= std_logic(cnt(0));
LED1 <= std_logic(cnt(1));
LED2 <= std_logic(cnt(2));
LED3 <= std_logic(cnt(3));

If your button bounces you should sometimes see more than one increment when you press it. It will be time to search a bit about debouncing and, if needed, to ask a new question.