[HWCDC] Improve HW CDC Implementation (#5643)

me-no-dev · web-flow · commit 078671d273cc · 2021-09-15T19:37:09.000+03:00
This pull request contains a few fixes and improvements to the HWCDC implementation.
- Rework `HWCDC::write()` to accept unlimited data
- Add Semaphore to guard the TX Ring Buffer
- Add events support
- Remove unnecessary 1200bps touch for flashing over HWCDC
- Fix `HardwareSerial::setDebugOutput()` not resetting `putc` if the port is already selected, causing debug output to also show on HWCDC even when not selected.
diff --git a/boards.txt b/boards.txt
@@ -57,13 +57,6 @@ esp32c3.menu.CDCOnBoot.default.build.cdc_on_boot=0
 esp32c3.menu.CDCOnBoot.cdc=Enabled
 esp32c3.menu.CDCOnBoot.cdc.build.cdc_on_boot=1
 
-esp32c3.menu.UploadMode.default=UART0
-esp32c3.menu.UploadMode.default.upload.use_1200bps_touch=false
-esp32c3.menu.UploadMode.default.upload.wait_for_upload_port=false
-esp32c3.menu.UploadMode.cdc=Internal USB
-esp32c3.menu.UploadMode.cdc.upload.use_1200bps_touch=true
-esp32c3.menu.UploadMode.cdc.upload.wait_for_upload_port=true
-
 esp32c3.menu.PartitionScheme.default=Default 4MB with spiffs (1.2MB APP/1.5MB SPIFFS)
 esp32c3.menu.PartitionScheme.default.build.partitions=default
 esp32c3.menu.PartitionScheme.defaultffat=Default 4MB with ffat (1.2MB APP/1.5MB FATFS)
diff --git a/cores/esp32/HWCDC.cpp b/cores/esp32/HWCDC.cpp
@@ -24,15 +24,47 @@
 #include "soc/periph_defs.h"
 #include "hal/usb_serial_jtag_ll.h"
 
+ESP_EVENT_DEFINE_BASE(ARDUINO_HW_CDC_EVENTS);
+
 static RingbufHandle_t tx_ring_buf = NULL;
 static xQueueHandle rx_queue = NULL;
 static uint8_t rx_data_buf[64];
 static intr_handle_t intr_handle = NULL;
 static volatile bool initial_empty = false;
+static xSemaphoreHandle tx_lock = NULL;
+static uint32_t tx_timeout_ms = 200;
+static esp_event_loop_handle_t arduino_hw_cdc_event_loop_handle = NULL;
+
+static esp_err_t arduino_hw_cdc_event_post(esp_event_base_t event_base, int32_t event_id, void *event_data, size_t event_data_size, BaseType_t *task_unblocked){
+    if(arduino_hw_cdc_event_loop_handle == NULL){
+        return ESP_FAIL;
+    }
+    return esp_event_isr_post_to(arduino_hw_cdc_event_loop_handle, event_base, event_id, event_data, event_data_size, task_unblocked);
+}
+
+static esp_err_t arduino_hw_cdc_event_handler_register_with(esp_event_base_t event_base, int32_t event_id, esp_event_handler_t event_handler, void *event_handler_arg){
+    if (!arduino_hw_cdc_event_loop_handle) {
+        esp_event_loop_args_t event_task_args = {
+            .queue_size = 5,
+            .task_name = "arduino_hw_cdc_events",
+            .task_priority = 5,
+            .task_stack_size = 2048,
+            .task_core_id = tskNO_AFFINITY
+        };
+        if (esp_event_loop_create(&event_task_args, &arduino_hw_cdc_event_loop_handle) != ESP_OK) {
+            log_e("esp_event_loop_create failed");
+        }
+    }
+    if(arduino_hw_cdc_event_loop_handle == NULL){
+        return ESP_FAIL;
+    }
+    return esp_event_handler_register_with(arduino_hw_cdc_event_loop_handle, event_base, event_id, event_handler, event_handler_arg);
+}
 
 static void hw_cdc_isr_handler(void *arg) {
     portBASE_TYPE xTaskWoken = 0;
     uint32_t usbjtag_intr_status = 0;
+    arduino_hw_cdc_event_data_t event = {0};
     usbjtag_intr_status = usb_serial_jtag_ll_get_intsts_mask();
 
     if (usbjtag_intr_status & USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY) {
@@ -45,6 +77,7 @@ static void hw_cdc_isr_handler(void *arg) {
                 initial_empty = true;
                 //send event?
                 //ets_printf("CONNECTED\n");
+                arduino_hw_cdc_event_post(ARDUINO_HW_CDC_EVENTS, ARDUINO_HW_CDC_CONNECTED_EVENT, &event, sizeof(arduino_hw_cdc_event_data_t), &xTaskWoken);
             }
             size_t queued_size;
             uint8_t *queued_buff = (uint8_t *)xRingbufferReceiveUpToFromISR(tx_ring_buf, &queued_size, 64);
@@ -58,6 +91,8 @@ static void hw_cdc_isr_handler(void *arg) {
                 usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
                 //send event?
                 //ets_printf("TX:%u\n", queued_size);
+                event.tx.len = queued_size;
+                arduino_hw_cdc_event_post(ARDUINO_HW_CDC_EVENTS, ARDUINO_HW_CDC_TX_EVENT, &event, sizeof(arduino_hw_cdc_event_data_t), &xTaskWoken);
             }
         } else {
             usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
@@ -77,13 +112,16 @@ static void hw_cdc_isr_handler(void *arg) {
         }
         //send event?
         //ets_printf("RX:%u/%u\n", i, rx_fifo_len);
+        event.rx.len = i;
+        arduino_hw_cdc_event_post(ARDUINO_HW_CDC_EVENTS, ARDUINO_HW_CDC_RX_EVENT, &event, sizeof(arduino_hw_cdc_event_data_t), &xTaskWoken);
     }
 
     if (usbjtag_intr_status & USB_SERIAL_JTAG_INTR_BUS_RESET) {
         usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_BUS_RESET);
         initial_empty = false;
         usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
         //ets_printf("BUS_RESET\n");
+        arduino_hw_cdc_event_post(ARDUINO_HW_CDC_EVENTS, ARDUINO_HW_CDC_BUS_RESET_EVENT, &event, sizeof(arduino_hw_cdc_event_data_t), &xTaskWoken);
     }
 
     if (xTaskWoken == pdTRUE) {
@@ -95,13 +133,13 @@ static void ARDUINO_ISR_ATTR cdc0_write_char(char c) {
     if(xPortInIsrContext()){
         xRingbufferSendFromISR(tx_ring_buf, (void*) (&c), 1, NULL);
     } else {
-        xRingbufferSend(tx_ring_buf, (void*) (&c), 1, 0);
+        xRingbufferSend(tx_ring_buf, (void*) (&c), 1, tx_timeout_ms / portTICK_PERIOD_MS);
     }
     usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
 }
 
 HWCDC::HWCDC() {
-    
+
 }
 
 HWCDC::~HWCDC(){
@@ -113,8 +151,19 @@ HWCDC::operator bool() const
     return initial_empty;
 }
 
+void HWCDC::onEvent(esp_event_handler_t callback){
+    onEvent(ARDUINO_HW_CDC_ANY_EVENT, callback);
+}
+
+void HWCDC::onEvent(arduino_hw_cdc_event_t event, esp_event_handler_t callback){
+    arduino_hw_cdc_event_handler_register_with(ARDUINO_HW_CDC_EVENTS, event, callback, this);
+}
+
 void HWCDC::begin(unsigned long baud)
 {
+    if(tx_lock == NULL) {
+        tx_lock = xSemaphoreCreateMutex();
+    }
     setRxBufferSize(256);//default if not preset
     setTxBufferSize(256);//default if not preset
 
@@ -123,6 +172,7 @@ void HWCDC::begin(unsigned long baud)
     if(!intr_handle && esp_intr_alloc(ETS_USB_INTR_SOURCE/*ETS_USB_SERIAL_JTAG_INTR_SOURCE*/, 0, hw_cdc_isr_handler, NULL, &intr_handle) != ESP_OK){
         isr_log_e("HW USB CDC failed to init interrupts");
         end();
+        return;
     }
 }
 
@@ -132,8 +182,19 @@ void HWCDC::end()
     usb_serial_jtag_ll_disable_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY | USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT | USB_SERIAL_JTAG_INTR_BUS_RESET);
     esp_intr_free(intr_handle);
     intr_handle = NULL;
+    if(tx_lock != NULL) {
+        vSemaphoreDelete(tx_lock);
+    }
     setRxBufferSize(0);
     setTxBufferSize(0);
+    if (arduino_hw_cdc_event_loop_handle) {
+        esp_event_loop_delete(arduino_hw_cdc_event_loop_handle);
+        arduino_hw_cdc_event_loop_handle = NULL;
+    }
+}
+
+void HWCDC::setTxTimeoutMs(uint32_t timeout){
+    tx_timeout_ms = timeout;
 }
 
 /*
@@ -157,21 +218,57 @@ size_t HWCDC::setTxBufferSize(size_t tx_queue_len){
 
 int HWCDC::availableForWrite(void)
 {
-    if(tx_ring_buf == NULL){
-        return -1;
+    if(tx_ring_buf == NULL || tx_lock == NULL){
+        return 0;
+    }
+    if(xSemaphoreTake(tx_lock, tx_timeout_ms / portTICK_PERIOD_MS) != pdPASS){
+        return 0;
     }
-    return xRingbufferGetCurFreeSize(tx_ring_buf);
+    size_t a = xRingbufferGetCurFreeSize(tx_ring_buf);
+    xSemaphoreGive(tx_lock);
+    return a;
 }
 
 size_t HWCDC::write(const uint8_t *buffer, size_t size)
 {
-    // Blocking method, Sending data to ringbuffer, and handle the data in ISR.
-    if(xRingbufferSend(tx_ring_buf, (void*) (buffer), size, 200 / portTICK_PERIOD_MS) != pdTRUE){
-        log_e("Write Failed");
+    if(buffer == NULL || size == 0 || tx_ring_buf == NULL || tx_lock == NULL){
         return 0;
     }
-    // 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);
+    if(xSemaphoreTake(tx_lock, tx_timeout_ms / portTICK_PERIOD_MS) != pdPASS){
+        return 0;
+    }
+    size_t max_size = xRingbufferGetMaxItemSize(tx_ring_buf);
+    size_t space = xRingbufferGetCurFreeSize(tx_ring_buf);
+    size_t to_send = size, so_far = 0;
+
+    if(space > size){
+        space = size;
+    }
+    // Non-Blocking method, Sending data to ringbuffer, and handle the data in ISR.
+    if(xRingbufferSend(tx_ring_buf, (void*) (buffer), space, 0) != pdTRUE){
+        size = 0;
+    } else {
+        to_send -= space;
+        so_far += space;
+        // 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);
+
+        while(to_send){
+            if(max_size > to_send){
+                max_size = to_send;
+            }
+            // Blocking method, Sending data to ringbuffer, and handle the data in ISR.
+            if(xRingbufferSend(tx_ring_buf, (void*) (buffer+so_far), max_size, tx_timeout_ms / portTICK_PERIOD_MS) != pdTRUE){
+                size = so_far;
+                break;
+            }
+            so_far += max_size;
+            to_send -= max_size;
+            // 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);
+        }
+    }
+    xSemaphoreGive(tx_lock);
     return size;
 }
 
@@ -182,15 +279,23 @@ size_t HWCDC::write(uint8_t c)
 
 void HWCDC::flush(void)
 {
-    if(tx_ring_buf == NULL){
+    if(tx_ring_buf == NULL || tx_lock == NULL){
+        return;
+    }
+    if(xSemaphoreTake(tx_lock, tx_timeout_ms / portTICK_PERIOD_MS) != pdPASS){
         return;
     }
     UBaseType_t uxItemsWaiting = 0;
     vRingbufferGetInfo(tx_ring_buf, NULL, NULL, NULL, NULL, &uxItemsWaiting);
+    if(uxItemsWaiting){
+        // 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);
+    }
     while(uxItemsWaiting){
         delay(5);
         vRingbufferGetInfo(tx_ring_buf, NULL, NULL, NULL, NULL, &uxItemsWaiting);
     }
+    xSemaphoreGive(tx_lock);
 }
 
 /*
diff --git a/cores/esp32/HWCDC.h b/cores/esp32/HWCDC.h
@@ -17,16 +17,41 @@
 #if CONFIG_IDF_TARGET_ESP32C3
 
 #include <inttypes.h>
+#include "esp_event.h"
 #include "Stream.h"
 
+ESP_EVENT_DECLARE_BASE(ARDUINO_HW_CDC_EVENTS);
+
+typedef enum {
+    ARDUINO_HW_CDC_ANY_EVENT = ESP_EVENT_ANY_ID,
+    ARDUINO_HW_CDC_CONNECTED_EVENT = 0,
+    ARDUINO_HW_CDC_BUS_RESET_EVENT,
+    ARDUINO_HW_CDC_RX_EVENT,
+    ARDUINO_HW_CDC_TX_EVENT,
+    ARDUINO_HW_CDC_MAX_EVENT,
+} arduino_hw_cdc_event_t;
+
+typedef union {
+    struct {
+            size_t len;
+    } rx;
+    struct {
+            size_t len;
+    } tx;
+} arduino_hw_cdc_event_data_t;
+
 class HWCDC: public Stream
 {
 public:
     HWCDC();
     ~HWCDC();
 
+    void onEvent(esp_event_handler_t callback);
+    void onEvent(arduino_hw_cdc_event_t event, esp_event_handler_t callback);
+
     size_t setRxBufferSize(size_t);
     size_t setTxBufferSize(size_t);
+    void setTxTimeoutMs(uint32_t timeout);
     void begin(unsigned long baud=0);
     void end();
     
diff --git a/cores/esp32/esp32-hal-uart.c b/cores/esp32/esp32-hal-uart.c
@@ -368,13 +368,9 @@ void uartSetDebug(uart_t* uart)
 {
     if(uart == NULL || uart->num >= SOC_UART_NUM) {
         s_uart_debug_nr = -1;
-        //ets_install_putc1(NULL);
-        //return;
-    } else
-    if(s_uart_debug_nr == uart->num) {
-        return;
-    } else
-    s_uart_debug_nr = uart->num;
+    } else {
+        s_uart_debug_nr = uart->num;
+    }
     uart_install_putc();
 }
 
