Strange LSB flip bug when passing data out of component

[kureigu]

Hey all,
I was wondering if someone might be able to help me rid my code of an unusual bug…

I wrote an SPI slave component to receive and decode incoming commands from a microcontroller. I've tested it on it's own and it works perfectly.
However, when I include it in my top level design and try to make it pass some data out to act upon, something unusual happens; The LSB of the 'target_new' signal (unsigned, 4 bits) is flipped.
Now I know that the SPI slave is still receiving the correct data because it's required to echo the command back to the master where it is verified. So this leads me to believe it's something to do with passing the signal back from the component…

Here's the SPI slave/decoding component:

-- SPI Slave 
-- Tailored to work with netduino

library ieee;
use ieee.std_logic_1164.all;
use ieee.std_logic_arith.all;
use ieee.std_logic_unsigned.all;

entity netduino_bridge is
	generic( data_width: integer := 8
				);
	port( -- Hard ports
			clk: in std_logic;
			SCLK, MOSI, CS: in std_logic;
			MISO: out std_logic := '0';
			-- Soft ports
			target_out: out unsigned(3 downto 0);	
			target_in: in unsigned(3 downto 0);
			status: in unsigned(3 downto 0);
			cmd: out unsigned(7 downto 0)
			);

end netduino_bridge;


architecture behavioural of netduino_bridge is
	
	type state is (IDLE, Rx, Tx);
	signal SPI_state: state := IDLE;
	signal Rx_data, Tx_data: unsigned(data_width-1 downto 0);
	signal Rx_count, Tx_count: integer := 0;
	signal latch_data: std_logic := '0';
	signal target_reg: unsigned(3 downto 0) := "0001";
begin

	target_out <= target_reg;

-- Command decoding process
	process(latch_data)
	
	variable lock, search: std_logic := '0';
	
	begin
	
		if rising_edge(latch_data) then
		
			case Rx_data(7 downto 4) is		
			
				-- Set new target
				when x"1" =>
					Tx_data <= Rx_data;
					target_reg <= Rx_data(3 downto 0);

				-- Query current target/status
				when x"2" =>
					Tx_data <= status & target_in;
					
				-- Find 'real' zero
				-- A command to set the inclination to real zero level
				-- rather than zero relative to last position (based on step count)
									
				-- Echo
				when x"4" =>
					Tx_data <= x"4" & Rx_data(3 downto 0);		
			
				when others =>
					Tx_data <= x"E0";
			end case;
			
		end if;
		
	end process;


-- Communication process
	process(CS, SCLK, Rx_count, Tx_count) is
	begin
		
		if(CS = '0') then
		
			if rising_edge(SCLK) then	
				-- Receive			
				if(Rx_count < data_width) then
					Rx_count <= Rx_count + 1;
					Rx_data(data_width-1-Rx_count) <= MOSI; -- Shift in data on MOSI
				end if;
				-- Flag new data (Latch asynchronously)
				if(Rx_count = data_width-1) then
					latch_data <= '1';
				end if;	
				-- Hopefully time in here to decode the command and provide a reply.			
			end if;
			
			if falling_edge(SCLK) AND Rx_count = data_width then
			
				-- Transmit
				if(Tx_count < data_width) then
					Tx_count <= Tx_count + 1;
					MISO <= Tx_data(data_width-1-Tx_count);
				end if;
					
				
			end if;
		
		else
			-- Stop latching data_out to Rx_data
			Latch_data <= '0';
			-- Reset counters
			Tx_count <= 0;
			Rx_count <= 0;
			
		end if;
		
	end process;
	
end behavioural;

And here is what uses the commands in the top level:
There's obviously more in the top level than just this, but I omitted some chunks for clarity.

-- Signals and constants

	-- Positional
	signal azimuth, elevation: integer := 0;
	signal target_new: unsigned(3 downto 0) := x"1";
	signal target_set: unsigned(3 downto 0) := x"0";
	signal cmd: unsigned(7 downto 0) := x"00";
	
	-- Status flags
	signal Az_idle, El_idle: std_logic;
	signal status: unsigned(3 downto 0); -- ? zeroed, search, lock

-- Netduino interface
	netduino: entity work.netduino_bridge port map(mclk, net_SCLK, net_MOSI, net_CS, net_MISO, target_new, target_set, status, cmd);
	
------ PROCESSES ------	


	-- Deal with netduino commands
	
	process(target_fix) is
	begin
	
		-- if both axes are idling, accept new position.
		if(Az_idle = '1' and El_idle = '1') then
			
			case target_new is
				-- zero position
				when "0000" =>
					azimuth 	 <= 0;
					elevation <= 0;
				
				-- Sat 1	
				when "0001" =>
					azimuth 	 <= 50;
					elevation <= 50;
				
				-- Sat 2
				when "0010" =>
					azimuth 	 <= 100;
					elevation <= 300;
				
				-- Sat 3
				when "0011" =>
					azimuth 	 <= 200;
					elevation <= 180;
				
				-- Sat 4
				when "0100" =>
					azimuth 	 <= 300;
					elevation <= 100;
			
				-- Unrecognised; do nothing
				when others =>
					null;
			end case;
		
		end if;
		
		-- wait until pointing is finished and report lack of lock
		if (Az_idle = '0' AND El_idle = '0') then
			status(0) <= '0';
		else
		-- once pointing finished, update status
		target_set <= target_new;
		status(0) <= '1';
		end if;
		
	end process;

 

[TrickyDicky]

the assignment to target fix has the lsb of target_new inverted. Its right there in the code just remove the not from the target_fix assignment.

Other points to note though - I am pretty sure this code will not work in an FPGA:

1. You use both the rising edge and falling edge of SCLK
2. There is code using logic clocks.
3. You're missing signals in sensitivity lists, so simulation behaviour will not match behaviour of the real system.
4. Lots and lots of latches.

You need to make this design synchronous. You need a system clock to clock ALL of your logic, and then use things like SCK and latch_enable as clock enables.

 

[kureigu]

Ah crap, ignore that. I accidentally posted a different version of the code. That was me attempting to fix the problem, but it made no difference. You can see now 'target_fix' is gone and the case statement is based solely on 'target_new'.

1. I had to use both edges of SCLK to ensure that the SPI response was set on time for the master to read it.
2. I don't know what you mean by this...
3. I fully simulated the SPI Slave and it worked exactly to specification. I also tested it in hardware on its own and it works without any issue.
4. Latches seemed like the most straightforward way to do what I need to do, and the other components that deal with the latched variables (azimuth, elevation) rely on detecting changes to perform new actions. I don't see how else I'm supposed to do it.

Why does the whole design have to be synchronous though? Surely it's only things that rely specifically on timing and clock edges that need to be synchronous and the rest can just be transparent?

 

[TrickyDicky]

1. I am surprised the synthesisor let you get away with this. FPGAs only allow a single clock edge to be used.
2. Using latch enable as a clock. logic clocks and latches are very susseptable to timing problems (race conditions, temperature fluctuation etc).
3. What Im saying here is that yes, it may do, but in the process commented with "-- Deal with netduino commands", there are signals that are missing from the sensitivity list. The synthesisor ignores the sensitivity list and probably gave you a warning about the missing signals. the synthesisor will NOT match the behaviour here, it will synthesise the logic from the code.
4. As in 2, latches are suseptable to race conditions, metastability, temperature, glitches, basically all the things that can be easily removed with synchronous design.

If its working for you, great. But I wouldnt be surprised if you had some builds that worked and some that didnt, and then sometimes it works when cold but fails as the chip warms up. All because of the above problems. Synchronous design will remove ALL of these problems.

Your coding practices are very poor and would not be acceptable by most companies.

 

[kureigu]

Yeah, that's the trouble. No one has ever given me any guidance in how to achieve these things and avoid these problems, so I'm struggling as it is.

I thought sensitivity lists didn't matter though when it comes to hardware?

Could you possibly give me a little guidance on how to remove these latches and make this design more stable then? I'm still relatively new to VHDL so haven't a clue where to start with this.

 

[TrickyDicky]

Sensitivity lists dont matter when it comes to hardware, but they are crucial in simulation. A process in simulation will only be re-evaluated whenever a signal in the sensitivity list changes. The synthesisor ignores the list - it basically assumes all signals are in the list and then analyses the code to create the logic (apart from specific templates, below)

You need a system clock. I wouldnt use SCLK as this can be turned on and off by the master, but you might be able to use it. You need to follow the templates for synchronous design:

---

Code VHDL - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
process(clk, areset) --only put clock and async reset in here for a sync process
begin
  if areset = '1' then 
    --async reset
  elsif rising_edge(clk) then   --or falling edge, but never both for FPGA
    
    --this is the synchronous bit
 
    if sreset = '1' then
      --sync reset here
 
    elsif enable = '1' then
      --clock enable section
 
    end if;
 
    --code that doesnt need sync reset or enable goes here
  end if;
end process;

---

If you put everything in a synchronous process (apart from any glue logic) you should get no latches.

If you have an asynchronous process, to avoid latches here you need to ensure signals assigned in the process are assigned in every single path. So, in the process I previously highlightest, azimuth and elevation will be latched because in the case statement, for the others case you have put "null", meaning you want them to remember their values when the target_new value isnt one of those specified. But in the same process, status(0) isnt latched, bevause it is always asigned a value. So it will be '0' only when Az_idle = '0' AND El_idle = '0', otherwise it immediatly goes to '1', whereas target_set is latched.

 

[kureigu]

So if I did this...

Add a 'if risingedge(mclk)' around the whole thing.
Change each 'when' in the case statement to have a latch_data <= '1', and in my 'when others' have it clear the latch_data value.
Change the components that respond to the changes in elevation and azimuth to only latch the data when latch_Data is high.

Would this make my code more robust and more acceptable in general?

 

[TrickyDicky]

if you use a synchronous process, there is no need to make every signal in it have an assignment on every path (this then creates a clock enable).
You will need an mclk that is many times faster than SCLK, to ensure you sample SCLK safely.

But yes, synchronising the code would make your life easier.