Finite state machine - doesnt change state second time & glitch

[Altera_Forum]

I have a fairly simple state machine that when in idle state, if txval is high should take the data from input port, put txbus flag to 1, then write it to FT245 chip and when writing is finished and back in idle put txbus back to 0. Until txval is high. If txval was high when entering, it should take the data from input port and so on; or wait until input port data is high.  

I am using this state machine from another one, and they share the same clocks. FT245 chip stuff is registered since it is async.   I have noticed 2 things:  In case txval goes high while still in write_end state, txbus drops to 0 and goes high again. I set txbus to 0 only and only in idle state. In all others is it 1.  In case one cycle of state changes is done, it does not go into another cycle of state changes if all conditions for it are satisfied, however it does take the data from input port.   In hardware - in serial terminal I can see this 1 byte being succesfully sent, but nothing else.   I am trying to debug this for quite some time now...but....  https://www.alteraforum.com/forum/attachment.php?attachmentid=8289 https://www.alteraforum.com/forum/attachment.php?attachmentid=8290   Code is:   LIBRARY ieee; USE ieee.std_logic_1164.all; ENTITY FT245 IS PORT( clk : IN std_logic; rstn : IN std_logic; txrdy : OUT std_logic; txval : IN std_logic; txdat : IN std_logic_vector(7 downto 0); txbus : OUT std_logic; -- FT2232 Bus Signals usb_data : inout std_logic_vector(7 downto 0); usb_rd_n : out std_logic; usb_wr : out std_logic; usb_rxf_n : in std_logic; usb_txe_n : in std_logic; d_state : out std_logic_vector(2 downto 0) ); END FT245; ARCHITECTURE behv OF FT245 IS constant CMDDELAY : integer := 10; -- 10*20ns for 50Mhz clock = 200ns signal usb_input : STD_LOGIC_VECTOR (7 DOWNTO 0); signal usb_output_next: STD_LOGIC_VECTOR (7 DOWNTO 0); signal usb_output_reg : STD_LOGIC_VECTOR (7 DOWNTO 0); signal recv_data_reg : STD_LOGIC_VECTOR(7 DOWNTO 0); signal recv_data_next : STD_LOGIC_VECTOR(7 DOWNTO 0); signal rd_n_reg : STD_LOGIC; -- Registered RD_N (to be written) signal rd_n_next : STD_LOGIC; -- Next RD_N (to be registered) signal wr_reg : STD_LOGIC; -- Registered WR (to be written) signal wr_next : STD_LOGIC; -- Next WR (to be registered) signal rxf_reg : STD_LOGIC; -- Status of RX_F flag signal txe_n_reg : STD_LOGIC; -- Status of TXE_N flag signal oe_reg : STD_LOGIC; -- For Bi-Di bus (to be written) signal oe_next : STD_LOGIC; -- For Bi-Di bus (next output enable state - to be registered) signal delay_reg : integer range 0 to CMDDELAY; -- Delay counter (for start next state) signal delay_next: integer range 0 to CMDDELAY; -- Delay counter (for start next state) -- Build an enumerated type for the state machine type state_type is (init, idle, write_prepare, write_strobe, write_end); -- Register to hold the current state signal state_now : state_type; signal state_next : state_type; begin --{------------------------------------------------------------------------------} --{ Params : <CLK> Clock --{ <RST> Reset --{ Descript: State machine: State register --{ The state register is updated --{ Using synchronuous reset --{------------------------------------------------------------------------------} process (clk, rstn) begin if (rising_edge(clk)) then if (rstn = '0') then delay_reg <= 0; else if (delay_reg = 0) then state_now <= state_next; delay_reg <= delay_next; else delay_reg <= delay_reg - 1; end if; end if; IF(state_now = init) THEN d_state <= "000"; ELSIF(state_now = idle) THEN d_state <= "001"; ELSIF(state_now = write_prepare) THEN d_state <= "010"; ELSIF(state_now = write_strobe) THEN d_state <= "011"; ELSIF(state_now = write_end) THEN d_state <= "100"; ELSE d_state <= "111"; END IF; end if; end process; --{------------------------------------------------------------------------------} --{ Params : <CLK> Clock --{ Descript: State machine: Output buffer --{ The output registers are updated --{------------------------------------------------------------------------------} process (clk) begin if (rising_edge(clk)) then rd_n_reg <= rd_n_next; -- Register RD_N output wr_reg <= wr_next; -- Register WR output rxf_reg <= usb_rxf_n; -- Register RXF input txe_n_reg <= usb_txe_n; -- Register TXE_N input oe_reg <= oe_next; -- Register Bi-Di OE flag recv_data_reg <= recv_data_next; usb_output_reg <= usb_output_next; end if; end process; txrdy <= not txe_n_reg; -- assert txrdy when FIFO has space. --{------------------------------------------------------------------------------} --{ Params : <START> Start flag --{ <RST> Reset --{ <state_now> Current state --{ Descript: State machine: Next state logic --{ Notes : <delay_next> is the delay after the -next- state has been executed --{ and must be reset --{------------------------------------------------------------------------------} process (clk, rstn, state_now, txval, txdat, rd_n_reg, wr_reg, oe_reg, delay_reg, usb_output_reg, recv_data_reg, txe_n_reg) begin if (rstn = '0') then rd_n_next <= '1'; wr_next <= '0'; oe_next <= '1'; -- default in output bus usb_output_next <= (others=>'0'); recv_data_next <= (others=>'0'); delay_next <= 0; state_next <= init; txbus <= '1'; else rd_n_next <= rd_n_reg; wr_next <= wr_reg; oe_next <= oe_reg; delay_next <= delay_reg; state_next <= state_now; usb_output_next <= usb_output_reg; recv_data_next <= recv_data_reg; txbus <= '1'; case state_now is when init => rd_n_next <= '1'; wr_next <= '0'; oe_next <= '1'; -- default in output bus usb_output_next <= (others=>'0'); delay_next <= 0; state_next <= idle; txbus <= '0'; when idle => delay_next <= 0; IF(txval = '0') THEN txbus <= '0'; ELSIF(txval = '1') THEN state_next <= write_prepare; usb_output_next <= txdat; txbus <= '1'; END IF; when write_prepare => txbus <= '1'; if(txe_n_reg = '0') then -- wait until TX fifo has space oe_next <= '1'; delay_next <= 0; state_next <= write_strobe; end if; when write_strobe => txbus <= '1'; wr_next <= '1'; delay_next <= CMDDELAY; state_next <= write_end; when write_end => txbus <= '1'; wr_next <= '0'; delay_next <= CMDDELAY; state_next <= idle; when others => rd_n_next <= rd_n_reg; wr_next <= wr_reg; oe_next <= oe_reg; state_next <= init; delay_next <= 0; txbus <= '0'; end case; end if; end process; --{------------------------------------------------------------------------------} --{ Descript: Output --{ The actual outputs are set --{------------------------------------------------------------------------------} usb_rd_n <= rd_n_reg; usb_wr <= wr_reg; usb_data <= usb_output_reg when oe_reg = '1' else (others => 'Z'); usb_input <= usb_data when oe_reg = '0'; END behv;

