1
\$\begingroup\$

I'm studying VHDL and trying to simulate a UART design I took from this great book. I'm using vivado 2018.3. Here's the conceptual block diagram of the RX block:

enter image description here

At this moment I removed from the top the interface circuit (which is a FIFO buffer) and I'm testing only RX + TX connected together. So in my test bench I feed a 8-bit word in the TX and I look for it at the RX-end. What I want to do is to feed anothe 8-bit word only when the RX-end is done: i.e. when rx_done_tick goes to '1'.

So my test bench does:

  wait for 1000 * TbPeriod;

  -- Load 1st 8-bit word and start
  w_data <= "00010001";
  tx_start <= '1';

  wait until rx_done_tick = '1';

  -- Load 1st 8-bit word and start
  w_data <= "00000011";

I expect w_data to change only when rx_done_tick does. In the RX block, the process handling the done flag is:

  -- next-state logic & data path
  process(state_reg, s_reg, n_reg, b_reg, s_tick, sync_rx)
  begin
  state_next   <= state_reg;
  s_next       <= s_reg;
  n_next       <= n_reg;
  b_next       <= b_reg;
  rx_done_tick <= '0';
  case state_reg is

     [... other cases omitted for brevity ...]

     when stop =>
        if (s_tick = '1') then
           if s_reg = (SB_TICK - 1) then
              state_next   <= idle;
              rx_done_tick <= '1';
           else
              s_next <= s_reg + 1;
           end if;
        end if;
  end case;
 end process;

The problem is that w_data actually changes before the done flag is raised, as can be seen here.

enter image description here

Any help would be highly appreciated, just tell me if I should provide more information.

a_bet

Edit, I copy/paste here the full code: Test Bench:

entity uart_no_fifo_tb is
end uart_no_fifo_tb;

architecture tb of uart_no_fifo_tb is

component uart_no_fifo
    generic(
      DBIT    : integer := 8;   -- # data bits
      SB_TICK : integer := 16   -- # ticks for stop bits, 16 per bit
    );
    port (clk          : in std_logic;
          reset        : in std_logic;
          dvsr         : in std_logic_vector (10 downto 0);
          rx           : in std_logic;
          tx_start     : in std_logic;
          w_data       : in std_logic_vector (7 downto 0);
          r_data       : out std_logic_vector (7 downto 0);
          tx           : out std_logic;
          rx_done_tick : out std_logic;
          tx_done_tick : out std_logic);
end component;

constant DBIT : integer := 8;
constant SB_TICK : integer := 16;

signal clk          : std_logic;
signal reset        : std_logic;
signal dvsr         : std_logic_vector (10 downto 0);
signal rx           : std_logic;
signal tx_start     : std_logic := '0';
signal w_data       : std_logic_vector (7 downto 0);
signal r_data       : std_logic_vector (7 downto 0);
signal tx           : std_logic;
signal rx_done_tick : std_logic;
signal tx_done_tick : std_logic;

constant TbPeriod : time := 20 ns; -- EDIT Put right period here
signal TbClock : std_logic := '0';
signal TbSimEnded : std_logic := '0';

begin

dut : uart_no_fifo
port map (clk          => clk,
          reset        => reset,
          dvsr         => dvsr,
          rx           => rx,
          tx_start     => tx_start,
          w_data       => w_data,
          r_data       => r_data,
          tx           => tx,
          rx_done_tick => rx_done_tick,
          tx_done_tick => tx_done_tick);

-- Clock generation
TbClock <= not TbClock after TbPeriod/2 when TbSimEnded /= '1' else '0';

-- EDIT: Check that clk is really your main clock signal
clk <= TbClock;

rx <= tx;

stimuli : process
begin
  -- Reset generation
  -- EDIT: Check that reset is really your reset signal
  reset <= '1';
  tx_start <= '0';
  wait for 100 ns;
  reset <= '0';
  wait for 100 ns;


  dvsr <= "00000000010";

  -- EDIT Add stimuli here
  wait for 1000 * TbPeriod;
  w_data <= "00010001";
  tx_start <= '1';

  wait until rx_done_tick = '1';


  w_data <= "00000011";

  wait for 1000 * TbPeriod;
  w_data <= "00000011";

  wait for 1000 * TbPeriod;
  w_data <= "00000100";

  -- Stop the clock and hence terminate the simulation
  TbSimEnded <= '1';
  wait;
end process;

end tb;

RX-end:

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;
entity uart_rx is
generic(
   DBIT    : integer := 8;    -- # data bits
   SB_TICK : integer := 16    -- # ticks for stop bits
);
port(
   clk, reset   : in  std_logic;
   rx           : in  std_logic;
   s_tick       : in  std_logic;
   rx_done_tick : out std_logic;
   dout         : out std_logic_vector(7 downto 0)
);
end uart_rx;

