Comment on my code style

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

On Wed, 28 Apr 2004 11:37:22 -0700, Chris 
Carlen <crcarle@BOGUS.sandia.gov> wrote:

[...]
>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.

`define is global to a Verilog compilation, and therefore cannot be 
controlled instance-by-instance.  There's no way to conditionalise
compilation instance-by-instance in standard Verilog, but if your
tools support Verilog-2001 (not a done deal by any means) then 
you can use "generate...if" to conditionalise some code based
on the value of a parameter, which of course can be pushed down
into an instance from its parent.

[...]
>> 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.

If it's in AoE it *must* be good :-)

> 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? 

Exactly so.  In general, it is unsafe for an asynchronous signal
to influence more than one FF.  The jargon is "input hazard".

> 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.

This is indeed the case.  If the counter is held at zero, then
only the very first flip-flop can be affected by the first clock
after reset is removed;  so the timing of reset w.r.t. clock
doesn't matter (except that you might get metastability on the
LSB counter bit, but that's another story).  However, if you are
trying to clock the counter very fast, it's just conceivable that
the skew on Reset between bit 0 and bit 1 of the counter is
greater than one clock period;  in this case, it's just possible
that the counter could successfully increment from 0 to 1, but
the next bit is still held in reset by the time the NEXT clock
arrives - so the counter goes back to zero again.  However,
this is very unlikely because the synth tools will surely lay
out the counter so that its FFs are rather close together (so
that it can use the ripple carry chain) and therefore the reset
distribution skew is likely to be quite small.  So don't worry,
it's just me being paranoid as usual.

>> 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? 

I think it's OK.  Out is held in a FF.  I think the design is 
a reliable asynchronous state machine, but when I said "nervous" 
I meant it - these things are hard to analyze, and it's very 
easy to screw up by extrapolating from a working design to 
a slightly different one that fails horribly, usually just
after shipment.

>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.

Sure.  A fully synch design has -0/+1 "jitter" both at the start and 
at the end, I think.  No escape.  (It's not "jitter" in the usual
sense, rather "quantization error", but I guess that depends on what
you're trying to do with the resulting signal).

OTOH you should trawl through the last few months' comp.arch.fpga
archives and take a look at what Peter Alfke and others have had
to say about using polyphase clocks for very high resolution 
time measurements.  The Xilinx DLLs make all sorts of interesting
things possible.  I haven't followed that discussion in detail,
but it sounds like a lot of fun.
-- 
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.

Jonathan Bromley wrote:
>>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? 
> 
> 
> Exactly so.  In general, it is unsafe for an asynchronous signal
> to influence more than one FF.  The jargon is "input hazard".

I see.  I will have to keep this in mind for future stuff.

>>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.
> 
> 
> This is indeed the case.  If the counter is held at zero, then
> only the very first flip-flop can be affected by the first clock
> after reset is removed;  so the timing of reset w.r.t. clock
> doesn't matter (except that you might get metastability on the
> LSB counter bit, but that's another story).  However, if you are
> trying to clock the counter very fast, it's just conceivable that
> the skew on Reset between bit 0 and bit 1 of the counter is
> greater than one clock period;  in this case, it's just possible
> that the counter could successfully increment from 0 to 1, but
> the next bit is still held in reset by the time the NEXT clock
> arrives - so the counter goes back to zero again.  However,
> this is very unlikely because the synth tools will surely lay
> out the counter so that its FFs are rather close together (so
> that it can use the ripple carry chain) and therefore the reset
> distribution skew is likely to be quite small.  So don't worry,
> it's just me being paranoid as usual.

Things should be Ok at my 10kHz clock rate ;-D

> Sure.  A fully synch design has -0/+1 "jitter" both at the start and 
> at the end, I think.  No escape.  (It's not "jitter" in the usual
> sense, rather "quantization error", but I guess that depends on what
> you're trying to do with the resulting signal).
> 
> OTOH you should trawl through the last few months' comp.arch.fpga
> archives and take a look at what Peter Alfke and others have had
> to say about using polyphase clocks for very high resolution 
> time measurements.  The Xilinx DLLs make all sorts of interesting
> things possible.  I haven't followed that discussion in detail,
> but it sounds like a lot of fun.


I noticed that discussion, and my reaction was "what in the heck are 
they talking about?"  I think after getting some experience with logic 
at the CPLD level I will head towards FPGAs.  I'd like to first complete 
a formal text or course on the matter.

Thanks for the input.


Good day!


-- 
_____________________
Christopher R. Carlen
crobc@earthlink.net
Suse 8.1 Linux 2.4.19