0
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – user8352 Aug 28 '13 at 22:21
  • \$\begingroup\$ @DavidKoontz: It would seem that the problem is in the next cycle, where the output should be 31, not 11. \$\endgroup\$ – Dave Tweed Aug 28 '13 at 22:54
  • \$\begingroup\$ 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. \$\endgroup\$ – Dave Tweed Aug 28 '13 at 23:03
  • \$\begingroup\$ 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. \$\endgroup\$ – trondd Aug 29 '13 at 7:16
  • \$\begingroup\$ 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). \$\endgroup\$ – Martin Thompson Aug 29 '13 at 10:29
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – user929404 Aug 28 '13 at 20:23
  • \$\begingroup\$ 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. \$\endgroup\$ – akohlsmith Aug 28 '13 at 22:09
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – trondd Aug 29 '13 at 7:10
  • \$\begingroup\$ 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. \$\endgroup\$ – akohlsmith Aug 29 '13 at 15:49
  • \$\begingroup\$ bah, lack of rest is not the issue. stupid typo. \$\endgroup\$ – akohlsmith Sep 1 '13 at 13:47
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.