Merge branch 'fix/usb-cdc-non-blocking-read' into 'master'

fix(esp_vfs_console): USB CDC read when non blocking

See merge request espressif/esp-idf!36469

Author: SoucheSouche

components/esp_system/include/esp_private/usb_console.h

@@ -1,6 +1,6 @@
 
 /*
- * SPDX-FileCopyrightText: 2019-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2019-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -101,6 +101,14 @@ esp_err_t esp_usb_console_set_cb(esp_usb_console_cb_t rx_cb, esp_usb_console_cb_
  */
 bool esp_usb_console_is_installed(void);
 
+/**
+ * @brief Call the USB interrupt handler while any interrupts
+ * are pending, but not more than a few times. Non-static to
+ * allow placement into IRAM by ldgen.
+ *
+ */
+void esp_usb_console_poll_interrupts(void); // [refactor-todo] Remove when implementing IDF-12175
+
 #ifdef __cplusplus
 }
 #endif

components/esp_vfs_console/test_apps/usb_cdc_vfs/main/test_app_main.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Unlicense OR CC0-1.0
  */
@@ -11,23 +11,36 @@
 #include "esp_private/usb_console.h"
 #include "esp_vfs_cdcacm.h"
 
-static int read_bytes_with_select(FILE *stream, void *buf, size_t buf_len, struct timeval tv)
+static void flush_write(void)
+{
+    fflush(stdout);
+    fsync(fileno(stdout));
+}
+
+static void test_setup(const char* func_name, const size_t func_name_size)
+{
+    /* write the name of the test back to the pytest environment to start the test */
+    write(fileno(stdout), func_name, func_name_size);
+    write(fileno(stdout), "\n", 1);
+    flush_write();
+}
+
+static int read_bytes_with_select(FILE *stream, void *buf, size_t buf_len, struct timeval* tv)
 {
     int fd = fileno(stream);
     fd_set read_fds;
     FD_ZERO(&read_fds);
     FD_SET(fd, &read_fds);
-
-    /* call select with to wait for either a read ready or timeout to happen */
-    int nread = select(fd + 1, &read_fds, NULL, NULL, &tv);
+    /* call select to wait for either a read ready or an except to happen */
+    int nread = select(fd + 1, &read_fds, NULL, NULL, tv);
     if (nread < 0) {
         return -1;
     } else if (FD_ISSET(fd, &read_fds)) {
         int read_count = 0;
         int total_read = 0;
 
         do {
-            read_count = read(fileno(stream), buf + total_read, 1);
+            read_count = read(fd, buf + total_read, buf_len - total_read);
             if (read_count < 0 && errno != EWOULDBLOCK) {
                 return -1;
             } else if (read_count > 0) {
@@ -41,12 +54,31 @@ static int read_bytes_with_select(FILE *stream, void *buf, size_t buf_len, struc
 
         return total_read;
     } else {
+        /* select timed out */
         return -2;
     }
+    return nread;
 }
 
-void app_main(void)
+static bool wait_for_read_ready(FILE *stream)
+{
+    int fd = fileno(stream);
+    fd_set read_fds;
+    FD_ZERO(&read_fds);
+    FD_SET(fd, &read_fds);
+    /* call select to wait for either a read ready or an except to happen */
+    int nread = select(fd + 1, &read_fds, NULL, NULL, NULL);
+    if ((nread >= 0) && (FD_ISSET(fd, &read_fds))) {
+        return true;
+    } else {
+        return false;
+    }
+}
+
+static void test_usb_cdc_select(void)
 {
+    test_setup(__func__, sizeof(__func__));
+
     struct timeval tv;
     tv.tv_sec = 1;
     tv.tv_usec = 0;
@@ -70,7 +102,7 @@ void app_main(void)
     bool message_received = false;
     size_t char_read = 0;
     while (!message_received && out_buffer_len > char_read) {
-        int nread = read_bytes_with_select(stdin, out_buffer + char_read, out_buffer_len - char_read, tv);
+        int nread = read_bytes_with_select(stdin, out_buffer + char_read, out_buffer_len - char_read, &tv);
         if (nread > 0) {
             char_read += nread;
             if (out_buffer[char_read - 1] == '\n') {
@@ -83,6 +115,7 @@ void app_main(void)
             // function since the string is expected as is by the test environment
             char timeout_msg[] = "select timed out\n";
             write(fileno(stdout), timeout_msg, sizeof(timeout_msg));
+            flush_write();
         } else {
             break;
         }
@@ -91,6 +124,59 @@ void app_main(void)
     // write the received message back to the test environment. The test
     // environment will check that the message received matches the one sent
     write(fileno(stdout), out_buffer, char_read);
+    flush_write();
 
     vTaskDelay(10); // wait for the string to send
 }
+
+static void test_usb_cdc_read_non_blocking(void)
+{
+    test_setup(__func__, sizeof(__func__));
+
+    const size_t max_read_bytes = 16;
+    char read_data_buffer[max_read_bytes];
+
+    memset(read_data_buffer, 0x00, max_read_bytes);
+
+    /* reset errno to 0 for the purpose of the test to make sure it is no
+     * longer set to EWOULDBLOCK later in the test. */
+    errno = 0;
+
+    /* make sure to read in non blocking mode */
+    int stdin_fd = fileno(stdin);
+    int flags = fcntl(stdin_fd, F_GETFL);
+    flags |= O_NONBLOCK;
+    int res = fcntl(stdin_fd, F_SETFL, flags);
+    TEST_ASSERT(res == 0);
+
+    /* call read when no data is available for reading.
+     * expected result:
+     * - read returns -1 and errno has been set to EWOULDBLOCK */
+    int nread = read(stdin_fd, read_data_buffer, max_read_bytes);
+    TEST_ASSERT(nread == -1);
+    TEST_ASSERT(errno == EWOULDBLOCK);
+
+    /* reset errno to 0 for the purpose of the test to make sure it is no
+     * longer set to EWOULDBLOCK later in the test. */
+    errno = 0;
+
+    /* call read X number of bytes when less than X number of
+     * bytes are available for reading (Y bytes).
+     * expected result:
+     * - read returns at least 1 bytes errno is not set to EWOULDBLOCK */
+    char message_8[] = "send_bytes\n";
+    write(fileno(stdout), message_8, sizeof(message_8));
+    flush_write();
+
+    const bool is_ready = wait_for_read_ready(stdin);
+    TEST_ASSERT(is_ready);
+    nread = read(stdin_fd, read_data_buffer, max_read_bytes);
+    TEST_ASSERT(nread >= 1);
+    TEST_ASSERT(errno != EWOULDBLOCK);
+}
+
+void app_main(void)
+{
+    test_usb_cdc_select();
+    test_usb_cdc_read_non_blocking();
+}

components/esp_vfs_console/test_apps/usb_cdc_vfs/pytest_usb_cdc_vfs.py

@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
+# SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD
 # SPDX-License-Identifier: CC0-1.0
 import pytest
 from pytest_embedded import Dut
@@ -14,6 +14,13 @@
     indirect=True,)
 @pytest.mark.parametrize('test_message', ['test123456789!@#%^&*'])
 def test_usb_cdc_vfs_default(dut: Dut, test_message: str) -> None:
+    # test run: test_usb_cdc_select
+    dut.expect_exact('test_usb_cdc_select', timeout=2)
     dut.expect_exact('select timed out', timeout=2)
     dut.write(test_message)
     dut.expect_exact(test_message, timeout=2)
+
+    # test run: test_usb_cdc_read_non_blocking
+    dut.expect_exact('test_usb_cdc_read_non_blocking', timeout=2)
+    dut.expect_exact('send_bytes', timeout=2)
+    dut.write('abcdefgh')

components/esp_vfs_console/vfs_cdcacm.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -181,15 +181,15 @@ static ssize_t cdcacm_read(int fd, void *data, size_t size)
     ssize_t received = 0;
     _lock_acquire_recursive(&s_read_lock);
 
-    while (cdcacm_data_length_in_buffer() < size) {
-        if (!s_blocking) {
-            errno = EWOULDBLOCK;
-            _lock_release_recursive(&s_read_lock);
-            return -1;
+    if (s_blocking) {
+        while (cdcacm_data_length_in_buffer() < size) {
+            xSemaphoreTake(s_rx_semaphore, portMAX_DELAY);
         }
-        xSemaphoreTake(s_rx_semaphore, portMAX_DELAY);
+    } else {
+        /* process pending interrupts before requesting available data */
+        esp_usb_console_poll_interrupts();
+        size = MIN(size, cdcacm_data_length_in_buffer());
     }
-
     if (s_rx_mode == ESP_LINE_ENDINGS_CR || s_rx_mode == ESP_LINE_ENDINGS_LF) {
         /* This is easy. Just receive, and if needed replace \r by \n. */
         received = esp_usb_console_read_buf(data_c, size);
@@ -443,6 +443,9 @@ static esp_err_t cdcacm_start_select(int nfds, fd_set *readfds, fd_set *writefds
         return ret;
     }
 
+    /* make sure the driver processes any pending interrupts before checking read or write
+     * readiness */
+    esp_usb_console_poll_interrupts();
     bool trigger_select = false;
     if (FD_ISSET(USB_CDC_LOCAL_FD, &args->readfds_orig) &&
             esp_usb_console_available_for_read() > 0) {