I'm learning FPGA development, and this is my first Verilog module - a button bouncer using a state machine. The code works as I expected it to be, but I would like some feedback on the code itself such as: state machine design, naming and code style, and any potential edge case that I did not think of.
Here is my current implementation:
module button_debounce (
// Inputs
input [1:0] pmod,
input clk,
// Outputs
output reg [3:0] led
);
wire rst;
wire go;
// Inverse signal
assign rst = ~pmod[0];
assign go = ~pmod[1];
// States
localparam STATE_IDLE = 2'd0;
localparam STATE_PUSH = 2'd1;
localparam STATE_DONE = 2'd2;
reg [1:0] state;
reg [19:0] clk_count;
// State transition
always @ (posedge clk or posedge rst) begin
if (rst == 1) begin
state <= STATE_IDLE;
led <= 4'd0;
end else begin
case (state)
STATE_IDLE: begin
if (go == 1'b1) begin
state <= STATE_PUSH;
clk_count <= 0;
end
end
STATE_PUSH: begin
if (go == 1'b0) begin
state <= STATE_DONE;
led <= led + 1;
end
end
STATE_DONE: begin
if (clk_count != 20'd600000) begin
clk_count <= clk_count + 1;
end else begin
clk_count <= 20'b0;
state <= STATE_IDLE;
end
end
default: state <= STATE_IDLE;
endcase
end
end
endmodule
Any feedback on improving this code would be greatly appreciated.
1 Answer 1
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