-1
\$\begingroup\$

Given a signal of type integer, I want to create a process (or several) to increment or decrement the variable by 1. How can I do this though? Seems a signal cannot be driven by two clocks (in my case, key(0) and key(1)) and if I use a variable instead, I get the same problem of being unable to change it under multiple clocks.

This approach does not work:

process(clock_50,key)
begin
    if rising_edge(clock_50) then
        if rising_edge(key(0)) then
          setting.temp <= setting.temp + 1;
        elsif rising_edge(key(1)) then
            setting.temp <= setting.temp - 1;
        end if;
    end if;
end process;
\$\endgroup\$
  • \$\begingroup\$ I don't think there is a counter/register that can be driven two clocks. Can't the decrement/increment be controlled by low/high signal? Like LOW for decrement and HIGH for increment and vice versa. \$\endgroup\$ – user3219492 Nov 29 '16 at 16:17
  • \$\begingroup\$ @user3219492 wouldn't that imply me having to detect both the rising and falling edges of a signal, something that is impossible? \$\endgroup\$ – Dmitri Nesteruk Nov 29 '16 at 16:24
  • 1
    \$\begingroup\$ You should probably have a clock at a fixed frequency, and sample the key states using that clock. \$\endgroup\$ – dim lost faith in SE Nov 29 '16 at 16:25
  • \$\begingroup\$ @dim even if I do so, I need to be sensitive to key presses to avoid changing a variable a zillion times. \$\endgroup\$ – Dmitri Nesteruk Nov 29 '16 at 16:48
  • \$\begingroup\$ Can you or the two key signals together and use that as your clock? You can then use one of the two signals to give a direction. It works fine as long as they aren't going to press both buttons at once. \$\endgroup\$ – Andrew Nov 29 '16 at 16:51
3
\$\begingroup\$

How about this variant on Andrews code :

signal oldKeys, kd1, kd2 : std_logic_vector(1 downto 0);

process(clock_50)
begin
    if rising_edge(clock_50) then
        kd2 <= kd1; -- 2 stage flipflop syncroniser to 
        kd1 <= key; -- deal with metastability risk.

        if (kd2(0) and not oldkeys(0)) then
          setting.temp <= setting.temp + 1;
        elsif (kd2(1) and not oldkeys(1)) then
            setting.temp <= setting.temp - 1;
        end if;

        oldkeys <= kd2;
    end if;
end process;

The other variant I sometimes see is a two stage shift register and looking for "10", but it amounts to much the same thing.

Note that keys is fed in via a synchroniser to avoid metastability risk if the key happens to be pressed too close to a clock rising edge. In practise you would probably want some debounce logic in there as well, maybe inhibit each key for 10ms or so after a key press is detected?

\$\endgroup\$
  • \$\begingroup\$ I think you'd forgotten to change a key into kd2, feel free to undo the edit if I've missed something. Good point on the metastability issue. \$\endgroup\$ – Andrew Nov 30 '16 at 15:05
2
\$\begingroup\$

UPDATE - Please see Dan Mills answer, it also handles metastability issues that I had overlooked. This one has been left in place due to the number of references to it in other answers/comments.

Would this meet your requirements?

signal oldKeys : std_logic_vector(1 downto 0);

-- register the key input into oldKeys
-- the oldKeys signal will be delayed by a single clock cycle
reg : process(clock_50)
begin
    if rising_edge(clock_50) then
        oldKeys <= key;
    end if;
end process;

-- use the one clock delayed oldKeys and the current key signals to detect changes in key
process(clock_50)
begin
    if rising_edge(clock_50) then
        if (key(0) and not oldkeys(0)) then
          setting.temp <= setting.temp + 1;
        elsif (key(1) and not oldkeys(1)) then
            setting.temp <= setting.temp - 1;
        end if;
    end if;
end process;

The circuit that this will produce is roughly this.

schematic

simulate this circuit – Schematic created using CircuitLab

Where reg1 is the first process and the counter is the second process. As can be seen both processes run at the same time however as long as the timing difference of the CLK signal between the reg and the counter is less than the output delay of reg1 plus the propagation delay of the not gate plus the delay of the and gate then the behavior is correct. If this wasn't the case the compiler would throw out all sorts of warnings.

\$\endgroup\$
  • \$\begingroup\$ This doesn't work, and there's no reason it should. We cannot make any assumptions about order of signal assignments, so it cannot be argued that oldKeys is ever really old in the true sense of the word. It might work from time to time, but this is too fragile, and not really the solution to my problem. \$\endgroup\$ – Dmitri Nesteruk Nov 29 '16 at 17:20
  • \$\begingroup\$ I added some comments to make @Andrew's idea more clear, hopefully they help! \$\endgroup\$ – Lincoln Nov 29 '16 at 18:22
  • \$\begingroup\$ @DmitriNesteruk Can you elaborate on the "order of signal assignments" problem? All signals are assigned only once per conditional path, here. Andrew's solution seems correct to me. Perhaps the misunderstanding comes from the clock_50 signal. Andrew certainly assumed (as we all did) that it is some kind of arbitrary 50MHz clock, but you didn't actually described what it was in your post. \$\endgroup\$ – dim lost faith in SE Nov 29 '16 at 20:04
  • \$\begingroup\$ @Lincoln your assumption regarding old/new key is incorrect. both processes execute at the same time and we can make no assumptions as to ordering \$\endgroup\$ – Dmitri Nesteruk Nov 29 '16 at 21:41
  • 1
    \$\begingroup\$ @DmitriNesteruk You are correct, both execute at the same time. Exactly the same time. Not one then the other, both at the same time. Which means in the second process oldkeys is guaranteed to be the value that key was on the previous clock cycle. The only way this would not be true is if your design had excessive clock skew which would be reported as a timing error by the compiler. This isn't some assumption on how things work, it's part of the definition of VHDL. \$\endgroup\$ – Andrew Nov 30 '16 at 9:09
