From 69d283b9ae22e495e030d06528c9d39071e0159e Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Wed, 23 Aug 2023 16:04:57 +0800 Subject: [PATCH 1/3] fix(usb_serial_jtag): Fix usb_serial_jtag wrong return value, vfs lose data randomly, Closes https://github.com/espressif/esp-idf/issues/12119, Closes https://github.com/espressif/esp-idf/pull/11344, Closes https://github.com/espressif/esp-idf/issues/9318 Closes https://github.com/espressif/esp-idf/issues/11192 --- components/driver/usb_serial_jtag/usb_serial_jtag.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/driver/usb_serial_jtag/usb_serial_jtag.c b/components/driver/usb_serial_jtag/usb_serial_jtag.c index e1f1354c2a..e254e0cfd9 100644 --- a/components/driver/usb_serial_jtag/usb_serial_jtag.c +++ b/components/driver/usb_serial_jtag/usb_serial_jtag.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2021-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -163,10 +163,10 @@ int usb_serial_jtag_write_bytes(const void* src, size_t size, TickType_t ticks_t const uint8_t *buff = (const uint8_t *)src; // Blocking method, Sending data to ringbuffer, and handle the data in ISR. - xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*) (buff), size, ticks_to_wait); + BaseType_t result = xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*) (buff), size, ticks_to_wait); // Now trigger the ISR to read data from the ring buffer. - usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); - return size; + usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); + return (result == pdFALSE) ? 0 : size; } esp_err_t usb_serial_jtag_driver_uninstall(void) From 0ed7093fb2f548ee6079ce0b56dab312586d75c0 Mon Sep 17 00:00:00 2001 From: Chip Weinberger Date: Mon, 8 May 2023 20:37:06 -0700 Subject: [PATCH 2/3] [Usb Serial JTAG] printing to console could sometimes skip bytes --- .../driver/usb_serial_jtag/usb_serial_jtag.c | 61 ++++++++++++++++--- 1 file changed, 51 insertions(+), 10 deletions(-) diff --git a/components/driver/usb_serial_jtag/usb_serial_jtag.c b/components/driver/usb_serial_jtag/usb_serial_jtag.c index e254e0cfd9..d256abbb2c 100644 --- a/components/driver/usb_serial_jtag/usb_serial_jtag.c +++ b/components/driver/usb_serial_jtag/usb_serial_jtag.c @@ -32,16 +32,19 @@ typedef struct{ // TX parameters uint32_t tx_buf_size; /*!< TX buffer size */ RingbufHandle_t tx_ring_buf; /*!< TX ring buffer handler */ + uint8_t tx_data_buf[USB_SER_JTAG_ENDP_SIZE]; /*!< Data buffer to stash TX FIFO data */ + size_t tx_stash_cnt; /*!< Number of stashed TX FIFO bytes */ } usb_serial_jtag_obj_t; static usb_serial_jtag_obj_t *p_usb_serial_jtag_obj = NULL; static const char* USB_SERIAL_JTAG_TAG = "usb_serial_jtag"; -static void usb_serial_jtag_write_and_flush(const uint8_t *buf, uint32_t wr_len) +static int usb_serial_jtag_write_and_flush(const uint8_t *buf, uint32_t wr_len) { - usb_serial_jtag_ll_write_txfifo(buf, wr_len); + int size = usb_serial_jtag_ll_write_txfifo(buf, wr_len); usb_serial_jtag_ll_txfifo_flush(); + return size; } static void usb_serial_jtag_isr_handler_default(void *arg) { @@ -50,19 +53,54 @@ static void usb_serial_jtag_isr_handler_default(void *arg) { usbjtag_intr_status = usb_serial_jtag_ll_get_intsts_mask(); if (usbjtag_intr_status & USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY) { - // Interrupt tells us the host picked up the data we sent. If we have more data, we can put it in the buffer and the host will pick that up next. + // Interrupt tells us the host picked up the data we sent. + // If we have more data, we can put it in the buffer and the host will pick that up next. // Send data in isr. + // If the hardware fifo is avaliable, write in it. Otherwise, do nothing. if (usb_serial_jtag_ll_txfifo_writable() == 1) { // We disable the interrupt here so that the interrupt won't be triggered if there is no data to send. usb_serial_jtag_ll_disable_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); size_t queued_size; - uint8_t *queued_buff = (uint8_t *)xRingbufferReceiveUpToFromISR(p_usb_serial_jtag_obj->tx_ring_buf, &queued_size, 64); - // If the hardware fifo is avaliable, write in it. Otherwise, do nothing. - if (queued_buff != NULL) { //Although tx_queued_bytes may be larger than 0. We may have interrupt before xRingbufferSend() was called. - //Copy the queued buffer into the TX FIFO - usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); - usb_serial_jtag_write_and_flush(queued_buff, queued_size); - vRingbufferReturnItemFromISR(p_usb_serial_jtag_obj->tx_ring_buf, queued_buff, &xTaskWoken); + uint8_t *queued_buff = NULL; + bool is_stashed_data = false; + if (p_usb_serial_jtag_obj->tx_stash_cnt != 0) { + // Send stashed tx bytes before reading bytes from ring buffer + queued_buff = p_usb_serial_jtag_obj->tx_data_buf; + queued_size = p_usb_serial_jtag_obj->tx_stash_cnt; + is_stashed_data = true; + } + else { + // Max 64 data payload size in a single EndPoint + queued_buff = (uint8_t *)xRingbufferReceiveUpToFromISR(p_usb_serial_jtag_obj->tx_ring_buf, &queued_size, USB_SER_JTAG_ENDP_SIZE); + } + + usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); + + if (queued_buff != NULL) { + + // Although tx_queued_bytes may be larger than 0, we may have + // interrupted before xRingbufferSend() was called. + // Copy the queued buffer into the TX FIFO + + // On ringbuffer wrap-around the size can be 0 even though the buffer returned is not NULL + if (queued_size > 0) { + uint32_t sent_size = usb_serial_jtag_write_and_flush(queued_buff, queued_size); + + if (sent_size < queued_size) { + // Not all bytes could be sent at once, stash the unwritten bytes in a tx buffer + size_t stash_size = MIN(USB_SER_JTAG_ENDP_SIZE, queued_size - sent_size); + + // Copy the missed bytes to tx stash buffer. May copy from stash buffer to itself + memcpy(p_usb_serial_jtag_obj->tx_data_buf, &queued_buff[sent_size], stash_size); + p_usb_serial_jtag_obj->tx_stash_cnt = stash_size; + } + else { + p_usb_serial_jtag_obj->tx_stash_cnt = 0; + } + } + if (is_stashed_data == false) { + vRingbufferReturnItemFromISR(p_usb_serial_jtag_obj->tx_ring_buf, queued_buff, &xTaskWoken); + } usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); } } else { @@ -93,6 +131,7 @@ esp_err_t usb_serial_jtag_driver_install(usb_serial_jtag_driver_config_t *usb_se p_usb_serial_jtag_obj = (usb_serial_jtag_obj_t*) heap_caps_calloc(1, sizeof(usb_serial_jtag_obj_t), MALLOC_CAP_INTERNAL|MALLOC_CAP_8BIT); p_usb_serial_jtag_obj->rx_buf_size = usb_serial_jtag_config->rx_buffer_size; p_usb_serial_jtag_obj->tx_buf_size = usb_serial_jtag_config->tx_buffer_size; + p_usb_serial_jtag_obj->tx_stash_cnt = 0; if (p_usb_serial_jtag_obj == NULL) { ESP_LOGE(USB_SERIAL_JTAG_TAG, "memory allocate error"); err = ESP_ERR_NO_MEM; @@ -161,6 +200,8 @@ int usb_serial_jtag_write_bytes(const void* src, size_t size, TickType_t ticks_t ESP_RETURN_ON_FALSE(src != NULL, ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "Invalid buffer pointer."); ESP_RETURN_ON_FALSE(p_usb_serial_jtag_obj != NULL, ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "The driver hasn't been initialized"); + int ret_size = 0; + const uint8_t *buff = (const uint8_t *)src; // Blocking method, Sending data to ringbuffer, and handle the data in ISR. BaseType_t result = xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*) (buff), size, ticks_to_wait); From 518212a8f664beb475ebbbb9882637dbdce68348 Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Mon, 18 Sep 2023 19:28:09 +0800 Subject: [PATCH 3/3] fix(usb_serial_jtag): Clean-up usb_serial_jtag lose byte fix, Closes https://github.com/espressif/esp-idf/pull/11344 --- .../driver/usb_serial_jtag/usb_serial_jtag.c | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/components/driver/usb_serial_jtag/usb_serial_jtag.c b/components/driver/usb_serial_jtag/usb_serial_jtag.c index d256abbb2c..b39ed39839 100644 --- a/components/driver/usb_serial_jtag/usb_serial_jtag.c +++ b/components/driver/usb_serial_jtag/usb_serial_jtag.c @@ -27,7 +27,7 @@ typedef struct{ // RX parameters RingbufHandle_t rx_ring_buf; /*!< RX ring buffer handler */ uint32_t rx_buf_size; /*!< TX buffer size */ - uint8_t rx_data_buf[64]; /*!< Data buffer to stash FIFO data */ + uint8_t rx_data_buf[USB_SER_JTAG_ENDP_SIZE]; /*!< Data buffer to stash FIFO data */ // TX parameters uint32_t tx_buf_size; /*!< TX buffer size */ @@ -40,9 +40,9 @@ static usb_serial_jtag_obj_t *p_usb_serial_jtag_obj = NULL; static const char* USB_SERIAL_JTAG_TAG = "usb_serial_jtag"; -static int usb_serial_jtag_write_and_flush(const uint8_t *buf, uint32_t wr_len) +static size_t usb_serial_jtag_write_and_flush(const uint8_t *buf, uint32_t wr_len) { - int size = usb_serial_jtag_ll_write_txfifo(buf, wr_len); + size_t size = usb_serial_jtag_ll_write_txfifo(buf, wr_len); usb_serial_jtag_ll_txfifo_flush(); return size; } @@ -56,7 +56,7 @@ static void usb_serial_jtag_isr_handler_default(void *arg) { // Interrupt tells us the host picked up the data we sent. // If we have more data, we can put it in the buffer and the host will pick that up next. // Send data in isr. - // If the hardware fifo is avaliable, write in it. Otherwise, do nothing. + // If the hardware fifo is available, write in it. Otherwise, do nothing. if (usb_serial_jtag_ll_txfifo_writable() == 1) { // We disable the interrupt here so that the interrupt won't be triggered if there is no data to send. usb_serial_jtag_ll_disable_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); @@ -68,8 +68,7 @@ static void usb_serial_jtag_isr_handler_default(void *arg) { queued_buff = p_usb_serial_jtag_obj->tx_data_buf; queued_size = p_usb_serial_jtag_obj->tx_stash_cnt; is_stashed_data = true; - } - else { + } else { // Max 64 data payload size in a single EndPoint queued_buff = (uint8_t *)xRingbufferReceiveUpToFromISR(p_usb_serial_jtag_obj->tx_ring_buf, &queued_size, USB_SER_JTAG_ENDP_SIZE); } @@ -88,14 +87,14 @@ static void usb_serial_jtag_isr_handler_default(void *arg) { if (sent_size < queued_size) { // Not all bytes could be sent at once, stash the unwritten bytes in a tx buffer - size_t stash_size = MIN(USB_SER_JTAG_ENDP_SIZE, queued_size - sent_size); - - // Copy the missed bytes to tx stash buffer. May copy from stash buffer to itself + // stash_size will not larger than USB_SER_JTAG_ENDP_SIZE because queued_size is got from xRingbufferReceiveUpToFromISR + size_t stash_size = queued_size - sent_size; memcpy(p_usb_serial_jtag_obj->tx_data_buf, &queued_buff[sent_size], stash_size); p_usb_serial_jtag_obj->tx_stash_cnt = stash_size; - } - else { + } else { p_usb_serial_jtag_obj->tx_stash_cnt = 0; + // assert if sent_size is larger than queued_size. + assert(sent_size <= queued_size); } } if (is_stashed_data == false) { @@ -200,8 +199,6 @@ int usb_serial_jtag_write_bytes(const void* src, size_t size, TickType_t ticks_t ESP_RETURN_ON_FALSE(src != NULL, ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "Invalid buffer pointer."); ESP_RETURN_ON_FALSE(p_usb_serial_jtag_obj != NULL, ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "The driver hasn't been initialized"); - int ret_size = 0; - const uint8_t *buff = (const uint8_t *)src; // Blocking method, Sending data to ringbuffer, and handle the data in ISR. BaseType_t result = xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*) (buff), size, ticks_to_wait);