FPGA output unreliable

[Andrew Turner]

I am fairly new to VHDL and recently bought a Spartan 3 kit from Digilent
(S3Board with an XC3S1000-4). I found I was having problems with the some
of the outputs occasinally not having the values I expected. So I made the
simplest design I could think of that when synthesised has the same
problem, yet simulates correctly.

The design below is designed to be connected to a push button switch and
LED array (and clock) and to count (in binary) on the LEDs each time the
button is presed. To do this there is a delay counter to overcome contact
bounce and to make sure you have to press the button again before it will
increment.

I expect the values displayed on the LED array to be one behind the value
stored in cntr and this is usually the case. However, sometimes when you
press the button one bit of the LED array may not update, but when you
press the button again it (usually) goes to the next value OK.

eg (LED array output after each button press)
00000000
00000001
00000011 <-- bit 0 did not go low
00000011
00000100

or

00000000
00000001
00000010
00000010 <-- bit 0 did not go high
00000100

There does not seem to be any particular bit that is more prone to this
than any other, and I really have no idea what is causing this. I am using
ISE Webpack 7.1.03i (the latest version) and my design generates no
warnings/errors and the maximum clockspeed is far above the one I am
actually using.


Design:

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.numeric_std.all;

entity counter is
port (
clk : in std_logic;
button : in std_logic;
leds : out std_logic_vector(7 downto 0)
);
end counter;

architecture behav of counter is
signal cntr : std_logic_vector(7 downto 0) := x"00";
signal delay : natural range 0 to 5000000 := 0;
begin
process(clk)
begin
if (rising_edge(clk)) then
if (button='0' and delay/=0) then
delay <= delay - 1;
elsif (button='1' and delay=0) then
delay <= 5000000;
cntr <= std_logic_vector(to_unsigned
(to_integer(unsigned(cntr))+1,8));
leds <= cntr;
end if;
end if;
end process;
end behav;

I would greatly appreciate any help as I don't know whether I don't fully
understand VHDL or if there is some other cause of this problem.

Thanks in advance,
Andrew

 

[Peter]

As the button signal is asynchronous, it is a good idea to make it
synchronous with a 2-stage synchroniser.

Feeding an asynchronous signal into a synchronous system may lead to
all kind of strange problems.

/Peter

 

[Peter]

As the button signal is asynchronous, it is a good idea to make it
synchronous with a 2-stage synchroniser.

Feeding an asynchronous signal into a synchronous system may lead to
all kind of strange problems.

/Peter

 

[Ralf Hildebrandt]

However, sometimes when you
press the button one bit of the LED array may not update, but when you
press the button again it (usually) goes to the next value OK.

eg (LED array output after each button press)
00000000
00000001
00000011 <-- bit 0 did not go low
00000011
00000100

or

00000000
00000001
00000010
00000010 <-- bit 0 did not go high
00000100

There does not seem to be any particular bit that is more prone to this
than any other, and I really have no idea what is causing this. I am using
ISE Webpack 7.1.03i (the latest version) and my design generates no
warnings/errors and the maximum clockspeed is far above the one I am
actually using.


Design:

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.numeric_std.all;

entity counter is
port (
clk : in std_logic;
button : in std_logic;
leds : out std_logic_vector(7 downto 0)
);
end counter;

architecture behav of counter is
signal cntr : std_logic_vector(7 downto 0) := x"00";
signal delay : natural range 0 to 5000000 := 0;
begin
process(clk)
begin
if (rising_edge(clk)) then
if (button='0' and delay/=0) then
delay <= delay - 1;
elsif (button='1' and delay=0) then
delay <= 5000000;
cntr <= std_logic_vector(to_unsigned
(to_integer(unsigned(cntr))+1,8));

