VHDL--counter for 7 segment display

Question

I am writing a code that when the key(0) is pressed, the letter shown on the 7-segment display changes from the one to the next, like a to b, b to c. It starts from A. When the letter reaches H, it goes back to A. However, when I press the key, the letter changes,but it goes back to H when it is not pressed.I don't understand what the problem is.Here's the code:

    KEY : in std_logic_vector(1 downto 0);        
    HEX : out std_logic_vector(6 downto 0)

    architecture behavioural of display is        
    signal counter : std_logic_vector(2 downto 0):= "000";
    begin
    process (KEY)
    begin
    if (KEY(0)= '1') then
    counter <= counter + '1';
    end if;
    end process;

    process(counter)
    begin
    case counter is
        when "000" => HEX <= "0001000";
        when "001" => HEX <= "0000011";
        when "010" => HEX <= "1000110";
        when "011" => HEX <= "0100001";            
        when "100" => HEX <= "0000110";
        when "101" => HEX <= "0001110";
        when "110" => HEX <= "0010000";
        when "111" => HEX <= "0001001";
    end case;   
    end process;
    end behavioural;      

Answer 1

The first issue I can see is that you're using a "std_logic_vector" type for your counter. You can't increment a std_logic_vector type, and so the "+" operator you're using will be doing weird things. (I'm not completely sure what "000" + '1' would give you, but it won't be the integer addition you're hoping for.) Use an unsigned type instead.

Secondly, by using an if statement with no else statement in your asynchronous (unclocked) process, you're inferring a latch. (The synthesis process has to find somewhere to store the value of the counter for the process iterations where it's not incremented). This is generally considered bad practice - you're not fully defining the behaviour of the process, and the synthesis of latches can be very inefficient, using lots of space on the FPGA.

This would be much easier to approach if you used a synchronous (clocked) process. You could then compare the current switch value to the value at the last clock cycle. This would also deal with the inferred latch that you have in your if statement.

My counter process would look something like this:

...
signal counter : unsigned(2 downto 0);
...

process (RST, CLK)
begin
if RST = '1' then
    counter <= (others => '0')
elsif rising_edge(CLK) then
    if KEY(0) = '1' and key_prev = '0' then
        counter <= counter + 1;
    end if;
end if;
end process;

process (CLK)
begin
if rising_edge(CLK) then
    key_prev <= KEY(0);
end if;
end process;

Switch bounce will be an issue which I've not addressed. You'll need to have another process which deals with that and outputs a debounced std_logic which you can use instead of KEY(0).

Answer 2

The process that increments your counter is going to keep incrementing the counter as fast as it can for as long as you hold the key down. Then when you release the key the counter stops changing and you see whatever happened to be its last value.

You need to do two things: first, you must debounce the switch. Mechanical switch contacts bounce for a few milliseconds when the switch is pressed or released so you need be sure that you ignore those bounces. Second, you need to detect when the switch changes from the "not pressed" condition to the "pressed" condition and only increment the counter when that change occurs.