Problem with simple VHDL piece of code

Discussion in 'VHDL' started by Jaco Naude, Jul 12, 2007.

  1. Jaco Naude

    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, Jul 12, 2007
    #1
    1. Advertising

  2. Jaco Naude

    Frank Buss Guest

    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,
    http://www.frank-buss.de, http://www.it4-systems.de
    Frank Buss, Jul 12, 2007
    #2
    1. Advertising

  3. 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
    Mike Treseler, Jul 12, 2007
    #3
  4. On Thu, 12 Jul 2007 20:50:58 +0200, Frank Buss <>
    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

    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, Jul 12, 2007
    #4
  5. On Thu, 12 Jul 2007 11:18:18 -0700, Jaco
    Naude <> 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

    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, Jul 12, 2007
    #5
  6. On Thu, 12 Jul 2007 21:14:15 +0100, Jonathan Bromley
    <> 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

    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, Jul 12, 2007
    #6
  7. Jaco Naude

    Frank Buss Guest

    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,
    http://www.frank-buss.de, http://www.it4-systems.de
    Frank Buss, Jul 13, 2007
    #7
  8. On Fri, 13 Jul 2007 02:40:42 +0200,
    Frank Buss <> 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

    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, Jul 13, 2007
    #8
  9. Jaco Naude

    Jaco Naude Guest

    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
    Jaco Naude, Jul 13, 2007
    #9
  10. Jaco Naude

    Andy Guest

    On Jul 12, 3:14 pm, Jonathan Bromley <>
    wrote:
    > On Thu, 12 Jul 2007 11:18:18 -0700, Jaco
    >
    > Naude <> 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
    > ://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
    Andy, Jul 16, 2007
    #10
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. jblazi
    Replies:
    5
    Views:
    421
    jblazi
    Aug 16, 2004
  2. Replies:
    6
    Views:
    411
    Keith Thompson
    May 17, 2006
  3. Alexander

    What is the problem with this piece of code?

    Alexander, Sep 17, 2010, in forum: C Programming
    Replies:
    7
    Views:
    327
    Keith Thompson
    Sep 20, 2010
  4. Patrick Plattes

    Download a file piece by piece

    Patrick Plattes, Nov 30, 2006, in forum: Ruby
    Replies:
    2
    Views:
    195
    Patrick Plattes
    Nov 30, 2006
  5. Guest

    problem with a piece of code

    Guest, Apr 6, 2004, in forum: Javascript
    Replies:
    2
    Views:
    129
    Dr John Stockton
    Apr 6, 2004
Loading...

Share This Page