Test bench critique. First time using tasks and $display

[Dmeads]

Hello all, hobbyist here. If anyone has some time, could I get a little feedback on a test bench I wrote? It is my first time using tasks and the $display statement, but I like having an output log, and the tasks kind of seem like re-usable functions for a test bench, which simplify my "initial" block. I dont think I can use "function" in a test bench because those cant have "#time" in them? Is this correct?

The code is for SPI with a MCP3202 ADC. This is the third version, all have worked, but I feel like my coding and testing has gotten much better. Still looking for improvement though.

Previously, my test benches were extremely basic like below. I would just visually check the waveform to see if everything was working.

Code: [Select]

initial
  begin
    do_something
    #40
    do_something_else
    #700
    do_another_thing
    #500
    $finish;
  end

any feedback would be appreciated. Thanks in advance.

Here is a link to the github repo: https://github.com/dominic-meads/ECG/tree/main/ADC/SPI_core

[Rainwater]

I am new to verilog as well. but here is what I know

I dont think I can use "function" in a test bench because those cant have "#time" in them? Is this correct?

You can use functions in your test bench. You can not use #time statements in a function

The code is for SPI with a MCP3202 ADC. This is the third version, all have worked, but I feel like my coding and testing has gotten much better. Still looking for improvement though.

I started writing verilog earlier this year and quickly got caught in writing massive test benches, until I read about Yosys. Now my test benches have became more of a usage reference and API documentation than actual test.
Would give it 6 of 5 if the documentation was better, that being said, I now write more verification code than I do actually hardware.
I would have to say its about the same number of keystrokes, but the testing is much better than anything I could write or imagine.

The only real suggestion I have is one given to me a few months ago by a professor, and that is to give each register its own always block.
starting at MCP3202_SPI_500sps.v line 119 through to line 206 you have your state machine.
In it you are implementing a lot of conditional assignments.
Its easy to follow, but with larger modules will become harder to read.
by refactoring the code where each register is assigned in its own always block, operation becomes very clear.
for example, I had trouble figuring out where 'r_tcsh_clk_cntr_en' was being assigned.
its in all 4 states, even tho it only changes in 2 of those states.
it does not affect anything about how your code works, but this could be made more clear with only 2 assignments.

your timers, line 91 and 105 are using a 32 bit vector to count to 900.
if this is a fixed value, use a parameter to make it easy to change if needed and try something like

Code: [Select]

parameter COUNTER_1_CMP = 899;
...
reg[$clog2(COUNTER_1_CMP)-1:0] r_sck_cntr = 0;$clog2 is the equivalent of cmath.h ceil( log2( value ) ); it will tell you how many bits are needed to store a decimal value.
$clog(2) = 1; $clog(3) = 2; $clog(4) = 2; $clog2(5) = 3; $clog(900) =  10;
on a calculator its log(900)÷log(2) and then round up;

I like the

Code: [Select]

TCSH_CLK_CNTS_MAX = 2e-3 * FCLK - 15300; syntax, I didn't know we could do that.
Does that equal 2x10³ or 2x10^-3?
I did know about

Code: [Select]

localparam big_number = 300_000_000_000;
lookin good, fixing those big counters should really improve your fmax.

[Dmeads]

Thank you very much for taking the time to read and reply with feedback!

I will definitely check out Yosys.

Thanks for the suggestion to get the conditional assignments out of my state logic section. A larger state machine would have wayy to much going on. A single always block is much easier to follow for each register.

The tip about $clog2 is awesome. I will definitely be implementing that. It's like an automatic register size calculation.

the

Code: [Select]

TCSH_CLK_CNTS_MAX = 2e-3 * FCLK - 15300;
is pretty cool! it means 2*10^-3, I cant remember where I saw it in an example tho... It did give an error on synthesis when trying to use the parameter as a maximum for a counter in an always block, but was fixed by having a register with the same value. Probably just a data type thing.

I didnt know about the

Code: [Select]

localparam big_number = 300_000_000_000;Thats pretty cool too!