If button='1' before delay reaches 0, nothing will happen. This happens
if you press the button too soon after the last activation. ("bit 0 did
not go high" = no counting.)
If button is hold '1' longer while delay counts downward to 0 and
reaches zero, two counting steps will be done. ("bit 0 did not go low")

-> Try this
* Press the button very fast several times (bounce it) -> The counter
will not count every bounce.
* Hold the button for a long time. -> The counter will count several
times according to delay.


Ralf

 

[Benjamin Todd]

library IEEE;

use IEEE.STD_LOGIC_1164.ALL;
use IEEE.numeric_std.all;

entity counter is
port (
clk : in std_logic;
button : in std_logic;
leds : out std_logic_vector(7 downto 0)
);
end counter;

architecture behav of counter is
signal cntr : std_logic_vector(7 downto 0) := x"00";
signal delay : natural range 0 to 5000000 := 0;
begin
process(clk)
begin
if (rising_edge(clk)) then
if (button='0' and delay/=0) then
delay <= delay - 1;
elsif (button='1' and delay=0) then
delay <= 5000000;
cntr <= std_logic_vector(to_unsigned
(to_integer(unsigned(cntr))+1,8));
leds <= cntr;
end if;
end if;
end process;
end behav;

Just a couple of things of note - you're missing a global reset, not
drastic, but good design practice is to add one.
The startup values you assigned to the signals at the top are not taken into
accout for synthesis (depends on startup value of registers as you haven't a
reset)

I would try to separate this into two processes.

First process controls only the delay counter
Second process controls the led counter.

hmm.. Something like this, although it's probably more than is needed for
this circuit...

process (clk)
if rising_edge (clk) then
if delay = 0 then
if button = '1' then
delay <= 50000000;
end if;
else
delay <= delay - 1;
end if;
end if;
end process;

process (clk)
if rising_edge (clk)
if delay = 0 AND button = '1' then
cntr <= cntr + 1;
end if;
end if;
end process

leds <= std_logic_vector(to_unsigned(cntr) + 1, 8); -- This might be wrong,
check the FAQ.

Anyways, I haven't checked the code but i'd split it like this. HTH.
Ben

 

[Andrew Turner]

Firstly, thank you to everyone that has replied.

Peter:
It had never occured to me that the way I processed my input could be
the cause of my problems - I had become completely focussed on the
outputs. I will definately try adding a synchroniser and am hopeful
this could be the solution.


Ralf:
I agree that my second example could be explained by the button not
making contact, but for the first example you claim that if you keep
holding the button multiple counting steps can happen. I don't agree as
"delay" is only decreased when the button is '0'. Therefore you need to
remove your finger from the button for at least 5,000,000 cycles
(1/10th second at my 50MHz clock) before you can increment the counter
again.

In any case, a third (and better) example will hopefully show there is
more happening than just that:

00000110
00000111
00001010 <-- bit 1 did not go low
00001001
00001010

As you can see, cntr seems to have continued to count correctly (and
always does), but the LED array has not fully updated.


Ben:
I hadn't realised that synthesis does not use VHDL initialisation
values, but I don't think this has been a problem as it always seems to
start at 0 anyway, although I will fix this in future.
A global reset would also be a useful thing to add, however I was most
interested in making this design as short as possible, as it is only
meant as a demonstration of my problem, but I shall certainly use it in
future designs.

Thank you once again,
--Andrew

 

[Peter]

One thought that perhaps has nothing to do with your problem:

Your code would be cleaner if you declare cntr as an unsigned(7 downto
0).
Then your counter would be just:
cntr <= cntr + 1;
Because signal leds is a standard_logic_vector you must write:
leds <= std_logic_vector(cntr);

That gets rid of all the "to_unsigned
(to_integer(unsigned(cntr))+1,­8)); " stuff.

Regards, Peter

 

[Andrew Turner]

Again, thank you to everyone who has replied, but particularly to Peter,
whose first thoughts were correct - my asynchronous push-button input was
causing metastability problems with the synchronous logic.
Having added in a two-stage shift register to synchronise the input from
the push button, the circuit exhibits no more problems.

Thanks also to everyone who has given me advice to make my VHDL tidier and
clearer.


--Andrew

 

[Bert Cuzeau]

Again, thank you to everyone who has replied, but particularly to Peter,
whose first thoughts were correct - my asynchronous push-button input was
causing metastability problems with the synchronous logic.

Good fix, slightly wrong interpretation.

Amazing how often the basic rule of never using asynchronous inputs
in a state machine is violated and most designers still miss the point !
(a counter IS an FSM indeed)
Thanks for your testimony that even trivial examples DO fail.

In the coding style recommendations I posted, this is
one of the major rules. It's an extremely frequent cause of malfunction.

But the issue is *NOT* metastability, it is a difference of
input-to-multiple-D delays.
To make it short, some of the counter's Flip-Flops see the consequence
of a BP to 1, others see the consequence of a zero, hence the
discrepancies you've witnessed in the vector.
Some sophisticated tools like Quartus II can do their best to equalize
the different paths, but the solution is indeed to resynchronize
the input.

Bert Cuzeau

 

[Mike Treseler]

But the issue is *NOT* metastability, it is a difference of
input-to-multiple-D delays.

Exactly. Some call this a logic "race".
A one flop synchronizer will cover this problem,
which is easy to observe on the bench.
The second flop in a synchronizer covers metastable
events which are extremely difficult to see on the bench.

-- Mike Treseler