usb-cdc overwrites its own tx buffer during write, hence data arrives truncated (IDFGH-7469)

[moefear85]

Environment

• Module or chip used: esp32-s2
• IDF version: affects all versions from atleast v4.3.1 to latest master
• Build System: idf.py
• Operating System: Linux
• Using an IDE?: VSCode IDF Extension
• Power Supply: external 3.3V

Problem Description

When calling tinyusb_cdcacm_write_queue(), then the data sent out is always the last portion of whatever buffer was given, and it depends on the configured cdc tx fifo size in sdkconfig.

Also, any value for rx fifo size less than 64 causes no data to arrive or be detected.

Expected Behavior

That when sending a buffer larger than what tinyusb_cdcacm_write_queue() can handle, that it only write as much as it can, and return that value, so another call can be placed later at proper offsets, until the entire buffer is sent.

One also expects rx fifo to forward any incoming data as long as it fits in its buffer, no matter how small, even if only 1 byte in size (such as a user typing individual characters).

Actual Behavior

The function correctly reports the size it accepted & sent, but instead of taking the first n bytes of the buffer, it takes the last n bytes. This means if the buffer in sdkconfig is chosen large enough, such as 1024 bytes, one might never notice this problem. But at smaller sizes such as 64 bytes, it leads to stream corruption (truncation of beginnings).

if the rx fifo buffer size is any smaller than 64, nothing arrives anymore. If this is not fixable, perhaps a warning somewhere that it has to be atleast 64 bytes would be helpful. It seems if this value is smaller than a single usb packet size, it gets dropped by tinyusb.

Steps to reproduce

1. Any sample project that tests tinyusb_cdcacm_write_queue(), as long as the size of the buffer passed is larger than the tx FIFO size chosen in sdkconfig (CFG_TUD_CDC_TX_BUFSIZE).

Cause of Problem

The structure "tu_fifo_t" used to hold the outgoing data contains a boolean called "overwritable". The default value seems to be true. Hence when tud_cdc_n_write() is called, and the source buffer is larger than the ringbuffer in "tu_fifo_t", it will wrap around and overwrite itself, so that if the ringbuffer size is m and the passed buffer size is n, then the first (n-m) bytes get lost/overwritten, and only the last n bytes are saved and thus later flushed/written out.

Workaround

I manually set the value of "overwritable" to false before doing any sending, and this solves the problem. No matter how small CFG_TUD_CDC_TX_BUFSIZE (even a value of 1 works), and no matter how large the buffer I pass to tinyusb_cdcacm_write_queue(), it will only queue up to the first n bytes of the passed buffer, correctly returning the size it queued, so that later calls at proper offsets will correctly queue & send out the right offsets.

Note: Even after setting "overwritable" to true, then although I can successfully choose smaller tx fifo sizes without any loss, at some point, I think a size of 1 or 2, it no longer automatically flushes when necessary, hence calling tinyusb_cdcacm_write_queue() will always return 0 unless I manually call tinyusb_cdcacm_write_flush() then try again. But at any tx fifo sizes larger than that, it is not necessary to call tinyusb_cdcacm_write_flush().

[tore-espressif]

Hello @moefear85 , thank you for your debugging effort and I'm sorry for the troubles.

Update about the RX path:

It seems if this value is smaller than a single usb packet size, it gets dropped by tinyusb.

Indeed, in TinyUSB the RX FIFO buffer must be at least the size of OUT endpoint max packet size, which is currently fixed to 64. It is possible to change the max packet size to 8, 16 or 32 (according to USB specification) in both the endpoint descriptor and in implementation, but the current code doesn't make this change straight-forward.
So I'd suggest leaving it as it is and only add a constraint in menuconfig that won't allow you to set RX buf size lower than 64.

I'm checking the TX path issue as of now.

Let me know whether the 'solution' for RX path is good enough for you. Thanks!

[moefear85]

yeah my general thinking was placing a menuconfig constraint to prevent bewilderment when someone experiments with the value. as for the tx path, the solution in my experience is to simply set overwritable to false during cdc driver install.

but honestly, there are issues open since 2019 for which PRs have been submitted that haven't been merged yet. I again urge espressif to fix the uart BREAK issue. it is raised as an event immediately upon arrival, thus jumping lines relative to preexisting buffered data, which makes it unusable in most cases despite being important to properly delimit frames of any protocol sent over serial. currently i am forced to add a redundant layer of base64 encoding as a workaround

[tore-espressif]

About the TX path:

the data sent out is always the last portion of whatever buffer was given

Will fix. Thanks for letting us know.

it no longer automatically flushes when necessary

I could reproduced this too. For buffer sizes < 64 this condition is never met https://github.com/hathach/tinyusb/blob/master/src/class/cdc/cdc_device.c#L174

I'll publish PR in upstream TinyUSB project.

[moefear85]

thx i'll close this

[Alvin1Zhang]

Thanks for reporting, reopen until the fix is available on GitHub. Thanks.