-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
HWCDC.h needs a clearTxBuffer() function for ESP32C3 USBCDC to work properly #7779
Comments
As far as I can tell, that only fixes blocking to the led, but the core issue remains and will likely case other unforseen blocks. There's actually a few issues in how the usbcdc functions for the ESP32C3. I just went ahead and modified the HWCDC.cpp file to eliminate the issues (including clearing the TX Buffer). Most of the issues appear to be fixed now (LED works perfectly as well now, without PR #7221), but I need to run some more tests to make sure the changes I made don't break anything else. I should be done in a few hours but I don't know how to submit changes to the repo. |
All appears to ne working perfectly after the changes. Basically, all that I had to do was add 2 global variables (user_tx_timeout_ms and user_txBufferSize) and change This: To This: Now there's no more blocking, hanging or saving the filled up TX Buffer from the beginning of runtime and printing it whenever you open the port, before current data. |
And because I changed the HWCDC.cpp file, it allowed me to make the USBSerial Example from earlier, MUCH simpler: `#if !ARDUINO_USB_CDC_ON_BOOT unsigned long timer; // for non-blocking delay void setup() { Serial.begin(); // start the CDC port timer = millis() + 1000; // set timer to 1 second from now void loop() { static void usbEventCallback(void* arg, esp_event_base_t event_base, int32_t event_id, void* event_data){
} |
The default USBSerial example has the device using ARDUINO_USB_CDC_CONNECTED_EVENT, but the ESP32C3 actually uses ARDUINO_HW_CDC_CONNECTED_EVENT as well as other things that also don't work, or just don't make sense.. Anyway, the full edited HWCDC.cpp file is `// Copyright 2015-2020 Espressif Systems (Shanghai) PTE LTD #include "esp32-hal.h" ESP_EVENT_DEFINE_BASE(ARDUINO_HW_CDC_EVENTS); volatile uint16_t user_tx_timeout_ms = 100; static RingbufHandle_t tx_ring_buf = NULL; static esp_err_t arduino_hw_cdc_event_post(esp_event_base_t event_base, int32_t event_id, void *event_data, size_t event_data_size, BaseType_t *task_unblocked){ static esp_err_t arduino_hw_cdc_event_handler_register_with(esp_event_base_t event_base, int32_t event_id, esp_event_handler_t event_handler, void *event_handler_arg){ static void hw_cdc_isr_handler(void *arg) {
} static void ARDUINO_ISR_ATTR cdc0_write_char(char c) { HWCDC::HWCDC() { } HWCDC::~HWCDC(){ HWCDC::operator bool() const void HWCDC::onEvent(esp_event_handler_t callback){ void HWCDC::onEvent(arduino_hw_cdc_event_t event, esp_event_handler_t callback){ void HWCDC::begin(unsigned long baud)
} void HWCDC::end() void HWCDC::setTxTimeoutMs(uint32_t timeout){ /*
size_t HWCDC::setTxBufferSize(size_t tx_queue_len){ int HWCDC::availableForWrite(void) size_t HWCDC::write(const uint8_t *buffer, size_t size)
} size_t HWCDC::write(uint8_t c) void HWCDC::flush(void) /*
size_t HWCDC::setRxBufferSize(size_t rx_queue_len){ int HWCDC::available(void) int HWCDC::peek(void) int HWCDC::read(void) size_t HWCDC::read(uint8_t *buffer, size_t size) /*
void HWCDC::setDebugOutput(bool en) #if ARDUINO_USB_MODE #endif /* CONFIG_TINYUSB_CDC_ENABLED */` |
Actually, while the above HWCDC.cpp works on my work computer and when the device isn't plugged into a data port, it doesn't work properly on my home computer. Some more work is needed. |
@specternecter sorry but your comments have bad formatting ad it is not very clear The new code will skip completely the write if the usb is not connected or opened, not even written to the internal buffer Nothing of what is issued with write or print should be buffered if usb is not connected |
Okay, I found the issue with my home computer. I wasn't running the example above but running LoRa communication. 1/2 mile from my home it worked fine, but in my home there was blocking happening. I assume because when close things are happening much faster. It was easily solved by just defaulting TxTimeoutMs to 0. It apparently isn't needed. But no, I'm not really interested in running a workaround. I want the core to work properly. |
And sorry, I try to use 'add code' and copying and pasting directly from the same formatting as arduino's source code but this is how it comes. I don't know how to fix it to make it look good |
This may be a better way to show the file. This fixes the core issues. tx_timeout_ms apparently isn't necessary and as long as it's set to 0, I can't find any problems. I assume the issue with tx_timout_ms is that it's always called with portTICK_PERIOD_MS like "tx_timeout_ms / portTICK_PERIOD_MS", except if you trace portTICK_PERIOD_MS through the core, you'll find that it always equals 1. It seems pretty nonsensical to me. But then again so does setting tx_timeout_ms when it seems that leaving it set to 0 eliminates all the blocking/hanging. Tested on 3 different computers and a combined 17 completely different sketches that all showed problems before I modified the HWCDC.cpp file as attached. I have yet to find any issues whatsoever with the USB after these final fixes. Between fixing the hanging and fixing the serious txBuffer issue, I am confident it's a great improvement over the ridiculously buggy USBCDC of before. It still prints "ESP-ROM:esp32c3-api1-20210207" whenever I open the port but that appears to be either the bootloader or an Arduino menuConfig issue. I'm not really concerned about it because I no longer get the very first chunk of outdated Serial prints before current prints when I open a port. Now when the port is opened I get the "ESP-ROM:esp32c3-api1-20210207" followed only by the current data that it actually should be printing. The only thing I would consider changing beyond this point is removing the setTxTimeoutMs() function altogether and just defining it as 0. I can almost guarantee that if you try the sketch that inspired the pull request example you wanted me to try, it'll work perfectly if the HWCDC.cpp file is replaced with the one attached. |
The original issue is related to the JTAG/HW CDC Serial peripheral from ESP32-S3, ESP32-C3, ESP32-C6 and ESP32-H2. One issue is that when USB cable is unplugged, The second issue is that It won't work properly when the sketch waits for the CDC to be connected using a Serial Monitor, using a code like These and some other issues were fixed by the PR #9275 When USB is unplugged, nothing will block any HW CDC writing or flushing.
It has been fixed for arduino core 3.0.0-RC1 and it is available in the master branch. Check the PR examples. |
Related area
Board Support
Hardware specification
Support for ESP32C3
Is your feature request related to a problem?
https://github.com/espressif/arduino-esp32/issues/6089
Describe the solution you'd like
It needs a function to clear the TX Buffer or anything in there will be printed when port is opened, It would seem that data should be held and printed when the port is opened, but that's actually a significant problem as the sketch below demonstrates.
Describe alternatives you've considered
I tried toggling the setTxBufferSize() but it only slightly helps. I tried many others that didn't help at all and mostly only created more problems. As a temporary solution, I added the waitForCDC variable to mask the issue, but it's designed so that the board will only run if the port is open when the variable is set to true.
Additional context
The following sketch demonstrates why this function is necessary. The USBSerial example in the IDE is absolutely wrong for the ESP32C3. I fixed it, along with fixing many other issues I've seen people post about or personally experienced. If you run this proposed example sketch, leave the port closed for at least 15 seconds and then open it. The longer it's closed, the more noticeable the need for a clearTxBuffer() function is. It will print the bootloader start message and the first 13 counts, then start printing what it's supposed to print.
The text was updated successfully, but these errors were encountered: