[Solved] Usbserial: "Hello World!" becomes "Heeeeeeeeee...."


#1

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

Any ideas? @lawrie.griffiths @david


#2

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.

Can someone explain why these lines are required? I’m clueless…


#3

It is a long time since i did anything with this. @david made significant improvements to what I did.


#4

I see, but it is you that laid a solid foundation for all of this. Thanks for your effort!

I am not sure if this issue lies in the usb_uart module. Probably this is just a generic question about valid/ready handshaking. I did some research and realized that we have valid-before-ready, ready-before-valid, and valid-with-ready (reference). It appears to me that both tinyfpga_bx_usbserial and your tiny_usb_examples requires ready to be asserted before valid can be asserted, a.k.a. a ready-before-valid approach. However, the README of tinyfpga_bx_usbserial says

The key to the streaming pipeline interface is that there are three ports: data, valid, and ready. A sender will place new data on the data port and raise the valid flag. If a receiver is ready to receive, it will raise the ready flag. So in any connection there will be two signals generated by the sender (data and valid) and one by the receiver ready. And here’s the key part if valid and ready are both raised at the same time the data is considered to be transfered. This interface can clunk along - the sender raising valid now and then, and provided the ready signal is raised, data will flow, but pretty amazingly, this interface can really rock - if there is data to be had at every clock cycle, just leave valid and ready on and one word of data will flow at each clock.

This describes a valid-with-ready approach. In addition, I cannot understand how do data flow “at each clock”: why can we leave the ready signal asserted when we have just accepted a word from the upstream? In the next cycle, if the downstream’s ready signal becomes deasserted and the upstream’s valid signal is still asserted, then this word will be overwrote before it can be passed down (unless we use an extra internal register to hold it).

I would love to get an answer from @david, but he hasn’t been on this site for two months, and I am wondering if he is still active. While it has been over a year since your last commit, maybe you still remember some high-level stuff like the protocol specification? I will try to figure out the low-level implementation details myself. Anyway, thanks for your awesome tiny_usb_examples project!


#5

Never mind. 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.


#6

Update: thanks to the help of @jjj11x on GitHub, I have realized my mistake: the signal uart_<in|out>_ready is active high instead of active low as I incorrectly assumed. Thus, the condition checking if the current byte has been successfully transmitted should be uart_in_valid && uart_in_ready rather than uart_in_valid && !uart_in_ready in my original code.