Comment on my code style

C

Chris Carlen

Guest
Greetings:

I am learning Verilog from the book "A Verilog Primer 2nd. ed." by J.
Bhasker.

Here's my crack at a digital one shot. It works fine, or course, since
I have already used it in real hardware. But I am interested in
pointers on coding style or any other comments people might like to make:

Note: some of the delays and the initial statement are for making the
functional simulation work, but I know they get ignored during synthesis.


----------------------------------------------------------
/* This module implements a digital one-shot with N-bit resolution.
Ports:
Trig_in rising edge causes output pulse to begin
Out positive going output pulse appears here
Clk_in user must supply a clock here
Delay user must supply an N-bit value here for the number of

clock periods to delay

How it works:

An N-bit counter gets clocked all the time by the incoming clock signal.
A D-flop latches the input trigger and also enables the counter. The
output pulse is taken from the D-flop Q output. Thus the leading edge
of the output pulse is synchronous with the trigger edge. But the
trailing edge of the output will be synchronous with the clock.

At the end of a delay cycle, the D-flop Q output falls and resets the
counter, preparing it to count again. An incoming trigger pulse sets
the D-flop, causing Q to rise, releasing the CLR of the counter. When
the count reaches the Delay value, the D-flop is reset. This causes the
Q to fall again, clearing and holding the counter until the next trigger.
*/

module DelayNbit(Trig_in, Out, Clk_in, Delay);
parameter NUM_BITS = 8; // This sets the default number of bits of

counter resolution
input Trig_in, Clk_in;
input [NUM_BITS-1:0] Delay;
output Out;
wire Trig_in, Clk_in;
wire [NUM_BITS-1:0] Delay;
reg Out;

wire Clr_ff; // flip-flop asynchronous clear input

reg [NUM_BITS-1:0] Q_c; // register for N-bit counter

/* BEGIN behavioral model of the D flip-flop with asynchronous clear,
adapted from Xilinx <lib.pdf>:
*/
initial
Out = 0;

always @ ( posedge Trig_in or posedge Clr_ff )
begin
if ( Clr_ff == 1 )
#4 Out <= 0;
else
#4 Out <= 1; // we tie the D input to logical high
end

// END of D flip-flop behavioral model


/* BEGIN behavioral model of N-bit counter with asynchronous clear,
adapted from Xilinx <lib.pdf> :
*/
always @ ( posedge Clk_in or negedge Out )
begin
if ( !Out )
#4 Q_c <= 0;
else
#4 Q_c <= Q_c + 1;
end

// END of N-bit counter behavioral model


assign #4 Clr_ff = ( Q_c == Delay ); /* this compares the count with
Delay, and when equal
resets the flip-flop. */

endmodule
----------------------------------------------------------

Thanks for input.

Good day!



--
____________________________________
Christopher R. Carlen
Principal Laser/Optical Technologist
Sandia National Laboratories CA USA
crcarle@sandia.gov
 
On Wed, 28 Apr 2004 08:29:38 -0700, Chris Carlen
<crcarle@BOGUS.sandia.gov> wrote:

I am interested in
pointers on coding style or any other
comments people might like to make:
Interesting...

You've coded two standard clocked "always" blocks - fine.

You've declared all your inputs as "wire". That's not
necessary, but it's a good idea.

I have a few issues with the cosmetics of your code -
in particular I don't like using /*...*/ block
comments, because they don't nest and you can easily
get yourself into trouble with them; but that's
a matter of style choice.

Note: some of the delays and the initial statement are for making the
functional simulation work, but I know they get ignored during synthesis.
You don't *need* the delays for functional simulation, because
nonblocking (<=) assignment introduces a "delta" delay anyhow;
but it can often be nicer to *see* the delays on a waveform
viewer. Perhaps you could consider parameterising the delays,
or using `define so it's easy to switch them in or out
according to what you're doing with the model...

// Remove "#4" from this line if you don't want delays...
`define `FF_DELAY #4
....

always @(...)
Q <= `FF_DELAY D+1;

