Merge branch 'bugfix/uart_poll_fails_for_pollout_event' into 'master'

UART: UART_SELECT_WRITE_NOTIF event added in UART driver

Closes IDFGH-9640

See merge request espressif/esp-idf!23870

components/driver/uart/uart.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -839,7 +839,9 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
     uint32_t uart_intr_status = 0;
     uart_event_t uart_event;
     portBASE_TYPE HPTaskAwoken = 0;
+    bool need_yield = false;
     static uint8_t pat_flg = 0;
+    BaseType_t sent = pdFALSE;
     while (1) {
         // The `continue statement` may cause the interrupt to loop infinitely
         // we exit the interrupt here
@@ -861,6 +863,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
             if (p_uart->tx_waiting_fifo == true && p_uart->tx_buf_size == 0) {
                 p_uart->tx_waiting_fifo = false;
                 xSemaphoreGiveFromISR(p_uart->tx_fifo_sem, &HPTaskAwoken);
+                need_yield |= (HPTaskAwoken == pdTRUE);
             } else {
                 //We don't use TX ring buffer, because the size is zero.
                 if (p_uart->tx_buf_size == 0) {
@@ -887,6 +890,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
                                 }
                                 //We have saved the data description from the 1st item, return buffer.
                                 vRingbufferReturnItemFromISR(p_uart->tx_ring_buf, p_uart->tx_head, &HPTaskAwoken);
+                                need_yield |= (HPTaskAwoken == pdTRUE);
                             } else if (p_uart->tx_ptr == NULL) {
                                 //Update the TX item pointer, we will need this to return item to buffer.
                                 p_uart->tx_ptr = (uint8_t *)p_uart->tx_head;
@@ -909,6 +913,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
                         if (p_uart->tx_len_cur == 0) {
                             //Return item to ring buffer.
                             vRingbufferReturnItemFromISR(p_uart->tx_ring_buf, p_uart->tx_head, &HPTaskAwoken);
+                            need_yield |= (HPTaskAwoken == pdTRUE);
                             p_uart->tx_head = NULL;
                             p_uart->tx_ptr = NULL;
                             //Sending item done, now we need to send break if there is a record.
@@ -926,6 +931,12 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
                                 //enable TX empty interrupt
                                 en_tx_flg = true;
                             }
+                            UART_ENTER_CRITICAL_ISR(&uart_selectlock);
+                            if (p_uart->uart_select_notif_callback) {
+                                p_uart->uart_select_notif_callback(uart_num, UART_SELECT_WRITE_NOTIF, &HPTaskAwoken);
+                                need_yield |= (HPTaskAwoken == pdTRUE);
+                            }
+                            UART_EXIT_CRITICAL_ISR(&uart_selectlock);
                         } else {
                             //enable TX empty interrupt
                             en_tx_flg = true;
@@ -973,13 +984,16 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
                     UART_ENTER_CRITICAL_ISR(&uart_selectlock);
                     if (p_uart->uart_select_notif_callback) {
                         p_uart->uart_select_notif_callback(uart_num, UART_SELECT_READ_NOTIF, &HPTaskAwoken);
+                        need_yield |= (HPTaskAwoken == pdTRUE);
                     }
                     UART_EXIT_CRITICAL_ISR(&uart_selectlock);
                 }
                 p_uart->rx_stash_len = rx_fifo_len;
                 //If we fail to push data to ring buffer, we will have to stash the data, and send next time.
                 //Mainly for applications that uses flow control or small ring buffer.
-                if (pdFALSE == xRingbufferSendFromISR(p_uart->rx_ring_buf, p_uart->rx_data_buf, p_uart->rx_stash_len, &HPTaskAwoken)) {
+                sent = xRingbufferSendFromISR(p_uart->rx_ring_buf, p_uart->rx_data_buf, p_uart->rx_stash_len, &HPTaskAwoken);
+                need_yield |= (HPTaskAwoken == pdTRUE);
+                if (sent == pdFALSE) {
                     p_uart->rx_buffer_full_flg = true;
                     UART_ENTER_CRITICAL_ISR(&(uart_context[uart_num].spinlock));
                     uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), UART_INTR_RXFIFO_TOUT | UART_INTR_RXFIFO_FULL);
@@ -998,7 +1012,9 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
                                                  p_uart->rx_buffered_len + pat_idx);
                         }
                         UART_EXIT_CRITICAL_ISR(&(uart_context[uart_num].spinlock));
