From 139df47cf38ec3ead0ded3bac25677e09bee115f Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 5 Jun 2025 12:15:53 +0200 Subject: [PATCH] feat(usb_cdc): Update vfs read() to comply with POSIX standards --- .../main/test_vfs_usb_serial_jtag.c | 37 +++++++++++++-- .../pytest_usb_serial_jtag_vfs.py | 24 ++++++++-- .../usb_cdc_vfs/main/test_app_main.c | 45 +++++++++++++++++++ .../usb_cdc_vfs/pytest_usb_cdc_vfs.py | 5 +++ components/esp_vfs_console/vfs_cdcacm.c | 36 +++++++-------- 5 files changed, 121 insertions(+), 26 deletions(-) diff --git a/components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/main/test_vfs_usb_serial_jtag.c b/components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/main/test_vfs_usb_serial_jtag.c index 9c418c57fe..4e305baa87 100644 --- a/components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/main/test_vfs_usb_serial_jtag.c +++ b/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 +} diff --git a/components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/pytest_usb_serial_jtag_vfs.py b/components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/pytest_usb_serial_jtag_vfs.py index 60ad7bcc3d..1dd6946f00 100644 --- a/components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/pytest_usb_serial_jtag_vfs.py +++ b/components/esp_driver_usb_serial_jtag/test_apps/usb_serial_jtag_vfs/pytest_usb_serial_jtag_vfs.py @@ -26,9 +26,9 @@ 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, ) @@ -36,7 +36,25 @@ def test_usj_vfs_select(dut: Dut, test_message: list) -> None: @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) diff --git a/components/esp_vfs_console/test_apps/usb_cdc_vfs/main/test_app_main.c b/components/esp_vfs_console/test_apps/usb_cdc_vfs/main/test_app_main.c index a446d4a871..a82a7a6d18 100644 --- a/components/esp_vfs_console/test_apps/usb_cdc_vfs/main/test_app_main.c +++ b/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(); } diff --git a/components/esp_vfs_console/test_apps/usb_cdc_vfs/pytest_usb_cdc_vfs.py b/components/esp_vfs_console/test_apps/usb_cdc_vfs/pytest_usb_cdc_vfs.py index 76ea464780..e1408e95a3 100644 --- a/components/esp_vfs_console/test_apps/usb_cdc_vfs/pytest_usb_cdc_vfs.py +++ b/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) diff --git a/components/esp_vfs_console/vfs_cdcacm.c b/components/esp_vfs_console/vfs_cdcacm.c index 8c2b5b2904..2575909cd4 100644 --- a/components/esp_vfs_console/vfs_cdcacm.c +++ b/components/esp_vfs_console/vfs_cdcacm.c @@ -176,13 +176,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 { @@ -190,21 +193,13 @@ static ssize_t cdcacm_read(int fd, void *data, size_t size) 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) {