state machine problem

[janlee]

I wrote a state machine something like this:
--
process(clk,rst_n)
begin
if rst_n = '0' then
current_state <= s0;
elsif clk'event and clk = '1' then
current_state <= next_state;
-- toggle some bit according to state
...
end if;
end process;

process(clk)
begin
case current_state is
when s0 =>
if "some invent" then
next_state <= s1;
end if;
when s1 =>
if "some invent" then
next_state <= s2;
end if;
when s2 =>
if "some invent" then
next_state <= s3;
end if;
when others =>
next_state <= s0;
end case;
end process;

The problem is this state machine some times runs good , some times it runs into unknow state . I use altera signal tap II to trace all the state s0~s3.when state machine goes wrong ,s0~s3 are all '0',that is to say current state is not in the s0~s3. I was very puzzled now.
Is someone out there can give me some suggestions and coding style about state machine.
Many appreciations!

 

[MWind]

You have state 1 (S1) feeding back to S1 - should have next state assigned to S2. Tools will not implement an state machine without being able to resolve all the states.

 

[lambtron]

Regarding coding style, it's always a good idea to label your processes with meaningful names.

The second process includes clk in its sensitivity list, but clk is not used in any way to generate next_state. This is not exactly an error, but it will likely generate at least a warning.

Since next_state is combinatorial, and it is used exclusively by the first process, it might be a good idea to make next-state a variable that is defined within the first process. There really is no good reason to expose next_state outside the scope of the first process, which is the essence of your state machine.

 

[yasser_shoukry]

To implement state machines, you need two processes.

The First one is a sequential process, this means it should be sensitive to the clock and reset edges.

The second one is the combinational one (I think you should revise it). Being combiantional , this means it should be senstive to all the signals you check inside it (e.g some_invent signal).

The other functionality for this combinational process is to toggle the signals according to the state (not in the sequential one). If you want to save these signals in some register bits, then you will need to "assert" flag signals to be toggled in the combinational process, check these signlas in the sequential one, if its is 1 then toggle the register bits.

Hope It helps you.
Good Luck

 

[janlee]

First of all ,thanks for your replay.
This time I post my code here.I want to use PCI9054 to control my FPGA and other peripherals.So I have to convert PCI9054 synchronous signals to asynchronous signals ,just like wr_n,rd_n,ack_n and etc.
The problem is the code sometime behaves good ,sometime it makes the system into crash. How can I do?

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

entity PCI9054_IF is
port(
		RST_N : in std_logic;
		LCLK : in std_logic;
		LHOLD : in std_logic;
		ADS_N : in std_logic;
		BLAST_N : in std_logic;
		LWDRD_N : in std_logic;
		LA : in std_logic_vector(31 downto 2);
		LD : inout std_logic_vector(31 downto 0);
		LHOLDA : out std_logic;
		READY_N : out std_logic;
		ACK_n : in std_logic;
		WR_n : out std_logic;
		RD_n : out std_logic
				
	);
end PCI9054_IF;

architecture RTL of PCI9054_IF is
type STATE_TYPE is (IDLE,START,WAITSTATE,LAST);
signal CURRENT_STATE,NEXT_STATE : STATE_TYPE;
signal iaddr : std_logic_vector(31 downto 2);

