12
\$\begingroup\$

I have written a UART module in Verilog. By using that module I get data from PC via UART and then send that data back again via this UART module. I uploaded it to FPGA for testing. It works flawless no matter how many characters I send with the below code (Actually you may not need the TX state but I put the full code for clearance):

module uart_transceiver

 #(parameter [15:0] CLK_PERIOD = 434) //50000000/115200

(
    input sys_rst,
    input sys_clk,

    input uart_rx,
    output reg uart_tx,

    //input [15:0] divisor,

    output reg [7:0] rx_data,
    output reg rx_err,
    output [1:0] state_uart,
    output [7:0] clock_most8,
    output [7:0] clock_least8,

    input [7:0] tx_data,
    input tx_wr,
    output reg rx_busy,
    output reg rx_done,
    output reg tx_busy
);

reg [1:0]   state_tx, state_rx;

//-----------------------------------------------------------------
// UART RX Logic
//-----------------------------------------------------------------
reg [3:0]   rx_bitcount;
reg [15:0]  clk_count_rx = 16'h0001;
reg [7:0]   rx_reg;
reg             rx1;

assign state_uart = state_rx;
//assign clock_least8 = clk_count_rx[7:0];
//assign clock_most8 = clk_count_rx[15:8];


parameter   IDLERX      = 2'd0, 
                TAKESTART   = 2'd1,
                TAKE8BITS   = 2'd2,
                TAKESTOP    = 2'd3;

parameter   [15:0] FIRST_CHECK = CLK_PERIOD / 2;



always @(posedge sys_clk) begin

    if(sys_rst) begin

        state_rx <= IDLERX;
        rx_err <= 1'b0;
        rx_done <= 1'b0;

    end else begin

        case(state_rx)

            IDLERX: begin
                rx_bitcount <= 4'd0;
                rx_busy <= 1'b0;
                if(uart_rx == 1'b0 && rx_err != 1'b1) begin
                    clk_count_rx <= 16'h0001;
                    state_rx <= TAKESTART;
                end
            end

            // Take one start signal and check if it is 0
            TAKESTART: begin
                rx_busy <= 1'b1;
                rx_done <= 1'b0;
                clk_count_rx <= clk_count_rx + 16'h0001;

                if(clk_count_rx == FIRST_CHECK) 
                    begin
                    rx1 <= uart_rx;
