dcd_synopsis driver fails to flush data of length 256 bytes when in HS mode

[BennyEvans]

Operating System

Windows 10

Board

STM32H7

Firmware

examples/device/dcd_msc (tinyUSB 10.1)

What happened ?

The dcd_synopsys.c driver fails to flush when the data to flush is exactly 256 bytes in length in HS mode. Writing more data will eventually cause a flush but calling to flush with 256 bytes of data does not actually perform the transfer.

The line that I believe causes the issue is:

tinyusb/src/portable/st/synopsys/dcd_synopsys.c

Line 659 in 21bfd11

uint8_t const short_packet_size = total_bytes % xfer->max_size;

When total_bytes is 256 and xfer->max_size is 512, this line will evaluate to 256, but as the result type is a uin8_t, the result is 0. Further on in the code, this prevents num_packets from being incremented.

I'll do more testing but I believe the soultion to this might simply be to change the line to:
uint16_t const short_packet_size = total_bytes % xfer->max_size;

Please let me know if you require anymore information. I'll try to perform some more testing and put together a pull request over the next week or so when I have some free time.

UPDATE:
There are two instances of this by the look of it:

tinyusb/src/portable/st/synopsys/dcd_synopsys.c

Line 659 in 21bfd11

uint8_t const short_packet_size = total_bytes % xfer->max_size;

tinyusb/src/portable/st/synopsys/dcd_synopsys.c

Line 690 in 21bfd11

uint8_t const short_packet_size = total_bytes % xfer->max_size;

How to reproduce ?

1. Load the examples/device/dcd_msc on a board using the dcd_synopsys.c driver in HS mode.
2. From your PC, send a chunk of data with length 256, expecting it to be echoed back.
3. You will not receive a response until you send more data. Other lengths will echo back immediately.

Debug Log

Logs after sending data of length 256 bytes and 257 bytes below. Note that the 256 byte transfer is not completed.

256 byte transfer:

USBD Setup Received A1 21 00 00 00 00 07 00
CDC control request
Get Line Coding
Queue EP 80 with 7 bytes ...
USBD Xfer Complete on EP 80 with 7 bytes
CDC control complete
Queue EP 00 with 0 bytes ...
USBD Xfer Complete on EP 00 with 0 bytes
USBD Xfer Complete on EP 02 with 256 bytes
CDC xfer callback
Queue EP 02 with 512 bytes ...
Queue EP 82 with 256 bytes ...

257 byte transfer:

USBD Setup Received A1 21 00 00 00 00 07 00
CDC control request
Get Line Coding
Queue EP 80 with 7 bytes ...
USBD Xfer Complete on EP 80 with 7 bytes
CDC control complete
Queue EP 00 with 0 bytes ...
USBD Xfer Complete on EP 00 with 0 bytes
USBD Xfer Complete on EP 02 with 257 bytes
CDC xfer callback
Queue EP 02 with 512 bytes ...
Queue EP 82 with 257 bytes ...
USBD Xfer Complete on EP 82 with 257 bytes
CDC xfer callback

USBD Setup Received 21 22 02 00 00 00 00 00
CDC control request
Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes
Set Control Line State: DTR = 0, RTS = 1

Screenshots

No response

[cr1901]

Unsigned overflow/changing requirements strikes again :(. This is definitely wrong, and was probably there since the initial PR. When I originally wrote the driver, I wrote it for a board without a HS PHY. HS was completely out of scope, and I don't think tinyusb supported it at all. In FS, the max xfer is 64 (IIRC), so I used a uint8_t and forgot about it. When HS support was added, the maximum possible xfer size went from 64 (IIRC) to 512, and nobody (incl me) updated that var to account for it.

Good catch! Even if that's not the cause of your woes, it's a bug/oversight that should be changed.

[hathach]

@BennyEvans ah great, thanks, these are spot-on, as @cr1901 mentioned, this is originally written for FS. I did an update to support HS, but didn't update it properly. Would you like to submit an PR for the fix. If you don't have time, no problem, I could fix it myself.

[BennyEvans]

Cheers, no problem. I've just created the PR.