fix(uart_vfs): read() now aligned to POSIX defined behavior

songruo · songruo · commit 32c17b419a6f · 2024-12-03T16:32:27.000+08:00
- For blocking mode, block until data available - Return with the bytes available in the file at the time, it should not block until reaching the requested size And read() should not realy return on the newline character Closes espressif#14155
diff --git a/components/esp_driver_uart/src/uart_vfs.c b/components/esp_driver_uart/src/uart_vfs.c
@@ -53,14 +53,18 @@
 typedef void (*tx_func_t)(int, int);
 // UART read bytes function type
 typedef int (*rx_func_t)(int);
+// UART get available received bytes function type
+typedef size_t (*get_available_data_len_func_t)(int);
 
-// Basic functions for sending and receiving bytes over UART
+// Basic functions for sending, receiving bytes, and get available data length over UART
 static void uart_tx_char(int fd, int c);
 static int uart_rx_char(int fd);
+static size_t uart_get_avail_data_len(int fd);
 
-// Functions for sending and receiving bytes which use UART driver
+// Functions for sending, receiving bytes, and get available data length which use UART driver
 static void uart_tx_char_via_driver(int fd, int c);
 static int uart_rx_char_via_driver(int fd);
+static size_t uart_get_avail_data_len_via_driver(int fd);
 
 typedef struct {
     // Pointers to UART peripherals
@@ -82,6 +86,8 @@ typedef struct {
     tx_func_t tx_func;
     // Functions used to read bytes from UART. Default to "basic" functions.
     rx_func_t rx_func;
+    // Function used to get available data bytes from UART. Default to "basic" functions.
+    get_available_data_len_func_t get_avail_data_len_func;
 } uart_vfs_context_t;
 
 #define VFS_CTX_DEFAULT_VAL(uart_dev) (uart_vfs_context_t) {\
@@ -91,6 +97,7 @@ typedef struct {
     .rx_mode = DEFAULT_RX_MODE,\
     .tx_func = uart_tx_char,\
     .rx_func = uart_rx_char,\
+    .get_avail_data_len_func = uart_get_avail_data_len,\
 }
 
 //If the context should be dynamically initialized, remove this structure
@@ -162,6 +169,19 @@ static int uart_open(const char *path, int flags, int mode)
     return fd;
 }
 
+size_t uart_get_avail_data_len(int fd)
+{
+    uart_dev_t* uart = s_ctx[fd]->uart;
+    return uart_ll_get_rxfifo_len(uart);
+}
+
+size_t uart_get_avail_data_len_via_driver(int fd)
+{
+    size_t buffered_size = 0;
+    uart_get_buffered_data_len(fd, &buffered_size);
+    return buffered_size;
+}
+
 static void uart_tx_char(int fd, int c)
 {
     uart_dev_t* uart = s_ctx[fd]->uart;
@@ -253,38 +273,65 @@ static ssize_t uart_read(int fd, void* data, size_t size)
     assert(fd >= 0 && fd < 3);
     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[fd]->read_lock);
