Button Debounce in Verilog

The code follows good practices in a number of areas, such as:

• Usage of ANSI-style ports
• Good layout and indentation
• Meaningful names for the design and signals
• Using a reset input to the design

Comments

You should add a comment block at the top of your code to describe the design. For example:

/*

    A button bouncer using a state machine.
    Summarize how it works...
    Describe the module ports ...

*/

There is no need for these comments because it is clear from your code what the port directions are:

// Inputs
// Outputs

Those comments can be deleted.

Simpler

When comparing a single-bit signal to 1 like this:

if (rst == 1) begin

it is much more common to omit the comparison:

if (rst) begin

Synthesis tools also accept this coding style.

The same is true for:

if (go == 1'b1) begin

When comparing to 0:

if (go == 1'b0) begin

it is common to use the bitwise inversion operator:

if (~go) begin

For simple if/else statement like this:

if (clk_count != 20'd600000) begin

I find the code easier to understand using == instead of !=:

if (clk_count == 20'd600000) begin

This means you need to swap the order of the statements, which I'll show below.

Instead of using separate wire and assign statements, it would be simpler to combine them:

wire rst = ~pmod[0];
wire go  = ~pmod[1];

Consistency

For consistency's sake, I recommend this line:

default: state <= STATE_IDLE;

use the begin/end keywords just like the other case items:

default: begin
    state <= STATE_IDLE;
end

When you clear the clk_count signal, you use 2 different constants: 0 and 20'b0. It would be better to be consistent; I prefer the simpler 0. The same goes for led.

It would be good to use the asynchronous reset for clk_count as you have for the other signals:

if (rst) begin
    state <= STATE_IDLE;
    led <= 4'd0;
    clk_count <= 0;
end else begin

Numbers

Use underscore characters to make large numeric literals easier to read and understand. For example:

20'd600000

is better as:

20'd600_000

It would be good to assign that constant to a localparam with a meaningful name. For example:

localparam BUTTON_DELAY = 20'd600_000;

Partitioning

While I understand the temptation to have all the logic in a single always block for such a small design, it would make the design easier to understand if each of the following had its own dedicated always block:

state
led
clk_count

This becomes more important as the design grows.

SystemVerilog

For sequential logic, consider using always_ff, which is a SystemVerilog feature:

always_ff @(posedge clk or posedge rst) begin

The advantage is that it better conveys the design intent and it provides some built-in checks.

Testbench

If you decide to ever post another question, keep in mind that you can also post your Verilog testbench code to be reviewed.

Here is the code with some of the suggestions:

module button_debounce (
    input       [1:0]   pmod,
    input               clk,
    output  reg [3:0]   led
);

    // Inverse signal
    wire rst = ~pmod[0];
    wire go  = ~pmod[1];

    // States
    localparam STATE_IDLE     = 2'd0;
    localparam STATE_PUSH     = 2'd1;
    localparam STATE_DONE     = 2'd2;

    localparam BUTTON_DELAY = 20'd600_000;

    reg [1:0]    state;
    reg [19:0]   clk_count;

    // State transition
    always @ (posedge clk or posedge rst) begin
        if (rst) begin
            state <= STATE_IDLE;
            led <= 0;
            clk_count <= 0;
        end else begin
            case (state)
                STATE_IDLE: begin
                    if (go) begin
                        state <= STATE_PUSH;
                        clk_count <= 0;
                    end
                end

                STATE_PUSH: begin
                    if (~go) begin
                        state <= STATE_DONE;
                        led <= led + 1;
                    end
                end

                STATE_DONE: begin
                    if (clk_count == BUTTON_DELAY) begin
                        clk_count <= 0;
                        state <= STATE_IDLE;
                    end else begin
                        clk_count <= clk_count + 1;
                    end
                end

                default: begin
                    state <= STATE_IDLE;
                end
            endcase
        end
    end
endmodule