From 199205026e74294b844b457b0bba67763b2a52c2 Mon Sep 17 00:00:00 2001 From: Sonika Rathi Date: Mon, 22 May 2023 13:59:37 +0530 Subject: [PATCH] UART: UART_SELECT_WRITE_NOTIF event added in UART driver Closes https://github.com/espressif/esp-idf/issues/10986 --- components/driver/uart/uart.c | 33 +++++++++++--- components/vfs/test/test_vfs_select.c | 64 ++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 15 deletions(-) diff --git a/components/driver/uart/uart.c b/components/driver/uart/uart.c index 31dc602919..be1f6e1aca 100644 --- a/components/driver/uart/uart.c +++ b/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 */ @@ -771,7 +771,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 @@ -793,6 +795,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) { @@ -819,6 +822,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; @@ -841,6 +845,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. @@ -858,6 +863,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; @@ -905,13 +916,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); @@ -930,7 +944,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 @@ -971,6 +987,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); @@ -982,6 +999,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); @@ -990,6 +1008,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); @@ -1008,6 +1027,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)); @@ -1046,6 +1066,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 @@ -1060,14 +1081,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(); } } diff --git a/components/vfs/test/test_vfs_select.c b/components/vfs/test/test_vfs_select.c index 3dbc33dbf1..ceab7004f7 100644 --- a/components/vfs/test/test_vfs_select.c +++ b/components/vfs/test/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(¶m); - - 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(¶m); + 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);