-    while (received < size) {
-        int c = uart_read_char(fd);
-        if (c == '\r') {
-            if (s_ctx[fd]->rx_mode == ESP_LINE_ENDINGS_CR) {
-                c = '\n';
-            } else if (s_ctx[fd]->rx_mode == ESP_LINE_ENDINGS_CRLF) {
-                /* look ahead */
-                int c2 = uart_read_char(fd);
-                if (c2 == NONE) {
-                    /* could not look ahead, put the current character back */
-                    uart_return_char(fd, c);
-                    break;
-                }
-                if (c2 == '\n') {
-                    /* this was \r\n sequence. discard \r, return \n */
+
+    if (!s_ctx[fd]->non_blocking) {
+        c = uart_read_char(fd); // blocking until data available for non-O_NONBLOCK mode
+    }
+
+    // find the actual fetch size
+    available_size += s_ctx[fd]->get_avail_data_len_func(fd);
+    if (c != NONE) {
+        available_size++;
+    }
+    if (s_ctx[fd]->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 = uart_read_char(fd);
+            }
+            assert(c != NONE);
+
+            if (c == '\r') {
+                if (s_ctx[fd]->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.
-                     */
-                    uart_return_char(fd, c2);
+                } else if (s_ctx[fd]->rx_mode == ESP_LINE_ENDINGS_CRLF) {
+                    /* look ahead */
+                    int c2 = uart_read_char(fd);
+                    fetch_size--;
+                    if (c2 == NONE) {
+                        /* could not look ahead, put the current character back */
+                        uart_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.
+                         */
+                        uart_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
+        uart_return_char(fd, c);
     }
     _lock_release_recursive(&s_ctx[fd]->read_lock);
     if (received > 0) {
@@ -1061,6 +1108,7 @@ void uart_vfs_dev_use_nonblocking(int uart_num)
     _lock_acquire_recursive(&s_ctx[uart_num]->write_lock);
     s_ctx[uart_num]->tx_func = uart_tx_char;
     s_ctx[uart_num]->rx_func = uart_rx_char;
+    s_ctx[uart_num]->get_avail_data_len_func = uart_get_avail_data_len;
     _lock_release_recursive(&s_ctx[uart_num]->write_lock);
     _lock_release_recursive(&s_ctx[uart_num]->read_lock);
 }
@@ -1071,6 +1119,7 @@ void uart_vfs_dev_use_driver(int uart_num)
     _lock_acquire_recursive(&s_ctx[uart_num]->write_lock);
     s_ctx[uart_num]->tx_func = uart_tx_char_via_driver;
     s_ctx[uart_num]->rx_func = uart_rx_char_via_driver;
+    s_ctx[uart_num]->get_avail_data_len_func = uart_get_avail_data_len_via_driver;
     _lock_release_recursive(&s_ctx[uart_num]->write_lock);
     _lock_release_recursive(&s_ctx[uart_num]->read_lock);
 }
diff --git a/components/esp_driver_uart/test_apps/uart_vfs/main/test_vfs_uart.c b/components/esp_driver_uart/test_apps/uart_vfs/main/test_vfs_uart.c
@@ -74,6 +74,12 @@ TEST_CASE("CRs are removed from the stdin correctly", "[vfs_uart]")
     uart_vfs_dev_port_set_tx_line_endings(CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_CRLF);
 
     flush_stdin_stdout();
+
+    // A test case with no use of uart driver
+    // For non-uart-driver-involved uart vfs, all reads are non-blocking
+    // If no data at the moment, read() returns directly;
+    // If there is data available at the moment, read() also returns directly with the currently available size
+
     const char* send_str = "1234567890\n\r123\r\n4\n";
     /* with CONFIG_NEWLIB_STDOUT_ADDCR, the following will be sent on the wire.
      * (last character of each part is marked with a hat)
@@ -133,30 +139,46 @@ struct read_task_arg_t {
 
 struct write_task_arg_t {
     const char* str;
+    size_t str_len;
     SemaphoreHandle_t done;
 };
 
-static void read_task_fn(void* varg)
+static void read_blocking_task_fn(void* varg)
 {
     struct read_task_arg_t* parg = (struct read_task_arg_t*) varg;
-    parg->out_buffer[0] = 0;
+    memset(parg->out_buffer, 0, parg->out_buffer_len);
 
     fgets(parg->out_buffer, parg->out_buffer_len, stdin);
     xSemaphoreGive(parg->done);
     vTaskDelete(NULL);
 }
 
+static void read_non_blocking_task_fn(void* varg)
+{
+    struct read_task_arg_t* parg = (struct read_task_arg_t*) varg;
+    memset(parg->out_buffer, 0, parg->out_buffer_len);
+    char *ptr = parg->out_buffer;
+
+    while (fgets(ptr, parg->out_buffer_len, stdin) != NULL) {
+        while (*ptr != 0) {
+            ptr++;
+        }
+    }
+    xSemaphoreGive(parg->done);
+    vTaskDelete(NULL);
+}
+
 static void write_task_fn(void* varg)
 {
     struct write_task_arg_t* parg = (struct write_task_arg_t*) varg;
-    fwrite_str_loopback(parg->str, strlen(parg->str));
+    fwrite_str_loopback(parg->str, parg->str_len);
     xSemaphoreGive(parg->done);
     vTaskDelete(NULL);
 }
 
-TEST_CASE("can write to UART while another task is reading", "[vfs_uart]")
+TEST_CASE("read with uart driver (blocking)", "[vfs_uart]")
 {
-    char out_buffer[32];
+    char out_buffer[32] = {};
     size_t out_buffer_len = sizeof(out_buffer);
 
     struct read_task_arg_t read_arg = {
@@ -165,8 +187,12 @@ TEST_CASE("can write to UART while another task is reading", "[vfs_uart]")
         .done = xSemaphoreCreateBinary()
     };
 
+    // Send a string with length less than the read requested length
+    const char in_buffer[] = "!(@*#&(!*@&#((SDasdkjhadsl\n";
+    size_t in_buffer_len = sizeof(in_buffer);
     struct write_task_arg_t write_arg = {
-        .str = "!(@*#&(!*@&#((SDasdkjhadsl\n",
+        .str = in_buffer,
+        .str_len = in_buffer_len,
         .done = xSemaphoreCreateBinary()
     };
 
@@ -176,14 +202,18 @@ TEST_CASE("can write to UART while another task is reading", "[vfs_uart]")
                                         256, 0, 0, NULL, 0));
     uart_vfs_dev_use_driver(CONFIG_ESP_CONSOLE_UART_NUM);
 
-    xTaskCreate(&read_task_fn, "vfs_read", 4096, &read_arg, 5, NULL);
-    vTaskDelay(10);
+    // Start the read task first, it will block until data incoming
+    xTaskCreate(&read_blocking_task_fn, "vfs_read", 4096, &read_arg, 5, NULL);
+
+    int res = xSemaphoreTake(read_arg.done, 100 / portTICK_PERIOD_MS);
+    TEST_ASSERT_FALSE(res);
+
     xTaskCreate(&write_task_fn, "vfs_write", 4096, &write_arg, 6, NULL);
 
-    int res = xSemaphoreTake(write_arg.done, 100 / portTICK_PERIOD_MS);
+    res = xSemaphoreTake(write_arg.done, 100 / portTICK_PERIOD_MS);
     TEST_ASSERT(res);
 
-    res = xSemaphoreTake(read_arg.done, 100 / portTICK_PERIOD_MS);
+    res = xSemaphoreTake(read_arg.done, 100 / portTICK_PERIOD_MS); // read() returns with currently available size
     TEST_ASSERT(res);
 
     TEST_ASSERT_EQUAL(0, strcmp(write_arg.str, read_arg.out_buffer));
@@ -195,6 +225,69 @@ TEST_CASE("can write to UART while another task is reading", "[vfs_uart]")
     vTaskDelay(2);  // wait for tasks to exit
 }
 
+TEST_CASE("read with uart driver (non-blocking)", "[vfs_uart]")
+{
+    char out_buffer[32] = {};
+    size_t out_buffer_len = sizeof(out_buffer);
+
+    struct read_task_arg_t read_arg = {
+        .out_buffer = out_buffer,
+        .out_buffer_len = out_buffer_len,
+        .done = xSemaphoreCreateBinary()
+    };
+
+    // Send a string with length less than the read requested length
+    const char in_buffer[] = "!(@*#&(!*@&#((SDasdkjhad\nce"; // read should not early return on \n
+    size_t in_buffer_len = sizeof(in_buffer);
+    struct write_task_arg_t write_arg = {
+        .str = in_buffer,
+        .str_len = in_buffer_len,
+        .done = xSemaphoreCreateBinary()
+    };
+
+    flush_stdin_stdout();
+
+    ESP_ERROR_CHECK(uart_driver_install(CONFIG_ESP_CONSOLE_UART_NUM,
+                                        256, 0, 0, NULL, 0));
+    uart_vfs_dev_use_driver(CONFIG_ESP_CONSOLE_UART_NUM);
+
+    uart_vfs_dev_port_set_rx_line_endings(CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_LF);
+    uart_vfs_dev_port_set_tx_line_endings(CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_LF);
+
+    int flags = fcntl(STDIN_FILENO, F_GETFL, 0);
+    fcntl(STDIN_FILENO, F_SETFL, flags | O_NONBLOCK);
+
+    // If start the read task first, it will return immediately
+    xTaskCreate(&read_non_blocking_task_fn, "vfs_read", 4096, &read_arg, 5, NULL);
+
+    int res = xSemaphoreTake(read_arg.done, 100 / portTICK_PERIOD_MS);
+    TEST_ASSERT(res);
+
+    xTaskCreate(&write_task_fn, "vfs_write", 4096, &write_arg, 6, NULL);
+    vTaskDelay(10);
+    xTaskCreate(&read_non_blocking_task_fn, "vfs_read", 4096, &read_arg, 5, NULL);
+
+    res = xSemaphoreTake(write_arg.done, 100 / portTICK_PERIOD_MS);
+    TEST_ASSERT(res);
+
+    res = xSemaphoreTake(read_arg.done, 1000 / portTICK_PERIOD_MS); // read() returns with currently available size
+    TEST_ASSERT(res);
+
+    // string compare
+    for (int i = 0; i < in_buffer_len; i++) {
+        TEST_ASSERT_EQUAL(in_buffer[i], out_buffer[i]);
+    }
+
+    uart_vfs_dev_use_nonblocking(CONFIG_ESP_CONSOLE_UART_NUM);
+    fcntl(STDIN_FILENO, F_SETFL, flags);
+    uart_vfs_dev_port_set_rx_line_endings(CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_CRLF);
+    uart_vfs_dev_port_set_tx_line_endings(CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_CRLF);
+    uart_driver_delete(CONFIG_ESP_CONSOLE_UART_NUM);
+    vSemaphoreDelete(read_arg.done);
+    vSemaphoreDelete(write_arg.done);
+    vTaskDelay(2);  // wait for tasks to exit
+}
+
 TEST_CASE("fcntl supported in UART VFS", "[vfs_uart]")
 {
     int flags = fcntl(STDIN_FILENO, F_GETFL, 0);
