Merge branch 'feat/remove-unecessray-condition-in-usj-read_v5.3' into 'release/v5.3'

fix(driver): remove unecessary if conditions in the read function (v5.3)

See merge request espressif/esp-idf!39945

components/esp_driver_usb_serial_jtag/include/driver/usb_serial_jtag.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2021-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2021-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -96,6 +96,34 @@ esp_err_t usb_serial_jtag_driver_uninstall(void);
  */
 bool usb_serial_jtag_is_connected(void);
 
+/**
+ * @brief Get information whether the USB serial JTAG driver is installed or not
+ *
+ * @return True if driver is installed and False if driver not installed
+ */
+bool usb_serial_jtag_is_driver_installed(void);
+
+/**
+ * @brief Return the number of bytes available for reading
+ *
+ * @return the number of bytes available for reading in the buffer
+ */
+size_t usb_serial_jtag_get_read_bytes_available(void);
+
+/**
+ * @brief Return the readiness status of the driver for read operation
+ *
+ * @return true if driver read ready, false if not
+ */
+bool usb_serial_jtag_read_ready(void);
+
+/**
+ * @brief Return the readiness status of the driver for write operation
+ *
+ * @return true if driver is write ready, false if not
+ */
+bool usb_serial_jtag_write_ready(void);
+
 #ifdef __cplusplus
 }
 #endif

components/esp_driver_usb_serial_jtag/src/usb_serial_jtag.c

@@ -256,3 +256,33 @@ esp_err_t usb_serial_jtag_driver_uninstall(void)
     p_usb_serial_jtag_obj = NULL;
     return ESP_OK;
 }
+
+bool usb_serial_jtag_is_driver_installed(void)
+{
+    return (p_usb_serial_jtag_obj != NULL);
+}
+
+size_t usb_serial_jtag_get_read_bytes_available(void)
+{
+    // sign the the driver is read ready is that data is waiting in the RX ringbuffer
+    UBaseType_t bytes_available = 0;
+    if (usb_serial_jtag_is_driver_installed()) {
+        vRingbufferGetInfo(p_usb_serial_jtag_obj->rx_ring_buf, NULL, NULL, NULL, NULL, &bytes_available);
+        if (bytes_available <= 0) {
+            return 0;
+        }
+    }
+
+    return (size_t)bytes_available;
+}
+
+bool usb_serial_jtag_read_ready(void)
+{
+    return usb_serial_jtag_get_read_bytes_available() != 0;
+}
+
+bool usb_serial_jtag_write_ready(void)
+{
+    // sign that the driver is write ready is that the TX ring buffer is not full
+    return (xRingbufferGetCurFreeSize(p_usb_serial_jtag_obj->tx_ring_buf) > 0);
+}

components/esp_driver_usb_serial_jtag/src/usb_serial_jtag_vfs.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
  */
