Problem with simple VHDL piece of code

J

Jaco Naude

Hi all,

I'm wondering if any VHDL expert out there can tell me why the
following piece of code isn't working. It should be a simple clock
divider, enabling the CE signal on every counter'th pulse. However the
counter increase line counter := 1+ counter does not increment the
variable. If I change the 1 to for example 20 it stays 20 throughout
the simulation. I'm sure the input signals are correct... Any
suggestions would be helpful.

Thanks,
Jaco

=================================
begin

process (clk)
variable counter : integer :=0;

begin
if (clk'event and clk = '1') then
counter_out <= counter;

if rst <= '1' then
counter := 0;
ce_out <= '1';
end if;

if start <= '1' and rst <= '0' then
if counter < counter_top + 1 then
ce_out <= '0';
counter := 1 + counter;
else
counter := 0;
ce_out <= '1';
end if;
else
ce_out <= '1';
end if;
end if;
end process;


end Behavioral;
==================================
 
F

Frank Buss

Jaco said:
process (clk)
variable counter : integer :=0;

"counter" is initialized to 0 every time again when the process is entered,
use a signal.
if rst <= '1' then

This looks strange. "<=" is an assignment operator, not a compare operator.
Use "=".
 
M

Mike Treseler

Jaco said:
Hi all,

I'm wondering if any VHDL expert out there can tell me why the
following piece of code isn't working.

That's

if rst = '1'

Not

if rst <= '1'

etc.

-- Mike Treseler
 
J

Jonathan Bromley

"counter" is initialized to 0 every time again
when the process is entered,

This is untrue. Variable initialisations in a process
occur precisely once, before the process begins to execute
at time zero. Maybe you're thinking about variables in
a *procedure* or function, which are re-initialised
(indeed, are constructed from scratch) every time
the subprogram is executed.
use a signal.

If that's your favoured coding style, so be it;
for me the variable makes good sense, although
I'm not totally sure the OP really wants a
pipeline delay of one clock from the counter
itself to the output register count_out.
i
This looks strange. "<=" is an assignment operator,
not a compare operator.

Well, it's a compare operator too; but as Mike said,
"less than or equal" on a single-bit std_logic is
not a very useful thing to do.
--
Jonathan Bromley, Consultant

DOULOS - Developing Design Know-how
VHDL * Verilog * SystemC * e * Perl * Tcl/Tk * Project Services

Doulos Ltd., 22 Market Place, Ringwood, BH24 1AW, UK
(e-mail address removed)
http://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
J

Jonathan Bromley

I'm wondering if any VHDL expert out there can tell me why the
following piece of code isn't working. It should be a simple clock
divider, enabling the CE signal on every counter'th pulse. However the
counter increase line counter := 1+ counter does not increment the
variable. If I change the 1 to for example 20 it stays 20 throughout
the simulation. I'm sure the input signals are correct... Any
suggestions would be helpful.

I'm fairly sure that it's the <= comparisons that are breaking
your design, but while we have the code in front of us there
are a few design style issues that might be worth pursuing.
Would it be fair to guess that you are by habit a software
person, moving into hardware? :)
process (clk)
variable counter : integer :=0;

