Search code examples
vhdlmodelsim

Unsigned Addition with Counter Doesn't Work

I'm building a counter that counts rising edges from an input channel. I've simplified my design to include two states, one and two, where counting is done. For some reason, whenever I try to add 1 to counter_reg, or try to assign any number at all to it, the signal becomes red with an X in ModelSim. The code and picture of what happens to the signal are provided below.

I have included the IEEE.NUMERIC_STD.ALL, so I should be able to do unsigned addition. I am not sure what is wrong with counter_reg. Is there anything I'm doing wrong with the counter?

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;

entity photon_counter is
    Port ( clk,reset : in STD_LOGIC;
           arm,shifter,channel : in STD_LOGIC;
           start : in STD_LOGIC);
end photon_counter;

architecture fsm_arch of photon_counter is
type state_type is (idle,zero,one);
type array_type is array (1 downto 0) of UNSIGNED (15 downto 0);
signal state_reg,state_next : state_type;
signal arm_prev,shifter_prev,channel_prev : STD_LOGIC;
signal counter : array_type;
signal counter_reg,counter_next : UNSIGNED (15 downto 0);
begin
--------------------------------------
--State Register
--------------------------------------
process(clk,reset)
begin
if reset='1' then
    state_reg <= zero;
    counter_reg <= (others => '0');
    counter <= (others => (others => '0'));
elsif rising_edge(clk) then
    state_reg <= state_next;
    counter_reg <= counter_next;
    arm_prev <= arm;
    shifter_prev <= shifter;
    channel_prev <= channel;
end if;
end process;
--------------------------------------
--Next-State Logic/Output Logic
--------------------------------------
process(clk,reset,state_reg,start,counter_reg,shifter_prev,shifter,arm,channel_prev,channel)
begin
--default actions
    state_next <= state_reg;
    counter_next <= counter_reg;
    counter_reg <= counter_reg;
    case state_reg is
        when idle =>
            counter_reg <= (others => '0');
            counter <= (others => (others => '0'));
            if start = '1' then
                state_next <= zero;
            end if;
        when zero =>
            if (shifter = '1') and (shifter_prev = '0') then
                state_next <= one;
                counter(0) <= counter_reg;
            end if;
            if (channel = '1') and (channel_prev = '0') then
                counter_next <= counter_reg + 1;
            end if;
        when one =>
            if arm = '1' then
                state_next <= zero;
                counter(1) <= counter_reg;
            end if;
            if (channel = '1') and (channel_prev = '0') then
                counter_reg <= counter_reg + 1;
            end if;
    end case;
end process;
end fsm_arch;

As shown below, counter_reg and counter_next start off with a value of 0 until I try to add 1 to counter_next. The moment channel_prev rises, both counter_reg and counter_next become X (error) and turn red.

ModelSim Waveform

Solution

  • Your counter_reg signal is assigned in two different processes. This is what we call a "multiple drive" situation. It is usually undesirable, just like any short circuit, because when the two processes disagree about the value to assign things are getting very bad.

    Solution: drive your counter from one single process.

    A bit more about this: if this is bad, why didn't you get an error when compiling or when launching your simulation? Because most people do not know or care about unresolved/resolved types in VHDL. By default, a VHDL type is unresolved. This means that, if you try to drive a signal of this type from more than one process you will get an error at compilation or elaboration time that basically says "I cannot decide what value to assign if your processes disagree, this is forbidden". And this is a very nice feature because such accidental short circuits can have serious consequences. You can try this and see the errors by replacing your unsigned (resolved) counter by a natural (unresolved) one:

    signal counter_reg,counter_next : natural 0 to 2**16 - 1;

    adapt the rest of your code, and see what happens when compiling.

    Sometimes, rarely, it is useful to drive a signal from more than one process (high impedance shared bus, bi-directional RAM data bus...) So VHDL allows to define a resolution function that computes the resulting value of several drivers. This function can be used to define the resolved subtype of a unresolved parent type. If you can find the source code of ieee.std_logic_1164 you will see the declaration of the unresolved, 9-valued std_ulogic type (u for unresolved), the resolution function, and the declaration of the resolved subtype std_logic (see? no u).

    But when using resolved types you must yourself take care of not creating short-circuits. There is no compiler error any more, no seatbelt. When one of your driving processes drives a strong value ('0' or '1'), all the others must drive a weak value ('Z' for high impedance). Else, you will get unknown resulting values, represented in red by Modelsim, as you saw.

    Unfortunately, most people do not really know what the U stands for in std_Ulogic. So, in order to simplify they always use std_logic instead of what they should use: std_ulogic. Moreover, vendors of logic synthesisers push in the same direction because they frequently favour std_logic (when they do not simply force you to use it). And the people who standardized the ieee.numeric_std package did the same: they declared the unsigned and signed types as resolved types (if fact, they have the same declaration as std_logic_vector). This is as unfortunate as driving full speed, at night, wearing sunglasses, without light and without your seatbelt fasten.

    Finally, someone realized how unfortunate this was and the current version of ieee.numeric_std now also declares UNRESOLVED_UNSIGNED (alias U_UNSIGNED) and UNRESOLVED_SIGNED (alias U_SIGNED). Alas, this is a bit too late, most designers will never change their existing code or habits, and I wonder how many bugs could have been avoided if the first choice had been different.

    My advices:

    • never drive a signal from several processes if you do not really intend to have multiple physical drivers, manually avoid short circuits, and so on,
    • never use a resolved type if you do not need it, so that the tools will raise an error if you accidentally create a multiple drive situation,
    • in your case, drive counter_reg from one single process, declare it as U_UNSIGNED or NATURAL, and declare all your other signals as STD_ULOGIC, not STD_LOGIC.