[Altera_Forum]

Im guessing its because the delay_next is set to 10 during the write_end state, and next state can only be assigned when the counter reaches 0. Hence you need to hold txval high until you're allowed to change state again.

[Altera_Forum]

DO you have a testbench for this?

[Altera_Forum]

This is a timed state machine, since my external hardware has timing constraints, so delay_next is set to 10 to ensure that state machine is in that state for 10 clock cycles and then it is allowed to change state to the desired state (in this case idle state).  

 In the waveform I have attached, it is visible that state machine is in writing_end for 10 clock cycles and then it goes into the idle state. Only when it is again in the idle state, txval goes high but FSM does not go into write_prepare state as it should and as it did in the first case where whole cycle was correctly performed.   I used waveform to test my code since this FSM has only 5 states (one is init) and it has only 1 external stimuli (txval).   Basically, it should be in idle state until for the rising edge of the clock txval is 1. If it is one, it should put txbus to 1 in all states until reaching back into idle. If it reaches idle while txval is still high, it would keep txbus at 1 and go into next writing cycle. This would allow user of this FSM to send only 1 byte or more efficiently a stream of bytes while keeping txval high all the time.   Anyhow, in my waveform testing I am testing for the simple case where FSM user would put data, put txval to high, wait for txbus to go high, put txval to low, wait for txbus to go to low (=back in idle) and then it would put another byte, assert txval again and so on...

