vfs_uart: fix write operation blocked by a read

vfs_uart used same locks for read and write operations on a single UART.
If read operation was blocking (i.e. carried out via UART driver), the
lock was held by reading task until it received a line. During this time,
other tasks trying to write to the same UART would get blocked.

This change introduces separate read/write locks, and adds a test.

Another vfs_uart test if fixed (it was disabled since the
CONFIG_NEWLIB_STDOUT_ADDCR option was removed).

diff --git a/components/vfs/test/test_vfs_uart.c b/components/vfs/test/test_vfs_uart.c
index 2a74db61f795a7fbb7a18ec67ce451a20ff55b3b..20684cc2b425e321a714e5581605dd50d6a5c11c 100644 (file)
--- a/components/vfs/test/test_vfs_uart.c
+++ b/components/vfs/test/test_vfs_uart.c
@@ -20,6 +20,9 @@
 #include "soc/uart_struct.h"
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
+#include "freertos/semphr.h"
+#include "driver/uart.h"
+#include "esp_vfs_dev.h"
 #include "sdkconfig.h"
 
 static void fwrite_str_loopback(const char* str, size_t size)
@@ -68,10 +71,11 @@ TEST_CASE("can read from stdin", "[vfs]")
 }
 
 
-#if CONFIG_NEWLIB_STDOUT_ADDCR
-
 TEST_CASE("CRs are removed from the stdin correctly", "[vfs]")
 {
+    esp_vfs_dev_uart_set_rx_line_endings(ESP_LINE_ENDINGS_CRLF);
+    esp_vfs_dev_uart_set_tx_line_endings(ESP_LINE_ENDINGS_CRLF);
+
     flush_stdin_stdout();
     const char* send_str = "1234567890\n\r123\r\n4\n";
     /* with CONFIG_NEWLIB_STDOUT_ADDCR, the following will be sent on the wire.
@@ -123,5 +127,74 @@ TEST_CASE("CRs are removed from the stdin correctly", "[vfs]")
     TEST_ASSERT_EQUAL_UINT8_ARRAY("4\n", dst, 2);
 }
 
-#endif //CONFIG_NEWLIB_STDOUT_ADDCR
+TEST_CASE("can write to UART while another task is reading", "[vfs]")
+{
+    struct read_task_arg_t {
+        char* out_buffer;
+        size_t out_buffer_len;
+        SemaphoreHandle_t ready;
+        SemaphoreHandle_t done;
+    };
+
+    struct write_task_arg_t {
+        const char* str;
+        SemaphoreHandle_t done;
+    };
+
+    void read_task_fn(void* varg)
+    {
+        struct read_task_arg_t* parg = (struct read_task_arg_t*) varg;
+        parg->out_buffer[0] = 0;
+
+        fgets(parg->out_buffer, parg->out_buffer_len, stdin);
+        xSemaphoreGive(parg->done);
+        vTaskDelete(NULL);
+    }
+
+    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));
+        xSemaphoreGive(parg->done);
+        vTaskDelete(NULL);
+    }
+
+    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()
+    };
 
+    struct write_task_arg_t write_arg = {
+            .str = "!(@*#&(!*@&#((SDasdkjhadsl\n",
+            .done = xSemaphoreCreateBinary()
+    };
+
+    flush_stdin_stdout();
+
+    ESP_ERROR_CHECK( uart_driver_install(CONFIG_CONSOLE_UART_NUM,
+            256, 0, 0, NULL, 0) );
+    esp_vfs_dev_uart_use_driver(CONFIG_CONSOLE_UART_NUM);
+
+
+    xTaskCreate(&read_task_fn, "vfs_read", 4096, &read_arg, 5, NULL);
+    vTaskDelay(10);
+    xTaskCreate(&write_task_fn, "vfs_write", 4096, &write_arg, 6, NULL);
+
+
+    int res = xSemaphoreTake(write_arg.done, 100 / portTICK_PERIOD_MS);
+    TEST_ASSERT(res);
+
+    res = xSemaphoreTake(read_arg.done, 100 / portTICK_PERIOD_MS);
+    TEST_ASSERT(res);
+
+    TEST_ASSERT_EQUAL(0, strcmp(write_arg.str, read_arg.out_buffer));
+
+    esp_vfs_dev_uart_use_nonblocking(CONFIG_CONSOLE_UART_NUM);
+    uart_driver_delete(CONFIG_CONSOLE_UART_NUM);
+    vSemaphoreDelete(read_arg.done);
+    vSemaphoreDelete(write_arg.done);
+}

diff --git a/components/vfs/vfs_uart.c b/components/vfs/vfs_uart.c
index 0ec6cf6b0d62edb87c21b3cca5bc913e17ab0d90..e477e0e84e9a6ab252f28e2d987f45251e11f54b 100644 (file)
--- a/components/vfs/vfs_uart.c
+++ b/components/vfs/vfs_uart.c
@@ -47,7 +47,8 @@ static int uart_rx_char_via_driver(int fd);
 // Pointers to UART peripherals
 static uart_dev_t* s_uarts[UART_NUM] = {&UART0, &UART1, &UART2};
 // per-UART locks, lazily initialized
-static _lock_t s_uart_locks[UART_NUM];
+static _lock_t s_uart_read_locks[UART_NUM];
+static _lock_t s_uart_write_locks[UART_NUM];
 // One-character buffer used for newline conversion code, per UART
 static int s_peek_char[UART_NUM] = { NONE, NONE, NONE };
 // Per-UART non-blocking flag. Note: default implementation does not honor this
@@ -144,7 +145,7 @@ static ssize_t uart_write(int fd, const void * data, size_t size)
      *  a dedicated UART lock if two streams (stdout and stderr) point to the
      *  same UART.
      */
-    _lock_acquire_recursive(&s_uart_locks[fd]);
+    _lock_acquire_recursive(&s_uart_write_locks[fd]);
     for (size_t i = 0; i < size; i++) {
         int c = data_c[i];
         if (c == '\n' && s_tx_mode != ESP_LINE_ENDINGS_LF) {
@@ -155,7 +156,7 @@ static ssize_t uart_write(int fd, const void * data, size_t size)
         }
         s_uart_tx_func[fd](fd, c);
     }
-    _lock_release_recursive(&s_uart_locks[fd]);
+    _lock_release_recursive(&s_uart_write_locks[fd]);
     return size;
 }
 
