0
\$\begingroup\$

I've recently started learning VHDL and have attempted to make a simple UART transmitter. Here is the code I've come up with so far.

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

entity uart_tx is
     generic (clk_div: integer := 3332);
    port (
           clk      : in  std_logic;  -- Clock signal
             rst      : in  std_logic;  -- Reset signal
             tx_en    : in  std_logic;  -- if '1', data is latched and ready
             tx_data  : in  std_logic_vector(7 downto 0);
             tx_ready : out std_logic;  -- if '1', transmitter is ready to take in next byte
             tx       : out std_logic   -- Data out
         );
end uart_tx;

architecture Behavioral of uart_tx is
    signal data_sr : std_logic_vector(7 downto 0) := (others => '0');

    -- FSM variables
    type uart_state is (idle, start, data, stop);
    signal prev_state, next_state : uart_state;
begin
    -- Sequential logic for FSM
    fsm_seq : process (clk, rst)
        variable count : integer range 0 to clk_div := 0;
    begin
        if (rst = '1') then
            count := 0;
            prev_state <= idle;
        elsif rising_edge(clk) then
            count := count + 1;
            if (count >= clk_div) then
                prev_state <= next_state;
                count := 0;
            end if;
        end if;
    end process;

    -- Combinational logic for FSM
    fsm_comb : process (prev_state, tx_data, tx_en, rst)
        variable bit_cnt : integer range 0 to 7 := 0;
    begin
        if (rst = '1') then
            data_sr(7 downto 0) <= (others => '0');
            tx <= '1';
            tx_ready <= '1';
            bit_cnt := 0;
            next_state <= idle;
        else
            case prev_state is
                when idle =>
                    data_sr(7 downto 0) <= (others => '0');
                    tx <= '1';
                    tx_ready <= '1';
                    bit_cnt := 0;

                    if tx_en = '1' then
                        next_state <= start;
                    else
                        next_state <= idle;
                    end if;
                when start =>
                    data_sr(7 downto 0) <= tx_data(7 downto 0);  -- Load data into shift register
                    tx <= '0';                  -- Start bit
                    tx_ready <= '0';            -- Busy
                    bit_cnt := 0;

                    next_state <= data; 
                when data =>
                    -- Shift register, LSB out first
                    data_sr(7 downto 0) <= '0' & data_sr(7 downto 1);
                    tx <= data_sr(0);
                    tx_ready <= '0';
                    bit_cnt := bit_cnt + 1;

                    if (bit_cnt >= 7) then
                        next_state <= stop;
                    else
                        next_state <= data;
                    end if;
                when stop =>
                    data_sr(7 downto 0) <= (others => '0');
                    tx <= '1';              -- Stop bit
                    tx_ready <= '1';            -- Ready to receive next byte
                    bit_cnt := 0;

                    next_state <= idle;
            end case;
        end if;
    end process;
end Behavioral;

I'm having trouble diagnosing the source of these warnings:

WARNING:Xst:2170 - Unit uart_tx : the following signal(s) form a combinatorial loop: prev_state_FSM_FFd2-In, Madd_prev_state_add0000_lut<2>, prev_state_cmp_ge0001, prev_state_add0000<2>.
WARNING:Xst:2170 - Unit uart_tx : the following signal(s) form a combinatorial loop: Madd_prev_state_add0000_cy<0>.
WARNING:Xst:2170 - Unit uart_tx : the following signal(s) form a combinatorial loop: Madd_prev_state_add0000_lut<1>.

What would be causing this? I've tried to look at the schematics but there is so much going on I can't really figure out where to start.

Also, I would greatly appreciate any other pointers. If I'm doing something ridiculous or the program could be dramatically simplified, please let me know so I can learn from my mistakes.

\$\endgroup\$
  • 1
    \$\begingroup\$ You shouldn't need to use reset in the combinatorial section, it should hold no state (else its not combinatorial), and its outputs should be unused, or driven from the reset 'state'. (prev and next to me have a gap between them - I'd use state <=next_state, or prev_state <=state, but that's just personal) \$\endgroup\$ – Sean Houlihane Feb 1 '17 at 8:36
  • \$\begingroup\$ Just one of the problems avoided by the single process form of state machine. If you REALLY MUST use the 2-process form take great care to avoid silly things like incrementing a variable in the comb process. \$\endgroup\$ – Brian Drummond Feb 1 '17 at 10:09
  • \$\begingroup\$ @BrianDrummond Interesting, we only learned the two process form when I learned VHDL in school. Is the single process form what most professionals use? \$\endgroup\$ – dgreenheck Feb 1 '17 at 17:58
  • \$\begingroup\$ In my experience yes. It's typically about 30% shorter, as well as being simpler, less prone to coding errors, and it registers every output giving cleaner timings. (There are ways to bypass that on the rare occasion you need to). And yet there are still academics writing books advising against it. \$\endgroup\$ – Brian Drummond Feb 1 '17 at 18:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.