begin
	--Grant for local bus request
	process(LCLK)
	begin
		if (LCLK'event and LCLK = '1') then
			if LHOLD = '1' then
				LHOLDA <= LHOLD;
			else
				LHOLDA <= '0';
			end if;
		end if;
	end process;
	
	--State Machine Transition
	process(LCLK)
	begin
		if RST_n = '0' then
			CURRENT_STATE <= IDLE;
		elsif(LCLK'event and LCLK = '1') then
			CURRENT_STATE <= NEXT_STATE;
		 	
			if CURRENT_STATE = IDLE and ADS_N = '0'then
				iaddr <= LA;
			end if;
			
			
		end if;
	end process;
	
	--State Machine
	process(CURRENT_STATE,ADS_N,LWDRD_N,BLAST_N)
	begin
		
		case CURRENT_STATE is
			when IDLE =>
				RD_n <= '1';
				WR_n <= '1';
				READY_n <= '1';
				if ADS_N = '0' then
					NEXT_STATE <= START;
				else
					NEXT_STATE <= IDLE;
				end if;
			
			when START =>
				RD_n <= '1';
				WR_n <= '1';
				READY_n <= '1';
				if BLAST_N = '0' then
					NEXT_STATE <= WAITSTATE;
				else
					NEXT_STATE <= START;
				end if;
			
			when WAITSTATE =>
				RD_n <= LWDRD_N;
				WR_n <= not LWDRD_N;
				
				if iaddr(31 downto 4) = B"0010_0000_0000_0000_1001_0000_0000"   then
					if ACK_n = '0' then
						NEXT_STATE <= LAST;
						READY_n <= '0';
					else
						NEXT_STATE <= WAITSTATE;
					end if;
				else
					NEXT_STATE <= LAST;
					READY_n <= '0';
				end if;
				
			
			when LAST =>
				RD_n <= '1';
				WR_n <= '1';
				READY_n <= '1';
				if ADS_N = '1' and BLAST_N = '1' then
					NEXT_STATE <= IDLE;
				elsif ADS_N = '0' then
					NEXT_STATE <= START;
				else
					NEXT_STATE <= LAST;
				end if;
			
			when others =>
				RD_n <= '1';
				WR_n <= '1';
				READY_n <= '1';
				NEXT_STATE <= IDLE;
		end case;
	end process;
	
	
end RTL;

Added after 10 minutes:

The following nop state was removed from the code provided before.
It is good situation

It is bad situations

[/img]

 

[Alex321]

What is undescribed state "nop" of CURRENT_STATE?

 

[nand_gates]

Check ur ACK_n signal which you are geting from ur peripherals needs to be
asserted at least one LCLK cycle; Ur using this signal to generate REARY_n signal
combinatorily which needs to be asserted for 1 LCLK cycle!!
Hope this helps!

Added after 13 minutes:

ur MEALY statemachine runs on LCLK for which ACK_n signal is asynchronous
and I think thats ur problem. Get the ACK_n signal in sync with LCLK first
before using it in state machine!


Also ACK_n is missing from Process sensitivity list??

 

[janlee]

Thanks for yasser_shoukry & nand_gates 's help.


To yasser_shoukry, I prefer your coding styles.Just like u said toggle all signal in the combinational process,and if u want the signal to be registed u can use dff in the sequential process.

To nand_gates,when I assert WR_n or RD_n ,I will wait for ACK_n to be asserted on the rising edge of LCLK,and on the next rising edge I registed the READY_n from the internal signal iready_n, I think it is the exactly the same way as ur replay.

This time I post my modified code here.

The lines commented by "new section" is the modified part.It is terribly wrong of the behavior of ready_n,I didn't all the conditions in the WAITSTATE.

Now it works fine

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

entity PCI9054_IF is
port(
		RST_N : in std_logic;
		LCLK : in std_logic;
		LHOLD : in std_logic;
		ADS_N : in std_logic;
		BLAST_N : in std_logic;
		LWDRD_N : in std_logic;
		LA : in std_logic_vector(31 downto 2);
		LD : inout std_logic_vector(31 downto 0);
		LHOLDA : out std_logic;
		READY_N : out std_logic;
		ACK_n : in std_logic;
		WR_n : out std_logic;
		RD_n : out std_logic
				
	);
end PCI9054_IF;

architecture RTL of PCI9054_IF is
type STATE_TYPE is (IDLE,START,WAITSTATE,LAST);
signal CURRENT_STATE,NEXT_STATE : STATE_TYPE;
signal iready_n : std_logic;

signal iwr_n,ird_n : std_logic;
signal iaddr : std_logic_vector(31 downto 2);

begin
	
	
	--
	--Grant for local bus request
	process(LCLK)
	begin
		if (LCLK'event and LCLK = '1') then
			if LHOLD = '1' then
				LHOLDA <= LHOLD;
			else
				LHOLDA <= '0';
			end if;
		end if;
	end process;
	
	--State Machine Transition
	process(LCLK)
	begin
		if RST_n = '0' then
			CURRENT_STATE <= IDLE;
		elsif(LCLK'event and LCLK = '1') then
			CURRENT_STATE <= NEXT_STATE;
		 	
			if CURRENT_STATE = IDLE and ADS_N = '0'then
				iaddr <= LA;
			end if;

			READY_n <= iready_n ;      --new section
			
		end if;
	end process;
	
	--State Machine
	process(CURRENT_STATE,ADS_N,LWDRD_N,BLAST_N,ACK_n)
	begin
		NEXT_STATE <= CURRENT_STATE;
		case CURRENT_STATE is
			when IDLE =>
				RD_n <= '1';
				WR_n <= '1';
				iready_n <= '1';
				if ADS_N = '0' then
					NEXT_STATE <= START;
				else
					NEXT_STATE <= IDLE;
				end if;
			
			when START =>
				RD_n <= '1';
				WR_n <= '1';
				iready_n <= '1';
				if BLAST_N = '0' then
					NEXT_STATE <= WAITSTATE;
				else
					NEXT_STATE <= START;
				end if;
			
			when WAITSTATE =>
				RD_n <= LWDRD_N;
				WR_n <= not LWDRD_N;
				
				if iaddr(31 downto 4) = B"0010_0000_0000_0000_1001_0000_0000"   then
					if ACK_n = '0' then
						NEXT_STATE <= LAST;
						iready_n <= '0';
					else
						NEXT_STATE <= WAITSTATE;
						iready_n <= '1';	--new section
					end if;
				else
					NEXT_STATE <= LAST;
					iready_n <= '0';		
				end if;
				
			
			when LAST =>
				RD_n <= '1';
				WR_n <= '1';
				iready_n <= '1';
				if ADS_N = '1' and BLAST_N = '1' then
					NEXT_STATE <= IDLE;
				elsif ADS_N = '0' then
					NEXT_STATE <= START;
				else
					NEXT_STATE <= LAST;
				end if;
			
			when others =>
				RD_n <= '1';
				WR_n <= '1';
				iready_n <= '1';
				NEXT_STATE <= IDLE;
		end case;
	end process;
	
	
end RTL;