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.

Sign up for GitHub

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

Jump to bottom

Fixes USB CDC setRxBufferSize(), begin(), _onRX() #6413

Merged
merged 11 commits into from Mar 28, 2022
Merged

Fixes USB CDC setRxBufferSize(), begin(), _onRX() #6413

Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1a5b3e2
Fixes USB CDC setRxBufferSize(), begin(), _onRX()
SuGlider Mar 10, 2022
84adb04
Merge branch 'master' into usb-cdc
SuGlider Mar 10, 2022
b7c6948
Fixes SetRxBufferSize(0) with end()
SuGlider Mar 11, 2022
70d7983
Merge branch 'usb-cdc' of https://github.com/suglider/arduino-esp32 i…
SuGlider Mar 11, 2022
9e9099e
Fixes reset when 2x call to end()
SuGlider Mar 11, 2022
b16f535
Merge branch 'master' into usb-cdc
SuGlider Mar 16, 2022
8044a38
Adds RX_OVERFLOW_EVENT and Queue Copy in setBufferSize
SuGlider Mar 17, 2022
8e83060
Merge branch 'usb-cdc' of https://github.com/suglider/arduino-esp32 i…
SuGlider Mar 17, 2022
d993809
changed event name to ARDUINO_USB_CDC_RX_OVERFLOW_EVENT
SuGlider Mar 19, 2022
57d8e83
Merge branch 'master' into usb-cdc
SuGlider Mar 19, 2022
023b9ef
Merge branch 'master' into usb-cdc
SuGlider Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 27 additions & 9 deletions cores/esp32/USBCDC.cpp
View file
Open in desktop
Expand Up @@ -114,11 +114,14 @@ void USBCDC::onEvent(arduino_usb_cdc_event_t event, esp_event_handler_t callback
}

size_t USBCDC::setRxBufferSize(size_t rx_queue_len){
if(rx_queue){
if(!rx_queue_len){
size_t currentQueueSize = rx_queue ?
uxQueueSpacesAvailable(rx_queue) + uxQueueMessagesWaiting(rx_queue) : 0;

if (rx_queue && (!rx_queue_len || rx_queue_len != currentQueueSize)) {
vQueueDelete(rx_queue);
rx_queue = NULL;
}
}
if(!rx_queue_len || rx_queue_len == currentQueueSize){
return 0;
}
rx_queue = xQueueCreate(rx_queue_len, sizeof(uint8_t));
Expand All @@ -133,7 +136,8 @@ void USBCDC::begin(unsigned long baud)
if(tx_lock == NULL) {
tx_lock = xSemaphoreCreateMutex();
}
setRxBufferSize(256);//default if not preset
// if rx_queue was set before begin(), keep it
if (!rx_queue) setRxBufferSize(256); //default if not preset
Copy link
Member

Choose a reason for hiding this comment

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

The point is this function to only be called before begin and never after. If you really want to make it changeable, you need to make sure that the old queue is either empty or you copy the data from it to the new one before deletion.

Copy link
Collaborator Author

@SuGlider SuGlider Mar 14, 2022 •
edited

Choose a reason for hiding this comment

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

OK, I agree that in this case it shall copy data from the old queue to the new one. I'll implement it this way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

devices[itf] = this;
}

Expand All @@ -144,6 +148,7 @@ void USBCDC::end()
setRxBufferSize(0);
if(tx_lock != NULL) {
vSemaphoreDelete(tx_lock);
tx_lock = NULL;
}
}

