Variable output stream delay no shift register

Question

I'm looking to improve my Verilog coding form by reducing utilization and learning any tricks more experienced Verilogers may know.

This module takes in a pulse data stream with pulses occurring between 500 and 12500 clk cycles, and it needs to delay the stream by a specified number of clk cycles between 500 and 12500 clk cycles. But without using a 12500 bit shift register.

The process I was thinking to use was have 25, 14 bit registers as memory to handle the worst case of a pulse every 500 cycles and max delay of 12500 cycles, count the cycles to each pulse store them in memory, wait for the delay and output the pulses according to what's stored in memory. And I needed to add that unusedStorage parameter in order to ensure it always used each stored memory delay only once.

The current code is shown below:

module data_delay_counter(
 input clk,
 input en,
 input [13:0] ShiftNum,
 input s_IN,
 output reg s_OUT
 );
 // what is the minimum pulse rate (ie every 12500 clk cycles)
 // what is the maximum pulse rate (ie every 500 clk cycles)
 // what is the maximum delay (ie 12500 clk cycles)
 // if maximum delay is 12500, and maximum pulse rate is 500, and minimum pulse rate is 12500
 // then need to store a number up to 12500 so 14 bits, and store a maximum of 12500/500 = 25 numbers
 // need memory that stores 25, 14 bit numbers
 // if operating at a pulse every 500 cycles then max delay must be kept at 12500, but increasing cycles between pulses allows the delay to be increased
 /////////////////
 // counting phase
 reg [13:0] cyclesToInputPulse = 0; // counted clk cycles before a pulse
 // storage phase
 reg [4:0] memoryIndxWrite = 0; // index to write pulse information to
 reg [13:0] storage [0:24]; // memory for pulse information
 // delay phase
 reg [13:0] delayCounter = 0; // output delay counter
 // output phase
 reg [4:0] memoryIndxRead = 0; // index to read out pulse information from
 reg [13:0] cyclesToOutputPulse = 0; // counted clk cycles up to number read out from storage
 reg [24:0] unusedStorage = 0; // specifies if storage pulse info has been used or not, 1 for unused 0 for used
 // old shiftnum
 reg [13:0] oldShiftNum = 0; // stores the old ShiftNum to check if it has changed
 
 always @(posedge clk) begin
 if(ShiftNum != oldShiftNum) begin // check if ShiftNum has changed and reset parameters
 oldShiftNum = ShiftNum;
 cyclesToInputPulse = 0;
 memoryIndxWrite = 0;
 delayCounter = 0;
 cyclesToOutputPulse = 0;
 memoryIndxRead = 0;
 unusedStorage = 0;
 end 
 else if (en == 1) begin
 // counting phase
 if (s_IN != 1)
 cyclesToInputPulse = cyclesToInputPulse + 14'd1;
 else begin
 // storage phase 
 cyclesToInputPulse = cyclesToInputPulse + 14'd1;
 storage[memoryIndxWrite] = cyclesToInputPulse;
 cyclesToInputPulse = 14'd0;
 // when storage is written to unused storage at that index is set to 1, signifying its unused or "fresh"
 unusedStorage[memoryIndxWrite] = 1;
 memoryIndxWrite = memoryIndxWrite + 14'd1;
 if (memoryIndxWrite == 25)
 memoryIndxWrite = 14'd0;
 end
 // delay phase
 if (delayCounter != ShiftNum)
 delayCounter = delayCounter + 1;
 // output phase
 else begin
 // counts up to delay specified in storage and outputs pulse if stored delay is unused
 cyclesToOutputPulse = cyclesToOutputPulse + 14'd1;
 if (cyclesToOutputPulse == storage[memoryIndxRead] && unusedStorage[memoryIndxRead] == 1) begin
 s_OUT = 1;
 cyclesToOutputPulse = 14'd0;
 // specifies stored value has been used for an output pulse and sets unusedStorage at that indx to zero
 unusedStorage[memoryIndxRead] = 0;
 memoryIndxRead = memoryIndxRead + 14'd1;
 if (memoryIndxRead == 25)
 memoryIndxRead = 0;
 end
 else begin
 s_OUT = 0;
 end
 end
 end
 else begin
 cyclesToInputPulse = 0;
 memoryIndxWrite = 0;
 delayCounter = 0;
 cyclesToOutputPulse = 0;
 memoryIndxRead = 0;
 unusedStorage = 0;
 s_OUT = 0;
 end
 end 
endmodule

And the simulation results are shown here and don't change much pre or post synthesis:

simulation results

There's lots for me to improve on. I know nonblocking assignments should be used as much as possible within clocked always blocks. In these situations it seems like I need to use a stored value then update its contents. I don't really understand how to use a nonblocking assignment in a situation like that. I appreciate all the guidance. I'm new to Verilog and appreciate everyone taking the time to help me improve

Answer 1

Generally speaking, the code has consistent layout and is easy to understand. It uses descriptive variable names and comments. I don't see much room for improvement.

The recommended Verilog coding style is to use nonblocking assignments (<=) to describe sequential logic. Since your code consists of a single always block, and this block is sequential (it has posedge clk), all the assignments should be nonblocking. For example:

 oldShiftNum <= ShiftNum;

This recommendation is mainly to achieve predictable simulation results.

The remaining suggestions are purely for improved coding style.

I see repeated use of fixed numeric constants 13 and 14. You could consider replacing those with a named constant. For example:

parameter WIDTH = 14;
// ...
reg [WIDTH-1:0] cyclesToInputPulse 

This is a standard practice. You would use a name that is more descriptive in your design (but WIDTH may suffice here). It is a common convention to use all caps for constant names so that they stand out from variable signal names. Using names instead of numbers can make the code easier to understand. It also makes the code easier to change. For example, if you realize you need 16 bits instead of 14, you can just change one line of code instead of several lines.