[Altera_Forum]

By email i have received a reply saying that fsm does not follow standard template and is not clocked - written by kaz. But it is not shown on the thread. 

 My code implements a two process FSM template having a clocked sequential part and combinatorial part

[Altera_Forum]

actually I posted then deleted hastily as I noticed it is clocked. 

However, your design is not following a good state machine template in a clean way as you are mixing it with other logic, though I don't think this explains things.  I notice your first observation of tx_bus in write_end state going low then high is actually a glitch of half clock period in first diagram and longer in second diagram. It occurs as state changes from write_end to idle judging by d_state value moving from 100 to 001 (after 10 clocks) and this is expected?? Frankly your design is too complicated to follow on the forum and your description needs further corrections for example tx_bus does go low in init state, idle state and others. The best approach is to simulate it fully before you move to any hardware and repair it accordingly.

[Altera_Forum]

--- Quote Start ---  This is a timed state machine, since my external hardware has timing constraints, so delay_next is set to 10 to ensure that state machine is in that state for 10 clock cycles and then it is allowed to change state to the desired state (in this case idle state).    --- Quote End ---    Yes, but as you havent included the delay counter on the diagram, you cannot see whether the counter is at 0 yet. Given it has just changed state 5 clocks previous, and needs 10 clocked before changing again, I suspect the counter isnt at 0 yet, hence cannot change state.

[Altera_Forum]

This is in addition to the poor design Kaz pointed out.

[Altera_Forum]

--- Quote Start ---  Yes, but as you havent included the delay counter on the diagram, you cannot see whether the counter is at 0 yet. Given it has just changed state 5 clocks previous, and needs 10 clocked before changing again, I suspect the counter isnt at 0 yet, hence cannot change state.  --- Quote End ---    I have added counter output to my waveform and the issue is there. I will post correct code when debugged.

[Altera_Forum]

--- Quote Start ---  This is in addition to the poor design Kaz pointed out.  --- Quote End ---    It follows template of 2 process state machine. Only for debugging purposes in my timing diagram, in the clocked process I have added code to show current state. In reality it is not needed there.

[Altera_Forum]

But it has glitches on the txbus output, as Kaz mentioned. All signals should be synchronous - but txbus is asynchronous. 

 2 process state machines have fallen out of favour in recent times, for this reason (async outputs). The single process state machine is now favoured.

[Altera_Forum]

