Multiple drivers problem with a simple set-enable counter process

[Maxim]

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.

 

[rickman]

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

 

[Maxim]

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]

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]

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

 

[rickman]

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]

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

 

[Maxim]

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