@@ -186,7 +187,7 @@ 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;
-    _lock_acquire_recursive(&s_uart_locks[fd]);
+    _lock_acquire_recursive(&s_uart_read_locks[fd]);
     while (received < size) {
         int c = uart_read_char(fd);
         if (c == '\r') {
@@ -219,7 +220,7 @@ static ssize_t uart_read(int fd, void* data, size_t size)
             break;
         }
     }
-    _lock_release_recursive(&s_uart_locks[fd]);
+    _lock_release_recursive(&s_uart_read_locks[fd]);
     if (received > 0) {
         return received;
     }
@@ -286,16 +287,20 @@ void esp_vfs_dev_uart_set_tx_line_endings(esp_line_endings_t mode)
 
 void esp_vfs_dev_uart_use_nonblocking(int fd)
 {
-    _lock_acquire_recursive(&s_uart_locks[fd]);
+    _lock_acquire_recursive(&s_uart_read_locks[fd]);
+    _lock_acquire_recursive(&s_uart_write_locks[fd]);
     s_uart_tx_func[fd] = uart_tx_char;
     s_uart_rx_func[fd] = uart_rx_char;
-    _lock_release_recursive(&s_uart_locks[fd]);
+    _lock_release_recursive(&s_uart_write_locks[fd]);
+    _lock_release_recursive(&s_uart_read_locks[fd]);
 }
 
 void esp_vfs_dev_uart_use_driver(int fd)
 {
-    _lock_acquire_recursive(&s_uart_locks[fd]);
+    _lock_acquire_recursive(&s_uart_read_locks[fd]);
+    _lock_acquire_recursive(&s_uart_write_locks[fd]);
     s_uart_tx_func[fd] = uart_tx_char_via_driver;
     s_uart_rx_func[fd] = uart_rx_char_via_driver;
-    _lock_release_recursive(&s_uart_locks[fd]);
+    _lock_release_recursive(&s_uart_write_locks[fd]);
+    _lock_release_recursive(&s_uart_read_locks[fd]);
 }