search

espressif / esp-usb

Other
15 stars 9 forks source link

fix(host/cdc): Fix TX timeout reaction #41

Closed tore-espressif closed 2 weeks ago

tore-espressif commented 2 weeks ago

After TX transfer timeout we reset the endpoint which causes all in-flight transfers to complete. The transfer finished callback gives a transfer finished semaphore which we must take, so we would not affect next TX transfers.

Test included

Closes https://github.com/espressif/esp-protocols/issues/514 Closes https://github.com/espressif/esp-usb/pull/40 Closes https://github.com/espressif/esp-idf/issues/13797

tore-espressif commented 2 weeks ago

@radiotommy I'd like to close your #40 in favor of this PR. Could you please check that this fix works for you? @wishinlife This should fix your https://github.com/espressif/esp-protocols/issues/514

@david-cermak PTAL too, we'd like to release usb_host_cdc_acm v2.0.3 ASAP. Are there any changes required to fix https://github.com/espressif/esp-protocols/issues/589 ? Or can everyhing be implemented in esp_modem_usb_dte? (these are 2 separate components)

tore-espressif commented 2 weeks ago

@peter-marcisovsky we discussed similar issue earlier today :)

david-cermak commented 2 weeks ago

@david-cermak PTAL too, we'd like to release usb_host_cdc_acm v2.0.3 ASAP. Are there any changes required to fix espressif/esp-protocols#589 ? Or can everyhing be implemented in esp_modem_usb_dte? (these are 2 separate components)

The issue reported in espressif/esp-protocols#589 is unrelated to this fix (although I noticed some of the related errors when testing this with more verbose output, probably causing some transmits to timeout). The fragmentation problem (espressif/esp-protocols#589) could be fixed on multiple layers: 1) esp_modem -> https://github.com/espressif/esp-protocols/pull/593 2) esp_modem_usb_dte -> https://github.com/espressif/esp-usb/blob/a94ccb2cd363c83ed457cb050edbfc9941aa0e32/host/class/cdc/esp_modem_usb_dte/esp_modem_usb.cpp#L120-L122 3) or even here, at https://github.com/espressif/esp-usb/blob/a94ccb2cd363c83ed457cb050edbfc9941aa0e32/host/class/cdc/usb_host_cdc_acm/cdc_acm_host.c#L1146

The best choice would be 2) (or maybe 3), as the fix 1) only addresses data packets and is not needed for UART and VFS transports),

I assume that it's been a design decision to return an error when trying to transmit more bytes than the configured Tx buffer rather then iterate over the bigger size until we transmit everything (as we see in some other IDF drivers, like uart, or essl). If this is the case then I agree there's nothing else to fix in usb_host_cdc_acm and I'll raise a separate PR in esp_modem_usb_dte component.

radiotommy commented 2 weeks ago

just tested it on my board. It fixes my problem as well. @tore-espressif, no problem, I like yours better :). hope this get merged ASAP

tore-espressif commented 2 weeks ago

I assume that it's been a design decision to return an error when trying to transmit more bytes than the configured Tx buffer rather then iterate over the bigger size until we transmit everything

@david-cermak yes, this is intentional. We will consider this feature in the future ;)

tore-espressif commented 2 weeks ago

@peter-marcisovsky thanks for the review!