.... you get the idea.


But my main concerns relate to...

Here's my crack at a digital one shot. It works fine,
or course, since I have already used it in real
hardware.
hmmmm.... the design has some "interesting" non-synchronous
features that don't sit comfortably in FPGAs - I guess you're
targeting Xilinx, from your comments - it would be very
interesting to know your reasons for being confident about
its reliability. I *think* I understand that it's OK to
remove the reset (!Out) on Q_c asynchronously, although it
could easily be subverted by a sufficiently fast Clk_in.
I also am nervous of the clearing of Out by a pulse
which is immediately terminated by Out clearing...

Generally it's very dangerous to use the asynchronous
resets of FPGA flip-flops for anything other than
startup initialisation. Timing analysis of the reset
paths into a FF is vexatious at best.

Finally, from an FPGA point of view, these timeout
counters are usually more compact if you use the trigger
to load the counter with the delay value, then count
down to zero; equality comparison with a constant is
much cheaper than equality comparison with a variable.
However, that strategy would (I think) demand a fully
synchronised design.
--
Jonathan Bromley, Consultant

DOULOS - Developing Design Know-how
VHDL, Verilog, SystemC, Perl, Tcl/Tk, Verification, Project Services

Doulos Ltd. Church Hatch, 22 Market Place, Ringwood, BH24 1AW, UK
Tel: +44 (0)1425 471223 mail:jonathan.bromley@doulos.com
Fax: +44 (0)1425 471573 Web: http://www.doulos.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
"Chris Carlen" <crcarle@BOGUS.sandia.gov> wrote in message
news:c6oil102ndg@news3.newsguy.com...
Greetings:

I am learning Verilog from the book "A Verilog Primer 2nd. ed." by J.
Bhasker.

Here's my crack at a digital one shot. It works fine, or course, since
I have already used it in real hardware. But I am interested in
pointers on coding style or any other comments people might like to make:
[snip]

always @ ( posedge Trig_in or posedge Clr_ff )
begin
if ( Clr_ff == 1 )
#4 Out <= 0;
else
#4 Out <= 1; // we tie the D input to logical high
end
I have a personal preference to improve readability of my own code. I have
trouble digesting code that's sprawled over several pages so I try to
compress things vertically when I can without crowding the code too much.

When an always block has just one construct - an if/else for async
set/reset, for instance - the begin/end is superfluous. Indentation keeps
my visual clues telling me what belongs in one begin/end construct or stands
alone as one statement. When if/else constructs have short assignments, I
like to do those in-line if the code isn't crowded. I'd code the block
above as:

always @ ( posedge Trig_in or posedge Clr_ff )
if ( Clr_ff == 1 ) #4 Out <= 0;
else #4 Out <= 1;

If you're viewing with the default proportianal-spaced font, however, the
"cleanliness" of the code is lost. All my code authoring is done in
fixed-width fonts allowing a clean alignment. A complex always block
usually takes up a half screen or less on my system allowing a chance to
study what's going on just by staring at it long enough.

But other people like more stuff & structure.