Expand Down Expand Up @@ -246,14 +251,27 @@ void USBCDC::_onLineCoding(uint32_t _bit_rate, uint8_t _stop_bits, uint8_t _pari
void USBCDC::_onRX(){
uint8_t buf[CONFIG_TINYUSB_CDC_RX_BUFSIZE+1];
uint32_t count = tud_cdc_n_read(itf, buf, CONFIG_TINYUSB_CDC_RX_BUFSIZE);

if(rx_queue == NULL) {
return;
}
if (uxQueueSpacesAvailable(rx_queue) < count) {
//this VTaskDelay gives, to Arduino's task, time to the CPU do its processing
//without it, data may be lost when the number of bytes received is higher than CDC buffer size
vTaskDelay(10);
}
for(uint32_t i=0; i<count; i++){
if(rx_queue == NULL || !xQueueSend(rx_queue, buf+i, 0)){
Copy link
Member

Choose a reason for hiding this comment

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

just changing the timeout here from 0 to something else would "fix" this issue. No need for the other changes (and no need for set delay). Can't stop stressing enough that you should use Arduino API when possible, not vTaskDelay

Copy link
Collaborator Author

@SuGlider SuGlider Mar 14, 2022 •
edited

Choose a reason for hiding this comment

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

Actually, just doing it doesn't fix it because, data may be partially copied from TUD CDC and it will return when the Arduino CDC queue is full, not sending Arduino USB CDC event to the application and dropping data.
It's actually a problem of Race Condition between USB TUD Driver Task (usb_device_task) and the Arduino main Task / user Tasks.

Copy link
Collaborator Author

@SuGlider SuGlider Mar 14, 2022 •
edited

Choose a reason for hiding this comment

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

Regarding using Arduino delay() or FreeRTOS vTaskDelay(), I think that in this case, it is not about delaying the execution, but actually RTOS Scheduler giving the control to a lower priority Task (Arduino Main Task) in order to allow the consumer Task to do its job.

In the case of this issue, I think it is a lot clearer to use vTaskDelay() instead.
The real issue here is about a sort of Race Condition to the queues and Arduino Task Starvation when a lot of data comes from USB CDC.

For example, executing taskYIELD() doesn't solve the issue because of the very high priority of usb_device_task.
It must pause for a number of Ticks and allow Arduino Task to take place - considering it may consume USB CDC queue, otherwise it will drop data when queue is full. Not sure if it should also flush data in order to keep receiving data and end up with the latest send data in its queue.

Please let me know your thoughts on it.

Copy link
Member

Choose a reason for hiding this comment

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

Are we talking about the same things? Doesn't the code below do exactly what we need?

void USBCDC::_onRX(){
    uint8_t buf[CONFIG_TINYUSB_CDC_RX_BUFSIZE+1];
    uint32_t count = tud_cdc_n_read(itf, buf, CONFIG_TINYUSB_CDC_RX_BUFSIZE);
    for(uint32_t i=0; i<count; i++){
        if(rx_queue == NULL || !xQueueSend(rx_queue, buf+i, 10)){
            count = i;
            break;
        }
    }
    if (count) {
        arduino_usb_cdc_event_data_t p;
        p.rx.len = count;
        arduino_usb_event_post(ARDUINO_USB_CDC_EVENTS, ARDUINO_USB_CDC_RX_EVENT, &p, sizeof(arduino_usb_cdc_event_data_t), portMAX_DELAY);
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

re delay(): This is Arduino code. We use our APIs when we have them. It calls vTaskDelay in the background also. Calls, implementation, anything can change, but delay() will always delay. Please use Arduino APIs when such are available.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth adding a new event to tell how many bytes were missed?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least log_e here that something went wrong and bytes were dropped. It could help us in the future as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have created an event ARDUINO_USB_CDC_OVERFLOW that will tell in rx_overflow.dropped_bytes the number of bytes that were lost. Should I also add a log_e or with the event would be enough?
I also added that event to the Library examples.

Copy link
Member

Choose a reason for hiding this comment

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

log_e is always good for us. Will print even if the user is not listening to events

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All Done - including copying queue data in setRxBufferSize()

return;
if(!xQueueSend(rx_queue, buf+i, 0)){
// rx_queue overflow - data will be lost
count = i;
break;
}
}
arduino_usb_cdc_event_data_t p;
p.rx.len = count;
arduino_usb_event_post(ARDUINO_USB_CDC_EVENTS, ARDUINO_USB_CDC_RX_EVENT, &p, sizeof(arduino_usb_cdc_event_data_t), portMAX_DELAY);
if (count) {
SuGlider marked this conversation as resolved.
Show resolved Hide resolved
arduino_usb_cdc_event_data_t p;
p.rx.len = count;
arduino_usb_event_post(ARDUINO_USB_CDC_EVENTS, ARDUINO_USB_CDC_RX_EVENT, &p, sizeof(arduino_usb_cdc_event_data_t), portMAX_DELAY);
}
}

void USBCDC::_onTX(){
Expand Down