VHDL - Why is my testbench signal "X" (= bus conflict)?

[soFPG]

Hello,

I have to implement a SPI transceiver module in VHDL for a class at uni but for whatever reason I have some problems with a signal in my test bench which is "X" (e.g. bus conflict state).

This is my SPI process:

Code: [Select]

    process(clk)
    begin
        if rising_edge(clk) then
       
            if reset = '1' then
                remaining_transmit_bits <= (others => '0');
                tx_data <= (others => '0');
                rx_data <= (others => '0');
                sclk_prescaler <= (others => '0');
                in_transmit <= '0';
                tx_done <= '0';
                temp_sclk <= '0';
                mosi <= '0';
                cs <= '1';
            elsif latch_tx_data = '1' then
                in_transmit <= '0';
                tx_data <= tx_vec;
                rx_data <= (others => '0');
                remaining_transmit_bits <= (others => '1');
                temp_sclk <= '0';
            elsif remaining_transmit_bits = all_zeros and in_transmit = '1' then
                in_transmit <= '0';
                cs <= '1';
                temp_sclk <= '0';
                tx_done <= '1';
            elsif start_transmission = '1' and in_transmit = '0' then
                in_transmit <= '1';
                cs <= '0';
                temp_sclk <= '0';
            elsif in_transmit <= '1' then
                temp_sclk <= temp_sclk xor '1';
                rx_data <= rx_data(rx_data'left-1 downto 0) & miso;
                tx_data <= std_logic_vector( shift_right(unsigned(tx_data), 1) );
                remaining_transmit_bits <= std_logic_vector( unsigned(remaining_transmit_bits) - 1 );
            else
               
            end if;
        end if;
    end process;
   
    sclk <= temp_sclk;
    rx_vec <= rx_data;
    mosi <= '1'; --tx_data(0);

As you can see I am writing a constant '1' to 'mosi' for testing purposes.

This is my test bench setup:

Code: [Select]

    stimuli: process
    begin

        tb_reset <= '1';
        tb_spi_miso <= '0';
        tb_latch_tx_data <= '0';
        tb_start_transmission <= '0';
        tb_spi_trans_data <= x"00000000";

        wait for CLOCK_PERIOD;
       
        wait for CLOCK_PERIOD;
       
        tb_reset <= '0';
       
        tb_spi_trans_data <= x"AAAAAAAA";       -- 0101 0101 0101 0101 0101 0101 0101 0101
        tb_latch_tx_data <= '1';
       
        wait for CLOCK_PERIOD;
       
        tb_latch_tx_data <= '0';
        tb_start_transmission <= '1';
        tb_spi_miso <= '1';
       
        wait for CLOCK_PERIOD;
       
       
        wait;
    end process;

I don't know really why tb_spi_mosi is a bus conflict, but it is.

Any help very much appreciated.

[rstofer]

You are setting the MOSI signal inside the process and also outside.

[SiliconWizard]

You are setting the MOSI signal inside the process and also outside.

Yep.
The culprit is in the reset part of the transmit process, where there is 'mosi <= '0';'.
Note that you'd get the same conflict whether you assign '1' to mosi or 'tx_data(0)' (which is commented out here) outside of the process. You can't do that.

[soFPG]

Thank you

Do you have any idea why my tb_spi_sclk signal continues to toggle even though transmit should be finished after 32 bits?


also remaining_transmit_bits somehow magically resets itself   

Code: [Select]

    process(clk)
    begin
        if rising_edge(clk) then
       
            if reset = '1' then
                remaining_transmit_bits <= (others => '0');
                tx_data <= (others => '0');
                rx_data <= (others => '0');
                sclk_prescaler <= (others => '0');
                in_transmit <= '0';
                temp_tx_done <= '0';
                temp_sclk <= '0';
                cs <= '1';
            elsif latch_tx_data = '1' then
                temp_tx_done <= '0';
                in_transmit <= '0';
                tx_data <= tx_vec;
                rx_data <= (others => '0');
                remaining_transmit_bits <= std_logic_vector( to_unsigned(BITWIDTH, remaining_transmit_bits'length) ); --(others => '1');
                temp_sclk <= '0';
            elsif remaining_transmit_bits = all_zeros and in_transmit = '1' then
                in_transmit <= '0';
                cs <= '1';
                temp_sclk <= '0';
                temp_tx_done <= '1';
            elsif start_transmission = '1' and in_transmit = '0' then
                in_transmit <= '1';
                cs <= '0';
                temp_sclk <= '0';
            elsif in_transmit <= '1' then
                temp_sclk <= temp_sclk xor '1';
                rx_data <= rx_data(rx_data'left-1 downto 0) & miso;
                tx_data <= std_logic_vector( shift_right(unsigned(tx_data), 1) );
                remaining_transmit_bits <= std_logic_vector( unsigned(remaining_transmit_bits) - 1 );
            elsif temp_tx_done = '1' then
               
            end if;
        end if;
       
        if falling_edge(clk) then
            if in_transmit = '1' and not reset = '1' then
                temp_sclk <= temp_sclk xor '1';
            else
               
            end if;
        end if;
    end process;
   
    sclk <= temp_sclk;
    rx_vec <= rx_data;
    mosi <= tx_data(0);
    tx_done <= temp_tx_done;


[rstofer]

I would make this code into a more conventional Finite State Machine using the 'case' statement with specific states rather than a priority tree configuration.

Every output signal (those that appear on the left hand side of <=) in your tree must be defined in every path, you can't leave the signals as some kind of assumption.  If the signals are not defined in every path, latches will be inferred and you should get some kind of warning.  I'm not sure that every signal you are dealing with is actually defined in every 'elseif'.  If you have an if-elseif-else structure, you need the signal defined (one way or another) in all 3 places.

Look at page 105 (PDF page 117) here:

http://freerangefactory.org/pdf/df344hdh4h8kjfh3500ft2/free_range_vhdl.pdf

Notice how signals Z1 and Z2 are defined default values such that they don't need to be defined in each and every state.  These default values are redefined in specific states and the last assignment in the code loop prevails.  The point is:  make certain that every single signal is defined under all possible conditions.  Otherwise, your logic will fail spectacularly.

BTW, this code is a little pedantic because Z1 and Z2 are defined in every case and under every condition even without the default assignment.  That's fine when the FSM only has a few outputs but if you get into something with 50 or 60 outputs, you really don't want to have to define them under all conditions, one line each, in a FSM with 100 states.

If you develop code with a 'stepwise refinement' approach, it is pretty easy to add a new signal to an FSM and forget to include a default value.  When I do this, nothing works and I get a warning about inferring a latch.  I have made that mistake more than once...

You will notice that the code is also a two-process variant.  One process is synchronous and deals with next state (PS <= NS) and the other is combinatorial and does all of the real work.  There are also one-process and three-process configurations.

https://vhdlwhiz.com/n-process-state-machine/

I use only the two-process configuration...

Here's a small sample of a two-process FSM.  Note the number of default assignments:

Code: [Select]

   -- FSM Next State
    process(reset,Pulse,NextState)
    begin
        if rising_edge(pulse) then
            if reset = '1' then
                State <= 18;
            else
                State <= NextState;
            end if;
        end if;
    end process;
   
    -- FSM
    process(State, BEN, IR,MemReady, Immediate, PSR, Interrupt)
    begin
        -- set default values for all signals
        GateBusSelect   <= GatePC;
        MIOenable       <= '0';
        LD_MAR          <= '0';
        LD_MDR          <= '0';
        LD_IR           <= '0';
        LD_BEN          <= '0';
        LD_PC           <= '0';
        LD_Reg          <= '0';
        LD_CC           <= '0';
        LD_Privilege    <= '0';
        LD_Priority     <= '0';
        LD_PSR          <= '0';
        LD_SavedUSP     <= '0';
        LD_SavedSSP     <= '0';
        LD_VectorAddr   <= '0';
        PCselect        <= PCplus1;
        MARSelect       <= MARzext;
        Addr1Select     <= Addr1PC;
        Addr2Select     <= Addr2Zero;
        SR2select       <= SR2OutSel;
        SR1addrSelect   <= SR1usesSR1;
        ALUop           <= ALUpassA;
        DRaddrSelect    <= DRusesDR;
        MemWr           <= '0';
        SPselect        <= SPminusOnesel;
        VectorMux       <= "00";
        Privilege       <= '0';
       
        case State is
            when  0     =>  -- Branch Instruction
                            if BEN = '1' then
                                NextState   <= 22;  -- branch is taken
                            else
                                NextState   <= 18;  -- branch not taken   
                            end if;                             
            when  1     =>  -- ADD Instruction
                            if Immediate = '1' then
                                SR2select <= Sext5sel;
                            else
                                SR2select <= SR2outSel;
                            end if;
                            ALUop           <= ALUadd;
                            GateBusSelect   <= GateALU;
                            LD_Reg          <= '1';
                            LD_CC           <= '1';
                            NextState       <= 18;                           
            when  2     =>  -- LD Instruction
                            Addr1Select     <= Addr1PC;
                            Addr2Select     <= Addr2Sext9;
                            MARselect       <= MARadder;
                            GateBusSelect   <= GateMARmux;
                            LD_MAR          <= '1';
                            NextState       <= 25;

...and so on... 

[/font]

There are about 27 default assignments that I don't have to rewrite under every condition.

[soFPG]

I would make this code into a more conventional Finite State Machine using the 'case' statement with specific states rather than a priority tree configuration.

How would you implement the SPI clock?

I guess I somehow have to change the clock on every rising and falling edge of the input clk signal?

[rstofer]

I used a Maxim 16 digit 7 segment display chip for my IBM1130 project.  It is based on an SPI input.  Here's my code.  The input Clk is divided by the upper level logic from 50 MHz (system clock) to 25 MHz.

I'm not sure how ISE messed up the tabs.  Fixing them is an exercise for the reader.  I made a pass at it.  Watch the nested case satements and their related when's

Code: [Select]

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.STD_LOGIC_ARITH.ALL;
use IEEE.STD_LOGIC_UNSIGNED.ALL;

---- Uncomment the following library declaration if instantiating
---- any Xilinx primitives in this code.
--library UNISIM;
--use UNISIM.VComponents.all;

entity SPI is
    Port ( Clk        : in std_logic;
           SPI_Clk    : out std_logic;
           SPI_DataOut  : out std_logic := '0';
           SPI_CS_n    : out std_logic := '1';
        Reg_0      : in std_logic_vector(15 downto 0);
        Reg_1      : in std_logic_vector(15 downto 0);
        Reg_2      : in std_logic_vector(15 downto 0);
        Reg_3      : in std_logic_vector(15 downto 0)
          );
end SPI;

architecture Behavioral of SPI is

   type  SPI_type is (
                INIT,
                        IDLE,
                        LOAD,
                        CS,
                        CLOCK,
                        CS1,
                        DONE,
                        STALL,
                        STALL1,
                        STALL2
                     );
   type   DisplayType is (
                           DisplayUnblank,
                           DisplayIntensity,
                           DisplayTopRow,
                           DisplayBottomRow
                         );
   signal state         : SPI_type := INIT;
   signal Displaying    : DisplayType := DisplayUnblank;
   signal DisplayValue  : std_logic_vector(31 downto 0);
   signal Digit         : std_logic_vector(2 downto 0);
   signal Shifter       : std_logic_vector(15 downto 0);
   signal BitCount      : integer;
   signal HighBit       : std_logic;

begin

   SPI_DataOut <= Shifter(15);

   process(Clk) is
   begin
    if Rising_Edge(Clk) then
         case state is
            when INIT  => state <= IDLE;
            when IDLE   => case Displaying is
                when DisplayUnblank  =>  Shifter        <= x"0401";
                                         splaying       <= DisplayIntensity;
                                         Digit          <= "111";
                                         BitCount       <= 15;
                                         state          <= CS;
                when DisplayIntensity=>  Shifter        <= x"0208";
                                         Displaying     <= DisplayTopRow;
                                         Digit          <= "111";
                                         BitCount       <= 15;
                                         state          <= CS;
                when DisplayTopRow   =>  DisplayValue   <= Reg_1 & Reg_0;
                                         Displaying     <= DisplayBottomRow;
                                         Digit          <= "000";
                                         HighBit        <= '0';
                                         state          <= LOAD;
                when DisplayBottomRow=>  DisplayValue   <= Reg_3 & Reg_2;
                                         Displaying     <= DisplayTopRow;
                                         Digit          <= "000";
                                         HighBit        <= '1';
                                         state          <= LOAD;
                end case;
            when LOAD   => case Digit is
                              when "000"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue(31 downto 28);
                              when "001"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue(27 downto 24);
                              when "010"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue(23 downto 20);
                              when "011"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue(19 downto 16);
                              when "100"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue(15 downto 12);
                              when "101"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue(11 downto  8);
                              when "110"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue( 7 downto  4);
                              when "111"  => Shifter <= "0010" & HighBit & Digit & "0000" & DisplayValue( 3 downto  0);
                              when others =>
                           end case;
                           BitCount <= 15;
                           state <= CS;
            when CS     => SPI_CS_n <= '0';
                           state <= CLOCK;
            when CLOCK  => SPI_Clk  <= '1';
                           if BitCount = 0 then
                              state <= CS1;
                           else
                              state <= STALL;
                           end if;
                           BitCount    <= BitCount - 1;
            when CS1    => SPI_Clk    <= '0';
                           Digit       <= Digit + 1;
                           state       <= DONE;
            when DONE   => SPI_CS_n   <= '1';
                           if Digit = "000" then
                              state <= IDLE;   -- all digits shifted out
                           else
                              state <= LOAD;   -- more digits to shift
                           end if;
            when STALL  => state    <= STALL1;
            when STALL1 => SPI_Clk  <= '0';
                           Shifter  <= Shifter(14 downto 0) & '0';
                           state    <= STALL2;
            when STALL2 => state    <= CLOCK;
         end case;
      end if;
   end process;

end Behavioral;

[/font]

[SiliconWizard]

I would make this code into a more conventional Finite State Machine using the 'case' statement with specific states rather than a priority tree configuration.

How would you implement the SPI clock?

I guess I somehow have to change the clock on every rising and falling edge of the input clk signal?

The usual if you're implementing SPI master is to clock the transmit process at twice the frequency of the SPI clock. It also gives you the opportunity to set the new data bit at the right SPI clock edge.

Doing stuff on both the rising and falling edge of a clock is to be generally avoided on FPGAs. Most of them don't have dual-edge flip-flops. So the tools may either prevent you from doing this (inside one process), or give very poor results timing-wise.

[soFPG]

If I don't want a signal to change while my state machine process is executed, is this considered a good idea?

Code: [Select]

my_signal <= my_signal

e.g.

Code: [Select]

    process(state, latch_tx_data, start_transmission, remaining_transmit_bits)
    begin
        tx_data <= (others => '0');
        rx_data <= (others => '0');
       
        in_transmit <= '0';
        temp_sclk <= '0';
        temp_tx_done <= '0';
       
        cs <= '1';
       
        case state is
            -- default
            when "000" =>
                if latch_tx_data = '1' then
                    next_state <= "001";
                    tx_data <= tx_vec;
                    remaining_transmit_bits <= std_logic_vector( to_unsigned(BITWIDTH, remaining_transmit_bits'length) );
                else
                    next_state <= "000";
                end if;
            when "001" =>
                if start_transmission = '1' then
                    cs <= '0';
                    next_state <= "010";
                else
                    next_state <= "001";
                end if;
            when "010" =>
                if remaining_transmit_bits = all_zeros then
                    cs <= '1';
                    next_state <= "100";
                else
                    cs <= '0';
                    temp_sclk <= temp_sclk xor '1';
                    rx_data <= rx_data(rx_data'left-1 downto 0) & miso;
                    tx_data <= std_logic_vector( shift_right(unsigned(tx_data), 1) );
                    remaining_transmit_bits <= std_logic_vector( unsigned(remaining_transmit_bits) - 1 );
                    next_state <= "011";
                end if;
            when "011" =>
                cs <= '0';
                temp_sclk <= temp_sclk xor '1';
                rx_data <= rx_data;
                tx_data <= tx_data;
                next_state <= "010";
            when "100" =>
                temp_tx_done <= '1';
                next_state <= "000";
            when others =>
                next_state <= "000";
        end case;
    end process;

Btw. how do I enable warning / error in Vivado if latches are inferred?

[rstofer]

If I don't want a signal to change while my state machine process is executed, is this considered a good idea?

Code: [Select]

my_signal <= my_signal


IMO, if my_signal is generated inside a combinatorial process (not clocked), it doesn't exist over the transition.  Since my_signal is being asked to retain state, it will infer a latch.

I don't know that a signal can default to itself in the combinatorial process.  It would still seem to require a latch to retain state.

In the one-process FSM, every signal is registered because all assignments are done at a clock edge.  In this case, the assignment will work.  The two and three process configurations do all the heavy lifting in a combinatorial process.

Btw. how do I enable warning / error in Vivado if latches are inferred?

I'm pretty sure it is on by default

https://www.xilinx.com/support/answers/12832.html

There are a number of links returned by a Google query for 'vivado inferred latch'.

[soFPG]

IMO, if my_signal is generated inside a combinatorial process (not clocked), it doesn't exist over the transition

No, it is generated in the architecture "header" (between the keywords architecture and begin).

I think I got it working but what I still don't understand why "remaining_transmit_bits" is reset to "0" after being set to "32" in case I take it up into the sensitivity list, but if I leave it out it works as expected (reduced one by one):

Code: [Select]

architecture behav of SPI is

    constant all_zeros :                std_logic_vector(natural(ceil(log2(real(BITWIDTH)))) downto 0) := (others => '0');

    signal remaining_transmit_bits :    std_logic_vector(natural(ceil(log2(real(BITWIDTH)))) downto 0); -- How many bits are left before transmit is finished
    signal tx_data:                     std_logic_vector(BITWIDTH - 1 downto 0);    -- Latched data user wants to transmit when enable goes high
    signal rx_data:                     std_logic_vector(BITWIDTH - 1 downto 0);    -- Received data from slave
   
    signal in_transmit:                 std_logic;                                  -- Indicates whether we are currently transmitting data
   
    signal temp_sclk:                   std_logic;
    signal temp_tx_done:                std_logic;
   
    signal state:                       std_logic_vector(2 downto 0);
    signal next_state:                  std_logic_vector(2 downto 0);

begin

    process(clk)
    begin
        if rising_edge(clk) then
            if reset = '1' then
                state <= "000";
            else
                state <= next_state;
            end if;
        end if;
    end process;

    process(state, latch_tx_data, start_transmission)
    begin
        remaining_transmit_bits <= remaining_transmit_bits;
        rx_data <= rx_data;
        tx_data <= tx_data;
       
        in_transmit <= '0';
        temp_sclk <= '0';
        temp_tx_done <= temp_tx_done;
       
        cs <= '1';
       
        case state is
            when "000" =>
                if latch_tx_data = '1' then
                    next_state <= "001";
                    tx_data <= tx_vec;
                    rx_data <= (others => '0');
                    temp_tx_done <= '0';
                    remaining_transmit_bits <= std_logic_vector( to_unsigned(BITWIDTH, remaining_transmit_bits'length) );
                else
                    next_state <= "000";
                end if;
            when "001" =>
                if start_transmission = '1' then
                    cs <= '0';
                    next_state <= "010";
                else
                    next_state <= "001";
                end if;
            when "010" =>
                if remaining_transmit_bits = all_zeros then
                    cs <= '1';
                    next_state <= "100";
                else
                    cs <= '0';
                    temp_sclk <= temp_sclk xor '1';
                    rx_data <= rx_data(rx_data'left-1 downto 0) & miso;
                    tx_data <= std_logic_vector( shift_right(unsigned(tx_data), 1) );
                    remaining_transmit_bits <= std_logic_vector( unsigned(remaining_transmit_bits) - 1 );
                    next_state <= "011";
                end if;
            when "011" =>
                cs <= '0';
                temp_sclk <= temp_sclk xor '1';
                next_state <= "010";
            when "100" =>
                temp_tx_done <= '1';
                next_state <= "000";
            when others =>
                next_state <= "000";
        end case;
    end process;
   
    sclk <= temp_sclk;
    rx_vec <= rx_data;
    mosi <= tx_data(0);
    tx_done <= temp_tx_done;

end behav;

Do I have to add "remaining_transmit_bits" to the sensitivity list or not?

[rstofer]

You are asking "remaining_transmit_bits" to retain state which should give you a latch error.  You simply can't do it that way.

You are asking some other signals to retain state when you define the default values.

Look again at the spi code I posted above.  Despite what I have said about one, two or three process FSMs, that is a 1 process variant so all signals generated are registered by default.  As a result, BitCount can be a simple counter and retain it's value during transitions.  Everything is done under a clock.

ETA
Sometimes I pull state retaining logic outside the combinatorial process by creating a multitude of small clocked processes like:

Code: [Select]

process(Clk, Reset, ShifterCmd)
begin
if Reset = '1' then
Shifter <= (others => '0');
elsif rising_edge(Clk) then
case ShifterCmd is
when ShifterNOP => null;
when ShifterLoad => Shifter <= "00" & PlotterDataIn(15 downto 10);
when ShifterShift => Shifter <= Shifter(6 downto 0) & PLOTTER_SDO;
when others => null;
end case;
end if;
end process;

[/font]

I generate the ShifterCmd signal in a combinatorial process like

Code: [Select]

...other code in FSM...
when LOAD => ShifterCmd <= ShifterLoad;
...more code in FSM
[/font]

There is also a default value at the top of the process

Code: [Select]

  ShifterCmd <= ShifterNOP;
[/font]

When I use one-process FSMs, I know that everything is registered but I tend to prefer the two-process variant.  It's a choice but it does affect timing.  In general, outputs are delayed by 1 clock.  That is, I can determine what the output should be in a clock period but it doesn't show up at the output until the next clock.  Other logic that depends on the output will be delayed.  Sometimes it matters, sometimes it doesn't.

[rstofer]

Here's a nice explanation of sensitivity lists and what goes where

https://stackoverflow.com/questions/8991223/when-must-a-signal-be-inserted-into-the-sensitivity-list-of-a-process

I think I'm going to start using the 'all' keyword and blow off the list.  It is only used in simulation and is totally ignored in synthesis.  You might still get a warning but it omitting a signal won't result in incorrect logic.

[soFPG]

Hmm I don't get a latch error, simulation and synthesis run fine.

But it wasn't a big problem transferring the combinatorial process into the first clocked process so I guess it should be ok now, thank you! 

Quite confusing

[Bassman59]

I would make this code into a more conventional Finite State Machine using the 'case' statement with specific states rather than a priority tree configuration.

Every output signal (those that appear on the left hand side of <=) in your tree must be defined in every path, you can't leave the signals as some kind of assumption.  If the signals are not defined in every path, latches will be inferred and you should get some kind of warning.

This is why we write state machines in a single synchronous process, instead of having two processes, one synchronous and one combinatorial.

I've never understood why anyone writes a two-process state machine.

[rstofer]

The choice of one, two or three process FSMs is fairly subjective.  They all work.

The author has an interesting conclusion about who picks which style.  I don't think it is universally true but it is interesting.

https://vhdlwhiz.com/n-process-state-machine/

[SiliconWizard]

The choice of one, two or three process FSMs is fairly subjective.  They all work.

The author has an interesting conclusion about who picks which style.  I don't think it is universally true but it is interesting.

https://vhdlwhiz.com/n-process-state-machine/

I think we had a fairly lengthy discussion about this topic a while ago. Don't have the link right now but AFAIR, it was fairly comprehensive, and I remember someone posting a paper studying various approaches for FSMs. Although being somewhat subjective style-wise, there were a few objective points, but AFAIR, the one-process style would usually "win", except in particular situations. If someone can find this thread...