Initialisation doesn't usually work in hardware. Better
to use an explicit reset of some kind (as you have done).
begin
if (clk'event and clk = '1') then
counter_out <= counter;

I'm guessing this is just a diagnostic output, so that you
can easily see what "counter" is doing? Note that it lags
behind "counter" by one clock cycle.
if rst <= '1' then
counter := 0;
ce_out <= '1';
end if;

OK. Synchronous reset; I'm sure that's what you intended,
but it's worth checking...
if start <= '1' and rst <= '0' then

if rst='1' then
....reset actions
elsif start='1' then
....main code body
end if

would perhaps have been neater, and easier to follow.
if counter < counter_top + 1 then

Again I'm guessing. counter_top is an incoming signal, perhaps
the contents of a writeable register? This comparison is rather
expensive in hardware, since you are building an incrementer
and a magnitude comparator. For dividers like this, it's
almost always better to preset the counter to the limit
value and then count it down until it reaches a constant
(1 or 0, in most situations).
ce_out <= '0';
counter := 1 + counter;

Are you happy for ce_out to freeze at '1' if someone drops the
'start' signal at an inopportune moment? It may be better to
default ce_out to '0' and set it to '1' only for a single clock
when the wraparound occurs.
else
counter := 0;
ce_out <= '1';
end if;
else
ce_out <= '1';
end if;
end if;
end process;

Finally, your integer counter seems to be unconstrained;
consequently, it will probably be synthesised to 32 bits.
There are various opinions about this, but my own practice
is always to use the numeric_std UNSIGNED or SIGNED vector
types rather than integers.

If I take all my own advice, I end up with something like
this:

constant counter_bits: positive := 16; -- or maybe a generic
signal counter_top: unsigned(counter_bits-1 downto 0);
signal ce_out, start, rst: std_logic;
....
process (clk)
variable counter: unsigned(counter_top'range);
begin
if rising_edge(clk) then
ce_out <= '0';
if rst = '1' then
ce_out <= '1';
counter := counter_top;
elsif start = '1' then
if counter = 0 then
ce_out <= '1';
counter := counter_top;
else
counter := counter - 1;
end if;
end if;
end process;

Note that this generates ce_out with a period of
(counter_top + 1) cycles. If you want the period
to be exactly (counter_top) then you should test
"if counter=1" for the wraparound. The limit
comparator is now trivial, and involves no arithmetic.

Also, note that ce_out will be asserted for the whole
time that rst is asserted. If you don't want that, it
might be better to reset the counter to 1 so that ce_out
is asserted on the first clock after reset is released.

I hope it's clear from the above that you have a lot of
choices, and my suggestions may or may not be useful
depending on what else is happening in your system.

HTH
--
Jonathan Bromley, Consultant

DOULOS - Developing Design Know-how
VHDL * Verilog * SystemC * e * Perl * Tcl/Tk * Project Services

Doulos Ltd., 22 Market Place, Ringwood, BH24 1AW, UK
(e-mail address removed)
http://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
J

Jonathan Bromley

If I take all my own advice, I end up with something like
this:

But not *exactly* like that. I missed an "end if". Whoops.
elsif start = '1' then
if counter = 0 then
ce_out <= '1';
counter := counter_top;
else
counter := counter - 1;
end if; ---<<<<<<<---------

See my forthcoming monograph on the unavoidable race conditions
that exist between hitting Send and reviewing the code :)
--
Jonathan Bromley, Consultant

DOULOS - Developing Design Know-how
VHDL * Verilog * SystemC * e * Perl * Tcl/Tk * Project Services

Doulos Ltd., 22 Market Place, Ringwood, BH24 1AW, UK
(e-mail address removed)
http://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
F

Frank Buss

Jonathan said:
This is untrue. Variable initialisations in a process
occur precisely once, before the process begins to execute
at time zero.

Thanks, you are right. I thought variable initialization are executed
multiple times, but this doesn't make sense for processes, which are always
running all the time.
Well, it's a compare operator too; but as Mike said,
"less than or equal" on a single-bit std_logic is
not a very useful thing to do.

Ok, but then I don't understand why "counter" is incremented one time, but
not multiple times. I think one reason could be that "start" is 1 for one
clock cycle, only.
 
J

Jonathan Bromley

On Fri, 13 Jul 2007 02:40:42 +0200,

[...]
I don't understand why "counter" is incremented one time, but
not multiple times. I think one reason could be that "start"
is 1 for one clock cycle, only.

Agreed. In my longer response I took the easy way out and
assumed that "start" in the OP's design was in fact an enable
signal that would normally be held asserted throughout the
divider's operation, and could be taken false to pause it.
If it's intended to be a one-shot "starting pistol" signal,
then it would be necessary to hold the counter at some fixed
value until "start", or alternatively build a little state
machine to derive an enable from the "start" input.

It's astonishing how many ambiguities can arise in the
description or specification of such a simple design.
Coding an RTL design is the easy part. Getting the spec
right, and verifying your design's conformance to it, is
massively more difficult.
--
Jonathan Bromley, Consultant

DOULOS - Developing Design Know-how
VHDL * Verilog * SystemC * e * Perl * Tcl/Tk * Project Services

Doulos Ltd., 22 Market Place, Ringwood, BH24 1AW, UK
(e-mail address removed)
http://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
J

Jaco Naude

Thanks alot for all the input, really appreciated.

Yip I'm much more familiar with C/C++ etc. than with VHDL and what I
know about it I learned myself so no textbook stuff as you pointed
out.

The problem was indeed with the following section of the code. When I
changed it to a = operator it incremented correctly:
if rst <= '1' then
counter := 0;
ce_out <= '1';
end if;

Jonathan you listed a few very interesting points that I was not aware
of and I will definitely use it.

A few notes on what you guys wrote and that I probably should have
included in my original post:
Start signal will be an enable signal that stays high while the code
must be active. rst is the system reset. The clock divider circuit is
intended to decimate a LFSR sequence during the generation of a Kasami
sequence.

Thanks again for the input,
Jaco
 
A

Andy

I'm fairly sure that it's the <= comparisons that are breaking
your design, but while we have the code in front of us there
are a few design style issues that might be worth pursuing.
Would it be fair to guess that you are by habit a software
person, moving into hardware? :)