-                        if ((p_uart->event_queue != NULL) && (pdFALSE == xQueueSendFromISR(p_uart->event_queue, (void * )&uart_event, &HPTaskAwoken))) {
+                        sent = xQueueSendFromISR(p_uart->event_queue, (void * )&uart_event, &HPTaskAwoken);
+                        need_yield |= (HPTaskAwoken == pdTRUE);
+                        if ((p_uart->event_queue != NULL) && (sent == pdFALSE)) {
 #ifndef CONFIG_UART_ISR_IN_IRAM     //Only log if ISR is not in IRAM
                             ESP_EARLY_LOGV(UART_TAG, "UART event queue full");
 #endif
@@ -1039,6 +1055,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
             UART_ENTER_CRITICAL_ISR(&uart_selectlock);
             if (p_uart->uart_select_notif_callback) {
                 p_uart->uart_select_notif_callback(uart_num, UART_SELECT_ERROR_NOTIF, &HPTaskAwoken);
+                need_yield |= (HPTaskAwoken == pdTRUE);
             }
             UART_EXIT_CRITICAL_ISR(&uart_selectlock);
             uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), UART_INTR_RXFIFO_OVF);
@@ -1050,6 +1067,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
             UART_ENTER_CRITICAL_ISR(&uart_selectlock);
             if (p_uart->uart_select_notif_callback) {
                 p_uart->uart_select_notif_callback(uart_num, UART_SELECT_ERROR_NOTIF, &HPTaskAwoken);
+                need_yield |= (HPTaskAwoken == pdTRUE);
             }
             UART_EXIT_CRITICAL_ISR(&uart_selectlock);
             uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), UART_INTR_FRAM_ERR);
@@ -1058,6 +1076,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
             UART_ENTER_CRITICAL_ISR(&uart_selectlock);
             if (p_uart->uart_select_notif_callback) {
                 p_uart->uart_select_notif_callback(uart_num, UART_SELECT_ERROR_NOTIF, &HPTaskAwoken);
+                need_yield |= (HPTaskAwoken == pdTRUE);
             }
             UART_EXIT_CRITICAL_ISR(&uart_selectlock);
             uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), UART_INTR_PARITY_ERR);
@@ -1076,6 +1095,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
                 p_uart->tx_waiting_brk = 0;
             } else {
                 xSemaphoreGiveFromISR(p_uart->tx_brk_sem, &HPTaskAwoken);
+                need_yield |= (HPTaskAwoken == pdTRUE);
             }
         } else if (uart_intr_status & UART_INTR_TX_BRK_IDLE) {
             UART_ENTER_CRITICAL_ISR(&(uart_context[uart_num].spinlock));
@@ -1114,6 +1134,7 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
                 }
                 UART_EXIT_CRITICAL_ISR(&(uart_context[uart_num].spinlock));
                 xSemaphoreGiveFromISR(p_uart_obj[uart_num]->tx_done_sem, &HPTaskAwoken);
+                need_yield |= (HPTaskAwoken == pdTRUE);
             }
         }
     #if SOC_UART_SUPPORT_WAKEUP_INT
@@ -1128,14 +1149,16 @@ static void UART_ISR_ATTR uart_rx_intr_handler_default(void *param)
         }
 
         if (uart_event.type != UART_EVENT_MAX && p_uart->event_queue) {
-            if (pdFALSE == xQueueSendFromISR(p_uart->event_queue, (void * )&uart_event, &HPTaskAwoken)) {
+            sent = xQueueSendFromISR(p_uart->event_queue, (void * )&uart_event, &HPTaskAwoken);
+            need_yield |= (HPTaskAwoken == pdTRUE);
+            if (sent == pdFALSE) {
 #ifndef CONFIG_UART_ISR_IN_IRAM     //Only log if ISR is not in IRAM
                 ESP_EARLY_LOGV(UART_TAG, "UART event queue full");
 #endif
             }
         }
     }
