A Missing Edge in My Debounce Circuit

A while back a reader named Sam asked a question about the debounce circuit in Tutorial 11. In simulation, the rise and fall outputs fired two clocks after the input changed, and he expected them to wait until the bounce timer had run out. His reading was correct, and that part is by design. But while I was answering him I found a real bug, and it has been sitting in that tutorial (and in Tutorials 12 and 13, which reuse the module) since 2015.

How the debouncer works

This is a “lockout” debouncer. It reports an edge as soon as the input gets through the synchronizer, then ignores the input until a timer runs out. That gives you the lowest possible latency, which is nice for buttons. Here’s the original always block:

  always @(posedge clk)
    if (bounce_count == 0)
      begin
        switch_rise <= switch_shift == 2'b01;
        switch_fall <= switch_shift == 2'b10;
        switch_out <= switch_shift[0];
        if (switch_shift[1] != switch_shift[0])
          bounce_count <= bounce_limit-1;
      end
    else
      begin
        switch_rise <= 0;
        switch_fall <= 0;
        bounce_count <= bounce_count-1;
      end

When the timer isn’t running, it looks for an edge in the two-bit shift register. If it sees one, it pulses switch_rise or switch_fall and starts the timer.

The bug

The problem is what happens when the switch changes and changes back while the timer is running. Say you tap the button for less than the debounce time. The press is reported, and the release happens during the lockout. When the timer expires, switch_out picks up the new value, so the level ends up correct. But by then there is no edge in the shift register anymore, so switch_fall never fires.

The same thing happens the other way around: a short release while the button is held loses the following switch_rise. If all you use is switch_out you’d never notice. But if you count presses, or toggle something on switch_rise and expect a matching switch_fall, you will occasionally get out of step, and only when someone taps the button quickly. That’s the worst kind of bug to track down on the bench.

The fix

Instead of looking for an edge, compare the synchronized input with the debounced output:

  always @(posedge clk)
    if (bounce_count == 0)
      begin
        switch_rise <= switch_shift[1] && !switch_out;
        switch_fall <= !switch_shift[1] && switch_out;
        switch_out <= switch_shift[1];
        if (switch_shift[1] != switch_out)
          bounce_count <= bounce_limit-1;
      end
    else
      begin
        switch_rise <= 0;
        switch_fall <= 0;
        bounce_count <= bounce_count-1;
      end

Now any difference left over when the timer expires gets reported, along with a fresh lockout period. The latency is unchanged. While I was at it, I switched to bit 1 of the shift register, so the input actually goes through both synchronizer flip-flops; the original used bit 0, which is only one flip-flop away from the asynchronous input.

Testing it

To make sure this was really fixed, I wrote a small testbench, debounce_test.v. It uses a short bounce_limit of 16 clocks so the lockout is easy to land inside. An always block counts every switch_rise and switch_fall pulse, and flags an error if both ever fire at once. Each test case changes the input, bumps the number of rises and falls it expects, and then calls a check task, which waits for everything to settle and then checks two things: that switch_out matches the input, and that the pulse counts match what was expected.

A bounce task toggles the input a few times with random spacing before leaving it at its final value, which gives a reasonable imitation of a real switch. The test cases are:

  • a clean press and release
  • a bouncy press and release
  • a short tap, pressed and released inside the lockout
  • a bouncy short tap
  • a short release while the button is held

Here’s the short tap case, so you can see the pattern:

      // Press and release both inside the lockout window
      switch_in = 1;
      repeat (4) @(posedge clk);
      #1 switch_in = 0;
      expected_rises = expected_rises+1;
      expected_falls = expected_falls+1;
      check("short tap");

Against the original module, the clean and bouncy cases pass but the three short ones fail, each one missing a pulse:

ERROR short tap: rises=3 (expected 3) falls=2 (expected 3)
ERROR bouncy short tap: rises=4 (expected 4) falls=3 (expected 4)
ERROR short release: rises=5 (expected 6) falls=5 (expected 5)
FAIL: 3 errors

With the fix, all nine checks pass. It’s plain Verilog, so it runs in any simulator. I used Icarus Verilog:

iverilog -o debounce_test debounce_test.v debounce.v
vvp debounce_test

The fixed debounce.v is now in the tutorial repository, Tutorials 11, 12 and 13 all link to it, and Tutorial 11 has been updated with the new code. Thanks to Sam for the question that made me look.

A Testbench Race I Should Have Caught

Every so often a reader reports that the testbench for Tutorial 19 fails, with the comparisons coming out one sample off. Back in 2018 I said “nobody has reported this before” and asked which version of Vivado they were using. Recently a reader named detiber found a workaround, and when I finally sat down and looked at it, the bug was mine. It’s a nice little example of something I wrote about years ago in Tutorial 3, so I’m going to own up to it here.

The code

The first I2S sample out of the transmitter is junk, because the I2S clock is already running before any data shows up. So the testbench waits until it has sent some data, waits for the start of a left channel word, and then turns on the output checkers.

  event enable_output_checking;
  reg output_checking_enabled = 0;
  initial
    begin
      @(enable_output_checking);
      @(negedge ws);
      repeat (2) @(posedge sck);
      output_checking_enabled = 1;
    end

The checkers are ordinary always blocks clocked on the same sck. Here’s the right channel one, trimmed down:

  always @(posedge sck)
    if (output_checking_enabled && !wsd[0] && wsp)
      util.check(shift,expected_right.dequeue(0)&mask);

The race

Two rising edges of sck after ws falls, the word boundary pulse wsp is still high. On that edge two blocks wake up: the initial block, which sets output_checking_enabled, and the checker, which reads it. Verilog doesn’t say which one runs first.

The ws signal changes on the falling edge of sck, and the testbench’s little I2S receiver delays it through wsd on rising edges, so wsp is a one clock pulse at each word boundary. With repeat (2) the flag is set on the second rising edge, the same edge where the checker still sees wsp high.

  • If the checker runs first, it sees the flag as 0 and skips. The first comparison is the left word, which is what I intended.
  • If the initial block runs first, the checker sees a 1, compares the junk right channel word, and pulls an entry off the expected queue. From then on every right channel comparison is one sample behind.

The simulator I used in 2015 happened to pick the first order. Newer versions of XSim evidently pick the second. Neither one is wrong; my testbench was.

To prove it, I forced the bad ordering in Icarus Verilog by setting the flag a nanosecond before that edge instead of on it. The original testbench went from passing with 797 comparisons to failing with 398 errors out of 798, and every received word matched the previous expected word. That’s exactly what readers were reporting.

The fix

detiber’s workaround was to change repeat (2) to repeat (3). That works: by the third edge wsp is low, so the checkers can’t fire no matter who goes first. But it steps around the race rather than removing it. The real fix is one character:

      output_checking_enabled <= 1;

With a nonblocking assignment, every block triggered on that edge sees the old value, and the new value shows up after the edge. Simulation order no longer matters.

The lesson

This is the same rule from Tutorial 3, and it applies to testbench code just as much as RTL: if a value is written on a clock edge and read by another block on the same edge, use a nonblocking assignment. A testbench that only passes because of the order your simulator happens to pick is a testbench that will break when you upgrade.

The fixed i2s_transmit_test.v is in the tutorial repository. Thanks to detiber and Zdzislaw for reporting it, and sorry it took me so long.