Fixes USB CDC setRxBufferSize(), begin(), _onRX() #6413
Conversation
SuGlider
added
Type: Bug 🐛
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| for(uint32_t i=0; i<count; i++){ | ||
| if(rx_queue == NULL || !xQueueSend(rx_queue, buf+i, 0)){ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe worth adding a new event to tell how many bytes were missed?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
log_e is always good for us. Will print even if the user is not listening to events
There was a problem hiding this comment.
All Done - including copying queue data in setRxBufferSize()
|
@me-no-dev - PR is ready for a final review. |
cores/esp32/USBCDC.h
Outdated
| @@ -33,6 +33,7 @@ typedef enum { | |||
| ARDUINO_USB_CDC_LINE_CODING_EVENT, | |||
| ARDUINO_USB_CDC_RX_EVENT, | |||
| ARDUINO_USB_CDC_TX_EVENT, | |||
| ARDUINO_USB_CDC_OVERFLOW, | |||
There was a problem hiding this comment.
let's name this ARDUINO_USB_CDC_RX_OVERFLOW_EVENT for clarity (and the option to add TX in the future)
|
@me-no-dev - this PR is ready, just waiting for your final review. |
Summary
this PR fixes these issues:
1-
USBCDC::setRxBufferSize()doesn't change RX Queue Size2-
USBCDC::begin()always force 256 byte as RX Queue Size3-
USBCDC::_onRX()drops data when RX Queue Size is full4-
USBCDC::_onRX()implies in Arduino Task starvation when receiving more bytes than RX Queue can handle5-
USBCDC::end()resets the board when executed twice in sequenceImpact
This PR allows
USBCDC::setRxBufferSize()to be called either, before or after,USBCDC::begin().It will work in any use case. Data from original RX queue will be copied to the new one, unless it is smaller than the necessary to store previously queued data - in such case an OVERFLOW event will be issued.
It adds a new
ARDUINO_USB_CDC_OVERFLOWevent that has as parameter the number of bytes lost because of the overflow. Documentation and examples have been updated to reflect this change.Related links
Fixes #6221
Fixes #5727