When I see a line that has several operators, I sometimes find that hard to read. I prefer to use extra parentheses to separate the individual terms. For example:

if ((cyclesToOutputPulse == storage[memoryIndxRead]) && (unusedStorage[memoryIndxRead] == 1)) begin

I added 2 sets of extra parens. Sometimes this also avoids functional bugs due to operator precedence.

You have one very long comment line. You should definitely split that up into at least 2 lines.

When you have multiple single-line comments one after the other like that, you could convert that into a block comment using /**/. I find block comments to be easier to read and to maintain.

Currently, all your code is contained in a single always block. This is fine, especially since it fits all on one screen. If you need to make changes to the design to add logic, I would consider splitting it up into multiple always blocks. It could make the design easier to understand and maintain in the future.

---

SystemVerilog features

You could start adopting some newer features of the language. This may require you to explicitly enable these features in your tool set (or they may even be enabled by default).

One feature simplifies the design be replacing zero constants like 14'd0 with '0. This eliminates the need to specify the bit width.

Another feature is the always_ff keyword. Replace:

always @(posedge clk) begin

with:

always_ff @(posedge clk) begin

This better conveys the design intent, and it also allows tools to perform additional checks.

Refer to the IEEE Std 1800-2017 document for further details.

---

Here is a modified version of your code, employing several (but not all) the suggestions above. CAUTION: the code compiles without errors, but I did not simulate it.

I made a couple other style changes.

All the if/else clauses now use begin/end for consistency. I also prefer to keep end else begin on one line to save some vertical whitespace.

/*
 what is the minimum pulse rate (ie every 12500 clk cycles)
 what is the maximum pulse rate (ie every 500 clk cycles)
 what is the maximum delay (ie 12500 clk cycles)
 if maximum delay is 12500, and maximum pulse rate is 500, and minimum pulse rate is 12500
 then need to store a number up to 12500 so 14 bits, and store a maximum of 12500/500 = 25 numbers
 need memory that stores 25, 14 bit numbers
 if operating at a pulse every 500 cycles then max delay must be kept at 12500,
 but increasing cycles between pulses allows the delay to be increased
*/
module data_delay_counter (
 input clk,
 input en,
 input [13:0] ShiftNum,
 input s_IN,
 output reg s_OUT
 );
 // counting phase
 reg [13:0] cyclesToInputPulse = 0; // counted clk cycles before a pulse
 // storage phase
 reg [4:0] memoryIndxWrite = 0; // index to write pulse information to
 reg [13:0] storage [0:24]; // memory for pulse information
 // delay phase
 reg [13:0] delayCounter = 0; // output delay counter
 // output phase
 reg [4:0] memoryIndxRead = 0; // index to read out pulse information from
 reg [13:0] cyclesToOutputPulse = 0; // counted clk cycles up to number read out from storage
 reg [24:0] unusedStorage = 0; // specifies if storage pulse info has been used or not, 1 for unused 0 for used
 // old shiftnum
 reg [13:0] oldShiftNum = 0; // stores the old ShiftNum to check if it has changed
 
 always @(posedge clk) begin
 if (ShiftNum != oldShiftNum) begin // check if ShiftNum has changed and reset parameters
 oldShiftNum <= ShiftNum;
 cyclesToInputPulse <= 0;
 memoryIndxWrite <= 0;
 delayCounter <= 0;
 cyclesToOutputPulse <= 0;
 memoryIndxRead <= 0;
 unusedStorage <= 0;
 end else if (en) begin
 // counting phase
 if (s_IN != 1) begin
 cyclesToInputPulse <= cyclesToInputPulse + 14'd1;
 end else begin
 // storage phase 
 cyclesToInputPulse <= cyclesToInputPulse + 14'd1;
 storage[memoryIndxWrite] <= cyclesToInputPulse;
 cyclesToInputPulse <= 14'd0;
 // when storage is written to unused storage at that index is set to 1, signifying its unused or "fresh"
 unusedStorage[memoryIndxWrite] <= 1;
 memoryIndxWrite <= memoryIndxWrite + 14'd1;
 if (memoryIndxWrite == 25) begin
 memoryIndxWrite <= 14'd0;
 end
 end
 // delay phase
 if (delayCounter != ShiftNum) begin
 delayCounter <= delayCounter + 1;
 // output phase
 end else begin
 // counts up to delay specified in storage and outputs pulse if stored delay is unused
 cyclesToOutputPulse <= cyclesToOutputPulse + 14'd1;
 if ((cyclesToOutputPulse == storage[memoryIndxRead]) && (unusedStorage[memoryIndxRead] == 1)) begin
 s_OUT <= 1;
 cyclesToOutputPulse <= 14'd0;
 // specifies stored value has been used for an output pulse and sets unusedStorage at that indx to zero
 unusedStorage[memoryIndxRead] <= 0;
 memoryIndxRead <= memoryIndxRead + 14'd1;
 if (memoryIndxRead == 25) begin
 memoryIndxRead <= 0;
 end 
 end else begin
 s_OUT <= 0;
 end
 end
 end else begin
 cyclesToInputPulse <= 0;
 memoryIndxWrite <= 0;
 delayCounter <= 0;
 cyclesToOutputPulse <= 0;
 memoryIndxRead <= 0;
 unusedStorage <= 0;
 s_OUT <= 0;
 end
 end 
endmodule

• FillenNaymeer: Thank you so much for the help, both here and helping me getting it working over on stack overflow. This has been clear and concise and easy to understand. I've learned more on Verilog reading those 2 posts then all my time spent searching the net. Thanks again