"Hello World!" becomes "Heeeeeeeeee…."

[nalzok]

I am trying to output "Hello World!" to the serial port by using tinyfpga_bx_usbserial. Here is my source code usbserial_hello.v:

/*
    USB Serial

    Wrapping usb/usb_uart_ice40.v to output "hello, world".
*/

`default_nettype none

module usbserial_hello (
        input  pin_clk,

        inout  pin_usb_p,
        inout  pin_usb_n,
        output pin_pu,

        output pin_led,

        output [3:0] debug
    );

    wire clk_48mhz;

    wire clk_locked;

    // Use an icepll generated pll
    pll pll48( .clock_in(pin_clk), .clock_out(clk_48mhz), .locked( clk_locked ) );

    // Generate reset signal
    reg [5:0] reset_cnt = 0;
    wire reset = ~reset_cnt[5];
    always @(posedge clk_48mhz)
        if ( clk_locked )
            reset_cnt <= reset_cnt + reset;

    // uart pipeline in
    reg [7:0] uart_in_data;
    reg       uart_in_valid;
    wire      uart_in_ready;

    // Create the text string
    localparam TEXT_LEN = 13;

    reg [7:0] hello [0:TEXT_LEN-1];
    reg [3:0] char_count;

    initial begin
        hello[0]  <= "H";
        hello[1]  <= "e";
        hello[2]  <= "l";
        hello[3]  <= "l";
        hello[4]  <= "o";
        hello[5]  <= " ";
        hello[6]  <= "W";
        hello[7]  <= "o";
        hello[8]  <= "r";
        hello[9]  <= "l";
        hello[10] <= "d";
        hello[11] <= "!";
        hello[12] <= "\n";
    end

    // send text through the serial port
    reg [22:0] delay_counter;

    always @(posedge clk_48mhz) begin
        if (!reset) begin
            if (char_count < TEXT_LEN) begin
                uart_in_valid <= 1;
                uart_in_data <= hello[char_count];
                if (uart_in_valid && !uart_in_ready) begin
                    char_count <= char_count + 1;
                end
            end else begin
                uart_in_valid <= 0;
                delay_counter <= delay_counter + 1;
                if (&delay_counter) begin
                    char_count <= 0;
                end
            end
        end
    end

    // usb uart - this instanciates the entire USB device.
    usb_uart uart (
        .clk_48mhz  (clk_48mhz),
        .reset      (reset),

        // pins
        .pin_usb_p( pin_usb_p ),
        .pin_usb_n( pin_usb_n ),

        // uart pipeline in
        .uart_in_data( uart_in_data ),
        .uart_in_valid( uart_in_valid ),
        .uart_in_ready( uart_in_ready ),
    );

    // USB Host Detect Pull Up
    assign pin_pu = 1'b1;

endmodule

I was expecting to see

Hello World!
Hello World!
Hello World!
Hello World!
Hello World!
Hello World!

only to get

HeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeHeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeHeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeHeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeHeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeHeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee

Curiously, the following code gives the correct output, i.e. Hello World!:

    always @(posedge clk_48mhz) begin
        if (!reset) begin
            if (char_count < TEXT_LEN) begin
                uart_in_valid <= 1;
                uart_in_data <= hello[char_count];
                if (uart_in_valid && !uart_in_ready) begin
                    char_count <= char_count + 1;
                end

                // !!!MAGIC!!!
                if (uart_in_ready) begin
                    uart_in_valid <= 0;
                end
            end else begin
                uart_in_valid <= 0;
                delay_counter <= delay_counter + 1;
                if (&delay_counter) begin
                    char_count <= 0;
                end
            end
        end
    end

Note the three lines marked !!!MAGIC!!!. They are making the difference here. However, as far as I know, we should NOT change uart_in_valid based on uart_in_ready, in order to avoid some kind of deadlock.

Any ideas? I'm clueless...

[nalzok]

Here is another example. While the module is named usbserial_toggle, currently it just adds a pipeline stage between uart_in_* and uart_out_* by connecting them to each other with the module toggle_case. The expected behavior is identical to that of usbserial_tbx.v, i.e. a loopback. However, no characters are echoed back on my serial terminal while testing.

I'm not sure if there is a bug with my code or in your library. Please advice.

/*
    USB Serial

    Wrapping usb/usb_uart_ice40.v to create a "loopback" that toggles the case.
*/

`default_nettype none

