From 1d1896d4093cb75acb22bf69692e25565b85fdd5 Mon Sep 17 00:00:00 2001 From: Andrei Gramakov Date: Fri, 6 Nov 2020 13:45:42 +0100 Subject: [PATCH] tinyusb: Add fix for tinyusb reading Closes IDF-2029 --- .../tinyusb/additions/include/tinyusb.h | 6 +- .../tinyusb/additions/include/tusb_cdc_acm.h | 7 +- .../tinyusb/additions/src/tusb_cdc_acm.c | 75 ++++++++++++++++--- .../tinyusb/additions/src/vfs_tinyusb.c | 2 +- components/tinyusb/tinyusb | 2 +- .../main/tusb_serial_device_main.c | 2 +- 6 files changed, 76 insertions(+), 18 deletions(-) diff --git a/components/tinyusb/additions/include/tinyusb.h b/components/tinyusb/additions/include/tinyusb.h index 8a31086b3b..6704becf7d 100644 --- a/components/tinyusb/additions/include/tinyusb.h +++ b/components/tinyusb/additions/include/tinyusb.h @@ -42,9 +42,9 @@ extern "C" { # endif # if CFG_TUD_CDC -# if (CFG_TUD_CDC_EPSIZE < 4) -# define CFG_TUD_CDC_EPSIZE 4 -# warning "CFG_TUD_CDC_EPSIZE was too low and was set to 4" +# if (CFG_TUD_CDC_EP_BUFSIZE < 4) +# define CFG_TUD_CDC_EP_BUFSIZE 4 +# warning "CFG_TUD_CDC_EP_BUFSIZE was too low and was set to 4" # endif # endif diff --git a/components/tinyusb/additions/include/tusb_cdc_acm.h b/components/tinyusb/additions/include/tusb_cdc_acm.h index b5f5801951..af7985e6c4 100644 --- a/components/tinyusb/additions/include/tusb_cdc_acm.h +++ b/components/tinyusb/additions/include/tusb_cdc_acm.h @@ -160,7 +160,12 @@ size_t tinyusb_cdcacm_write_queue_char(tinyusb_cdcacm_itf_t itf, char ch); size_t tinyusb_cdcacm_write_queue(tinyusb_cdcacm_itf_t itf, uint8_t *in_buf, size_t in_size); /** - * @brief Send all data from a write buffer. Use `tinyusb_cdcacm_write_queue` to add data to the buffer + * @brief Send all data from a write buffer. Use `tinyusb_cdcacm_write_queue` to add data to the buffer. + * + * WARNING! TinyUSB can block output Endpoint for several RX callbacks, after will do additional flush + * after the each trasfer. That can leads to the situation when you requested a flush, but it will fail until + * ont of the next callbacks ends. + * SO USING OF THE FLUSH WITH TIMEOUTS IN CALLBACKS IS NOT RECOMENDED - YOU CAN GET A LOCK FOR THE TIMEOUT * * @param itf - number of a CDC object * @param timeout_ticks - waiting until flush will be considered as failed diff --git a/components/tinyusb/additions/src/tusb_cdc_acm.c b/components/tinyusb/additions/src/tusb_cdc_acm.c index 97e2a2abaf..f9f728a414 100644 --- a/components/tinyusb/additions/src/tusb_cdc_acm.c +++ b/components/tinyusb/additions/src/tusb_cdc_acm.c @@ -31,6 +31,7 @@ typedef struct { bool initialized; size_t rx_unread_buf_sz; RingbufHandle_t rx_unread_buf; + xSemaphoreHandle ringbuf_read_mux; uint8_t *rx_tfbuf; tusb_cdcacm_callback_t callback_rx; tusb_cdcacm_callback_t callback_rx_wanted_char; @@ -226,6 +227,35 @@ esp_err_t tinyusb_cdcacm_unregister_callback(tinyusb_cdcacm_itf_t itf, /*********************************************************************** TinyUSB callbacks*/ /* CDC-ACM ********************************************************************* */ + +static esp_err_t read_from_rx_unread_to_buffer(esp_tusb_cdcacm_t *acm, uint8_t *out_buf, size_t req_bytes, size_t *read_bytes) +{ + uint8_t *buf = xRingbufferReceiveUpTo(acm->rx_unread_buf, read_bytes, 0, req_bytes); + if (buf) { + memcpy(out_buf, buf, *read_bytes); + vRingbufferReturnItem(acm->rx_unread_buf, (void *)(buf)); + return ESP_OK; + } else { + return ESP_ERR_NO_MEM; + } +} + +static esp_err_t ringbuf_mux_take(esp_tusb_cdcacm_t *acm){ + if(xSemaphoreTake(acm->ringbuf_read_mux, 0) != pdTRUE){ + ESP_LOGW(TAG, "Read error: ACM is busy"); + return ESP_ERR_INVALID_STATE; + } + return ESP_OK; +} + +static esp_err_t ringbuf_mux_give(esp_tusb_cdcacm_t *acm){ + if(xSemaphoreGive(acm->ringbuf_read_mux) != pdTRUE){ + ESP_LOGW(TAG, "Can't return the ringbuff mutex: mutex hasn't been taken"); + return ESP_ERR_INVALID_STATE; + } + return ESP_OK; +} + esp_err_t tinyusb_cdcacm_read(tinyusb_cdcacm_itf_t itf, uint8_t *out_buf, size_t out_buf_sz, size_t *rx_data_size) { esp_tusb_cdcacm_t *acm = get_acm(itf); @@ -233,15 +263,26 @@ esp_err_t tinyusb_cdcacm_read(tinyusb_cdcacm_itf_t itf, uint8_t *out_buf, size_t ESP_LOGE(TAG, "Interface is not initialized. Use `tinyusb_cdc_init` for initialization"); return ESP_ERR_INVALID_STATE; } - uint8_t *buf = xRingbufferReceiveUpTo(acm->rx_unread_buf, rx_data_size, 0, out_buf_sz); - if (buf) { - memcpy(out_buf, buf, *rx_data_size); - vRingbufferReturnItem(acm->rx_unread_buf, (void *)buf); - return ESP_OK; - } else { - ESP_LOGE(TAG, "Failed to receive item"); - return ESP_ERR_NO_MEM; + size_t read_sz; + + /* Take a mutex to proceed two uninterrupted read operations */ + ESP_RETURN_ON_ERROR(ringbuf_mux_take(acm)); + + esp_err_t res = read_from_rx_unread_to_buffer(acm, out_buf, out_buf_sz, &read_sz); + if (res != ESP_OK){ + ESP_RETURN_ON_ERROR(ringbuf_mux_give(acm)); + return res; } + + *rx_data_size = read_sz; + /* Buffer's data can be wrapped, at that situations we should make another retrievement */ + if (read_from_rx_unread_to_buffer(acm, out_buf+read_sz, out_buf_sz-read_sz, &read_sz) == ESP_OK) { + ESP_LOGV(TAG, "Buffer was wrapped, data obtained using two read operations."); + *rx_data_size += read_sz; + } + + ESP_RETURN_ON_ERROR(ringbuf_mux_give(acm)); + return ESP_OK; } @@ -262,7 +303,9 @@ size_t tinyusb_cdcacm_write_queue(tinyusb_cdcacm_itf_t itf, uint8_t *in_buf, siz return tud_cdc_n_write(itf, in_buf, in_size); } - +static uint32_t tud_cdc_n_write_occupied(tinyusb_cdcacm_itf_t itf){ + return CFG_TUD_CDC_TX_BUFSIZE - tud_cdc_n_write_available(itf); +} esp_err_t tinyusb_cdcacm_write_flush(tinyusb_cdcacm_itf_t itf, uint32_t timeout_ticks) { @@ -273,9 +316,11 @@ esp_err_t tinyusb_cdcacm_write_flush(tinyusb_cdcacm_itf_t itf, uint32_t timeout_ if (!timeout_ticks) { // if no timeout - nonblocking mode int res = tud_cdc_n_write_flush(itf); if (!res) { + ESP_LOGW(TAG, "flush fauled (res: %d)", res); return ESP_FAIL; } else { - if (tud_cdc_n_write_available(itf)) { + if (tud_cdc_n_write_occupied(itf)) { + ESP_LOGW(TAG, "remained data to flush!"); return ESP_FAIL; } } @@ -285,7 +330,7 @@ esp_err_t tinyusb_cdcacm_write_flush(tinyusb_cdcacm_itf_t itf, uint32_t timeout_ uint32_t ticks_now = ticks_start; while (1) { // loop until success or until the time runs out ticks_now = xTaskGetTickCount(); - if (!tud_cdc_n_write_available(itf)) { // if nothing to write - nothing to flush + if (!tud_cdc_n_write_occupied(itf)) { // if nothing to write - nothing to flush break; } if (tud_cdc_n_write_flush(itf)) { // Success @@ -347,6 +392,14 @@ esp_err_t tusb_cdc_acm_init(const tinyusb_config_cdcacm_t *cfg) } /* Buffers */ + + acm->ringbuf_read_mux = xSemaphoreCreateMutex(); + if (acm->ringbuf_read_mux == NULL){ + ESP_LOGE(TAG, "Creation of a ringbuf mutex failed"); + free_obj(itf); + return ESP_ERR_NO_MEM; + } + acm->rx_tfbuf = malloc(CONFIG_USB_CDC_RX_BUFSIZE); if (!acm->rx_tfbuf) { ESP_LOGE(TAG, "Creation buffer error"); diff --git a/components/tinyusb/additions/src/vfs_tinyusb.c b/components/tinyusb/additions/src/vfs_tinyusb.c index 2348405554..0349bb8696 100644 --- a/components/tinyusb/additions/src/vfs_tinyusb.c +++ b/components/tinyusb/additions/src/vfs_tinyusb.c @@ -153,7 +153,7 @@ static ssize_t tusb_write(int fd, const void *data, size_t size) } } - tinyusb_cdcacm_write_flush(s_vfstusb.cdc_intf, 0); + tud_cdc_n_write_flush(s_vfstusb.cdc_intf); _lock_release(&(s_vfstusb.write_lock)); return written_sz; } diff --git a/components/tinyusb/tinyusb b/components/tinyusb/tinyusb index a2ba3dcccc..6d66d6bb21 160000 --- a/components/tinyusb/tinyusb +++ b/components/tinyusb/tinyusb @@ -1 +1 @@ -Subproject commit a2ba3dccccf94022d31e939fa2ce4dca5f0a34f0 +Subproject commit 6d66d6bb21f35aec8f7d00857862d3d025df80de diff --git a/examples/peripherals/usb/tusb_serial_device/main/tusb_serial_device_main.c b/examples/peripherals/usb/tusb_serial_device/main/tusb_serial_device_main.c index 8d5652b404..8e1d74e3b8 100644 --- a/examples/peripherals/usb/tusb_serial_device/main/tusb_serial_device_main.c +++ b/examples/peripherals/usb/tusb_serial_device/main/tusb_serial_device_main.c @@ -38,7 +38,7 @@ void tinyusb_cdc_rx_callback(int itf, cdcacm_event_t *event) /* write back */ tinyusb_cdcacm_write_queue(itf, buf, rx_size); - tinyusb_cdcacm_write_flush(itf, portMAX_DELAY); + tinyusb_cdcacm_write_flush(itf, 0); } void tinyusb_cdc_line_state_changed_callback(int itf, cdcacm_event_t *event)