VHDL variable behaving strangely

Question

I have the following snippet of VHDL code which is misbehaving and I don't know why:

process (clock)

variable counter : std_logic_vector (15 downto 0) :=  x"0000";
variable op : std_logic_vector (7 downto 0);

begin

if (clock = '1') then

    op := cw (15 downto 8);

    -- This if statement is misbehaving 
    if (op = x"20") then
        counter  := counter + cw (7 downto 0);
    end if;

    counter := counter + 2;

    ia <= counter;

    end if;

end process;

As the simulation shows, the code increments in counts of 2, but the cycle after the if statement, weird things happen

Simulation

Sorry, the top waveform is the clock, then "cw" and "ia" at the bottom

EDIT: The code is supposed to count up by 2 by default. If the upper byte of "cw" is 0x20, then it is incremented by 2 and another number

As per the waveform cw, these appear to be displayed as decimal radix values. Counter = 12 + cw (equal to 15) is 27, plus two in the following counter increment statement = 29. Your code appears to be behaving exactly as written. You haven't told us what it's supposed to do. — , Aug 28 '13 at 22:21
@DavidKoontz: It would seem that the problem is in the next cycle, where the output should be 31, not 11. — Dave Tweed, Aug 28 '13 at 22:54
Is the waveform from a simulator or a logic analyzer? What do the empty brackets mean? It looks like `cw` is not synchronized to `clock`, which could cause the probems discussed in [this question](http://electronics.stackexchange.com/q/79821/11683). — Dave Tweed, Aug 28 '13 at 23:03
It is stated in the question that it is simulation results so the lack of synchronization does not matter. Could you post the testbench as well so others can run the simulation themselves? It seems there are several transitions on the `ie` signal after the rising edge of the clock which would be interesting to take a closer look at. — trondd, Aug 29 '13 at 07:16
Can you post your testbench code and your complete unit under test - so we can try it in another simulator. Make sure you post everything required (and nothing more - strip it back to a good testcase). — Martin Thompson, Aug 29 '13 at 10:29

akohlsmith · Answer 1 · 2013-08-29T15:53:44.323

You don't say what you want the code to do, but I can see a couple of trouble areas. The first is that you don't have any reset logic in your process. It's important to make sure that you properly initialize everything in order to prevent unusual operation. This however is not likely the cause of your trouble.

What I think is causing the incorrect operation is that you are looking for the clock signal to be high instead of checking for the clock to transition from a low to high. This is called the "rising edge" of the clock. Synchronous logic operates on clock edges rather than clock levels.

You mention that you should not have to look for a rising edge since the clock is in the sensitivity list, and thus if your process is executed you can only check the level of the clock. This may be theoretically true but synthesis tools work by inferring logic from the language used to describe it. Most synthesis tools I have used are designed to produce correct logic when described below.

process(clock, reset)

variable counter : std_logic_vector (15 downto 0) :=  x"0000";
variable op : std_logic_vector (7 downto 0);

begin

if reset = '1' then
    -- reset everything to a known state here

elsif rising_edge(clock) then
    op := cw(15 downto 8);

    if op = x"20" then
        counter := counter + cw(7 downto 0);
    end if;

    counter := counter + 2;
    ia <= counter;
end if;
end process;

Writing good HDL is just as much about getting the logic correct as it is describing it in the way the synthesis tools are expecting to see it. If you decide "it's the same thing" and write things in a non-standard way you may well find that the simulator tells you one thing but the synthesizer fails to see it the same way. This is a fine point to learn, and one that can cause you a lot of headache and frustration should you decide otherwise.

That did not fix the problem. I have edited my question so you know what it should do. The reason why I did not using rising_edge() was because this process is only triggered on the clock. Hence, the code inside the process executing means that the clock must have changed — user929404, Aug 28 '13 at 20:23
Your question does not show where cw ever has a value of 20xx, or at least it is not clear. Please consider revising your answer so it is clear what is happening. Your comment about the clock being in the sensitivity list will also be addressed. — akohlsmith, Aug 28 '13 at 22:09
I see this in a lot of the answers here on stackexchange, and it is just not right. Reset is _not_ required to produce working hardware or simulation results. It depends on you design and requirements if you need the reset branch. FPGA will for instance power up to a known state, so you shouldn't make this a general requirement. — trondd, Aug 29 '13 at 07:10
I'd already stated that the lack of reset is the issue, I was pointing out a stylistic issue. It's not about getting simulation right. It's about good design practise. Learn it the right way or spend a lot of time and frustration unlearning bad habits and bad assumptions. We've all been there and I'm trying to help prevent someone else from falling in the same traps. — akohlsmith, Aug 29 '13 at 15:49

score 0 · Answer 2 · answered Aug 29 '13 at 10:29

In theory...

process (clock)
...
begin
   if (clock = '1') then

should simulate the same as

process (clock)
...
begin
   if rising_edge(clock) then

but I don't know if Altera's simulator will behave the same as the first code is "unusual" and hence possibly untested!

The fact that your simulation waveforms seem to show a brief transition of something else between [0][29] and [0][11] makes me wonder if something weird is going on in the simulator scheduling.

user929404 · Accepted Answer · 2013-09-01T11:25:59.373

The solution ended up being:

counter  := cw (7 downto 0) + counter;

as opposed to what I had before:

counter  := counter + cw (7 downto 0);

Although "counter" is 16 bits and I am adding an 8 bit std_logic_vector, I did not have to concatenate it with:

counter  := ("0000000" & cw (7 downto 0) ) + counter;

This method is actually not robust as it assumes only unsigned numbers. The above method handles sign extension if the library, ieee.std_logic_signed.all, is used

VHDL variable behaving strangely

3 Answers3