//                  clk_count_rx <= clk_count_rx + 16'd1;
                    end 

                if(clk_count_rx == CLK_PERIOD && rx1 != 1'b0) 
                    begin
                    rx_err <= 1'b1;
                    state_rx <= IDLERX;
                    end 

                else if(clk_count_rx == CLK_PERIOD && rx1 == 1'b0) 
                    begin
                    clk_count_rx <= 16'h0001;
                    state_rx <= TAKE8BITS;
                    end 

            end


            // Take 8 bits of data but be careful to wait for 1 CLK_PERIOD for each bit
            TAKE8BITS: begin

                if(rx_bitcount < 4'd8) 
                    begin               
                    clk_count_rx <= clk_count_rx + 16'h0001;

                    if(clk_count_rx == FIRST_CHECK) 
                        begin
                        rx1 <= uart_rx;
                        end 

                    else if(clk_count_rx == CLK_PERIOD) 
                        begin
                        clk_count_rx <= 16'h0001;
                        rx_reg <= {rx1, rx_reg[7:1]};   
                        rx_bitcount <= rx_bitcount + 1'b1;                                                  
                        end

                    end 

                else                
                    begin               
                    clk_count_rx <= 16'h0001;
                    state_rx <= TAKESTOP;
                    end

            end

            // Take 1 stop bit in last CLK_PERIOD
            TAKESTOP: begin

                clk_count_rx <= clk_count_rx + 16'h0001;

                if(clk_count_rx == FIRST_CHECK) 
                    begin
                    rx1 <= uart_rx;
                    end 

                else if(clk_count_rx == CLK_PERIOD && rx1 != 1'b1) 
                    begin
                    rx_err <= 1'b1;
                    state_rx <= IDLERX;
                    end 

                else if(clk_count_rx == CLK_PERIOD && rx1 == 1'b1) 
                    begin   
                    rx_data <= rx_reg;
                    rx_done <= 1'b1;
                    clk_count_rx <= 16'h0001;
                    state_rx <= IDLERX;
                    end 
                else if(clk_count_rx > CLK_PERIOD)
                    begin
                    rx_err <= 1'b1;
                    state_rx <= IDLERX;
                    end
            end

        endcase
    end

end



//-----------------------------------------------------------------
// UART TX Logic
//-----------------------------------------------------------------
parameter   IDLE            = 2'd0, 
                SENDSTART   = 2'd1,
                SEND8BITS   = 2'd2,
                SENDSTOP    = 2'd3;


reg [3:0] tx_bitcount;
reg [15:0] clk_count;
reg [7:0] tx_reg;

always @(posedge sys_clk) begin

    if(sys_rst) begin

        state_tx <= IDLE;

    end else begin

        case(state_tx)

            IDLE: begin
                uart_tx <= 1'b1;
                //tx_reg <= tx_data;
                clk_count <= 16'd0;
                tx_bitcount <= 4'd0;
                tx_busy <= 1'b0;
                if(tx_wr) begin
                    tx_reg <= tx_data;
                    state_tx <= SENDSTART;
                end
            end

            // Send one start signal for one CLK_PERIOD
            SENDSTART: begin
                tx_busy <= 1'b1;
                uart_tx <= 1'b0;
                if(clk_count == CLK_PERIOD) begin
                    clk_count <= 16'd0;
                    //tx_reg <= tx_data;
                    state_tx <= SEND8BITS;
                end else begin
                    clk_count <= clk_count + 16'd1; 
                end

            end


            // Send 8 bits of data but be careful to wait for 1 CLK_PERIOD for each bit
            SEND8BITS: begin
                if(tx_bitcount == 4'd0) begin
                    tx_bitcount <= tx_bitcount + 4'd1;
                    uart_tx <= tx_reg[0];   
                    tx_reg <= {1'b0, tx_reg[7:1]};
                end 
                if(clk_count == CLK_PERIOD) begin
                    clk_count <= 16'd0;
                    if(tx_bitcount < 4'd8) begin
                        tx_bitcount <= tx_bitcount + 4'd1;
                        uart_tx <= tx_reg[0];   
                        tx_reg <= {1'b0, tx_reg[7:1]};
                    end 
                    else begin
                        clk_count <= 16'd0;
                        state_tx <= SENDSTOP;
                    end
                end 
                else begin
                        clk_count <= clk_count + 16'd1;
                end
            end

            // Send 1 stop bit for one CLK_PERIOD
            SENDSTOP: begin
                uart_tx <= 1'b1;        
                if(clk_count == CLK_PERIOD) begin
                    clk_count <= 16'd0;
                    tx_busy <= 1'b0;
                    state_tx <= IDLE;
                end else begin
                    clk_count <= clk_count + 16'd1; 
                end                     
            end

        endcase
    end

end

endmodule

However, if I delete the last else if block below from TAKESTOP state shown above, the code stops working after working well for some random number of characters and becomes stuck at TAKESTOP state. When I apply sys_rst, it again works for some characters and then gets stuck at same TAKESTOP state. The interesting thing is that when this else if block is placed and the code working flawless, it never actually enters this else if. I knew that because otherwise it would be stuck at IDLERX state as rx_err would be 1.:

else if(clk_count_rx > CLK_PERIOD)
        begin
        rx_err <= 1'b1;
        state_rx <= IDLERX;
        end

I am getting mad about such problems. Instead of inserting that else if block, the problem also vanishes when I assign the clk_count_rx to an output by removing the comment from these two lines from the above full code shows:

assign clock_least8 = clk_count_rx[7:0];
assign clock_most8 = clk_count_rx[15:8];

In other words my state machine does not work well and stable if I do not assign a register to an output although I do not use that output. This has happened a couple more times for my different Verilog projects.

The question how can such irrelevant changes repair my code? What is wrong with me? The thing doesn't seem logical. Am I doing a structural conceptual mistake and this change seems to repair it by somehow? I do not know. I use Quartus 16.1 with Cyclone V GX Starter Board.

By the way this is a working UART module and any one is welcomed to use it. I will be surely very glad to whom helps me on this matter.

\$\endgroup\$
  • 1
    \$\begingroup\$ I suggest you start with adding a synchroniser to your uart_rx input. Until then your FSM is not stable and will behave erratic. \$\endgroup\$ – Oldfart Oct 12 at 12:02
  • \$\begingroup\$ Thanks for fast reply. Actually there is only one clock here: sys_clk. Do I still need a sychronizer? \$\endgroup\$ – Rehin Oct 12 at 12:06
  • 1
    \$\begingroup\$ Well, by definition, it can't be an irrelevant change - you're saying it's completely relevant. So that means you're not understanding what the machines are doing consistently doing. As @oldfart says, add an input circuit to reject fast noise and provide immunity from metastability. Your input is completely asynchronous to your clock domain. \$\endgroup\$ – TonyM Oct 12 at 12:09
  • \$\begingroup\$ Thanks TonyM. I see. You mean that uart_rx comes from output with another clock. So if I input uart_rx to a flip-flop and use the output of that flip flop as uart_rx, the problem must be solved. But then, what I do not understand is that there is surely no problem if I use that else if block, which does not solve the issue you refer. I tested the circuit for about 12 hours by sending characters continuously, but it never failed. \$\endgroup\$ – Rehin Oct 12 at 12:19
  • \$\begingroup\$ All data-handling blocks on the same stream should clock on the same clock to prevent data falling between the cracks. \$\endgroup\$ – Mast Oct 13 at 7:19
20
\$\begingroup\$

Your uart_rx signal is asynchronous to your clock. However, you have one place in your code where you use it directly in the state machine. This is curious, because in all other cases, you are careful to assign it to rx1 and then test that.

    IDLERX: begin
      rx_bitcount <= 4'd0;
      rx_busy <= 1'b0;
      if (uart_rx == 1'b0 && rx_err != 1'b1) begin
        clk_count_rx <= 16'h0001;
        state_rx <= TAKESTART;
      end
    end

The problem here is that this if statement modifies both the state register and the counter register, a total of 18 FFs. Each of those FFs has logic associated with it that determine what its next state is. When you feed this logic with an asynchronous input and that input changes close to the clock edge, some of the FFs will see it in one state, while others will see it in a different state. After the clock edge, they will be in an inconsistent state that has nothing to do with your actual logic.1

When you modify your code, you cause the hardware resources and the routing paths inside the chip to be configured differently, which means that the delays between uart_rx and those 18 FFs will change. This changes the behavior of the state machine in unpredictable ways.

Bottom line: ALL inputs to a state machine MUST be synchronized to the clock! In your case, the easiest way to do that is to put the assignment

rx1 <= uart_rx;

outside your case statement, and use rx1 exclusively inside it.

BTW, you're not the first to make this mistake: UART Receiver glitches ... and you won't be the last.


1 Note that this has nothing to do with the issue of metastability, which is also an issue with asynchronous inputs. For best results, you'd feed uart_rx through at least two FFs before using it.

\$\endgroup\$
  • 4
    \$\begingroup\$ That was the perfect answer and solved my issue and most probably most issues that would be faced ahead. A problem that seems to be solved by random changes makes one mad. But your answer fully explains that. Grateful for that. \$\endgroup\$ – Rehin Oct 12 at 12:40
  • \$\begingroup\$ In order to fully understand the issue this video lecture is helpful: youtube.com/watch?v=ZPpuDMk9BOU \$\endgroup\$ – Rehin Oct 12 at 19:40
  • \$\begingroup\$ @Rehin: That video is about metastability, which, as I said, is NOT the issue here. \$\endgroup\$ – Dave Tweed Oct 13 at 13:52
  • \$\begingroup\$ It also explains the synchronization or crossing of clock domains in the beginning. And the requirement for the second ff for decreasing metastability occuring probability in the remaining part as you devised at the end of your answer. \$\endgroup\$ – Rehin Oct 13 at 15:38

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.