SystemVerilog: LED blinker control logic

Question

I built this simple blinker in SystemVerilog and would very much like some help to make it better:

control.sv

module control
(
 input clk,
 input button_0,
 input button_1,
 output [2:0] led
);
 localparam WAIT_TIME = 13500000;
 reg [2:0] ledCounter = 0;
 reg [23:0] clockCounter = 0;
 reg start = 0;
 always @(posedge clk) begin
 if (button_0 == 0) start <= 1;
 if (button_1 == 0) start <= 0;
 if (start == 1) begin
 clockCounter <= clockCounter + 1;
 if (clockCounter == WAIT_TIME) begin
 clockCounter <= 0;
 ledCounter <= ledCounter + 1;
 end
 end
 end
 assign led = ~ledCounter;
endmodule

Testbench: control_tb.sv:

module control_tb;
 // CONFIG
 parameter DURATION = 2000;
 parameter DELAY_5 = 5;
 parameter DELAY_10 = 10;
 // GENERATE SIM
 initial begin
 $dumpfile("control_tb.vcd"); 
 $dumpvars(0, control_tb); 
 #(DURATION); 
 $finish; 
 end
 // TEST
 reg clk, button_0, button_1;
 wire [2:0] led;
 always #(DELAY_5) clk = ~clk;
 initial begin
 clk <= 0;
 button_0 <= 1;
 button_1 <= 1;
 #(DELAY_10);
 repeat (5) #(DELAY_10);
 button_0 <= 0;
 repeat (5) #(DELAY_10);
 button_0 <= 1;
 repeat (100) #(DELAY_10);
 button_1 <= 0;
 repeat (5) #(DELAY_10);
 button_1 <= 1;
 end
 control control_uut(
 .clk(clk),
 .button_0(button_0),
 .button_1(button_1),
 .led(led)
 );
endmodule

Please feel free to comment on anything and everything that in will make this code better.

• toolic: If the answer was helpful, please Accept it.

Answer 1

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

• Good partitioning of the code into a testbench module and design modules.
• Usage of ANSI-style ports
• Module instance connections-by-name. Although it is more verbose than connections-by-position, it avoids a common Verilog pitfall.

Here are some general style suggestions.

Documentation

It would be good to add block comments at the top of each module to summarize the purpose:

/*
 Control for simple LED blinker.
 List of features ...
 Describe the module ports ...
*/
module control

Naming

The name control is too generic. Consider blinker_control instead. This is important as you start connecting modules together in larger designs.

Numbers

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

localparam WAIT_TIME = 13500000;

is better as:

localparam WAIT_TIME = 13_500_000;

It would be helpful to add a comment to specify the time units (such as "ns").

Sequential logic

It is not a good practice to have multiple nonblocking assignments to the same signal like this:

if (button_0 == 0) start <= 1;
if (button_1 == 0) start <= 0;

It is more common to use an if/else:

if (button_1 == 0) begin
 start <= 0;
end else if (button_0 == 0) begin
 start <= 1;
end

It would be better to separate this logic into its own always block:

always @(posedge clk) begin
 if (button_1 == 0) begin
 start <= 0;
 end else if (button_0 == 0) begin
 start <= 1;
 end
end

It is tempting to cram all logic into a single always block, but as the design becomes larger, it is better to partition the code into many blocks.

For lines like this:

if (start == 1) begin

it is common and simpler to omit the comparison to 1:

if (start) begin

There are also multiple nonblocking assignments to clockCounter. This is preferable:

always @(posedge clk) begin
 if (start) begin
 if (clockCounter == WAIT_TIME) begin
 clockCounter <= 0;
 ledCounter <= ledCounter + 1;
 end else begin
 clockCounter <= clockCounter + 1;
 end
 end
end

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

always_ff @(posedge clk) begin

The advantage is that it better conveys the design intent and it provides some built-in checks. However, this would force you to add a reset input signal to the module so that you are not initializing registers during declaration. You would initialize registers inside the always_ff blocks instead.

---

Testbench

When I view signal waveforms, I see that the design input signals change at the negedge of the clock signal. However, that is not evident from the testbench code because you use # delays.

I recommend changing the testbench to drive the signals on the posedge of the clock as you do inside the design:

initial begin
 clk <= 0;
 button_0 <= 1;
 button_1 <= 1;
 #(DELAY_10);
 repeat (5) @(posedge clk);
 button_0 <= 0;
 repeat (5) @(posedge clk);
 button_0 <= 1;
 repeat (100) @(posedge clk);
 button_1 <= 0;
 repeat (5) @(posedge clk);
 button_1 <= 1;
end

Currently you declare the design inputs as 4-state reg types in the testbench:

reg clk, button_0, button_1;

It would be better to use the 2-state bit type instead:

bit clk, button_0, button_1;

There is rarely the need to drive inputs as x or z, and 2-state signals offer simulation performance advantages.

Constants

It is great that you use named parameters for the constants.

These 2 seem related to one another:

parameter DELAY_5 = 5;
parameter DELAY_10 = 10;

Consider replacing them with:

parameter CLK_PERIOD = 10;

This name conveys more meaning. You can generate the clock with:

always #(CLK_PERIOD/2) clk = ~clk;