I'm trying to get a simple UART receiver working and am close but I'm receiving slightly off values (some bits are shifted a couple places from where they should be).

Based on this answer I tried running RX through a flip flop (not sure I did that correctly) and oversampled RX. My target baud rate is 2500000, so that's 400ns per baud and 20 samples per baud (FPGA clock is 50 MHz).

Can anyone see what I'm doing wrong? Am I putting RX in the flip flop and oversampling correctly?

entity uart is port(
clk_50 : in std_logic;
reset : in std_logic;
tx : out std_logic;
rx : in std_logic;
data_in : out std_logic_vector(7 downto 0); -- data being received
data_out : in std_logic_vector(7 downto 0); -- data being sent
leds    : out std_logic_vector(7 downto 0));
end uart;

architecture behavioral of uart is

signal sample_counter : unsigned(7 downto 0);
signal data_count : unsigned(3 downto 0);
type uart_state_t is (IDLE, START, DATA, STOP, STALL);
signal uart_state: uart_state_t;
signal next_uart_state: uart_state_t;

signal rx_data : std_logic_vector(7 downto 0);

signal rx_d : std_logic;

begin

process(clk_50, reset)
begin
if (reset = '1') then
uart_state <= START;
data_count <= (others => '0');
sample_counter <= (others => '1');
leds <= (others => '0');
rx_data <= (others => '0');

elsif (rising_edge(clk_50)) then
uart_state <= next_uart_state;
rx_d <= rx;

if (next_uart_state = START) then
data_count <= (others => '1');

if (sample_counter = x"ff") then
if (rx_d = '0') then
sample_counter <= sample_counter + 1;
end if;
elsif (sample_counter = x"31") then -- 20 samples per baud
sample_counter <= (others => '1');
else
sample_counter <= sample_counter + 1;
end if;
end if;
elsif (next_uart_state = DATA) then
if (sample_counter = x"0a") then    -- check rx at sample 10
rx_data(to_integer(data_count)) <= rx_d;
leds(to_integer(data_count)) <= rx_d;
sample_counter <= sample_counter + 1;
elsif (sample_counter = x"13") then
sample_counter <= (others => '1');
data_count <= data_count + 1;
else
sample_counter <= sample_counter + 1;
end if;

elsif (next_uart_state = STOP) then
if (sample_counter = x"0a") then    -- check rx at sample 10
data_in <= rx_data;
sample_counter <= sample_counter + 1;
elsif (sample_counter = x"13") then
sample_counter <= (others => '1');
else
sample_counter <= sample_counter + 1;
end if;
end if;
end if;
end process;

process(uart_state, reset, data_count, sample_counter)
begin
if (reset = '1') then
next_uart_state <= START;
end if;
case uart_state is
when START =>
if (sample_counter = x"13") then
next_uart_state <= DATA;
else
next_uart_state <= START;
end if;
when DATA =>
if (data_count = x"8" and sample_counter = x"13") then
next_uart_state <= STOP;
else
next_uart_state <= DATA;
end if;
when STOP =>
if (sample_counter = x"13") then
next_uart_state <= START;
else
next_uart_state <= STOP;
end if;
when others =>
next_uart_state <= START;
end case;
end process;
end behavioral;


e: I think I got it - I reduced the baud rate to 1000000 and changed the logic for the START state.

• This receiver does not test for the stop bit. Using a shift register would simplify your code. Your counters are declared as UNSIGNED so you could use decimal literals instead of hex literals to increase readability. Why do you reset the sample counter to -1? Your FSM and counters have no initial state/value. – Paebbels Dec 6 '15 at 9:18
• Maybe you could back out the change and make an answer, along with an explanation? The only way reconstruct what's going on is to extract both versions from the history and diff them. – user8352 Dec 6 '15 at 9:19

Your edit doesn't make any sense — you've changed the timing for the start bit, but not for any of the other bits. Furthermore, you only changed it for the clocked process, but not the unclocked process that actually determines the value of next_uart_state.
For example, your sample_count runs from 0xFF (-1) through 0x13 (+19), which is a total of 21 states, not 20 as your comments imply. This creates a 5% error in the sampling rate, which is enough to create a problem after one or two bytes have been received.
In addition, your code waits for the full period of the stop bit before going back to look for the next start bit, which means that it can never "catch up". Instead, you should verify the value of the stop bit halfway through, and then immediately go back to the START state to look for the leading edge of the next start bit. If you do this, then even if the receive clock is a little slow with respect to the transmit clock, it will be able to keep up.
Finally, I've seen lots of ways to code state machines in HDL, but this one really takes the cake. Why is your clocked process casing on next_uart_state instead of uart_state? This creating a lot of strange boundary cases that will confuse anyone reading this code — and I think you have even confused yourself.