Multiple drivers problem with a simple set-enable counter process

Discussion in 'VHDL' started by Maxim, Mar 8, 2010.

  1. Maxim

    Maxim Guest

    Hi everyone!

    I am trying to code a watchdog timer in VHDL, but the simulator
    (ModelSim) says there are multiple drivers for signal timer_reg. The
    timer is described as :

    timer : process(clk) is
    begin
    if rising_edge(clk) and state_next = dps then -- dps state used
    as enable
    if fall_edge = '1' then -- falling edge tick used as set
    signal
    timer_reg <= "0111";
    else
    timer_reg <= timer_reg-1;
    end if;
    end if;
    end process timer;

    The timer is charged OK to "0111" value, then is stuck at this value
    and refuses to decrement... even though an assert put in the else
    statement confirms it is visited at each rising_edge of the clk when
    state_next = dps. Any thoughts about what I'm not doing right?

    Thank you very much.
     
    Maxim, Mar 8, 2010
    #1
    1. Advertising

  2. Maxim

    rickman Guest

    On Mar 8, 1:50 pm, Alan Fitch <> wrote:
    > On 08/03/10 17:38, Maxim wrote:
    >
    > > Hi everyone!

    >
    > > I am trying to code a watchdog timer in VHDL, but the simulator
    > > (ModelSim) says there are multiple drivers for signal timer_reg. The
    > > timer is described as :

    >
    > > timer : process(clk) is
    > >   begin
    > >     if rising_edge(clk) and state_next = dps then  -- dps state used
    > > as enable
    > >       if fall_edge  = '1' then  -- falling edge tick used as set
    > > signal
    > >         timer_reg <= "0111";
    > >       else
    > >         timer_reg <= timer_reg-1;
    > >       end if;
    > >     end if;
    > >   end process timer;

    >
    > > The timer is charged OK to "0111" value, then is stuck at this value
    > > and refuses to decrement... even though an assert put in the else
    > > statement confirms it is visited at each rising_edge of the clk when
    > > state_next = dps. Any thoughts about what I'm not doing right?

    >
    > You're not using a standard coding style - I would prefer
    >
    > if rising_edge(clk) then
    >   if state_next = dps then ...
    >
    > just in case your synthesis tool produces a clock enable using an and
    > gate on the clock line (ouch).


    I think that would be a rare thing for a synthesis tool to do in an
    FPGA. I've never seen it. The logic of the statements you use in an
    HDL are distilled down to the basic logic and the form of the source
    is not really regarded typically. I have seen many uses of clock
    enables that work as the above. I keep mine separate mainly because I
    find it much more readable.


    > Regarding the error, timer_reg must be driven somewhere else in your
    > architecture - can you post the whole architecture?


    Yes, I don't see anything in the code above that would cause two
    drivers.

    Rick
     
    rickman, Mar 8, 2010
    #2
    1. Advertising

  3. Maxim

    Maxim Guest

    Hello Allan, and thank you for your quick reply! I have modified my
    process accordingly, regarding the coding style. But I still can't get
    it to work : the timer is not decrementing. It's stuck at "0111",
    which is the proper value I load in it when the fall_edge tick is
    activated. Note that the "multiple drivers" thing is not an error or a
    warning during compilation, but something ModelSim tells me when I
    right click on the time_reg waveform and select "Show Drivers". It
    then highlights the two if clauses of the process timer, where values
    are assigned to timer_reg.

    So this might not be a "bug", but more something weird ModelSim does
    when one uses if construct to drive a signal in a process. But as it's
    in the same process, it cannot be multiple drivers.

    Next I tried so simplify the potential sources of errors, so I
    stripped down the generics and constants for time_reg signal
    declaration and load line (commented in the code below), which result
    in the lack of time_reg signal in the simulation. It cannot be added
    to waveforms, it's literally unseen by the simulator... `me scratching
    my head`. If I revert the comments it's back in the simulation
    waveforms, but again, it won't decrement.

    I don't get it.

    library ieee;
    use ieee.std_logic_1164.all;
    use ieee.numeric_std.all;
    use ieee.std_logic_misc.xor_reduce; -- used to find odd parity
    use ieee.std_logic_misc.or_reduce;
    use work.powerlib.all; -- custom package where is
    log2_ceil is defined

    entity ps2_rx is
    generic (FILTER_MSB : natural := 7;
    TIMER_VALUE : natural := 1000); -- timer de 20 us
    port(
    clk, reset : in std_logic;
    ps2d, ps2c : in std_logic; -- key data, key clock
    rx_en : in std_logic;
    rx_done_tick : out std_logic;
    rx_error_tick : out std_logic; -- start or stop bit error
    rx_wdt_tick : out std_logic; -- watchdog timer error
    dout : out std_logic_vector(7 downto 0));
    end entity ps2_rx;

    architecture arch of ps2_rx is
    constant TIMER_MSB : integer :=
    log2_ceil(TIMER_VALUE-1)-1;
    type statetype is (idle, dps, load);
    signal state_reg, state_next : statetype;
    signal filter_reg, filter_next : unsigned(FILTER_MSB downto 0);
    signal f_ps2c_reg, f_ps2c_next : std_ulogic;
    signal b_reg, b_next : std_ulogic_vector(10 downto 0);
    signal n_reg, n_next : unsigned(3 downto 0);
    signal fall_edge : std_ulogic;
    signal error_flags : std_logic_vector(2 downto 0);
    signal timer_set : std_ulogic;
    --signal timer_reg : unsigned(TIMER_MSB+1 downto 0);
    signal timer_reg : unsigned(3 downto 0);
    --alias timer_out : std_ulogic is
    timer_reg(TIMER_MSB+1);

    begin -- arch

    --=================================================
    -- filter and falling edge tick generation for ps2c
    --=================================================
    process(clk, reset)
    begin
    if reset = '1' then
    filter_reg <= (others => '0');
    f_ps2c_reg <= '0';
    elsif rising_edge(clk) then
    filter_reg <= filter_next;
    f_ps2c_reg <= f_ps2c_next;
    end if;
    end process;

    filter_next <= ps2c & filter_reg(FILTER_MSB downto 1);
    f_ps2c_next <= '1' when filter_reg = to_unsigned(2**(FILTER_MSB
    +1)-1, FILTER_MSB+1) else
    '0' when filter_reg = 0 else
    f_ps2c_reg;
    fall_edge <= f_ps2c_reg and (not f_ps2c_next); -- génère un tick

    --=================================================
    -- fsmd to extract the 8-bit data
    -- Because of right shifting, received bits order is
    -- reversed from sent bits. We obtains :
    -- --------------------------------------------------------------
    -- | stop_bit | odd_parity_bit | scancode | start_bit |
    -- --------------------------------------------------------------
    -- | b_reg(10) | b_reg(9) | b_reg(8 downto 1) | b_reg(0) |
    -- --------------------------------------------------------------
    --=================================================
    -- registers
    process (clk, reset)
    begin
    if reset = '1' then -- reset asynchrone
    state_reg <= idle;
    n_reg <= (others => '0');
    b_reg <= (others => '0');
    elsif rising_edge(clk) then -- registres clockés
    state_reg <= state_next;
    n_reg <= n_next; -- compteur à 4 bits
    b_reg <= b_next; -- data_stream reçu
    end if;
    end process;

    -- next-state / outputs logic
    process(state_reg, n_reg, b_reg, fall_edge, rx_en, ps2d)
    begin
    rx_done_tick <= '0'; -- valeurs par défaut
    rx_error_tick <= '0';
    timer_set <= '0';
    state_next <= state_reg;
    n_next <= n_reg;
    b_next <= b_reg;
    case state_reg is
    when idle =>
    if fall_edge = '1' and rx_en = '1' then
    -- shift in start bit, sur transition d'état (Mealy)
    b_next <= ps2d & b_reg(10 downto 1);
    n_next <= "1001"; -- On charge le compteur à "9"
    state_next <= dps;
    timer_set <= '1'; -- To load timer
    end if;
    when dps => -- réception des données
    if fall_edge = '1' then
    b_next <= ps2d & b_reg(10 downto 1);
    if n_reg = 0 then -- si compteur = 0, on passe à
    l'état
    -- load (dernier bit du
    data_stream)
    state_next <= load;
    else
    n_next <= n_reg - 1; -- on décompte
    end if;
    end if;
    when load =>
    -- one extra clock to complete the last shift
    state_next <= idle; -- only last one clk period
    rx_done_tick <= '1';
    if or_reduce(error_flags) = '1' then
    rx_error_tick <= '1';
    end if;
    end case;
    end process;
    -- output
    dout <= std_logic_vector(b_reg(8 downto 1)); -- data bits,
    excluding start and stop bits

    -- | start_bit_error | odd_parity_error | stop_bit_error |
    error_flags(2) <= '1' when b_reg(0) /=
    '0' else '0'; -- start_bit = '1'
    error_flags(1) <= '1' when not(xor_reduce(b_reg(8 downto 1))) /=
    b_reg(9) else '0';
    error_flags(0) <= '1' when b_reg(10) /=
    '1' else '0'; -- stop_bit = '0'


    -----------------------------------------------------------------------------
    -- Watchdog Timer
    -- The TIMER_MSB+1 bits becomes '1' when timer_reg is fully
    decremented+1 times,
    -- This signal will be used as timer_out, to generate an error
    signal.

    -----------------------------------------------------------------------------
    timer : process(clk) is
    begin
    if rising_edge(clk) then
    if state_next = dps then -- dps state used as enable
    if fall_edge = '1' then
    --timer_reg <= '0' & to_unsigned(TIMER_VALUE-1, TIMER_MSB
    +1);
    timer_reg <= "0111";
    else
    timer_reg <= timer_reg-1;
    end if;
    end if;
    end if;
    end process timer;

    end arch;
     
    Maxim, Mar 8, 2010
    #3
  4. Maxim

    Maxim Guest

    Ok. I understand now that if a signal is not used, ModelSim hides them
    from the simulation. That's what happening with my timer_reg signal.
    And the signal disappears when I use the stripped version (timer_reg
    <= "0111" instead of timer_reg <= '0' & to_unsigned(TIMER_VALUE-1,
    TIMER_MSB+1).

    So maybe it works with the simpler version. I will investigate this,
    and if it works, then I'll try to understand what is wrong with my
    generic and constant usage.
     
    Maxim, Mar 8, 2010
    #4
  5. Maxim

    Maxim Guest

    After some testing, the version with generics or without works the
    same (the problem is not there).
    I still have problems decrementing the counter though...

    Once charged, the timer_reg signal should decrement at every
    rising_edge of the clk (50 mhz / 20 ns). See the following waveform :
    http://img61.imageshack.us/img61/6130/wave.png

    Here's my last version, mostly the same as the above code, but the
    register logic was separated for the timer, allowing to create easily
    a tick on the rising edge of the leftmost timer_reg bit :

    process(clk)
    begin
    if rising_edge(clk) then
    timer_reg <= timer_next;
    end if;
    end process;

    timer : process(state_next, fall_edge) is
    begin
    if state_next = dps then -- dps is a state which is
    used as an enable
    if fall_edge = '1' then
    timer_next <= to_unsigned(TIMER_VALUE-1, TIMER_MSB+2); --
    equivalent to timer_next <= "0111" in my testbench
    else
    timer_next <= timer_reg - 1;
    end if;
    end if;
    end process timer;

    rx_wdt_tick <= timer_next(TIMER_MSB+1) and (not timer_reg(TIMER_MSB
    +1)); -- test timer_reg leftmost bit and gen. a tick on rising

    -- edge. rx_wdt_tick is an output port.

    TIA,
    Maxim
     
    Maxim, Mar 8, 2010
    #5
  6. Maxim

    rickman Guest

    On Mar 8, 6:19 pm, Maxim <> wrote:
    > After some testing, the version with generics or without works the
    > same (the problem is not there).
    > I still have problems decrementing the counter though...
    >
    > Once charged, the timer_reg signal should decrement at every
    > rising_edge of the clk (50 mhz / 20 ns). See the following waveform :http://img61.imageshack.us/img61/6130/wave.png
    >
    > Here's my last version, mostly the same as the above code, but the
    > register logic was separated for the timer, allowing to create easily
    > a tick on the rising edge of the leftmost timer_reg bit :
    >
    >   process(clk)
    >   begin
    >     if rising_edge(clk) then
    >       timer_reg <= timer_next;
    >     end if;
    >   end process;
    >
    >   timer : process(state_next, fall_edge) is
    >   begin
    >     if state_next = dps then            -- dps is a state which is
    > used as an enable
    >       if fall_edge = '1' then
    >         timer_next <= to_unsigned(TIMER_VALUE-1, TIMER_MSB+2); --
    > equivalent to timer_next <= "0111" in my testbench
    >       else
    >         timer_next <= timer_reg - 1;
    >       end if;
    >     end if;
    >   end process timer;
    >
    >   rx_wdt_tick <= timer_next(TIMER_MSB+1) and (not timer_reg(TIMER_MSB
    > +1));  -- test timer_reg leftmost bit and gen. a tick on rising
    >
    > -- edge. rx_wdt_tick is an output port.
    >
    > TIA,
    > Maxim


    I can't tell what you have done and what works and what doesn't work.
    Can you simplify your code to a version that doesn't work and has as
    little as possible. I can assure you that modelsim isn't doing
    anything weird with the code above.

    I think you said that the code above works, but I can see one problem
    with it in simulation. The combinatorial process defining timer_next
    does not include timer_reg in it's sensitivity list. Any time you
    have a problem with a combinatorial proccess, check the sensitivity
    list. In this case the process only runs when the two included
    signals change and so once falling_edge goes away timer_next is never
    recalculated. Add timer_reg to the sensitivity list and see what the
    simulation does.

    Rick
     
    rickman, Mar 9, 2010
    #6
  7. Maxim

    rickman Guest

    On Mar 8, 7:01 pm, rickman <> wrote:
    > On Mar 8, 6:19 pm, Maxim <> wrote:
    >
    >
    >
    > > After some testing, the version with generics or without works the
    > > same (the problem is not there).
    > > I still have problems decrementing the counter though...

    >
    > > Once charged, the timer_reg signal should decrement at every
    > > rising_edge of the clk (50 mhz / 20 ns). See the following waveform :http://img61.imageshack.us/img61/6130/wave.png

    >
    > > Here's my last version, mostly the same as the above code, but the
    > > register logic was separated for the timer, allowing to create easily
    > > a tick on the rising edge of the leftmost timer_reg bit :

    >
    > >   process(clk)
    > >   begin
    > >     if rising_edge(clk) then
    > >       timer_reg <= timer_next;
    > >     end if;
    > >   end process;

    >
    > >   timer : process(state_next, fall_edge) is
    > >   begin
    > >     if state_next = dps then            -- dps is a state which is
    > > used as an enable
    > >       if fall_edge = '1' then
    > >         timer_next <= to_unsigned(TIMER_VALUE-1, TIMER_MSB+2); --
    > > equivalent to timer_next <= "0111" in my testbench
    > >       else
    > >         timer_next <= timer_reg - 1;
    > >       end if;
    > >     end if;
    > >   end process timer;

    >
    > >   rx_wdt_tick <= timer_next(TIMER_MSB+1) and (not timer_reg(TIMER_MSB
    > > +1));  -- test timer_reg leftmost bit and gen. a tick on rising

    >
    > > -- edge. rx_wdt_tick is an output port.

    >
    > > TIA,
    > > Maxim

    >
    > I can't tell what you have done and what works and what doesn't work.
    > Can you simplify your code to a version that doesn't work and has as
    > little as possible.  I can assure you that modelsim isn't doing
    > anything weird with the code above.
    >
    > I think you said that the code above works, but I can see one problem
    > with it in simulation.  The combinatorial process defining timer_next
    > does not include timer_reg in it's sensitivity list.  Any time you
    > have a problem with a combinatorial proccess, check the sensitivity
    > list.  In this case the process only runs when the two included
    > signals change and so once falling_edge goes away timer_next is never
    > recalculated.  Add timer_reg to the sensitivity list and see what the
    > simulation does.
    >
    > Rick


    I forgot to mention that I don't see timer_reg being used anywhere
    other than in calculating timer_next. If synthesized, this register
    would be optimized away.

    Rick
     
    rickman, Mar 9, 2010
    #7
  8. Maxim

    Maxim Guest

    On 8 mar, 19:15, rickman <> wrote:
    > On Mar 8, 7:01 pm, rickman <> wrote:
    >
    >
    >
    > > On Mar 8, 6:19 pm, Maxim <> wrote:

    >
    > > > After some testing, the version with generics or without works the
    > > > same (the problem is not there).
    > > > I still have problems decrementing the counter though...

    >
    > > > Once charged, the timer_reg signal should decrement at every
    > > > rising_edge of the clk (50 mhz / 20 ns). See the following waveform :http://img61.imageshack.us/img61/6130/wave.png

    >
    > > > Here's my last version, mostly the same as the above code, but the
    > > > register logic was separated for the timer, allowing to create easily
    > > > a tick on the rising edge of the leftmost timer_reg bit :

    >
    > > >   process(clk)
    > > >   begin
    > > >     if rising_edge(clk) then
    > > >       timer_reg <= timer_next;
    > > >     end if;
    > > >   end process;

    >
    > > >   timer : process(state_next, fall_edge) is
    > > >   begin
    > > >     if state_next = dps then            -- dps is a state which is
    > > > used as an enable
    > > >       if fall_edge = '1' then
    > > >         timer_next <= to_unsigned(TIMER_VALUE-1, TIMER_MSB+2); --
    > > > equivalent to timer_next <= "0111" in my testbench
    > > >       else
    > > >         timer_next <= timer_reg - 1;
    > > >       end if;
    > > >     end if;
    > > >   end process timer;

    >
    > > >   rx_wdt_tick <= timer_next(TIMER_MSB+1) and (not timer_reg(TIMER_MSB
    > > > +1));  -- test timer_reg leftmost bit and gen. a tick on rising

    >
    > > > -- edge. rx_wdt_tick is an output port.

    >
    > > > TIA,
    > > > Maxim

    >
    > > I can't tell what you have done and what works and what doesn't work.
    > > Can you simplify your code to a version that doesn't work and has as
    > > little as possible.  I can assure you that modelsim isn't doing
    > > anything weird with the code above.

    >
    > > I think you said that the code above works, but I can see one problem
    > > with it in simulation.  The combinatorial process defining timer_next
    > > does not include timer_reg in it's sensitivity list.  Any time you
    > > have a problem with a combinatorial proccess, check the sensitivity
    > > list.  In this case the process only runs when the two included
    > > signals change and so once falling_edge goes away timer_next is never
    > > recalculated.  Add timer_reg to the sensitivity list and see what the
    > > simulation does.

    >
    > > Rick

    >
    > I forgot to mention that I don't see timer_reg being used anywhere
    > other than in calculating timer_next.  If synthesized, this register
    > would be optimized away.
    >
    > Rick


    Hey Rick! Thank you so much :)
    Adding timer_reg to the sensivity list did the trick! I don't exactly
    know what was my initial error, maybe I was mislead by the fact the
    signal timer_reg wasn't driving anything else (but for a reason
    appeared in the waveforms). But for now, I'm quite happy to see what I
    was trying to do for hours finally working!

    The final form looks like :

    process(clk)
    begin
    if rising_edge(clk) then
    timer_reg <= timer_next;
    end if;
    end process;

    timer :
    process(state_next, fall_edge, timer_reg) is
    begin
    if state_next = dps then -- état dps sert d'enable
    if fall_edge = '1' then
    timer_next <= to_unsigned(TIMER_VALUE-1, TIMER_MSB+2);
    else
    timer_next <= timer_reg -1;
    end if;
    end if;
    end process timer;

    rx_wdt_tick <= timer_next(TIMER_MSB+1) and (not timer_reg(TIMER_MSB
    +1)); -- génère un tick

    Thanks again,
    Maxim
     
    Maxim, Mar 9, 2010
    #8
    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. The Eeediot
    Replies:
    3
    Views:
    2,308
    =?Utf-8?B?UnVsaW4gSG9uZw==?=
    Dec 22, 2004
  2. Patrick
    Replies:
    1
    Views:
    714
  3. George2
    Replies:
    1
    Views:
    852
    Alf P. Steinbach
    Jan 31, 2008
  4. bhatia.ankur8

    multiple drivers problem,..please help

    bhatia.ankur8, Jun 27, 2011, in forum: VHDL
    Replies:
    2
    Views:
    2,612
    bhatia.ankur8
    Jun 30, 2011
  5. Ramya Murali

    Counter with asynchronous enable

    Ramya Murali, Feb 29, 2012, in forum: VHDL
    Replies:
    3
    Views:
    1,072
Loading...

Share This Page