Velocity Reviews (http://www.velocityreviews.com/forums/index.php)
-   VHDL (http://www.velocityreviews.com/forums/f18-vhdl.html)
-   -   Problem with simple VHDL piece of code (http://www.velocityreviews.com/forums/t521485-problem-with-simple-vhdl-piece-of-code.html)

 Jaco Naude 07-12-2007 06:18 PM

Problem with simple VHDL piece of code

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

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;
==================================

 Frank Buss 07-12-2007 06:50 PM

Re: Problem with simple VHDL piece of code

Jaco Naude wrote:

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

--
Frank Buss, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

 Mike Treseler 07-12-2007 06:58 PM

Re: Problem with simple VHDL piece of code

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.

That's

if rst = '1'

Not

if rst <= '1'

etc.

-- Mike Treseler

 Jonathan Bromley 07-12-2007 07:24 PM

Re: Problem with simple VHDL piece of code

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,

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.

>> if rst <= '1' then

>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
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 07-12-2007 08:14 PM

Re: Problem with simple VHDL piece of code

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

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

 Jonathan Bromley 07-12-2007 08:22 PM

Re: Problem with simple VHDL piece of code

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:

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;

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.

 Frank Buss 07-13-2007 12:40 AM

Re: Problem with simple VHDL piece of code

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.

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.

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

 Jonathan Bromley 07-13-2007 07:59 AM

Re: Problem with simple VHDL piece of code

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.

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.

 Jaco Naude 07-13-2007 09:29 AM

Re: Problem with simple VHDL piece of code

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

 Andy 07-16-2007 10:26 PM

Re: Problem with simple VHDL piece of code

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

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

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

 All times are GMT. The time now is 10:10 PM.