From 4b5e246b5be786c44edb095f09a16ba78d6e2c83 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Fri, 17 Jan 2025 07:45:12 +0100 Subject: [PATCH] fix(esp_vfs_console): USB CDC read when non blocking In non blocking mode, the read function is expected to return weather data is available for reading or not. In case data are available but the size does not match the expected size, the function read should return whatever data is available. Previously, the function was returning -1 with errno set to EWOULDBLOCK even if the size of data in the buffer was less than the requested size. It would only return the available data if the size in the buffer was greater or equal to the requested size. The implementation of cdcacm_read is modified to return the avilable data from the buffer even is the size is lesser than the requested size. --- .../include/esp_private/usb_console.h | 10 +- .../usb_cdc_vfs/main/test_app_main.c | 92 +++++++++++++++++++ .../usb_cdc_vfs/pytest_usb_cdc_vfs.py | 20 ++++ components/esp_vfs_console/vfs_cdcacm.c | 15 +-- 4 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 components/esp_vfs_console/test_apps/usb_cdc_vfs/main/test_app_main.c create mode 100644 components/esp_vfs_console/test_apps/usb_cdc_vfs/pytest_usb_cdc_vfs.py diff --git a/components/esp_system/include/esp_private/usb_console.h b/components/esp_system/include/esp_private/usb_console.h index c3877f245b..6387f8636f 100644 --- a/components/esp_system/include/esp_private/usb_console.h +++ b/components/esp_system/include/esp_private/usb_console.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2019-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2019-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -91,6 +91,14 @@ bool esp_usb_console_write_available(void); */ esp_err_t esp_usb_console_set_cb(esp_usb_console_cb_t rx_cb, esp_usb_console_cb_t tx_cb, void* arg); +/** + * @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 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 new file mode 100644 index 0000000000..4e0b84d5bb --- /dev/null +++ b/components/esp_vfs_console/test_apps/usb_cdc_vfs/main/test_app_main.c @@ -0,0 +1,92 @@ +/* + * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Unlicense OR CC0-1.0 + */ +#include +#include +#include +#include +#include "unity.h" +#include "esp_private/usb_console.h" +#include "esp_vfs_cdcacm.h" + +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 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_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_read_non_blocking(); +} 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 new file mode 100644 index 0000000000..b462d0f8b2 --- /dev/null +++ b/components/esp_vfs_console/test_apps/usb_cdc_vfs/pytest_usb_cdc_vfs.py @@ -0,0 +1,20 @@ +# SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD +# SPDX-License-Identifier: CC0-1.0 +import pytest +from pytest_embedded import Dut + + +@pytest.mark.esp32s3 +@pytest.mark.usb_device +@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', ['test123456789!@#%^&*']) +def test_usb_cdc_vfs_default(dut: Dut, test_message: str) -> None: + # 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') diff --git a/components/esp_vfs_console/vfs_cdcacm.c b/components/esp_vfs_console/vfs_cdcacm.c index 98b3330d54..3402e34220 100644 --- a/components/esp_vfs_console/vfs_cdcacm.c +++ b/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 */ @@ -151,13 +151,14 @@ 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) {