-    if (HPTaskAwoken == pdTRUE) {
+    if (need_yield) {
         portYIELD_FROM_ISR();
     }
 }

components/vfs/test_apps/main/test_vfs_select.c

@@ -199,7 +199,7 @@ TEST_CASE("UART can do select()", "[vfs]")
     deinit(uart_fd, socket_fd);
 }
 
-TEST_CASE("UART can do poll()", "[vfs]")
+TEST_CASE("UART can do poll() with POLLIN event", "[vfs]")
 {
     int uart_fd;
     int socket_fd;
@@ -259,6 +259,50 @@ TEST_CASE("UART can do poll()", "[vfs]")
 
     deinit(uart_fd, socket_fd);
 }
+
+TEST_CASE("UART can do poll() with POLLOUT event", "[vfs]")
+{
+    int uart_fd;
+    int socket_fd;
+    char recv_message[sizeof(message)];
+
+    init(&uart_fd, &socket_fd);
+
+    struct pollfd poll_fds[] = {
+        {
+            .fd = uart_fd,
+            .events = POLLOUT,
+        },
+        {
+            .fd = -1,  // should be ignored according to the documentation of poll()
+        },
+    };
+
+    const test_task_param_t test_task_param = {
+        .fd = uart_fd,
+        .delay_ms = 50,
+        .sem = xSemaphoreCreateBinary(),
+    };
+    TEST_ASSERT_NOT_NULL(test_task_param.sem);
+    start_task(&test_task_param);
+
+    poll(poll_fds, sizeof(poll_fds)/sizeof(poll_fds[0]), 100);
+    TEST_ASSERT_EQUAL(uart_fd, poll_fds[0].fd);
+    TEST_ASSERT_EQUAL(POLLOUT, poll_fds[0].revents);
+    TEST_ASSERT_EQUAL(-1, poll_fds[1].fd);
+    TEST_ASSERT_EQUAL(0, poll_fds[1].revents);
+
+    int read_bytes = read(uart_fd, recv_message, sizeof(message));
+    TEST_ASSERT_EQUAL(read_bytes, sizeof(message));
+    TEST_ASSERT_EQUAL_MEMORY(message, recv_message, sizeof(message));
+
+    TEST_ASSERT_EQUAL(xSemaphoreTake(test_task_param.sem, 1000 / portTICK_PERIOD_MS), pdTRUE);
+
+    vSemaphoreDelete(test_task_param.sem);
+
+    deinit(uart_fd, socket_fd);
+}
+
 #endif
 
 TEST_CASE("socket can do select()", "[vfs]")
@@ -509,19 +553,11 @@ TEST_CASE("concurrent selects work", "[vfs]")
             .errfds = NULL,
             .maxfds = uart_fd + 1,
             .tv = &tv,
-            .select_ret = 0, // expected timeout
+            .select_ret = 1,
             .sem = xSemaphoreCreateBinary(),
         };
         TEST_ASSERT_NOT_NULL(param.sem);
 
-        start_select_task(&param);
-
-        fd_set rdfds2;
-        FD_ZERO(&rdfds2);
-        FD_SET(uart_fd, &rdfds2);
-        FD_SET(socket_fd, &rdfds2);
-        FD_SET(dummy_socket_fd, &rdfds2);
-
         const test_task_param_t send_param = {
             .fd = uart_fd,
             .delay_ms = 50,
@@ -529,6 +565,14 @@ TEST_CASE("concurrent selects work", "[vfs]")
         };
         TEST_ASSERT_NOT_NULL(send_param.sem);
         start_task(&send_param);        // This task will write to UART which will be detected by select()
+        start_select_task(&param);
+        vTaskDelay(100 / portTICK_PERIOD_MS); //make sure the task has started and waits in select()
+
+        fd_set rdfds2;
+        FD_ZERO(&rdfds2);
+        FD_SET(uart_fd, &rdfds2);
+        FD_SET(socket_fd, &rdfds2);
+        FD_SET(dummy_socket_fd, &rdfds2);
 
         int s = select(MAX(MAX(uart_fd, dummy_socket_fd), socket_fd) + 1, &rdfds2, NULL, NULL, &tv);
         TEST_ASSERT_EQUAL(1, s);