Attached is a new code which works :) 

 Thanks for your help & feedback.   I have also cleaned up the code since I implement now single process delayed state machine; so I do not need to register outputs separately as before but I update them in the FSM.  Inputs from the chip are registered though in a separate process.   I use this on Wayengineer Cyclone IVE FPGA board and I have tested it with another code that uses buttons to update 7-segment display and also sends current data there to the PC over USB. If someone wants, I can provide that code as well.   Next step would be reading from USB and notifying...   LIBRARY ieee; USE ieee.std_logic_1164.all; USE ieee.numeric_std.all; ENTITY FT245 IS PORT( clk : IN std_logic; rstn : IN std_logic; txrdy : OUT std_logic; txval : IN std_logic; txdat : IN std_logic_vector(7 downto 0); txbus : OUT std_logic; -- debug -- d_delay_reg : out std_logic_vector(3 DOWNTO 0); -- debug only -- d_state : out std_logic_vector(2 downto 0); -- debug only -- FT2232 Bus Signals usb_data : inout std_logic_vector(7 downto 0); usb_rd_n : out std_logic; usb_wr : out std_logic; usb_rxf_n : in std_logic; usb_txe_n : in std_logic ); END FT245; ARCHITECTURE behv OF FT245 IS constant CMDDELAY : integer := 10; -- 10*20ns for 50Mhz clock = 200ns signal usb_input : STD_LOGIC_VECTOR (7 DOWNTO 0); signal usb_output : STD_LOGIC_VECTOR (7 DOWNTO 0); signal rd_n : STD_LOGIC; -- RD_N (to be written) signal wr : STD_LOGIC; -- WR (to be written) signal rxf_reg : STD_LOGIC; -- Status of RX_F flag signal txe_n_reg : STD_LOGIC; -- Status of TXE_N flag signal oe : STD_LOGIC; -- For Bi-Di bus (to be written) begin --{------------------------------------------------------------------------------} --{ Params : <CLK> Clock --{ Descript: State machine: Output buffer --{ The output registers are updated --{------------------------------------------------------------------------------} process (clk) begin if (rising_edge(clk)) then rxf_reg <= usb_rxf_n; -- Register RXF input txe_n_reg <= usb_txe_n; -- Register TXE_N input end if; end process; txrdy <= not txe_n_reg; -- assert txrdy when FIFO has space. PROCESS(clk, rstn, txval, txdat, rd_n, wr, oe, usb_output, txe_n_reg, rxf_reg) type state_type is (init, idle, write_prepare, write_strobe, write_end); -- Register to hold the current state VARIABLE state_now : state_type; VARIABLE state_next : state_type; -- Counters to implement state change delays VARIABLE currCnt : UNSIGNED(3 DOWNTO 0); VARIABLE cnt : UNSIGNED(3 DOWNTO 0); CONSTANT DELAY : UNSIGNED(3 DOWNTO 0) := "1010"; BEGIN IF(rstn = '0') THEN cnt := (others => '0'); state_now := init; state_next := init; rd_n <= '1'; wr <= '0'; oe <= '1'; -- default in output bus usb_output <= (others=>'0'); txbus <= '1'; ELSIF(rising_edge(clk)) THEN CASE state_now IS when init => rd_n <= '1'; wr <= '0'; oe <= '1'; -- default in output bus usb_output <= (others=>'0'); txbus <= '1'; cnt := (others => '0'); state_next := idle; when idle => IF(txval = '0') THEN txbus <= '0'; ELSIF(txval = '1') THEN state_next := write_prepare; usb_output <= txdat; txbus <= '1'; END IF; when write_prepare => if(txe_n_reg = '0') then -- wait until TX fifo has space oe <= '1'; state_next := write_strobe; end if; when write_strobe => wr <= '1'; cnt := DELAY; state_next := write_end; when write_end => wr <= '0'; cnt := (others => '0'); state_next := idle; when others => cnt := (others => '0'); state_next := init; END CASE; -- handle delay between change states currCnt := currCnt + 1; IF(currCnt >= cnt) THEN state_now := state_next; cnt := (others => '0'); currCnt := (others => '0'); END IF; -- debug only -- IF(state_now = init) THEN -- d_state <= "000"; -- ELSIF(state_now = idle) THEN -- d_state <= "001"; -- ELSIF(state_now = write_prepare) THEN -- d_state <= "010"; -- ELSIF(state_now = write_strobe) THEN -- d_state <= "011"; -- ELSIF(state_now = write_end) THEN -- d_state <= "100"; -- ELSE -- d_state <= "111"; -- END IF; -- d_delay_reg <= std_logic_vector(cnt); -- debug only END IF; END PROCESS; --{------------------------------------------------------------------------------} --{ Descript: Output --{ The actual outputs are set --{------------------------------------------------------------------------------} usb_rd_n <= rd_n; usb_wr <= wr; usb_data <= usb_output when oe = '1' else (others => 'Z'); usb_input <= usb_data when oe = '0'; END behv;

[Altera_Forum]

--- Quote Start ---  But it has glitches on the txbus output, as Kaz mentioned. All signals should be synchronous - but txbus is asynchronous.  2 process state machines have fallen out of favour in recent times, for this reason (async outputs). The single process state machine is now favoured.  --- Quote End ---    I have implemented it as a single process FSM now, without reading your post. Code is simpler, shorter and easier to understand. Plus yes - output like txbus changes only on clk edges.

[Altera_Forum]

TO_BE_DONE