Outputs aren't propagating out of case statement

[kureigu]

I've recently ripped apart some of my code to cut it into modular chunks, and I'm experiencing a problem that I've seen before but never found a way around...

When I change an output of my component inside the case statement, it doesn't seem to propagate out at all. I've got round this in the past by setting a flag and using a concurrent statement outside the process to apply the signal I wanted, but to do that in this situation would just be silly, especially when there's likely a fix.

Here's the code where the problem is:

-- Search_pattern.vhd --
------------------------
-- Carries out the search motion
-- Use with Control_arbiter.


library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity search_pattern is

    generic (step_inc: unsigned(7 downto 0) := "00010000" );

    port (clk: in std_logic;
            enable: in std_logic;
            dir: out std_logic;
            steps, speed, hold_time: out integer;
            Az_rdy, El_rdy: in std_logic;
            Az_run, El_run: out std_logic;
            lock: in std_logic
            );

end search_pattern;



architecture behavioural of search_pattern is

	type action is (LATCH_WAIT, WAIT_FOR_COMPLETE, MOVE_AZ_CW, MOVE_EL_CW, MOVE_AZ_CCW, MOVE_EL_CCW, FINAL_MOVE, RESET, LOCKED);
	signal search_state: action := WAIT_FOR_COMPLETE;
	signal last_state: action := MOVE_EL_CCW;


begin

	hold_time <= 30;
	speed <= 250000;
	steps <= 50;

	process(enable, Az_rdy, El_rdy)
	
	begin
			
		if rising_edge(clk) then
		
			if(enable = '1') then
			
				case search_state is
				
					when LATCH_WAIT =>
						-- Make sure a GPMC has registered the command before waiting for it to complete
						if(Az_rdy = '0' OR El_rdy = '0') then    -- xx_rdy will go low if a stepper starts moving
							search_state <= WAIT_FOR_COMPLETE;    -- Go to waiting state and get ready to issue next cmd
						end if;
				
					when WAIT_FOR_COMPLETE =>
						-- De-assert run commands
						El_run <= '0';
						-- Wait for the movement to complete before making next
						if(Az_Rdy = '1' and El_rdy = '1') then		
							-- Choose next command based on the last
							case last_state is
								when MOVE_EL_CCW  => search_state <= MOVE_EL_CW;
								when MOVE_EL_CW  => search_state <= MOVE_EL_CCW;
								when others => null; -- when locked wait for unlock?
							end case;
						end if;
						-- Constantly check for lock as well
						if(lock = '1') then
							search_state <= LOCKED;
						end if;					

					when MOVE_EL_CW =>
						dir <= '1';
						El_run <= '1';
						last_state <= MOVE_EL_CW;
						search_state <= LATCH_WAIT;

					when MOVE_EL_CCW =>
						dir <= '0';
						El_run <= '1';
						last_state <= MOVE_EL_CCW;
						search_state <= LATCH_WAIT;
						
					when others =>
						null;
				end case;
			else
				-- need to add reset and tracking functionality here.
			end if;
			
		end if;
	
	end process;
	
end behavioural;

The LATCH_WAIT and WAIT_FOR_COMPLETE states are just handshaking with the motor driver component. The two are linked through a higher level component that controls the signal linkages to the motor driver... essentially a multiplexer.

This component is supposed to drive a stepper in a back and forth motion, but what I've noticed is that the direction ('dir') output is always low, unless I set it outside the process. Same applies for 'steps'; If i set that inside one of the cases, it just ignores it and uses the default value.

 

[TrickyDicky]

Have you got a testbench for this code?

- - - Updated - - -

And any reason why you dont have clock in the sensitivity list, but you have other signals instead (that shouldnt be there)?

 

[kureigu]

I'm testing the code in hardware so I've not got a testbench for it but have been tracking relavent signals via test pins with a scope.

And the clock is in the sensitivity list in the version I'm running, I've just posted this from another computer and I think my dropbox is just a bit behind on updating.
Also, shouldn't those signals be there? I thought that anything that was read inside the process had to appear in the sensitivity list (which is irrelevant in this case anyway since I'm testing in hardware)

 

[TrickyDicky]

