Sensored BLDC commutation controller testbench code

Question

This is testbench code for a combinational logic circuit with 6 input variables and 6 output variables. The 3 binary HS (hall sensor) input variables are signals given by the BLDC motors' sensors that cycles through the binary state 001,011,010,110,101. The 3 UI (User inputs) are binary signals that is any of the 8 possible combinations of the 3 bits.
For more context, this is its truth table along with the wave generated by this code: https://github.com/SimNabong/Sensored-Brushless-DC-Motor-Controller/blob/main/Commutation%20Table%20and%20Wave%20Simulation.png

Here are extra reading materials to learn more about Brushless DC Motor Commutation Using Hall-Effect Sensors: https://www.ti.com/lit/ab/slvaeg3b/slvaeg3b.pdf?ts=1717465284379

`timescale 1ns/1ps
module CommutationControl_testbench();
 reg clk;
 reg [2:0] UI;
 reg [2:0] HS;
 wire [5:0] PT;
 
 CommutationControl stbinst(.clk(clk), .UI(UI), .HS(HS), .PT(PT));
 
 // Initialize the inputs and generate the clock signal
 initial begin
 clk = 0; // Set the clock input to 0
 forever #1 clk = ~clk; // Toggle the clock every T/2 units of time
 end 
 initial begin
 UI[2:0] = 3'b000;
 HS = 3'd0;
 
 //all off except the HS sensors
 #5 UI[2:0] = 3'b000; HS = 3'b100; //state 1
 #5 HS = 3'b110; //state 2
 #5 HS = 3'b010; //state 3
 #5 HS = 3'b011; //state 4
 #5 HS = 3'b001; //state 5
 #5 HS = 3'b101; //state 6
 
 // ccw on with HS sensors
 #5 UI[2:0] = 3'b010; HS = 3'b101; //state 6
 #5 HS = 3'b100; //state 1
 #5 HS = 3'b110; //state 2
 #5 HS = 3'b010; //state 3
 #5 HS = 3'b011; //state 4
 #5 HS = 3'b001; //state 5
 #5 HS = 3'b101; //state 6
 
 #5 UI[2:0] = 3'b000; HS = 3'b101; //state 6 with all off
 
 // cw on with HS sensors
 #5 UI[2:0] = 3'b100; HS = 3'b101; //state 6
 #5 HS = 3'b001; //state 5
 #5 HS = 3'b011; //state 4
 #5 HS = 3'b010; //state 3
 #5 HS = 3'b110; //state 2
 #5 HS = 3'b100; //state 1
 
 // Regen Break Two(cw and ccw) on with HS sensors
 #5 UI[2:0] = 3'b110; HS = 3'b101; //state 6
 #5 HS = 3'b001; //state 5
 #5 HS = 3'b011; //state 4
 #5 HS = 3'b010; //state 3
 #5 HS = 3'b110; //state 2
 #5 HS = 3'b100; //state 1
 
 #5 UI[2:0] = 3'b000; HS = 3'b100; //state 1
 
 //Regen Break One on with HS sensors
 #5 UI[2:0] = 3'b001; HS = 3'b100; //state 1
 #5 HS = 3'b110; //state 2
 #5 HS = 3'b010; //state 3
 #5 HS = 3'b011; //state 4
 #5 HS = 3'b001; //state 5
 #5 HS = 3'b101; //state 6
 
 //regen break and cw and ccw on with HS sensors should output all off
 #5 UI[2:0] = 3'b111; HS = 3'b100; //state 1
 #5 HS = 3'b110; //state 2
 #5 HS = 3'b010; //state 3
 #5 HS = 3'b011; //state 4
 #5 HS = 3'b001; //state 5
 #5 HS = 3'b101; //state 6
 
 #5 UI[2:0] = 3'b101; HS = 3'b100; //state 1
 #5 HS = 3'b110; //state 2
 #5 HS = 3'b010; //state 3
 #5 HS = 3'b011; //state 4
 #5 HS = 3'b001; //state 5
 #5 HS = 3'b101; //state 6
 
 #0 $finish;
 end
 
 initial begin
 $monitor("simtime=%g, clk=%b, UI=%b, HS%b, PT=%b", $time, clk,UI,HS,PT);
 end
 
endmodule

Answer 1

It is good that you did the following:

• Used port connection-by-name for the CommutationControl module instantiate
• Used a separate initial block for the clock generation code
• Print the simulation time because it helps with debugging
• Added comments
• Used proper and consistent indentation

Here are some suggestions for improvement.

Comments

The following comment is unnecessary because the code it describes is self-evident.
You should delete this comment:

// Set the clock input to 0

In the comment which includes "T/2", you should describe what "T" is.

Simpler

The #0 in the following line is not needed and should be deleted:

 #0 $finish;

Race condition

There is a Verilog simulation race condition because you drive the inputs using blocking assignments (=) and you sometimes make the assignments at the same time the clock is rising. It is also unexpected that you sometimes drive the inputs at the falling edge of the clock.

From your previous question, I see that the design module samples the input signals on posedge clk. Assuming the inputs are really synchronous to that edge of the clock, to guarantee that you avoid race conditions, you need to drive the inputs the same way your design drives its output:

• @(posedge clk)
• Using nonblocking (<=) assignments

For example:

 @(posedge clk);
 UI[2:0] <= 3'b000;
 HS <= 3'b100; //state 1
 @(posedge clk);
 HS <= 3'b110; //state 2
 @(posedge clk);
 HS = 3'b010; //state 3

Monitor

If you make the inputs synchronous to the clock as described above, then you can simplify the displayed output. It is usually unnecessary to display any clock edges, and you can reduce the amount of output by removing the clk signal from $monitor. Then, replace initial block with $monitor using an always block with $display:

always @(negedge clk) begin
 $display("simtime=%g, UI=%b, HS%b, PT=%b", $time, UI, HS, PT);
end

It is useful to display signal values when the are not changing (on negedge clk).

DRY

You use the same numeric constants in several places. These can be replaced with parameter types. There are 2 advantages to this:

1. The code is self-documenting, which means you can avoid the comments
2. If the value for a state, like state 1, changes, you don't have to change the comments every time you use the constant

For example:

parameter STATE1 = 3'b100;
parameter STATE2 = 3'b110;
parameter STATE3 = 3'b010;
 ...
 @(posedge clk);
 UI[2:0] <= 3'b000;
 HS <= STATE1;
 @(posedge clk);
 HS <= STATE2;
 @(posedge clk);
 HS <= STATE3;

You should also consider encapsulating the input code into Verilog tasks.

• Mister Moron: Excellent suggestion, especially the use of "parameter" to make the code self-documenting. I'm a bit confused about the possible Verilog simulation race condition problem because the inputs are not synchronous, since they are just conditions for a "wire". In my previous code the "net" or "wire" for the input conditions are inside a clocked procedural block, but that's after the inputs has already been assigned. So, to me it doesn't make any sense to use a non-blocking assignment since that would just unnecessarily delay the inputs in the simulation.
• Mister Moron: in my previous code, the whole thing could actually just be the combinatorial part since the "introduced dead-time" aka the anti-shoot-through feature that is in the clocked procedural block are usually done on the power electronics side. I had some thoughts that it might be beneficial to put it in the control side since it would take less power to do.
• toolic: @MisterMoron: That is why I said Assuming the inputs are really synchronous. If they are not synchronous, then using blocking assignments is fine. However, there is still a Verilog simulation race condition when you change the clock and the UI/HS inputs at the same time with blocking assignments. The simulator is not guaranteed to capture the input the same way every time. If your design can tolerate that, then there is no problem. Remember: Code Review is all about recommending good practices.
• Mister Moron: I missed the word "assuming" sorry. That is an excellent advice. Usually, I just manipulate the time for when the inputs change in the simulation. But I'll use that technique for my next simulations.