Problem with simple VHDL piece of code

J

Jaco Naude

Guest
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;
==================================
 
Jaco Naude wrote:

process (clk)
variable counter : integer :=0;
Click to expand...
"counter" is initialized to 0 every time again when the process is entered,
use a signal.

if rst <= '1' then
Click to expand...
This looks strange. "<=" is an assignment operator, not a compare operator.
Use "=".

--
Frank Buss, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de
 
Jaco Naude wrote:
Hi all,

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

if rst = '1'

Not

if rst <= '1'

etc.

-- Mike Treseler
 
On Thu, 12 Jul 2007 20:50:58 +0200, Frank Buss <fb@frank-buss.de>
wrote:

Jaco Naude wrote:

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

"counter" is initialized to 0 every time again
when the process is entered,
Click to expand...
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.
Click to expand...
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.

if rst <= '1' then
i
This looks strange. "<=" is an assignment operator,
not a compare operator.
Click to expand...
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
jonathan.bromley@MYCOMPANY.com
http://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
On Thu, 12 Jul 2007 11:18:18 -0700, Jaco
Naude <naude.jaco@gmail.com> wrote:

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.
Click to expand...
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;
Click to expand...
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;
Click to expand...
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;
Click to expand...
OK. Synchronous reset; I'm sure that's what you intended,
but it's worth checking...

if start <= '1' and rst <= '0' then
Click to expand...
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
Click to expand...
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;
Click to expand...
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;
Click to expand...
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
jonathan.bromley@MYCOMPANY.com
http://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
On Thu, 12 Jul 2007 21:14:15 +0100, Jonathan Bromley
<jonathan.bromley@MYCOMPANY.com> wrote:

If I take all my own advice, I end up with something like
this:
Click to expand...
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; ---<<<<<<<---------
end if;
Click to expand...
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
jonathan.bromley@MYCOMPANY.com
http://www.MYCOMPANY.com

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

"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.
Click to expand...
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.

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.
Click to expand...
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.

--
Frank Buss, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de
 
On Fri, 13 Jul 2007 02:40:42 +0200,
Frank Buss <fb@frank-buss.de> wrote:

[...]
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.
Click to expand...
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
jonathan.bromley@MYCOMPANY.com
http://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
 
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
 
On Jul 12, 3:14 pm, Jonathan Bromley <jonathan.brom...@MYCOMPANY.com>
wrote:
On Thu, 12 Jul 2007 11:18:18 -0700, Jaco

Naude <naude.j...@gmail.com> wrote:
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
jonathan.brom...@MYCOMPANY.comhttp://www.MYCOMPANY.com

The contents of this message may contain personal views which
are not the views of Doulos Ltd., unless specifically stated.
Click to expand...
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
 
You must log in or register to reply here.

Welcome to EDABoard.com

Sponsor

Back
Top