SystemVerilog: LED blinker control logic

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;