@@ -191,38 +191,66 @@ static ssize_t usb_serial_jtag_read(int fd, void* data, size_t size)
 {
     char *data_c = (char *) data;
     size_t received = 0;
+    size_t available_size = 0;
+    int c = NONE; // store the read char
     _lock_acquire_recursive(&s_ctx.read_lock);
-    while (received < size) {
-        int c = usb_serial_jtag_read_char(fd);
-        if (c == '\r') {
-            if (s_ctx.rx_mode == ESP_LINE_ENDINGS_CR) {
-                c = '\n';
-            } else if (s_ctx.rx_mode == ESP_LINE_ENDINGS_CRLF) {
-                /* look ahead */
-                int c2 = usb_serial_jtag_read_char(fd);
-                if (c2 == NONE) {
-                    /* could not look ahead, put the current character back */
-                    usb_serial_jtag_return_char(fd, c);
-                    break;
-                }
-                if (c2 == '\n') {
-                    /* this was \r\n sequence. discard \r, return \n */
+
+    // if blocking read, wait for data to  be available
+    if (!s_ctx.non_blocking) {
+        c = usb_serial_jtag_read_char(fd);
+    }
+
+    // find the actual fetch size
+    available_size += usb_serial_jtag_get_read_bytes_available();
+    if (c != NONE) {
+        available_size++;
+    }
+    if (s_ctx.peek_char != NONE) {
+        available_size++;
+    }
+    size_t fetch_size = MIN(available_size, size);
+
+    if (fetch_size > 0) {
+        do {
+            if (c == NONE) { // for non-O_NONBLOCK mode, there is already a pre-fetched char
+                c = usb_serial_jtag_read_char(fd);
+            }
+            assert(c != NONE);
+
+            if (c == '\r') {
+                if (s_ctx.rx_mode == ESP_LINE_ENDINGS_CR) {
                     c = '\n';
-                } else {
-                    /* \r followed by something else. put the second char back,
-                     * it will be processed on next iteration. return \r now.
-                     */
-                    usb_serial_jtag_return_char(fd, c2);
+                } else if (s_ctx.rx_mode == ESP_LINE_ENDINGS_CRLF) {
+                    /* look ahead */
+                    int c2 = usb_serial_jtag_read_char(fd);
+                    fetch_size--;
+                    if (c2 == NONE) {
+                        /* could not look ahead, put the current character back */
+                        usb_serial_jtag_return_char(fd, c);
+                        c = NONE;
+                        break;
+                    }
+                    if (c2 == '\n') {
+                        /* this was \r\n sequence. discard \r, return \n */
+                        c = '\n';
+                    } else {
+                        /* \r followed by something else. put the second char back,
+                         * it will be processed on next iteration. return \r now.
+                         */
+                        usb_serial_jtag_return_char(fd, c2);
+                        fetch_size++;
+                    }
                 }
             }
-        } else if (c == NONE) {
-            break;
-        }
-        data_c[received] = (char) c;
-        ++received;
-        if (c == '\n') {
-            break;
-        }
+
+            data_c[received] = (char) c;
+            ++received;
+            c = NONE;
+        } while (received < fetch_size);
+    }
+
+    if (c != NONE) { // fetched, but not used
+        usb_serial_jtag_return_char(fd, c);
     }
     _lock_release_recursive(&s_ctx.read_lock);
     if (received > 0) {

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

@@ -40,6 +40,63 @@ static bool wait_for_read_ready(FILE *stream)
     }
 }
 
+static void test_usb_cdc_select(void)
+{
+    test_setup(__func__, sizeof(__func__));
+
+    struct timeval tv;
+    tv.tv_sec = 1;
+    tv.tv_usec = 0;
+    char out_buffer[32];
+    memset(out_buffer, 0, sizeof(out_buffer));
+    size_t out_buffer_len = sizeof(out_buffer);
+
+    // stdin needs to be non blocking to properly call read after select returns
+    // with read ready on stdin.
+    int fd = fileno(stdin);
+    int flags = fcntl(fd, F_GETFL);
+    flags |= O_NONBLOCK;
+    int res = fcntl(fd, F_SETFL, flags);
+    TEST_ASSERT(res == 0);
+
+    // init driver
+    // ESP_ERROR_CHECK(esp_usb_console_init());
+    // esp_vfs_dev_cdcacm_register();
+
+    // send the message from pytest environment and make sure it can be read
+    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);
+        if (nread > 0) {
+            char_read += nread;
+            if (out_buffer[char_read - 1] == '\n') {
+                message_received = true;
+            }
+        } else if (nread == -2) {
+            // time out occurred, send the expected message back to the testing
+            // environment to trigger the testing environment into sending the
+            // test message. don't update this message without updating the pytest
+            // 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;
+        }
+    }
+
+    // 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
+}
+
+/**
+ * @brief Test that the non blocking read is compliant with POSIX standards
+ */
 static void test_usb_cdc_read_non_blocking(void)
 {
     test_setup(__func__, sizeof(__func__));
@@ -86,7 +143,97 @@ static void test_usb_cdc_read_non_blocking(void)
     TEST_ASSERT(errno != EWOULDBLOCK);
 }
 
+/**
+ * @brief Test that the blocking read will return with the available
+ * data even if the size is less than the requested size.
+ */
+static void test_usb_cdc_read_blocking(void)
+{
+    test_setup(__func__, sizeof(__func__));
+
+    const size_t out_buffer_len = 32;
+    char out_buffer[out_buffer_len] = {};
+
+    ESP_ERROR_CHECK(esp_vfs_dev_cdcacm_register());
+
+    esp_vfs_dev_cdcacm_set_rx_line_endings(ESP_LINE_ENDINGS_LF);
+    esp_vfs_dev_cdcacm_set_tx_line_endings(ESP_LINE_ENDINGS_LF);
+
+    /* make sure blocking mode is enabled */
+    int flags = fcntl(STDIN_FILENO, F_GETFL, 0);
+    fcntl(STDIN_FILENO, F_SETFL, flags & (~O_NONBLOCK));
+
+    // trigger the test environment to send the test message
+    char ready_msg[] = "ready to receive\n";
+    write(fileno(stdout), ready_msg, sizeof(ready_msg));
+
+    const int nread = read(STDIN_FILENO, out_buffer, out_buffer_len);
+    TEST_ASSERT(nread > 0);
+    TEST_ASSERT(nread < out_buffer_len);
+
+    fcntl(STDIN_FILENO, F_SETFL, flags);
+    esp_vfs_dev_cdcacm_set_rx_line_endings(ESP_LINE_ENDINGS_CRLF);
+    esp_vfs_dev_cdcacm_set_tx_line_endings(ESP_LINE_ENDINGS_CRLF);
+    vTaskDelay(2);  // wait for tasks to exit
+}
+
+/**
+ * @brief Test that the read function does not prematurely return
+ * on reception of new line character.
+ */
+static void test_usb_cdc_read_no_exit_on_newline_reception(void)
+{
+    test_setup(__func__, sizeof(__func__));
+
+    // Send a string with length less than the read requested length
+    const char in_buffer[] = "!(@*#&(!*@&#((SDasdkjhad\nce"; // read should not early return on \n
+    const size_t in_buffer_len = sizeof(in_buffer);
+
+    const size_t out_buffer_len = in_buffer_len - 1; // don't compare the null character at the end of in_buffer string
+    char out_buffer[out_buffer_len] = {};
+
+    ESP_ERROR_CHECK(esp_vfs_dev_cdcacm_register());
+
+    esp_vfs_dev_cdcacm_set_rx_line_endings(ESP_LINE_ENDINGS_LF);
+    esp_vfs_dev_cdcacm_set_tx_line_endings(ESP_LINE_ENDINGS_LF);
+
+    int flags = fcntl(STDIN_FILENO, F_GETFL, 0);
+    fcntl(STDIN_FILENO, F_SETFL, flags | O_NONBLOCK);
+
+    // trigger the test environment to send the test message
+    char ready_msg[] = "ready to receive\n";
+    write(fileno(stdout), ready_msg, sizeof(ready_msg));
+
+    // wait for the string to be sent and buffered
+    vTaskDelay(pdMS_TO_TICKS(500));
+
+    char *out_buffer_ptr = out_buffer;
+    size_t bytes_read = 0;
+    do {
+        int nread = read(STDIN_FILENO, out_buffer_ptr, out_buffer_len);
+        if (nread > 0) {
+            out_buffer_ptr += nread;
+            bytes_read += nread;
+        }
+    } while (bytes_read < in_buffer_len);
+
+    // string compare
+    for (size_t i = 0; i < out_buffer_len; i++) {
+        TEST_ASSERT_EQUAL(in_buffer[i], out_buffer[i]);
+    }
+
+    fcntl(STDIN_FILENO, F_SETFL, flags);
+    esp_vfs_dev_cdcacm_set_rx_line_endings(ESP_LINE_ENDINGS_CRLF);
+    esp_vfs_dev_cdcacm_set_tx_line_endings(ESP_LINE_ENDINGS_CRLF);
+    vTaskDelay(2);  // wait for tasks to exit
+}
+
+/* Always make sure that the function calling sequence in the main
+ * function matches the expected order in the pytest function.
+ */
 void app_main(void)
 {
     test_usb_cdc_read_non_blocking();
+    test_usb_cdc_read_blocking();
+    test_usb_cdc_read_no_exit_on_newline_reception();
 }

components/esp_vfs_console/test_apps/usb_cdc_vfs/pytest_usb_cdc_vfs.py

@@ -18,3 +18,13 @@ def test_usb_cdc_vfs_default(dut: Dut, test_message: str) -> None:
     dut.expect_exact('test_usb_cdc_read_non_blocking', timeout=2)
     dut.expect_exact('send_bytes', timeout=2)
     dut.write('abcdefgh')
+
+    # test run: test_usb_cdc_read_blocking
+    dut.expect_exact('test_usb_cdc_read_blocking', timeout=2)
+    dut.expect_exact('ready to receive', timeout=2)
+    dut.write('testdata')
+
+    # test run: test_usb_cdc_read_no_exit_on_newline_reception
+    dut.expect_exact('test_usb_cdc_read_no_exit_on_newline_reception', timeout=2)
+    dut.expect_exact('ready to receive', timeout=2)
+    dut.write('!(@*#&(!*@&#((SDasdkjhad\nce')

components/esp_vfs_console/vfs_cdcacm.c

@@ -146,13 +146,16 @@ static void cdcacm_return_char(int c)
 
 static ssize_t cdcacm_read(int fd, void *data, size_t size)
 {
-    assert(fd == 0);
+    assert(fd == USB_CDC_LOCAL_FD);
     char *data_c = (char *) data;
     ssize_t received = 0;
     _lock_acquire_recursive(&s_read_lock);
 
     if (s_blocking) {
-        while (cdcacm_data_length_in_buffer() < size) {
+        /* in blocking mode, wait for data. by the POSIX standards,
+         * the amount of available bytes does not need to match the
+         * requested size to return */
+        while (cdcacm_data_length_in_buffer() <= 0) {
             xSemaphoreTake(s_rx_semaphore, portMAX_DELAY);
         }
     } else {
@@ -161,21 +164,12 @@ static ssize_t cdcacm_read(int fd, void *data, size_t size)
         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);
-        if (s_rx_mode == ESP_LINE_ENDINGS_CR) {
-            /* Change CRs to newlines */
-            for (ssize_t i = 0; i < received; i++) {
-                if (data_c[i] == '\r') {
-                    data_c[i] = '\n';
-                }
-            }
-        }
-    } else {
-        while (received < size) {
-            int c = cdcacm_read_char();
-            if (c == '\r') {
+    while (received < size) {
+        int c = cdcacm_read_char();
+        if (c == '\r') {
+            if (s_rx_mode == ESP_LINE_ENDINGS_CR) {
+                c = '\n';
+            } else if (s_rx_mode == ESP_LINE_ENDINGS_CRLF) {
                 /* look ahead */
                 int c2 = cdcacm_read_char();
                 if (c2 == NONE) {
@@ -192,14 +186,12 @@ static ssize_t cdcacm_read(int fd, void *data, size_t size)
                      */
                     cdcacm_return_char(c2);
                 }
-            } else if (c == NONE) {
-                break;
-            }
-            data_c[received++] = (char) c;
-            if (c == '\n') {
-                break;
             }
+
+        } else if (c == NONE) {
+            break;
         }
+        data_c[received++] = (char) c;
     }
     _lock_release_recursive(&s_read_lock);
     if (received > 0) {