diff --git a/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag.c b/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag.c index f7623139dd..b69b82b3af 100644 --- a/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag.c +++ b/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag.c @@ -19,35 +19,31 @@ #include "esp_check.h" #include "esp_private/periph_ctrl.h" +/* + Note: Before you add a workaround for an issue in this driver, please please try + to figure out the actual root cause first. The USB-serial-JTAG is a simple device, + it shouldn't need anything more than a simple, straightforward driver. +*/ + #if !SOC_RCC_IS_INDEPENDENT #define USJ_RCC_ATOMIC() PERIPH_RCC_ATOMIC() #else #define USJ_RCC_ATOMIC() #endif -typedef enum { - FIFO_IDLE = 0, /*!< Indicates the fifo is in idle state */ - FIFO_BUSY = 1, /*!< Indicates the fifo is in busy state */ -} fifo_status_t; - -// The hardware buffer max size is 64 +// The hardware buffer max size is 64, both for RX and TX. #define USB_SER_JTAG_ENDP_SIZE (64) -#define USB_SER_JTAG_RX_MAX_SIZE (64) +#define USB_SER_JTAG_RX_MAX_SIZE (USB_SER_JTAG_ENDP_SIZE) typedef struct { intr_handle_t intr_handle; /*!< USB-SERIAL-JTAG interrupt handler */ - portMUX_TYPE spinlock; /*!< Spinlock for usb_serial_jtag */ - _Atomic fifo_status_t fifo_status; /*!< Record the status of fifo */ // RX parameters RingbufHandle_t rx_ring_buf; /*!< RX ring buffer handler */ - uint32_t rx_buf_size; /*!< TX buffer size */ - 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 */ 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 */ + uint8_t tx_stash_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; @@ -55,13 +51,6 @@ static usb_serial_jtag_obj_t *p_usb_serial_jtag_obj = NULL; static const char* USB_SERIAL_JTAG_TAG = "usb_serial_jtag"; -static size_t usb_serial_jtag_write_and_flush(const uint8_t *buf, uint32_t wr_len) -{ - size_t 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) { BaseType_t xTaskWoken = 0; @@ -69,77 +58,70 @@ 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) { + //Clear interrupt so we won't be called until the next transfer finishes. + usb_serial_jtag_ll_clr_intsts_mask(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. - // Send data in isr. - // If the hardware fifo is available, write in it. Otherwise, do nothing. + // We expect the TX FIFO to be writable for this. If it's not, somehow someone else + // (ROM print routines?) have snuck in a full buffer before we got here. In that case, + // we simply ignore the interrupt, a new one will come if the buffer is empty again. 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. - + // Retrieve data from either the stash buffer or, if that's empty, from the ring buffer. size_t queued_size; - uint8_t *queued_buff = NULL; - bool is_stashed_data = false; + uint8_t *queued_buf = NULL; 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_buf = p_usb_serial_jtag_obj->tx_stash_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); + queued_buf = (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_buf != NULL && queued_size > 0) { + // We have some data to send. Send it. + uint32_t sent_size = usb_serial_jtag_ll_write_txfifo(queued_buf, queued_size); + usb_serial_jtag_ll_txfifo_flush(); - 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) { - portENTER_CRITICAL_ISR(&p_usb_serial_jtag_obj->spinlock); - atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_BUSY); - uint32_t sent_size = usb_serial_jtag_write_and_flush(queued_buff, queued_size); - portEXIT_CRITICAL_ISR(&p_usb_serial_jtag_obj->spinlock); - - if (sent_size < queued_size) { - // Not all bytes could be sent at once; stash the unwritten bytes in a tx buffer - // 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 { - p_usb_serial_jtag_obj->tx_stash_cnt = 0; - // assert if sent_size is larger than queued_size. - assert(sent_size <= queued_size); - } + // Check if we were able to send everything. + if (sent_size < queued_size) { + // Not all bytes could be sent at once; stash the unwritten bytes in a buffer + // This will happen if e.g. the rom output functions manage to sneak a few bytes into the + // TX FIFO before this interrupt triggers. Note stash_size will not larger than + // USB_SER_JTAG_ENDP_SIZE because queued_size is obtained from xRingbufferReceiveUpToFromISR. + size_t stash_size = queued_size - sent_size; + memcpy(p_usb_serial_jtag_obj->tx_stash_buf, &queued_buf[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); + // Return the buffer if we got it from the ring buffer. + if (queued_buf != p_usb_serial_jtag_obj->tx_stash_buf) { + vRingbufferReturnItemFromISR(p_usb_serial_jtag_obj->tx_ring_buf, queued_buf, &xTaskWoken); } } else { + // No data to send. // The last transmit may have sent a full EP worth of data. The host will interpret // this as a transaction that hasn't finished yet and keep the data in its internal // buffers rather than releasing it to the program listening on the CDC serial port. // We need to flush again in order to send a 0-byte packet that ends the transaction. usb_serial_jtag_ll_txfifo_flush(); - // Note that since this doesn't re-enable USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY, the - // flush will not by itself cause this ISR to be called again. + + // We will also disable the interrupt as for now there's no need to handle the + // TX interrupt again. We'll re-enable this externally if we need data sent. + usb_serial_jtag_ll_disable_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); } - } else { - atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_IDLE); - usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); } } if (usbjtag_intr_status & USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT) { - // read rx buffer(max length is 64), and send available data to ringbuffer. - // Ensure the rx buffer size is larger than RX_MAX_SIZE. + // Acknowledge interrupt usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT); - uint32_t rx_fifo_len = usb_serial_jtag_ll_read_rxfifo(p_usb_serial_jtag_obj->rx_data_buf, USB_SER_JTAG_RX_MAX_SIZE); - xRingbufferSendFromISR(p_usb_serial_jtag_obj->rx_ring_buf, p_usb_serial_jtag_obj->rx_data_buf, rx_fifo_len, &xTaskWoken); + // Read RX FIFO and send available data to ringbuffer. + uint8_t buf[USB_SER_JTAG_RX_MAX_SIZE]; + uint32_t rx_fifo_len = usb_serial_jtag_ll_read_rxfifo(buf, USB_SER_JTAG_RX_MAX_SIZE); + xRingbufferSendFromISR(p_usb_serial_jtag_obj->rx_ring_buf, buf, rx_fifo_len, &xTaskWoken); } if (xTaskWoken == pdTRUE) { @@ -157,15 +139,13 @@ 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); if (p_usb_serial_jtag_obj == NULL) { ESP_LOGE(USB_SERIAL_JTAG_TAG, "memory allocate error"); - err = ESP_ERR_NO_MEM; - goto _exit; + // no `goto _exit` here as there's nothing to clean up and that would make the uninstall + // routine unhappy. + return ESP_ERR_NO_MEM; } - 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; - p_usb_serial_jtag_obj->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED; - p_usb_serial_jtag_obj->rx_ring_buf = xRingbufferCreate(p_usb_serial_jtag_obj->rx_buf_size, RINGBUF_TYPE_BYTEBUF); + p_usb_serial_jtag_obj->rx_ring_buf = xRingbufferCreate(usb_serial_jtag_config->rx_buffer_size, RINGBUF_TYPE_BYTEBUF); if (p_usb_serial_jtag_obj->rx_ring_buf == NULL) { ESP_LOGE(USB_SERIAL_JTAG_TAG, "ringbuffer create error"); err = ESP_ERR_NO_MEM; @@ -183,7 +163,6 @@ esp_err_t usb_serial_jtag_driver_install(usb_serial_jtag_driver_config_t *usb_se USJ_RCC_ATOMIC() { usb_serial_jtag_ll_enable_bus_clock(true); } - atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_IDLE); // Configure PHY #if USB_SERIAL_JTAG_LL_EXT_PHY_SUPPORTED @@ -193,10 +172,14 @@ esp_err_t usb_serial_jtag_driver_install(usb_serial_jtag_driver_config_t *usb_se usb_serial_jtag_ll_phy_set_defaults(); // External PHY not supported. Set default values. #endif // USB_WRAP_LL_EXT_PHY_SUPPORTED - usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY | - USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT); - usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY | - USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT); + // Note: DO NOT clear the interrupt status bits here. The output routine needs + // USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY set because it needs the ISR to trigger + // as soon as data is sent; the input routine needs the status to retrieve any + // data that is still in the FIFOs. + + // We only enable the RX interrupt; we'll enable the TX one when we actually + // have anything to send. + usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT); err = esp_intr_alloc(ETS_USB_SERIAL_JTAG_INTR_SOURCE, 0, usb_serial_jtag_isr_handler_default, NULL, &p_usb_serial_jtag_obj->intr_handle); if (err != ESP_OK) { @@ -238,37 +221,29 @@ 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"); - size_t sent_data = 0; BaseType_t result = pdTRUE; - const uint8_t *buff = (const uint8_t *)src; - if (p_usb_serial_jtag_obj->fifo_status == FIFO_IDLE) { - portENTER_CRITICAL(&p_usb_serial_jtag_obj->spinlock); - atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_BUSY); - sent_data = usb_serial_jtag_write_and_flush(src, size); - portEXIT_CRITICAL(&p_usb_serial_jtag_obj->spinlock); - } - - // Blocking method, Sending data to ringbuffer, and handle the data in ISR. - if (size - sent_data > 0) { - result = xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*)(buff + sent_data), size - sent_data, ticks_to_wait); - } else { - atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_IDLE); - } + result = xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*)src, size, ticks_to_wait); + // Re-enable the TX interrupt. If this was disabled, this will immediately trigger the ISR + // and send the things we just put in the ringbuffer. usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY); return (result == pdFALSE) ? 0 : size; } +//Note that this is also called when usb_serial_jtag_driver_install errors out and as such should +//work on a half-initialized driver as well. esp_err_t usb_serial_jtag_driver_uninstall(void) { if (p_usb_serial_jtag_obj == NULL) { - ESP_LOGI(USB_SERIAL_JTAG_TAG, "ALREADY NULL"); + ESP_LOGE(USB_SERIAL_JTAG_TAG, "uninstall without install called"); return ESP_OK; } - /* Not disable the module clock and usb_pad_enable here since the USJ stdout might still depends on it. */ - //Disable tx/rx interrupt. + /* Don't disable the module clock and usb_pad_enable here since the USJ stdout might still depends on it. */ + usb_serial_jtag_ll_disable_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY | USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT); - esp_intr_free(p_usb_serial_jtag_obj->intr_handle); + if (p_usb_serial_jtag_obj->intr_handle) { + esp_intr_free(p_usb_serial_jtag_obj->intr_handle); + } if (p_usb_serial_jtag_obj->rx_ring_buf) { vRingbufferDelete(p_usb_serial_jtag_obj->rx_ring_buf); diff --git a/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag_vfs.c b/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag_vfs.c index 9446a6d24e..ea400bbb65 100644 --- a/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag_vfs.c +++ b/components/esp_driver_usb_serial_jtag/src/usb_serial_jtag_vfs.c @@ -121,6 +121,11 @@ static void usb_serial_jtag_tx_char(int fd, int c) do { if (usb_serial_jtag_ll_txfifo_writable()) { usb_serial_jtag_ll_write_txfifo(&cc, 1); + if (c == '\n') { + //Make sure line doesn't linger in fifo + usb_serial_jtag_ll_txfifo_flush(); + } + //update time of last successful tx to now. s_ctx.last_tx_ts = esp_timer_get_time(); break; } @@ -155,10 +160,6 @@ static ssize_t usb_serial_jtag_write(int fd, const void * data, size_t size) } } s_ctx.tx_func(fd, c); - if (c == '\n') { - //Make sure line doesn't linger in fifo - usb_serial_jtag_ll_txfifo_flush(); - } } _lock_release_recursive(&s_ctx.write_lock); return size;