4
\$\begingroup\$

I'm creating an n bit shift register. When the enable signal is high, I want the shift register to shift n times, irrespective of whether enable continues to be high or low. I've put a for loop to shift n times inside a process. My code is given below.

I don't think the for loop is working, as the shifting is not restricted to n times. Where am I going wrong?

library ieee;
use ieee.std_logic_1164.all;

entity SReg is

  generic (
     n  : integer := 4 
    );

  port(
    clk:              in std_logic; 
    reset:            in std_logic; 
    enable:           in std_logic;    --enables shifting
    parallel_in:      in std_logic_vector(n-1 downto 0);
    s_in:             in std_logic;    --serial input
    s_out:            out std_logic   --serial output
    );
end SReg;

architecture behavioral of SReg is
  signal temp_reg: std_logic_vector(n-1 downto 0) := (Others => '0');
begin
  process (clk,reset)
  begin
    if (reset = '1') then
      temp_reg <= parallel_in;   
    elsif (clk'event and clk='1') then
      if (enable ='1') then
        --shifting n number of bits
        for i in 0 to n-1 loop
          s_out <= temp_reg(0);
          temp_reg <= s_in & temp_reg(n-1 downto 1);
        end loop;
      end if;
    end if;
  end process;
end behavioral;
\$\endgroup\$
0

2 Answers 2

23
\$\begingroup\$

In VHDL, a for loop executes in zero time. This means that instead of waiting a clock cycle between each iteration, the entire loop is run within one clock cycle, with only the final result of the loop being shown at the end. This is what's happening in your code. The entire loop is executing in a single clock cycle.

What you really want is a loop where each iteration occurs on a new clock edge. This allows for s_in to be shifted out of s_out ever clock cycle.

Performing a loop where each iteration occurs on a clock edge does not require a for loop command, instead it takes advantage of the sensitivity list of the process. Here's how:

A process is triggered every time one of the signals on the sensitivity list ("clk, reset" in this case) changes. This means that the process is already looping every clock cycle (if a clock is in the sensitivity list). You can use this to your advantage in order to perform a for-loop type operation, where every iteration of the loop occurs on a clock cycle.

First you need a counter:

process(clk,reset)
    variable shift_counter: integer := 0;
begin

shift_counter keeps track of how many iterations (or shifts) have occurred so far. You'll compare shift_counter to n-1 to see if you're done yet.

Next it might be a good idea to think of the states your process will be in. Perhaps a wait state for when the process is not shifting, and a shifting state for when it is.

The state signal definition:

 TYPE POSSIBLE_STATES IS (waiting, shifting);
 signal state : POSSIBLE_STATES;

In the process proper:

case state is
   when waiting =>

Okay, so what happens when we're waiting for an enable? It would be a good idea to set all (driven) variables to a known value. This means that maybe something like this is a good idea:

shift_counter := 0;
temp_reg <= parallel_in;
s_out <= '0';

This is useful to do because then you know exactly what your signal values are when enable goes high. Also, at the end of the shift, you can change states back to "waiting" in order to get ready for enable again.

So what is going to trigger a state change from waiting to shifting ? That's easy:

if(enable = '1') then
    state <= shifting;
else
    state <= waiting;
end if;

Okay, so next state. shifting.

First, we want to increment the shift counter, and perform the actual shift:

when shifting =>
    shift_counter := shift_counter + 1;
    s_out <= temp_reg(0);
    temp_reg <= s_in & temp_reg(n-1 downto 1);

And then also detect when the shifting is done, in order to leave the shift state and go back to waiting:

if (shift_counter >= n-1) then
    state <= waiting;
else
    state <= shifting;
end if; 

And that's it!

In the below chunk of code, note that the "reset" state and the "waiting" state are distinct. This is useful because generally the asynchronous reset only occurs at startup and is not expected to process any data during this time. By moving the temp_reg <= parallel_in to the waiting state (outside of the asynchronous reset), we are allowing the module driving parallel_in to start up correctly without having to send data during reset. Also, now the waiting state can be entered as necessary, without having to perform an asynchronous reset.

Also notice that I'm only driving 3 signals (4 counting the variable) in my process, and only those signals. If a signal is driven in one process, it shouldn't be driven anywhere else but that process. Not outside the process, not in another process. A signal is driven inside one process and one process only. You can compare the signal to other signals in other places (if statements, and such), but don't give the signal a value anywhere except in one process. And generally, it is defined in the reset portion, and then wherever necessary in the process proper. But only 1 process. If I'd been told this, it would have saved me tons of time while I was learning.

Here's the whole code in one chunk:

library ieee; 

use ieee.std_logic_1164.all;

entity SReg is

generic ( n : integer := 4);

port( clk: in std_logic; 
      reset: in std_logic; 
      enable: in std_logic; --enables shifting 
      parallel_in: in std_logic_vector(n-1 downto 0); 
      s_in: in std_logic; --serial input 
      s_out: out std_logic --serial output

);
end SReg;

architecture behavioral of SReg is

signal temp_reg: std_logic_vector(n-1 downto 0) := (Others => '0');
TYPE POSSIBLE_STATES IS (waiting, shifting);
signal state : POSSIBLE_STATES;
begin

process(clk,reset)
    variable shift_counter: integer := 0;
begin

   if(reset = '1') then
      temp_reg <= (others => '0');   
      state <= waiting;
      shift_counter := 0;
   elsif(clk'event and clk='1') then
        case state is
            when waiting =>
                shift_counter := 0;
                temp_reg <= parallel_in;
                s_out <= '0';
                if(enable = '1') then
                    state <= shifting;
                else
                    state <= waiting;
                end if;
            when shifting =>
                shift_counter := shift_counter + 1;
                s_out <= temp_reg(0);
                temp_reg <= s_in & temp_reg(n-1 downto 1);
                if (shift_counter >= n-1) then
                    state <= waiting;
                else
                    state <= shifting;
                end if; 
        end case;
    end if;
end process;

end behavioral;
\$\endgroup\$
5
  • 1
    \$\begingroup\$ stanri, that taught me alot. I sincerely thank you for your help. \$\endgroup\$
    – Orange
    Feb 6, 2013 at 14:35
  • 1
    \$\begingroup\$ If a signal is driven in one process, it shouldn't be driven anywhere else but that process. This makes a lot of sense. All too often am I met with the "multiple driver" error! What exactly is s_in doing here ? This shift register seems to latch in the parallel data and then shift out serially. \$\endgroup\$
    – sherrellbc
    Jan 27, 2017 at 0:01
  • 1
    \$\begingroup\$ @ShashankVM, In the case of the question, parallel_in supplies the reset value of the internal shift register. So I kept it with the same name and function. It is otherwise unused, OP must explain what it was originally intended for. I agree that a SISO register should be otherwise strictly serial. \$\endgroup\$
    – stanri
    Oct 25, 2020 at 12:18
  • 1
    \$\begingroup\$ @ShashankVM I wrote this answer 7 years ago with the intention of demonstrating the thinking behind using a state machine to solve problems like this. The intention is to demonstrate a general concept. The OP is welcome to modify it to his needs. My coding practices have changed since then and I'm sure there are multiple things anyone could fine wrong with this, since I wrote it in my free time and not as a rigorous academic exercise. You are welcome to write an answer if you think mine is inadequate. \$\endgroup\$
    – stanri
    Oct 26, 2020 at 6:40
  • 1
    \$\begingroup\$ Please see a discussion of your answer on Meta electronics.meta.stackexchange.com/q/7298/238188 \$\endgroup\$ Dec 9, 2020 at 12:33
-1
\$\begingroup\$

@stanri's answer is impresively thorough and quite accurate... if I may summarize/clarify the first statement though, the 'for' statement in an HDL simply expresses 'syntactic replication' not 'sequential execution'.

That is to say, it simply generates more hardware elements (gates), and does not inform process flow. I would say the loop is expanded at elaboration time (compilation), not that it 'executes in zero time', after all at runtime there will still be propagation delay through the elements generated by the 'for' construct.

Don't start by writing VHDL code, start by drawing logic schematics (at least at some level of abstraction). At the end of the day HDL is just a text-based way of expressing the content of logic schematics.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Although people have different approaches to digital design, your description of the for statement is not entirely correct. If you consider the generate statement then your description is true, but if you consider the loop statement inside a process, this is sequential code. If you loop contain signal assignments to the same signal then the previous assignment is overwritten by the next one, but if your dealing with variable it behaves just like regular sequential code. \$\endgroup\$
    – trondd
    Jan 20, 2014 at 12:37

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