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

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

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Conversation

tore-espressif
Copy link
Collaborator

@tore-espressif tore-espressif commented Jun 14, 2024

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 espressif/esp-protocols#514
Closes #40
Closes espressif/esp-idf#13797

@tore-espressif
Copy link
Collaborator Author

@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 espressif/esp-protocols#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 espressif/esp-protocols#589 ? Or can everyhing be implemented in esp_modem_usb_dte? (these are 2 separate components)

@tore-espressif
Copy link
Collaborator Author

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

@david-cermak
Copy link
Contributor

@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 -> fix(modem): Fragment network packet by configured DTE buffer size esp-protocols#593
  2. esp_modem_usb_dte ->
    if (this->CdcAcmDevice::tx_blocking(data, len) != ESP_OK) {
    return -1;
    }
  3. or even here, at
    CDC_ACM_CHECK(data_len <= cdc_dev->data.out_xfer->data_buffer_size, ESP_ERR_INVALID_SIZE);

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
Copy link

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
Copy link
Collaborator Author

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 ;)

Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments. LGTM.

@tore-espressif
Copy link
Collaborator Author

@peter-marcisovsky thanks for the review!

Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nitpick about the constant value in the code.
Otherwise, LGTM.

host/class/cdc/usb_host_cdc_acm/cdc_acm_host.c Outdated Show resolved Hide resolved
@tore-espressif tore-espressif merged commit d938736 into master Jun 17, 2024
17 checks passed
@tore-espressif tore-espressif deleted the fix/cdc_tx_timeout branch June 17, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants