feat(usb_cdc): Update vfs read() to comply with POSIX standards

Author: SoucheSouche

components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/main/test_vfs_usb_serial_jtag.c

@@ -124,7 +124,9 @@ TEST_CASE("test select read, write and timeout", "[usb_serial_jtag vfs]")
     usb_serial_jtag_driver_uninstall();
 }
 
-TEST_CASE("read with usj driver (non-blocking)", "[usb serial jtag vfs]")
+/* Test that the read function does not return prematurely when receiving a new line character
+ */
+TEST_CASE("read does not return on new line character", "[usb serial jtag vfs]")
 {
     // Send a string with length less than the read requested length
     const char in_buffer[] = "!(@*#&(!*@&#((SDasdkjhad\nce"; // read should not early return on \n
@@ -133,8 +135,6 @@ TEST_CASE("read with usj driver (non-blocking)", "[usb serial jtag vfs]")
     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] = {};
 
-    // flush_stdin_stdout();
-
     usb_serial_jtag_driver_config_t usj_config = USB_SERIAL_JTAG_DRIVER_CONFIG_DEFAULT();
     ESP_ERROR_CHECK(usb_serial_jtag_driver_install(&usj_config));
     usb_serial_jtag_vfs_use_driver();
@@ -175,3 +175,34 @@ TEST_CASE("read with usj driver (non-blocking)", "[usb serial jtag vfs]")
     ESP_ERROR_CHECK(usb_serial_jtag_driver_uninstall());
     vTaskDelay(2);  // wait for tasks to exit
 }
+
+TEST_CASE("blocking read returns with available data", "[usb serial jtag vfs]")
+{
+    const size_t out_buffer_len = 32;
+    char out_buffer[out_buffer_len] = {};
+
+    usb_serial_jtag_driver_config_t usj_config = USB_SERIAL_JTAG_DRIVER_CONFIG_DEFAULT();
+    ESP_ERROR_CHECK(usb_serial_jtag_driver_install(&usj_config));
+    usb_serial_jtag_vfs_use_driver();
+
+    usb_serial_jtag_vfs_set_rx_line_endings(ESP_LINE_ENDINGS_LF);
+    usb_serial_jtag_vfs_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));
+
+    const int nread = read(STDIN_FILENO, out_buffer, out_buffer_len);
+    TEST_ASSERT(nread > 0);
+    TEST_ASSERT(nread < out_buffer_len);
+
+    usb_serial_jtag_vfs_use_nonblocking();
+    fcntl(STDIN_FILENO, F_SETFL, flags);
+    usb_serial_jtag_vfs_set_rx_line_endings(ESP_LINE_ENDINGS_CRLF);
+    usb_serial_jtag_vfs_set_tx_line_endings(ESP_LINE_ENDINGS_CRLF);
+    ESP_ERROR_CHECK(usb_serial_jtag_driver_uninstall());
+    vTaskDelay(2);  // wait for tasks to exit
+}

components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/pytest_usb_serial_jtag_vfs.py

@@ -26,17 +26,35 @@ def test_usj_vfs_select(dut: Dut, test_message: list) -> None:
 
 @pytest.mark.usb_serial_jtag
 @pytest.mark.parametrize(
-    'port, config',
+    'port, flash_port, config',
     [
-        pytest.param('/dev/ttyACM1', 'release'),
+        pytest.param('/dev/serial_ports/ttyACM-esp32', '/dev/serial_ports/ttyUSB-esp32', 'release'),
     ],
     indirect=True,
 )
 @pytest.mark.parametrize('test_message', ['!(@*#&(!*@&#((SDasdkjhad\nce'])
 @idf_parametrize('target', ['esp32s3', 'esp32c3', 'esp32c6', 'esp32h2'], indirect=['target'])
 def test_usj_vfs_read(dut: Dut, test_message: list) -> None:
     dut.expect_exact('Press ENTER to see the list of tests')
-    dut.write('"read with usj driver (non-blocking)"')
+    dut.write('"read does not return on new line character"')
+    dut.expect_exact('ready to receive', timeout=2)
+    dut.write(test_message)
+    dut.expect(r'\d{1} Tests 0 Failures 0 Ignored', timeout=10)
+
+
+@pytest.mark.usb_serial_jtag
+@pytest.mark.parametrize(
+    'port, flash_port, config',
+    [
+        pytest.param('/dev/serial_ports/ttyACM-esp32', '/dev/serial_ports/ttyUSB-esp32', 'release'),
+    ],
+    indirect=True,
+)
+@pytest.mark.parametrize('test_message', ['testdata'])
+@idf_parametrize('target', ['esp32s3', 'esp32c3', 'esp32c6', 'esp32h2'], indirect=['target'])
+def test_usj_vfs_read(dut: Dut, test_message: list) -> None:
+    dut.expect_exact('Press ENTER to see the list of tests')
+    dut.write('"blocking read returns with available data"')
     dut.expect_exact('ready to receive', timeout=2)
     dut.write(test_message)
     dut.expect(r'\d{1} Tests 0 Failures 0 Ignored', timeout=10)

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

@@ -129,6 +129,9 @@ static void test_usb_cdc_select(void)
     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__));
@@ -175,6 +178,44 @@ 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__));
@@ -222,9 +263,13 @@ static void test_usb_cdc_read_no_exit_on_newline_reception(void)
     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_select();
     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

@@ -27,6 +27,11 @@ def test_usb_cdc_vfs_default(dut: Dut, test_message: str) -> None:
     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)

components/esp_vfs_console/vfs_cdcacm.c

@@ -176,35 +176,30 @@ 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 {
         /* 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);
-        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) {
@@ -221,11 +216,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;
+
+        } else if (c == NONE) {
+            break;
         }
+        data_c[received++] = (char) c;
     }
     _lock_release_recursive(&s_read_lock);
     if (received > 0) {