module usbserial_toggle (
        input  pin_clk,

        inout  pin_usb_p,
        inout  pin_usb_n,
        output pin_pu,

        output pin_led,

        output [3:0] debug
    );

    wire clk_48mhz;

    wire clk_locked;

    // Use an icepll generated pll
    pll pll48( .clock_in(pin_clk), .clock_out(clk_48mhz), .locked( clk_locked ) );

    // Generate reset signal
    reg [5:0] reset_cnt = 0;
    wire reset = ~reset_cnt[5];
    always @(posedge clk_48mhz)
        if ( clk_locked )
            reset_cnt <= reset_cnt + reset;

    // uart pipeline in
    wire [7:0] uart_in_data;
    wire       uart_in_valid;
    wire       uart_in_ready;

    // uart pipeline out
    wire [7:0] uart_out_data;
    wire       uart_out_valid;
    wire       uart_out_ready;

    toggle_case foo(
        .clk(clk_48mhz),
        .reset(reset),

        .in_data(uart_out_data),
        .in_valid(uart_out_valid),
        .in_ready(uart_out_ready),

        .out_data(uart_in_data),
        .out_valid(uart_in_valid),
        .out_ready(uart_in_ready)
    );

    // assign debug = { uart_in_valid, uart_in_ready, reset, clk_48mhz };

    wire usb_p_tx;
    wire usb_n_tx;
    wire usb_p_rx;
    wire usb_n_rx;
    wire usb_tx_en;

    // LED
    reg [22:0] ledCounter;
    wire led_nonzero = |ledCounter;
    always @(posedge clk_48mhz) begin
        if (led_nonzero || (uart_in_valid && !uart_in_ready))
            ledCounter <= ledCounter + 1;
    end
    assign pin_led = led_nonzero;

    // usb uart - this instanciates the entire USB device.
    usb_uart uart (
        .clk_48mhz  (clk_48mhz),
        .reset      (reset),

        // pins
        .pin_usb_p( pin_usb_p ),
        .pin_usb_n( pin_usb_n ),

        // uart pipeline in
        .uart_in_data( uart_in_data ),
        .uart_in_valid( uart_in_valid ),
        .uart_in_ready( uart_in_ready ),

        .uart_out_data( uart_out_data ),
        .uart_out_valid( uart_out_valid ),
        .uart_out_ready( uart_out_ready  )

        //.debug( debug )
    );

    // USB Host Detect Pull Up
    assign pin_pu = 1'b1;

endmodule

module toggle_case(
    input wire clk,
    input wire reset,

    input wire [7:0] in_data,
    input wire       in_valid,
    output reg       in_ready,

    output reg [7:0] out_data,
    output reg       out_valid,
    input wire       out_ready
);

    always @(posedge clk) begin
        if (reset) begin
            in_ready <= 0;
            out_valid <= 0;
        end else begin
            // Input only
            if (in_valid && !in_ready) begin
                in_ready <= 1;
                out_valid <= 1;
                // TODO: toggle case here
                out_data <= in_data;
            end

            // Output only
            if (out_valid && !out_ready) begin
                in_ready <= 0;
                out_valid <= 0;
            end
        end
    end

endmodule

[nalzok]

I figured it out myself! Here is a working example

/*
    USB Serial

    Wrapping usb/usb_uart_ice40.v to output "hello, world".
*/