A note on my indenting:
always @(...
single <= statement;
^^ indentation

always @(...
begin
multiple;
statements;
end
^^ indentation of statements, not of begin/end keywords

Again, personal preference.
 
Jonathan Bromley wrote:
On Wed, 28 Apr 2004 08:29:38 -0700, Chris Carlen
crcarle@BOGUS.sandia.gov> wrote:


I am interested in
pointers on coding style or any other
comments people might like to make:


Interesting...

You've coded two standard clocked "always" blocks - fine.

You've declared all your inputs as "wire". That's not
necessary, but it's a good idea.

I have a few issues with the cosmetics of your code -
in particular I don't like using /*...*/ block
comments, because they don't nest and you can easily
get yourself into trouble with them; but that's
a matter of style choice.
Ok.

You don't *need* the delays for functional simulation, because
nonblocking (<=) assignment introduces a "delta" delay anyhow;
but it can often be nicer to *see* the delays on a waveform
viewer.
Yes, I discovered this, and simply wanted to make it look nicer so put
in the delays, also knowing the synthesis would simply whine then ignore
them.

Perhaps you could consider parameterising the delays,
or using `define so it's easy to switch them in or out
according to what you're doing with the model...

// Remove "#4" from this line if you don't want delays...
`define `FF_DELAY #4
...

always @(...)
Q <= `FF_DELAY D+1;

... you get the idea.
Maybe parameterizing is a good idea, because I was hoping to figure out
a way to have the delays active dependent upon who was instantiating the
module, which can be controlled by using "module instance parameter
value assignment" or defparam.

But there is still the question about how to direct conditional
compilation of statements in an instantiated module from the
instantiating module. Ie., I'd like to have an `ifdef to conditionally
compile for instance the initial statement which is supoerfluous to
implementation, but not for simulation. Or I'd like to not have to edit
the module to change the defines, but simply adjust them from the top
level. I don't quite know how this works yet. I'll have to do some
experimenting.

But my main concerns relate to...


Here's my crack at a digital one shot. It works fine,
or course, since I have already used it in real
hardware.


hmmmm.... the design has some "interesting" non-synchronous
features that don't sit comfortably in FPGAs - I guess you're
targeting Xilinx, from your comments - it would be very
interesting to know your reasons for being confident about
its reliability.
Good question. Actually I suppose I can't be certain when implementing
in a PLD. I am using XPLA3 CPLDs at the moment. I am confident in the
circuit implemented in discrete logic, of course, because one can depend
on the propagation delays to be sure that the main flip-flop reset
remains true until it is removed after propagating the counter reset
through the flip-flop, counter, and comparator.

But there are indeed questions about what goes on in the CPLD. Perhaps
it would only be 100% safe if I coded it structurally and gate level,
and used "WYSIWYG" mode of the implementation options.


I *think* I understand that it's OK to
remove the reset (!Out) on Q_c asynchronously, although it
could easily be subverted by a sufficiently fast Clk_in.
I hadn't thought about what could go wrong here with the clock being
close to the reset. I got the basic circuit from Art of Electronics,
2nd. ed., page 523. Are you thinking it is possible for some of the
flops in the counter to interpret a clock edge close to the release of
reset as happening before the reset removal, but some of the other flops
interpreting it as not happening before the reset removal? One can
envision this happening due to propagation delays over physical wires.
But perhaps in the case of starting from zero, this isn't an issue. I'd
have to think more carefully about the guts of the counter to be sure
about this.

I also am nervous of the clearing of Out by a pulse
which is immediately terminated by Out clearing...
Yeah, I addressed this above, but what do you think of my analysis?
First tell me (since you are probably a more experienced logic designer
in general) how comfortable you would be with the design in discrete
logic? Then in a PLD (considering my thoughts above)?

Generally it's very dangerous to use the asynchronous
resets of FPGA flip-flops for anything other than
startup initialisation. Timing analysis of the reset
paths into a FF is vexatious at best.

Finally, from an FPGA point of view, these timeout
counters are usually more compact if you use the trigger
to load the counter with the delay value, then count
down to zero; equality comparison with a constant is
much cheaper than equality comparison with a variable.
However, that strategy would (I think) demand a fully
synchronised design.
Well that's probably safer. I am becomming very interested in digital
delay generation and the question of how to reduce jitter. Of course,
my design has no leading edge jitter, and has up to one clock period of
trailing edge jitter. A fully synchronous design would do what? I
suppose there would be synchronization jitter on the leading edge (the
trigger edge), but then the time would be without jitter. There has to
be jitter somewhere.

Ok, thanks for the input.

Good day!



--
____________________________________
Christopher R. Carlen
Principal Laser/Optical Technologist
Sandia National Laboratories CA USA
crcarle@sandia.gov
 

Welcome to EDABoard.com

Sponsor

Back
Top