Don't set TX timeout to 0 anymore for HW/USB CDC #2340
+2
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Due to a change in the Espressif Arduino core, the TX timeout for the HW CDC (used in the ESP32-S3, for example) must not be set to 0, as otherwise, an integer underflow occurs. Removing the TX timeout is not necessary anymore anyways, because it is now detected when CDC is not active, and attempts to write will return immediately until the host read something again. Only when the transmit buffer becomes full initially, the default timeout of just 100ms takes effect once. For USB CDC (used with the ESP32-S2, for example), the timeout is not relevant either.
|
I also removed a 100ms delay. I'm not sure why that was there. Maybe so that there is some time to open a serial terminal before OpenDTU boots? In that case, 100ms would likely have been to short anyways. |
stefan123t
reviewed
Oct 10, 2024
| delay(100); | ||
| #else | ||
| #if !ARDUINO_USB_CDC_ON_BOOT | ||
| // Only wait for serial interface to be set up when not using CDC |
There was a problem hiding this comment.
Espressif's Documentation of USB CDC (Communications Device Class) and the respective event's.
#if !ARDUINO_USB_CDC_ON_BOOT
USBCDC USBSerial;
#endif
stefan123t
approved these changes
Oct 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to a change in the Espressif Arduino core, the TX timeout for the HW CDC (used in the ESP32-S3, for example) must not be set to 0, as otherwise, an integer underflow occurs (see here).
Removing the TX timeout is not necessary anymore anyways, because it is now detected when CDC is not active, and attempts to write will return immediately until the host read something again. Only when the transmit buffer becomes full initially, the default timeout of just 100ms takes effect once (and that is actually a good thing, because it prevents text from being swallowed unintentionally, when someone is actually using CDC).
For USB CDC (used with the ESP32-S2, for example), the timeout is not relevant either, although I could not test that myself.
This fixes an issue where OpenDTU won't fully boot on the ESP32-S3, unless CDC is used. Although I'm not sure yet why the issue sometimes occurs and sometimes not, I was relatively consistently able to reproduce the issue when using the latest
esptoolon Linux (Debian 12, Linux kernel v6.1) to reset the ESP32-S3:esptool --chip esp32s3 read_flash --flash_size detect 0x0 0x1000 /dev/null