I would highly suggest writing a testbench and simulatin your design. Then you know for sure that its not a code problem. I cant see from first inspection why it wouldnt work - Unless the compiler has ignored your initial values for "last_state" signal and defaulted it to LATCH_WAIT, which certainly would cause a problem.

You only need signals in the sensitivity list that cause other things to change in the process. So in a clocked process, things only change on the rising edge of a clock, so you only need clock in the sensitivity list. If there is no clock, then it is important all signals go in there, because it is just decribing asynchronous logic.

 

[barry]

I think your problem is that you don't assign DIR for EVERY state. You need to give it a value, even if it's dir<=dir;
As far as steps, all I can see is that you assign it 50 and that's it. Why would you expect it to change?

 

[TrickyDicky]

I think your problem is that you don't assign DIR for EVERY state. You need to give it a value, even if it's dir<=dir;
As far as steps, all I can see is that you assign it 50 and that's it. Why would you expect it to change?

It wont make a difference. Dir will hold its value in the cases where it is not assigned.

 

[barry]

It wont make a difference. Dir will hold its value in the cases where it is not assigned.

Perhaps, but I believe I have run into situations in the past where the synthesis tool gets confused by something like this. It might be worth trying just to see if that fixes it (because I can't think of anything else).

Also, you can't count on the default signal assignment to work with synthesis. It might; it might not.

 

[kureigu]

Also, you can't count on the default signal assignment to work with synthesis. It might; it might not.

That seems like a pretty glaring issue they should have fixed. I don't understand how such a well established HDL can be so fickle when it comes to synthesis. It's full of strange occurrences like this.

I'll gave what you suggested a try, but with no avail. I can't read directly from 'dir' since it's an output, but I made a signal called 'dir_set' and changed the value of that, then just applied in an assignment statement outwith the process.
I also changed the 'when others' clause in the last_state case statement to point back round to MOVE_EL_CW which should get rid of any issue with last_state not initialising properly.

If it did default into another state, since it can't rely on the default assignments working, what defines where it will start? is it the other they appear in the Case statement? or the order they appear in the 'type' definition?

Also, the steps thing I mentioned I had discovered from another test earlier on, where I had been changing the value depending on the state but it defaulted to 0 regardless.

 

[std_match]

With "default signal assignment", I think barry means the initial value in the signal declaration.
That will never work if you write code for an ASIC. It normally works for an FPGA.

For me, "default signal assignment" is an assignment at the beginning of a VHDL process, which will prevent unwanted latches.
That should always work for synthesis.

 

[TrickyDicky]

That seems like a pretty glaring issue they should have fixed. I don't understand how such a well established HDL can be so fickle when it comes to synthesis. It's full of strange occurrences like this.

This is nothing to do with the language, its the synthesis tools. The langauge itself can do a lot of things that do not translate into actual logic. Initialised signals should work - you might want to check the synthesis report to make sure it actually has used this value for the power up state. (PS. Ive not had a problem in Quartus with initial values on signals for 6 years - they have always worked for me. To make sure, there is an attribute you can use to force the power up value

eg:
attribute altera_attribute : string;
attribute altera_attribute of my_reg : signal is "-name POWER_UP_LEVEL HIGH";

Of course, this wont help you with your enumerated state type, unless you specify the state encoding yourself (and this might be the reason its not accepting the correct power up state - Ive only ever used initial values on bits and busses).

But you still havent said anything about your testbench. I promise you you will take twice as long debugging the design on the bench than in a simulator. Simulations can massivly reduce bench debugging time. We cannot see your bench, so there is no more we can do for you. I promise you this will be a code error, and from the code you posted, there should be no problems with your code at this level.

 

[kureigu]

Update:

I simulated the design in ModelSim. The states are changing as expected and when expected, and the 'dir' output is apparently changing when instructed to also.

I still don't understand why I can't see this change outside of the component though. As well as feeding the motor driver, I have 'dir' linked to a board on the LED so I can see if it changes. Still no luck so far though...

Here's my modelsim output anyway, and the HDL for the other two components used




This is pretty straightforward really, just links the two components:

---------------------------
-- Motor Control Arbiter --
---------------------------
-- 
-- - Directly controls motor driver paramters such as H/F, CTRL and RST
-- - Links GPMC output signals to hardware ports
-- - Re-maps IO to allow different components access to motors
-- - 
--
-- Possible new features --
-- Should probably keep track of overall motor position too.
--
------------------------------------------------------------------------------

library ieee;
use ieee.std_logic_1164.all;
use ieee.std_logic_unsigned.all;
 
 
entity Control_arbiter is
	
	port (clk: in std_logic;
			EN, RST, CTRL, HALF, SCLK, CW, test: out std_logic_vector(2 downto 0)
			-- needs signals for levelling and lock
			);
			
end Control_arbiter;
 
 
architecture fsm of Control_arbiter is

	type action is (INIT, LEVEL, POINT, SEARCH);
	signal arbitration: action := INIT;

	-- motor function flags
	signal smf_level, smf_search: std_logic;
	
	-- Motor controller arbiter signals
	-- AZIMUTH
	signal Az_spd, Az_stps, Az_hold, Az_pos: integer;
	signal Az_dir, Az_rdy, Az_run: std_logic;
	-- ELEVATION
	signal El_spd, El_stps, El_hold, El_pos: integer;
	signal El_dir, El_rdy, El_run: std_logic;

	
	-- Level + point signals
	signal lvl_Az_spd, lvl_Az_stps, lvl_Az_hold: integer;
	signal lvl_Az_dir, lvl_Az_run: std_logic := '0';
	signal lvl_El_spd, lvl_El_stps, lvl_El_hold: integer;
	signal lvl_El_dir, lvl_El_run: std_logic := '0';
	-- Search signals
	signal search_spd, search_stps, search_hold: integer; 
	signal search_dir, search_Az_run, search_El_run: std_logic := '0';
	-- status
	signal lock: std_logic := '0';
	
begin

	-- Motor controller components
	Az_motor: entity work.motor_ctrl port map(clk, SCLK(2), CW(2), EN(2), Az_spd, Az_stps, Az_dir, Az_hold, Az_rdy, Az_run);
	El_motor: entity work.motor_ctrl port map(clk, SCLK(0), CW(0), EN(0), El_spd, El_stps, El_dir, El_hold, El_rdy, El_run);														
	
	-- Hold level and point components
	--Az_level: entity work.Az_demo port map();
	--El_level: entity work.El_demo port map();
	
	-- Search component
	search_cpmnt: entity work.search_pattern port map(	clk, smf_search, search_dir, search_stps, search_spd, search_hold, 
																		Az_rdy, El_rdy, search_Az_run, search_El_run, lock);
			
			
	process(clk, arbitration)
	
	begin
	
		if rising_edge(clk) then
		
			case arbitration is
			
				when INIT =>
					-- Initialise driver signals
					EN(1) <= '0';
					CW(1) <= '0';
					SCLK(1) <= '0';
					RST  <= "101";
					CTRL <= "101";
					HALF <= "101";

					-- Move to first stage
					arbitration <= SEARCH;
				
				when LEVEL =>
					null;
					-- Pointing assignment triggers movement to new location
				
				when POINT =>
					null;
					-- deal with moving to specfic Az/El angles
					-- after moving, enter 'search' mode
					
				when SEARCH =>
					-- Pass control to search
					smf_level <= '0';
					smf_search <= '1';

					-- On board LEDs
					test(0) <= Az_rdy;
					test(1) <= El_rdy;
					test(2) <= search_dir;
					-- Map search signals to motor controllers
					Az_dir <= search_dir;
					El_dir <= search_dir;
					Az_stps <= search_stps;
					El_stps <= search_stps;
					Az_spd <= search_spd;
					El_spd <= search_spd;
					Az_hold <= search_hold;
					El_hold <= search_hold;
					Az_run <= search_Az_run;
					El_run <= search_El_run;
					-- Once pointed, begin search maneuvers
					 -- map search signals to motor controllers
					 -- set a flag to begin search
					 -- if new pointing instruction received, break and move to that position (keep track of change)
					 -- On sensing 'lock', halt search
					 -- return to holding that position
					
			
			end case;
			
		end if;
	
	end process;

	
end fsm;

And The motor driver:

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


entity motor_ctrl is

	port(	clk: in std_logic;
	
			-- Hardware ports
			SCLK, CW, EN: out std_logic;	-- stepper driver control pins
			
			-- Soft ports
			Speed, steps: in integer;	
			dir: in std_logic;			-- 1 = CW; 0 = CCW;
			hold_time: in integer;		-- if > 0, steppers will be held on for this many clock periods after moving
			ready: out std_logic;		-- indicates if ready for a movement
			activate: in std_logic;		-- setting activate starts instructed motion. 
			pos_out: out integer			-- starts at 2000, 180deg = 200 steps, 10 full rotations trackable
			);

end motor_ctrl;



architecture behavioural of motor_ctrl is

	type action is (IDLE, HOLD, MOVE);
	signal motor_action: action := IDLE;

	signal clk_new: std_logic;
	signal position: integer := 2000;
	
	signal step_count: integer := 0;
	signal drive: boolean := false;

begin

	-- Clock divider
	clk_manipulator: entity work.clk_divide port map(clk, speed, clk_new);

	-- Drive motors
	with drive select
		SCLK <= clk_new when true,
						'0' when false;
	
	pos_out <= position;
	
	process(clk_new)
		-- Counter variables
		variable hold_count: integer := 0;		
	begin
	
		if rising_edge(clk_new) then
			case motor_action is
				
				when IDLE =>
					-- reset counter vars, disable the driver and assert 'ready' signal
					hold_count := 0;
					step_count <= 0;
					drive <= false;
					EN <= '0';
					ready <= '1';
					
					-- When activated, start moving and de-assert ready signal
					if(activate = '1') then
						motor_action <= MOVE;
						ready <= '0'; 
					end if;
					
				when HOLD =>
					-- Stop the step clock signal
					drive <= false;	
					-- Hold for given number of clock periods before returning to idle state
					if(hold_count = hold_time) then
						motor_action <= IDLE;
					end if;
					-- increment counter
					hold_count := hold_count + 1;
					
				when MOVE =>					
					-- Enable driver, apply clock output and set direction
					EN <= '1';
					drive <= true;
					CW <= dir;		
					
					-- track the position of the motor
					if(dir = '1') then	
						position <= steps + step_count;
					else
						position <= steps - step_count;
					end if;
					
					-- Increment count until complete, then hold/idle
					if(step_count < steps) then
						step_count <= step_count + 1;
					else
						motor_action <= HOLD;
					end if;
								
			end case;
		end if;
		
	end process;

	
end behavioural;

 

[K-J]

Update:
I simulated the design in ModelSim. The states are changing as expected and when expected, and the 'dir' output is apparently changing when instructed to also.

Is what you're simulating the entire design or just the module that you're having trouble with? If you're only simulating the module then it could be that you simply didn't connect the 'dir' output up to the top level of the design. Re-simulate to make sure that the simulation of the entire design works.

I still don't understand why I can't see this change outside of the component though. As well as feeding the motor driver, I have 'dir' linked to a board on the LED so I can see if it changes. Still no luck so far though...

Assuming that you are simulating the entire FPGA design, then there are a couple possibilities, none of which has to do with the code...
- The 'dir' pin is mapped to a different pin than the PCBA has it connected to.
- The net on the PCBA is shorted

Kevin Jennings

 

[kureigu]

You're correct, I was just simulating the one component to make sure things were working correctly.
I'll simulate the whole chain just now and have a look though.

What I do know, though, is that the hardware pin that 'dir' is assigned to does work. I reverted to a different project to check, and it seems to change/drive fine. The only difference (other than most of the program) is that it changes on the clock edge of a slower clock in that project. Don't suppose the clock speed could be an issue? It should still have about 0.1s between changing states while the motor moves anyway.

I think I've determined that the 'search_pattern' state machine doesn't enter the MOVE_EL_CCW state. My logic behind this is that the FPGA generally drives 'dir' with whatever value it is set to in 'MOVE_EL_CW', but doesn't change at all after that. This seems strange seeing as it operates fine in the simulation above.

 

[K-J]

This seems strange seeing as it operates fine in the simulation above.

Did you fix the sensitivity list that I think somebody pointed out earlier in the thread? Right now you have 'process(enable, Az_rdy, El_rdy)', but the logic as written says that it should be 'process(clk)'. Since synthesis ignores the sensitivity list, but simulation does not it can lead to differences between simulation and reality which is what you're seeing.

Kevin

 

[kureigu]

Yeah, I had fixed the sensitivity list by the time I simulated it.

I'm working on simulating the whole chain, I'll post back here when I have some results.