0
\$\begingroup\$

With no experience in VHDL, I am trying to convert a C++ code for driving a CCD to VHDL based code. This is very likely not the best way to implement it, but it's intuitive to me. So, my counter counts clock cycles and based on the clock cycle number, I assign the specific state to the output. I am able to implement the design successfully:

enter image description here

However, when I run simulation, the ALLOUT does not change, in fact it stays in ST3 state. The warnings from the test bench console:

enter image description here

They repeat every 25ns up to the end of the simulation. Any suggestions on where I made the mistake?

\$\endgroup\$
  • \$\begingroup\$ Please post your code as text in the future, as it makes it much easier for other users to help you. \$\endgroup\$ – fru1tbat Apr 30 '14 at 17:48
  • \$\begingroup\$ It would be easier for me to post it as a text, but it looses structure and color, making it much more difficult to read. \$\endgroup\$ – Nazar Apr 30 '14 at 19:22
  • \$\begingroup\$ If you indent properly so it's interpreted as code, SE will apply syntax highlighting - see my snippet below. Either way, it's preferred by the community, so you may get more responses. \$\endgroup\$ – fru1tbat Apr 30 '14 at 19:36
3
\$\begingroup\$

There are a number of style issues here and one major problem.

Fru1tbat has pointed out that you never initialise MainCounter : one way to do so would be to drive the Reset input with a simple waveform :

RESET <= '0', '1' after 100 ns, '0' after 200 ns;

in the parallel execution region (after the architecture "begin") would do so. Note that this is for simulation only, and it would be MUCH better to make RESET an input port and drive it from your simulation testbench.

Secondly (and this is the major problem, waiting to bite you randomly in real hardware) the RESET clause in process COUNT should be simplified to ONLY clear MainCounter on reset. Then the wrapround clear should be moved into the clocked part of the process:

if Reset = '1' then 
   MainCounter <= (others => '0');
elsif rising_edge(Clk) then
   if MainCounter >= 12345 then
      MainCounter <= (others => '0');
   else
      MainCounter <= MainCounter + 1;
   end if;
end if;

The reason for this is subtle if you're not used to hardware design : as expressed, you have a pile of asynchronous logic waiting to clear the counter if it momentarily transitions to a high number while counting. So counting from X"7FFF" to X"8000" it will probably momentarily pass through X"FFFF" ... and reset itself.

By moving that logic to the clocked part, it will only reset if the temporary state persists until the next clock edge : and the timing analysis tools will report timing failure if that is even a possibility.

Now the warnings : Metavalues at 0 ns are basically harmless. Metavalues persisting AFTER reset is complete are a different matter...

And the style issues :

You can compare Unsigned against simple integer literals (because numeric_std overloads comparison operators with mixed operand types) and that would make the code MUCH more readable. Better : use constant declarations instead of magic numbers. (As you discovered, you can't assign an integer literal to an unsigned; because assignment is not an operator. There are to_unsigned and to_integer conversion functions for that purpose)

There is no need for parentheses around boolean expressions e.g. in if-statements. Some C programmers use them out of habit, but it looks needlessly cluttered to me.

Your SLOW process is unclocked, despite the clock in its sensitivity list... I prefer simple clocked processes; they avoid the problems that plague combinational processes such as SLOW - which will not work correctly unless you add all the RHS signals (here - only Maincounter) to the sensitivity list.

\$\endgroup\$
  • \$\begingroup\$ thank you so much for such detailed explanation, I greatly appreciate that. \$\endgroup\$ – Nazar May 1 '14 at 13:06
2
\$\begingroup\$

MainCounter is not initialized, and is never set anywhere else (you don't drive RESET to '1' anywhere). The "metavalue" message indicates you have 'U', 'X', etc. in your inputs to the comparison (a web search should have told you that much, by the way).

You may want to consider just using integer literals, by the way. numeric_std and unsigned let you do that, and it's much easier to read. For example:

if (RESET = '1' or MainCounter >= 110000) then
\$\endgroup\$
  • \$\begingroup\$ How do I properly define the MainCounter variable, so it can be compared to a number and/or incremented with +1? I did the following, but it does not work. Looking for proper definition \$\endgroup\$ – Nazar Apr 30 '14 at 19:19
  • \$\begingroup\$ VARIABLE MainCounter : UNSIGNED RANGE (0 TO 131071) := 0; Does not work. \$\endgroup\$ – Nazar Apr 30 '14 at 19:20
  • \$\begingroup\$ It was already defined properly. unsigned is a type similar to std_logic_vector - it's just a vector of std_logic, but with overloaded operators that allow you to treat it as a number. You can't assign integer literals, though - you will need to convert using to_unsigned(). \$\endgroup\$ – fru1tbat Apr 30 '14 at 19:34

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.