Initialisation doesn't usually work in hardware. Better
to use an explicit reset of some kind (as you have done).


I'm guessing this is just a diagnostic output, so that you
can easily see what "counter" is doing? Note that it lags
behind "counter" by one clock cycle.


OK. Synchronous reset; I'm sure that's what you intended,
but it's worth checking...


if rst='1' then
....reset actions
elsif start='1' then
....main code body
end if

would perhaps have been neater, and easier to follow.


Again I'm guessing. counter_top is an incoming signal, perhaps
the contents of a writeable register? This comparison is rather
expensive in hardware, since you are building an incrementer
and a magnitude comparator. For dividers like this, it's
almost always better to preset the counter to the limit
value and then count it down until it reaches a constant
(1 or 0, in most situations).


Are you happy for ce_out to freeze at '1' if someone drops the
'start' signal at an inopportune moment? It may be better to
default ce_out to '0' and set it to '1' only for a single clock
when the wraparound occurs.


Finally, your integer counter seems to be unconstrained;
consequently, it will probably be synthesised to 32 bits.
There are various opinions about this, but my own practice
is always to use the numeric_std UNSIGNED or SIGNED vector
types rather than integers.

If I take all my own advice, I end up with something like
this:

constant counter_bits: positive := 16; -- or maybe a generic
signal counter_top: unsigned(counter_bits-1 downto 0);
signal ce_out, start, rst: std_logic;
....
process (clk)
variable counter: unsigned(counter_top'range);
begin
if rising_edge(clk) then
ce_out <= '0';
if rst = '1' then
ce_out <= '1';
counter := counter_top;
elsif start = '1' then
if counter = 0 then
ce_out <= '1';
counter := counter_top;
else
counter := counter - 1;
end if;
end if;
end process;

Note that this generates ce_out with a period of
(counter_top + 1) cycles. If you want the period
to be exactly (counter_top) then you should test
"if counter=1" for the wraparound. The limit
comparator is now trivial, and involves no arithmetic.

Also, note that ce_out will be asserted for the whole
time that rst is asserted. If you don't want that, it
might be better to reset the counter to 1 so that ce_out
is asserted on the first clock after reset is released.

I hope it's clear from the above that you have a lot of
choices, and my suggestions may or may not be useful
depending on what else is happening in your system.

HTH
--
Jonathan Bromley, Consultant

DOULOS - Developing Design Know-how
VHDL * Verilog * SystemC * e * Perl * Tcl/Tk * Project Services

Doulos Ltd., 22 Market Place, Ringwood, BH24 1AW, UK
(e-mail address removed)://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.

OTOH, if you want to use integers, here's how:

....
variable counter : integer range 0 to 2**counter_bits-1;
....
counter := to_integer(counter_top); -- unless counter_top was also
integer
....

A smart synthesizer might do a reachability analysis on the counter,
and determine that it always contains values between 0 and
counter_top, inclusive, and size counter appropriately all by itself.
Most synthesizers would, I fear, not be so smart.

A smart synthesizer might also be able to use the carry (borrow) out
bit from the decrementer to determine if the count was zero.
Otherwise, one could code it as follows:

....
if count - 1 < 0 then -- don't try this with unsigned/slv
....

Andy
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,479
Members
44,900
Latest member
Nell636132

Latest Threads

Top