0
\$\begingroup\$

I think you're getting yourself in a mess, the solution of your problem is much easier!!

process(clock_50)
begin
    if rising_edge(clock_50) then
        if key(0) = '1' then
          setting.temp <= setting.temp + 1;
        elsif key(1) = '1' then
            setting.temp <= setting.temp - 1;
        end if;
    end if;
end process;

I don't understand why you check for rising edge of a signal if you already have a clock. In a sequential circuit, when a clock edge is produced the LEVEL inputs are read and an output is obtained.

\$\endgroup\$
  • \$\begingroup\$ If the key signals stay high for several clock cycles, you'll increment/decrement too many times, here. This was the reason of the oldKeys trick on Andrew's answer. \$\endgroup\$ – dim lost faith in SE Nov 30 '16 at 13:31
  • \$\begingroup\$ That's true! I didn't pretend to solve that. I only want to show one of the VHDL basics! Of course, Andrew's anwser is good! \$\endgroup\$ – ferdepe Nov 30 '16 at 13:36
  • \$\begingroup\$ This solution is wrong: when pressing the button, you are increasing the temp value 23652 times instead of increasing it just once. \$\endgroup\$ – Dmitri Nesteruk Nov 30 '16 at 23:05
  • 1
    \$\begingroup\$ Why 23652? I can't understand your point... \$\endgroup\$ – ferdepe Dec 1 '16 at 8:29
-1
\$\begingroup\$

I've managed to solve this problem but before I post the solution, just wanted to note a few things, namely:

  • You cannot have a signal that's sensitive to two clocks. That's why you cannot react to presses of key(0) and key(1) at the same time.
  • You cannot detect both a rising and falling edge of a signal. That's why, even if you merge both key presses into a signal bit, you still wouldn't be able to do much with it.
  • The solution presented by @Andrew doesn't work because it's built on unsafe assumptions. If you put it into hardware, the buttons will respond intermittently (unreliably). Which is easily explained by the fact that processes are concurrent in VHDL.

So now, the real solution. What we do is introduce two signals that indicate the number of times each button was pressed:

signal up, down : integer := 0;

Then, we simply assign these signals at each button press:

process(key)
begin
    if (key(0)'event and key(0) = '0') then
        up <= up + 1;
    end if;
    if (key(1)'event and key(1) = '0') then
        down <= down + 1;
    end if;
end process;

And then, at the architecture level, we simply put:

setting.temp <= up - down;

If you want to nitpick, this solution doesn't handle overflow (button pressed > 2^31 times) but that's not a real problem.

\$\endgroup\$
  • \$\begingroup\$ It is also using 2 32 bit counters and a 32 bit adder which are unnecessary. You are using 3 times the number of logic resources required and creating something that will have a lower maximum clock speed. It may work but it's massively wasteful and shows no appreciation of the underlying hardware implementation. \$\endgroup\$ – Andrew Nov 30 '16 at 9:30
  • 2
    \$\begingroup\$ The other problem with this implementation, which, at the first glance, sounds neat (because the counter is updated exactly when the key inputs change), is that, if you must use the result setting.temp from another clock domain (e.g. clock_50), you'll risk metastability problems. This is what I would call a fragile design. \$\endgroup\$ – dim lost faith in SE Nov 30 '16 at 9:35
  • 2
    \$\begingroup\$ @DmitriNesteruk with a 50MHz clock the keypress would have to be less than ~20ns to be missed by Andrew's design, which should be more than sufficient for any sort of mechanical switch. While your design may work perfectly in simulation it will be incredibly susceptible to metastability problems. \$\endgroup\$ – ks0ze Nov 30 '16 at 14:17
  • \$\begingroup\$ @ks0ze having put Andrew's solution into an actual FPGA, button presses are missed on timeframes much larger than 20ns. I wish that his solution would work, but it simply doesn't. \$\endgroup\$ – Dmitri Nesteruk Nov 30 '16 at 23:07
  • 1
    \$\begingroup\$ @DmitriNesteruk I don't think you quite understand what Andrew's code is doing. Here's a waveform diagram imgur.com/a/sDfJQ. You can get the rising edge by looking at when the current value (key) is 1 and previous (old_key) is still a 0. Old_key always lags key by exactly one clock cycle because it is just a registered version of the same signal. \$\endgroup\$ – ks0ze Dec 1 '16 at 2:36

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.