architecture arch of uart_rx is
type state_type is (idle, start, data, stop);
signal state_reg     : state_type;
signal state_next    : state_type;
signal s_reg, s_next : unsigned(4 downto 0);
signal n_reg, n_next : unsigned(2 downto 0);
signal b_reg, b_next : std_logic_vector(7 downto 0);
signal sync1_reg     : std_logic;
signal sync2_reg     : std_logic;
signal sync_rx       : std_logic;

begin
-- synchronization for rx
process(clk, reset)
begin
   if reset = '1' then
      sync1_reg <= '0';
      sync2_reg <= '0';
   elsif (clk'event and clk = '1') then
      sync1_reg <= rx;
      sync2_reg <= sync1_reg;
   end if;
end process;
sync_rx <= sync2_reg;
-- FSMD state & data registers
process(clk, reset)
begin
   if reset = '1' then
      state_reg <= idle;
      s_reg     <= (others => '0');
      n_reg     <= (others => '0');
      b_reg     <= (others => '0');
   elsif (clk'event and clk = '1') then
      state_reg <= state_next;
      s_reg     <= s_next;
      n_reg     <= n_next;
      b_reg     <= b_next;
   end if;
end process;
-- next-state logic & data path
process(state_reg, s_reg, n_reg, b_reg, s_tick, sync_rx)
begin
   state_next   <= state_reg;
   s_next       <= s_reg;
   n_next       <= n_reg;
   b_next       <= b_reg;
   rx_done_tick <= '0';
   case state_reg is
      when idle =>
         if sync_rx = '0' then
            state_next <= start;
            s_next     <= (others => '0');
         end if;
      when start =>
         if (s_tick = '1') then
            if s_reg = 7 then
               state_next <= data;
               s_next     <= (others => '0');
               n_next     <= (others => '0');
            else
               s_next <= s_reg + 1;
            end if;
         end if;
      when data =>
         if (s_tick = '1') then
            if s_reg = 15 then
               s_next <= (others => '0');
               b_next <= sync_rx & b_reg(7 downto 1);
               if n_reg = (DBIT - 1) then
                  state_next <= stop;
               else
                  n_next <= n_reg + 1;
               end if;
            else
               s_next <= s_reg + 1;
            end if;
         end if;
      when stop =>
         if (s_tick = '1') then
            if s_reg = (SB_TICK - 1) then
               state_next   <= idle;
               rx_done_tick <= '1';
            else
               s_next <= s_reg + 1;
            end if;
         end if;
   end case;
   end process;
   dout <= b_reg;
   end arch;
\$\endgroup\$
  • \$\begingroup\$ I'm gonna bet on bad sensitivity lists and the done tick going high and low in subsequent delta cycles, but you'll have to show more code, both of your tb and your dut. \$\endgroup\$ – DonFusili Apr 4 '19 at 9:33
  • \$\begingroup\$ Hi, thanks for the reply. I wish I could upload files, the code formatting was painful. Also, I looked for delta cycles view and seems that vivado simulator does not support them. \$\endgroup\$ – a_bet Apr 4 '19 at 9:48
1
\$\begingroup\$

It's possible to recreate your problem with VHDL code distributed on the companion web site for the book. Note the entire chapter on the UART is available as a PDF sample.

It requires modifying the mod_m_counter to include a dvsr input: Using VHDL from the book:

-- Listing 4.11
library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity mod_m_counter is
    generic (
        N: integer := 4         -- number of bits
        -- M: integer := 10     -- mod M      -- DELETED
    );
    port (
        clk, reset:   in  std_logic;
        dvsr:         in  std_logic_vector (10 downto 0); -- ADDED
        max_tick:     out std_logic;
        q:            out std_logic_vector(N - 1 downto 0)
    );
end entity mod_m_counter;

architecture arch of mod_m_counter is
    signal r_reg:   unsigned(N - 1 downto 0) := (others => '0'); -- ADD default
    signal r_next:  unsigned(N - 1 downto 0) := (others => '0'); -- ADD default
    signal M:       positive := 163;  -- ADDED defaults for metavalue WARNINGs
begin
    M <= to_integer(unsigned(dvsr)); -- ADDED
    -- register
    process (clk, reset)
    begin
        if reset = '1' then
            r_reg <= (others => '0');
        elsif clk'event and clk = '1' then
            r_reg <= r_next;
        end if;
    end process;
    -- next-state logic
    r_next <= (others => '0') when r_reg >= M - 1 else -- >= no reset required
              r_reg + 1;
    -- output logic
    q <= std_logic_vector(r_reg);
    max_tick <= '1' when r_reg = M - 1 else '0';
end architecture arch;

As well as creating a uart_no_fifo entity and architecture:

library ieee;
use ieee.std_logic_1164.all;

entity uart_no_fifo is
    generic (
        DBIT:     integer := 8;   -- number of  data bits
        SB_TICK:  integer := 16   -- number of clocks for stop bits, 16 per bit
    );
    port (
        clk:           in  std_logic;
        reset:         in  std_logic;
        dvsr:          in  std_logic_vector (10 downto 0);
        rx:            in  std_logic;
        tx_start:      in  std_logic;
        w_data:        in  std_logic_vector (7 downto 0);
        r_data:        out std_logic_vector (7 downto 0);
        tx:            out std_logic;
        rx_done_tick:  out std_logic;
        tx_done_tick:  out std_logic
    );
end entity uart_no_fifo;

architecture foo of uart_no_fifo is
    component mod_m_counter is  -- Chapter 4.11  (MODIFIED)
        generic (
            N: integer := 4      -- number of bits
            -- M: integer := 10     -- mod-M  -- DELETED
        );
        port (
            clk, reset:     in  std_logic;
            dvsr:           in  std_logic_vector (10 downto 0);  -- ADDED
            max_tick:       out std_logic;
            q:              out std_logic_vector(N - 1 downto 0)
        );
    end component mod_m_counter;
    component uart_tx is       -- Chapter 7.1
       generic (
          DBIT:     integer := 8;     -- # data bits
          SB_TICK:  integer := 16      -- # ticks for stop bits
       );
       port (
          clk, reset:   in  std_logic;
          tx_start:     in  std_logic;
          s_tick:       in  std_logic;
          din:          in  std_logic_vector(7 downto 0);
          tx_done_tick: out std_logic;
          tx:           out std_logic
       );
    end component uart_tx;
    component uart_rx is      -- Chapter 7.3
        generic (
            DBIT:       integer := 8;  -- # data bits
            SB_TICK:    integer := 16  -- # ticks for stop bits
        );
        port (
          clk, reset:   in  std_logic;
          rx:           in  std_logic;
          s_tick:       in  std_logic;
          rx_done_tick: out std_logic;
          dout:         out std_logic_vector(7 downto 0)
       );
    end component uart_rx;
    signal tick:    std_logic;
begin
BAUD_RATE_GENERATOR:
    mod_m_counter
        generic map (
            N => 16
        )
        port map (
            clk => clk,
            reset => reset,
            dvsr => dvsr,
            max_tick => tick,
            q => open
        );
UART_TXMT:
    uart_tx
        generic map (
            DBIT => DBIT,
            SB_TICK => SB_TICK
        )
        port map (
            clk => clk,
            reset => reset,
            tx_start => tx_start,
            s_tick => tick,
            din => w_data,
            tx_done_tick => tx_done_tick,
            tx => tx
        );
UART_RCV:
    uart_rx
        generic map (
            DBIT => DBIT,
            SB_TICK => SB_TICK
        )
        port map (
            clk => clk,
            reset => reset,
            rx => rx,
            s_tick => tick,
            rx_done_tick => rx_done_tick,
            dout => r_data
        );
end architecture foo;

Which replicates the glitch when used with your supplied testbench:

w_data responds to glitch

This is a static hazard where the output rx_done_tick goes from a '0' to a '1' and back to a '0', caused by evaluating s_tick in the uart_rx state machine:

                when stop =>
                    if s_tick = '1' then
                        if s_reg = SB_TICK - 1 then
                            state_next   <= idle;
                            rx_done_tick <= '1';
                        else
                            s_next <= s_reg + 1;
                        end if;
                    end if;
        end case;

It's necessary because the idle state doesn't take into effect s_tick. Note a common s_tick from the baud rate generator is used for both uart_tx and uart_rx. In duplex communications this wouldn't be the case, the two ends of the serial link would be running off different clocks and have different x16 baud sample ticks.

Evaluating s_tick could be added to to state idle, but wouldn't help. The output rx_done_tick isn't clock synchronous, the state machine is Mealy (when it doesn't have to be, it has separate processes for clocked state transitions and outputs) where in a Moore machine s_tick would be an enable for the clock and rx_done_tick would have to be predicatively set to a '1'.

One of the things you notice in Figure 7.5 is that the FIFO's you have eliminated are clock synchronous where outputs from the UART RX and TX are evaluated on a clock edge. Evaluating rx_done_tick on the rising edge of the clock overcomes the static hazard. That's not the case in your testbench.

Modifying your testbench to evaluate rx_done_tick synchronously:

  -- wait until rx_done_tick = '1';  -- CHANGE, now clock synchronous
  wait until rising_edge(clk) and rx_done_tick = '1';

yields useful results:

w_data delayed until after rx_done_tick evaluated synchronously

You'd also find waiting for expected clock synchronous events you could get rid of at least one other wait statement.

You can also eliminate the static hazard.

The idea here is to reduce impulse noise in digital logic.

We can modify the OP's version of uart_rx:

architecture a_bit of uart_rx is
    type state_type is (idle, start, data, stop);
    signal state_reg:       state_type;
    signal state_next:      state_type;
    signal s_reg, s_next:   unsigned(4 downto 0);
    signal n_reg, n_next:   unsigned(2 downto 0);
    signal b_reg, b_next:   std_logic_vector(7 downto 0);
    signal sync1_reg:       std_logic;
    signal sync2_reg:       std_logic;
    signal sync_rx:         std_logic;
    signal rx_done:         std_logic;
    signal rx_done_next:    std_logic;
begin
-- synchronization for rx
    process (clk, reset)
    begin
        if reset = '1' then
            sync1_reg <= '0';
            sync2_reg <= '0';
        elsif (clk'event and clk = '1') then
            sync1_reg <= rx;
            sync2_reg <= sync1_reg;
        end if;
    end process;
    sync_rx <= sync2_reg;
-- FSMD state & data registers
    process (clk, reset)
    begin
        if reset = '1' then
            state_reg <= idle;
            s_reg     <= (others => '0');
            n_reg     <= (others => '0');
            b_reg     <= (others => '0');
            rx_done   <= '0';
        elsif clk'event and clk = '1' then
            if s_tick = '0' then  -- ADDED enable
                state_reg <= state_next;
                s_reg     <= s_next;
                n_reg     <= n_next;
                b_reg     <= b_next;
                rx_done   <= rx_done_next;
            end if;
        end if;
end process;
-- next-state logic & data path
process (state_reg, s_reg, n_reg, b_reg, s_tick, sync_rx)
begin
   state_next   <= state_reg;
   s_next       <= s_reg;
   n_next       <= n_reg;
   b_next       <= b_reg;
   rx_done_next <= '0';
       case state_reg is
            when idle =>
                if sync_rx = '0' then
                    state_next <= start;
                    s_next     <= (others => '0');
                end if;
            when start =>
                -- if s_tick = '1' then
                    if s_reg = 7 then
                        state_next <= data;
                        s_next     <= (others => '0');
                        n_next     <= (others => '0');
                    else
                        s_next <= s_reg + 1;
                    end if;
                -- end if;
            when data =>
                -- if s_tick = '1' then 
                    if s_reg = 15 then
                        s_next <= (others => '0');
                        b_next <= sync_rx & b_reg(7 downto 1);
                        if n_reg = (DBIT - 1) then
                            state_next <= stop;
                        else
                            n_next <= n_reg + 1;
                        end if;
                        else
                            s_next <= s_reg + 1;
                        end if;
                    -- end if;
                when stop =>
                    -- if s_tick = '1' then
                        if s_reg = SB_TICK - 1 then
                            state_next   <= idle;
                            -- rx_done_next <= '1';
                        else
                            s_next <= s_reg + 1;
                        end if;
                        if s_reg = SB_TICK - 2 then  -- PREDICTIVELY
                            rx_done_next <= '1';
                        end if;
                    -- end if;
        end case;
    end process;
    rx_done_tick <= rx_done and s_tick; -- no static hazard
    dout <= b_reg;
end architecture a_bit;

where rx_done_tick is combinatorially derived from a register output rx_done and s_tick. This eliminates the immediate static hazard. With the original test bench evaluation of rx_done_tick:

enter image description here

It should be noted that a static hazard is still possible due to skew in routing of rx_done and s_tick (which isn't the clock) post synthesis. The testbench should still evaluate rx_done_tick on a clock edge.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Massive. Thank you very much for pointing out the fact that removing the fifo's was the cause for this. I would like to you use this slimmed down uart to test some other circuits and I will have to keep this in mind when sampling the rx_done_tick \$\endgroup\$ – a_bet Apr 8 '19 at 10:48
1
\$\begingroup\$

Ok, I got it. The problem was vivado simulator (XSIM) not displaying delta cycles. Here's a screenshot from ModelSim: enter image description here

In red the unintended variation. This is due to 2 signals in the sensitivity list triggering at the same moment in theory, but not in practice. In this case Xsim triggers the process in the RX when s_reg = (SB_TICK - 1).

        when stop =>
        if (s_tick = '1') then
           if s_reg = (SB_TICK - 1) then
              state_next   <= idle;
              rx_done_tick <= '1';
           else
              s_next <= s_reg + 1;
           end if;

At this time s_tick = '1' still holds and infact rx_done_tick <= '1';. Immediately after the neg edge of the s_tick signal triggers the process again. Now s_tick = '0' and the default value applies for rx_done_tick: that is, rx_done_tick <= '0';. So basically inspecting rx_done_tick we see a spike, which is enough to trigger the wait until rx_done_tick = '1'; statement in the test bench.

| improve this answer | |
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.