`default_nettype none

module usbserial_hello (
        input  pin_clk,

        inout  pin_usb_p,
        inout  pin_usb_n,
        output pin_pu,

        output pin_led,

        output [3:0] debug
    );

    wire clk_48mhz;

    wire clk_locked;

    // Use an icepll generated pll
    pll pll48( .clock_in(pin_clk), .clock_out(clk_48mhz), .locked( clk_locked ) );

    // Generate reset signal
    reg [5:0] reset_cnt = 0;
    wire reset = ~reset_cnt[5];
    always @(posedge clk_48mhz)
        if ( clk_locked )
            reset_cnt <= reset_cnt + reset;

    // uart pipeline in
    reg [7:0]  uart_in_data;
    reg        uart_in_valid;
    wire       uart_in_ready;

    // uart pipeline out
    wire [7:0] uart_out_data;
    wire       uart_out_valid;
    reg        uart_out_ready;

    // Create the text string
    localparam TEXT_LEN = 13;
    reg [7:0] hello [0:TEXT_LEN-1];
    reg [3:0] char_count;
    initial begin
        hello[0]  <= "h";
        hello[1]  <= "e";
        hello[2]  <= "l";
        hello[3]  <= "l";
        hello[4]  <= "o";
        hello[5]  <= ",";
        hello[6]  <= " ";
        hello[7]  <= "w";
        hello[8]  <= "o";
        hello[9]  <= "r";
        hello[10] <= "l";
        hello[11] <= "d";
        hello[12] <= "\n";
   end

    // send text through the serial port
    localparam DELAY_WIDTH = 26;
    reg [DELAY_WIDTH-1:0] delay_count;

    reg pardon;

    always @(posedge clk_48mhz) begin
        if (reset) begin
            uart_in_valid <= 0;
            delay_count <= 0;
            char_count <= 0;
            pardon <= 0;
        end else begin
            if (char_count < TEXT_LEN) begin
                if (pardon) begin
                    pardon <= 0;
                    uart_in_valid <= 0;
                    char_count <= char_count + 1;
                end else if (uart_in_valid) begin
                    if (!uart_in_ready) begin
                        pardon <= 1;
                    end
                end else begin
                    uart_in_valid <= 1;
                    uart_in_data <= hello[char_count];
                end
            end else begin
                delay_count <= delay_count + 1;
                if (&delay_count) begin
                    char_count <= 0;
                end
            end
        end
    end

    // LED
    reg [23:0] ledCounter;
    wire led_nonzero = |ledCounter;
    always @(posedge clk_48mhz) begin
        if (led_nonzero || uart_in_valid) begin
            ledCounter <= ledCounter + 1;
        end
    end
    assign pin_led = led_nonzero;

    // usb uart - this instanciates the entire USB device.
    usb_uart uart (
        .clk_48mhz  (clk_48mhz),
        .reset      (reset),

        // pins
        .pin_usb_p( pin_usb_p ),
        .pin_usb_n( pin_usb_n ),

        // uart pipeline in
        .uart_in_data( uart_in_data ),
        .uart_in_valid( uart_in_valid ),
        .uart_in_ready( uart_in_ready ),

        // uart pipeline out
        .uart_out_data( uart_out_data ),
        .uart_out_valid( uart_out_valid ),
        .uart_out_ready( uart_out_ready  )

        //.debug( debug )
    );

    // USB Host Detect Pull Up
    assign pin_pu = 1'b1;

endmodule

The module usb_uart seems to expect the signal uart_in_data to be asserted for one more cycle after the transmission is considered complete, so I added the pardon register as a workaround.

Is this a bug?

[jjj11x]

In your original code:

                if (uart_in_valid && !uart_in_ready) begin
                    char_count <= char_count + 1;
                end

should be:

                if (uart_in_valid && uart_in_ready) begin
                    char_count <= char_count + 1;
                end

You need to wait for valid and ready to both be high before considering the byte transferred and moving on to the next byte.

[nalzok]

@jjj11x Yeah, but the signal uart_in_ready is active low, so !uart_in_ready means asserted.

I have tried the change you proposed, but nothing is appearing on serial monitor with it :(

It turns out that I just connected to the wrong serial port. You are right, and it works with uart_in_valid && uart_in_ready. Well, so uart_in_ready is actually active HIGH. Thanks so much for pointing that out!

[jjj11x]

Glad I could help. I don't have an account to post, but could you update your post here also: https://discourse.tinyfpga.com/t/usbserial-hello-world-becomes-heeeeeeeeee/1361

[nalzok]

@jjj11x Sure! I have updated that post. Hopefully it can help someone in the future!