fix issue that out_xfer unintentionally returned by previous transmission (IEC-121) #40
Conversation
github-actions
bot
changed the title
|
Thank you @radiotommy for the PR, I'll have a look ASAP |
|
@tore-espressif |
Yes, we got a regression few days ago that should not be caused by your changes. We had a slightly different proposal to fix this issue. Let me check with my colleagues and I'll get back to you |
| uint32_t start = xTaskGetTickCount(); | ||
| uint32_t timeout_ticks = pdMS_TO_TICKS(timeout_ms); | ||
|
|
||
| while (cdc_dev->data.out_xfer->actual_num_bytes < data_len) { |
There was a problem hiding this comment.
Could you please explain the reason for this loop?
I can see two cases:
- The transfer finishes with
actual_num_bytes == data_len-> all OK, transfer finished. - The transfer finishes with
actual_num_bytes < data_len-> not OK. In this case we try to take the semaphore again, but we will never get it because we did not resubmit the transfer. So we end up withESP_ERR_TIMEOUT, same as before.
Could you please describe the bug (or provide a link to a GH issue) that you are trying to fix? It could help me understand better.
There was a problem hiding this comment.
for timeout or any reason the semaphore could left untaken before the new transmission start
To workaround this issue, we could
xSemaphoreTake((SemaphoreHandle_t)cdc_dev->data.out_xfer->context, 0);before we submit the transfer. If we set timeout to 0, the function will immediately return and we will make sure that the semaphore is not given from previous transfer. Does that make sense?
There was a problem hiding this comment.
Even better solution is to
if (!taken) {
// Reset the endpoint
cdc_acm_reset_transfer_endpoint(cdc_dev->dev_hdl, cdc_dev->data.out_xfer);
xSemaphoreTake((SemaphoreHandle_t)cdc_dev->data.out_xfer->context, portMAX_DELAY);
ret = ESP_ERR_TIMEOUT;
goto unblock;Here we take the semaphore only if the transfer really timed-out
There was a problem hiding this comment.
Great idea. I will give it a test on my board this weekend.
|
Related to espressif/esp-protocols#514 |
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. Closes espressif/esp-protocols#514 Closes #40
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. Closes espressif/esp-protocols#514 Closes #40
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. Closes espressif/esp-protocols#514 Closes #40
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. Closes espressif/esp-protocols#514 Closes #40 Closes espressif/esp-idf#13797
for timeout or any reason the semaphore could left untaken before the new transmission start. it causing the transmission pick up the previous actual_num_bytes and return failure because of incorrect number of bytes transferred.