Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

usb_serial_jtag_ll does not flush when TX buffer is full (IDFGH-11503) #12628

Closed
3 tasks done
campbell-brown opened this issue Nov 20, 2023 · 12 comments
Closed
3 tasks done
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Type: Bug bugs in IDF

Comments

@campbell-brown
Copy link

campbell-brown commented Nov 20, 2023

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v5.1.1-577-g6b1f40b9bf

Espressif SoC revision.

ESP32-C3 (revision v0.4)

Operating System used.

Linux

How did you build your project?

Command line with idf.py

If you are using Windows, please specify command line type.

None

Development Kit.

ESP32-C3-DevKitM-1

Power Supply used.

External 5V

What is the expected behavior?

I have an example program here: https://github.com/campbell-brown/esp-usb-example/blob/main/main/main.cpp

This uses the usb_serial_jtag_ll module to continuously send data via USB, one line every second:

68 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._.
69 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._._
70 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._._.
60 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._.
61 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._
62 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._.
63 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._
64 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._.
65 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._
66 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._.
67 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._
68 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._.
69 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._._
70 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._._.
60 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._.
61 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._
62 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._.

What is the actual behavior?

It doesn't send the 64-byte length packet (i.e. a packet that will fill the TX buffer), but when it sends the next packet (65-bytes), it sends both (i.e. a 129-byte length packet).

60 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._.
61 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._
62 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._.
63 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._
64 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._.    <- This line does not get sent
65 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._   <- until this line is sent
66 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._.
67 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._
68 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._.
69 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._._
70 bytes:_._._._._._._._._._._._._._._._._._._._._._._._._._._._._._.

Steps to reproduce.

  1. Clone the repository loaded above.
  2. Build and flash onto the device.
  3. Establish a PuTTY connection using the instructions in the repository README.md.
  4. Observe the output. Notice the delay when it prints the 64-byte line.

Debug Logs.

No response

More Information.

This was built on Linux (Windows WSL) and tested using PuTTY in Windows. It is also an issue on an RPi that I have tested it on.

I assume the issue is because I call usb_serial_jtag_ll_txfifo_flush() straight after usb_serial_jtag_ll_write_txfifo(), and the USB_SERIAL_JTAG.ep1_conf.wr_done flag might be set before the data is actually written into the TX buffer. But why does this work for non-64-byte packets?

@campbell-brown campbell-brown added the Type: Bug bugs in IDF label Nov 20, 2023
@espressif-bot espressif-bot added the Status: Opened Issue is new label Nov 20, 2023
@github-actions github-actions bot changed the title usb_serial_jtag_ll does not flush when TX buffer is full usb_serial_jtag_ll does not flush when TX buffer is full (IDFGH-11503) Nov 20, 2023
@igrr
Copy link
Member

igrr commented Nov 20, 2023

Hi @campbell-brown, the _ll functions are a private API and are not intended to be used from application code. Have you considered using the usb_serial_jtag driver APIs instead?

@campbell-brown
Copy link
Author

Hi @igrr thanks for your reply, do you know where I can find a working example using the usb_serial_jtag APIs? I can only find vfs_usb_serial_jtag.c, which seems way more complicated than I am needing.

@igrr
Copy link
Member

igrr commented Nov 20, 2023

There is no example yet in IDF, unfortunately. I've asked internally to consider creating one. What are you trying to implement? Perhaps it will be easier to give you an example if I know what you are looking for.

(In case it helps, I saw this simple usb_serial_jtag—to—UART bridge code in this project: https://github.com/aliengreen/esp32_uart_bridge/blob/f35b335f0f1d4e9fbaa5d1eb0d6d4f9010c051e5/main/esp32_uart_bridge.c#L127-L152 which, ironically, does use usb_serial_jtag_ll_txfifo_flush... :/)

@campbell-brown
Copy link
Author

Thanks, I will check that out. I see they use a combination of the usb_serial_jtag and the _ll library (usb_serial_jtag_ll_txfifo_flush).

That's exactly what I am trying to implement 😁 (a USB to UART bridge) but with the ability to swap between USB and BLE (so USB <-> UART, or BLE <-> UART). I have all parts working (UART, USB, and BLE), but I am running automated stress tests (by sending large amounts of data through all streams), and this 64-byte packet issue is causing 0.01% of failures. It's small, I know, but I still would like to make the bridge as reliable as possible.

@campbell-brown
Copy link
Author

I swapped to using the usb_serial_jtag library (see my recent changes in my example repository): https://github.com/campbell-brown/esp-usb-example/blob/379842588a48970499ba2805f7df9c553e461190/main/main.cpp

usb_serial_jtag_write_bytes(forward_buffer, string_length, 500 / portTICK_PERIOD_MS);
usb_serial_jtag_ll_txfifo_flush();

It still has the same issue where it doesn't send the 64-byte packet. The 64-byte packet only gets flushed when the next packet is sent. So I assume the project linked above has the same issue.
I also tried a more robust solution to make sure all of the bytes are sent, which has the same issue:

size_t bytes_written = 0;
while (bytes_written < string_length)
{
    usb_serial_jtag_write_bytes(
        forward_buffer + bytes_written, bytes_left, 500 / portTICK_PERIOD_MS);
    usb_serial_jtag_ll_txfifo_flush();
}

The issue is that usb_serial_jtag_write_bytes returns 64 even though 64 bytes are not sent. The 64 relates to the USB TX buffer size in the LL layer.

@campbell-brown
Copy link
Author

Ideally, I shouldn't have to call usb_serial_jtag_ll_txfifo_flush(), ideally the usb_serial_jtag driver does all that for me. I see the driver does call usb_serial_jtag_ll_txfifo_flush(), but if I don't add it in my code, I don't receive any data.

@igrr
Copy link
Member

igrr commented Nov 22, 2023

All clear, thanks for trying this! i'll update the ticket title to reflect the general problem rather something specific to the usage of the LL layer functions.

@mythbuster5
Copy link
Collaborator

This is because any packet that is sent over USB that is equal to the max packet size (64 bytes) is assumed to be part of a larger transfer, so the host will buffer it and not pass it through to userspace. I'm going to make a commit to fix it.

@campbell-brown
Copy link
Author

Thanks @mythbuster5, is there a patch I can apply to the SDK now as a quick workaround?

@igrr
Copy link
Member

igrr commented Nov 28, 2023

@campbell-brown please try this one:
27388.patch.txt

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new labels Dec 1, 2023
@greenaddress
Copy link

@mythbuster5 is there any plan to backport b8e8042 to release branches v5.1 and v5.2 ?

I noticed the fix is only in master, not in any stable tag or release branch.

Thanks

@greenaddress
Copy link

@igrr maybe you can answer my question? Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Type: Bug bugs in IDF
Projects
None yet
Development

No branches or pull requests

5 participants