From 5b2f2e05f31583b555c780f4e800ecbddbf0c7eb Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 5 Mar 2025 12:23:08 +0100 Subject: [PATCH] fix(console): add ability to unblock linenoise, to fix deadlock Closes https://github.com/espressif/esp-idf/pull/10580 Closes https://github.com/espressif/esp-idf/issues/9974 --- components/console/esp_console_common.c | 48 +++++- components/console/esp_console_repl_chip.c | 64 +++++++- components/console/linenoise/linenoise.c | 137 ++++++++++++------ components/console/linenoise/linenoise.h | 2 + .../console/private_include/console_private.h | 3 + .../test_apps/console/main/test_console.c | 26 +++- .../test_apps/console/pytest_console.py | 5 + 7 files changed, 232 insertions(+), 53 deletions(-) diff --git a/components/console/esp_console_common.c b/components/console/esp_console_common.c index 395aa272f7..8bbe84ef19 100644 --- a/components/console/esp_console_common.c +++ b/components/console/esp_console_common.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -10,9 +10,14 @@ #include "console_private.h" #include "esp_log.h" #include "linenoise/linenoise.h" +#include "esp_vfs_eventfd.h" static const char *TAG = "console.common"; +static bool s_init = false; +static int s_interrupt_reading_fd = -1; +static uint64_t s_interrupt_reading_signal = 1; + esp_err_t esp_console_setup_prompt(const char *prompt, esp_console_repl_com_t *repl_com) { /* set command line prompt */ @@ -94,8 +99,33 @@ esp_err_t esp_console_common_init(size_t max_cmdline_length, esp_console_repl_co linenoiseSetCompletionCallback(&esp_console_get_completion); linenoiseSetHintsCallback((linenoiseHintsCallback *)&esp_console_get_hint); + /* Tell linenoise what file descriptor to add to the read file descriptor set, + * that will be used to signal a read termination */ + esp_vfs_eventfd_config_t config = ESP_VFS_EVENTD_CONFIG_DEFAULT(); + ret = esp_vfs_eventfd_register(&config); + if (ret == ESP_OK) { + /* first time calling the eventfd register function */ + s_interrupt_reading_fd = eventfd(0, 0); + linenoiseSetInterruptReadingData(s_interrupt_reading_fd, s_interrupt_reading_signal); + } else if (ret == ESP_ERR_INVALID_STATE) { + /* the evenfd has already been registered*/ + } else { + /* issue with arg, this should not happen */ + goto _exit; + } + + repl_com->state_mux = xSemaphoreCreateMutex(); + if (repl_com->state_mux == NULL) { + ESP_LOGE(TAG, "state_mux create error"); + ret = ESP_ERR_NO_MEM; + goto _exit; + } + xSemaphoreGive(repl_com->state_mux); + + s_init = true; return ESP_OK; _exit: + s_init = false; return ret; } @@ -126,6 +156,10 @@ void esp_console_repl_task(void *args) * function is called. */ ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + assert(repl_com->state_mux != NULL); + BaseType_t ret_val = xSemaphoreTake(repl_com->state_mux, portMAX_DELAY); + assert(ret_val == pdTRUE); + /* Change standard input and output of the task if the requested UART is * NOT the default one. This block will replace stdin, stdout and stderr. */ @@ -187,6 +221,18 @@ void esp_console_repl_task(void *args) /* linenoise allocates line buffer on the heap, so need to free it */ linenoiseFree(line); } + + xSemaphoreGive(repl_com->state_mux); ESP_LOGD(TAG, "The End"); vTaskDelete(NULL); } + +esp_err_t esp_console_interrupt_reading(void) +{ + if (!s_init) { + return ESP_FAIL; + } + ssize_t ret = write(s_interrupt_reading_fd, &s_interrupt_reading_signal, sizeof(s_interrupt_reading_signal)); + assert(ret == sizeof(s_interrupt_reading_signal)); + return ESP_OK; +} diff --git a/components/console/esp_console_repl_chip.c b/components/console/esp_console_repl_chip.c index f0d2f6e002..743db4d500 100644 --- a/components/console/esp_console_repl_chip.c +++ b/components/console/esp_console_repl_chip.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2016-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2016-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -12,13 +12,14 @@ #include "esp_err.h" #include "esp_log.h" #include "esp_console.h" -#include "esp_vfs_cdcacm.h" -#include "driver/usb_serial_jtag_vfs.h" #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "driver/uart.h" #include "driver/uart_vfs.h" #include "driver/usb_serial_jtag.h" +#include "driver/usb_serial_jtag_vfs.h" +#include "esp_private/usb_console.h" +#include "esp_vfs_cdcacm.h" #include "console_private.h" @@ -294,7 +295,26 @@ static esp_err_t esp_console_repl_uart_delete(esp_console_repl_t *repl) ret = ESP_ERR_INVALID_STATE; goto _exit; } + + // set the state to deinit to force the while loop in + // esp_console_repl_task to break repl_com->state = CONSOLE_REPL_STATE_DEINIT; + + const esp_err_t read_interrupted = esp_console_interrupt_reading(); + if (read_interrupted != ESP_OK) { + return ESP_FAIL; + } + + // wait for the task to notify that + // esp_console_repl_task returned + assert(repl_com->state_mux != NULL); + BaseType_t ret_val = xSemaphoreTake(repl_com->state_mux, portMAX_DELAY); + assert(ret_val == pdTRUE); + + // delete the semaphore for the repl state + vSemaphoreDelete(repl_com->state_mux); + repl_com->state_mux = NULL; + esp_console_deinit(); uart_vfs_dev_use_nonblocking(uart_repl->uart_channel); uart_driver_delete(uart_repl->uart_channel); @@ -316,7 +336,26 @@ static esp_err_t esp_console_repl_usb_cdc_delete(esp_console_repl_t *repl) ret = ESP_ERR_INVALID_STATE; goto _exit; } + + // set the state to deinit to force the while loop in + // esp_console_repl_task to break repl_com->state = CONSOLE_REPL_STATE_DEINIT; + + const esp_err_t read_interrupted = esp_console_interrupt_reading(); + if (read_interrupted != ESP_OK) { + return ESP_FAIL; + } + + // wait for the task to notify that + // esp_console_repl_task returned + assert(repl_com->state_mux != NULL); + BaseType_t ret_val = xSemaphoreTake(repl_com->state_mux, portMAX_DELAY); + assert(ret_val == pdTRUE); + + // delete the semaphore for the repl state + vSemaphoreDelete(repl_com->state_mux); + repl_com->state_mux = NULL; + esp_console_deinit(); free(cdc_repl); _exit: @@ -336,7 +375,26 @@ static esp_err_t esp_console_repl_usb_serial_jtag_delete(esp_console_repl_t *rep ret = ESP_ERR_INVALID_STATE; goto _exit; } + + // set the state to deinit to force the while loop in + // esp_console_repl_task to break repl_com->state = CONSOLE_REPL_STATE_DEINIT; + + const esp_err_t read_interrupted = esp_console_interrupt_reading(); + if (read_interrupted != ESP_OK) { + return ESP_FAIL; + } + + // wait for the task to notify that + // esp_console_repl_task returned + assert(repl_com->state_mux != NULL); + BaseType_t ret_val = xSemaphoreTake(repl_com->state_mux, portMAX_DELAY); + assert(ret_val == pdTRUE); + + // delete the semaphore for the repl state + vSemaphoreDelete(repl_com->state_mux); + repl_com->state_mux = NULL; + esp_console_deinit(); usb_serial_jtag_vfs_use_nonblocking(); usb_serial_jtag_driver_uninstall(); diff --git a/components/console/linenoise/linenoise.c b/components/console/linenoise/linenoise.c index b1f16db017..0725da3048 100644 --- a/components/console/linenoise/linenoise.c +++ b/components/console/linenoise/linenoise.c @@ -122,6 +122,7 @@ #include #include #include +#include #include #include #include "linenoise.h" @@ -144,6 +145,9 @@ static int history_len = 0; static char **history = NULL; static bool allow_empty = true; +static int intr_reading_fd = -1; +static uint64_t intr_reading_signal = -1; + /* The linenoiseState structure represents the state during line editing. * We pass this state to functions implementing specific editing * functionalities. */ @@ -239,6 +243,37 @@ static void flushWrite(void) { fsync(fileno(stdout)); } +static int linenoiseReadBytes(int fd, void *buf, size_t max_bytes) +{ + fd_set read_fds; + FD_ZERO(&read_fds); + FD_SET(fd, &read_fds); + if (intr_reading_fd !=-1) { + FD_SET(intr_reading_fd, &read_fds); + } + int maxFd = MAX(fd, intr_reading_fd); + /* call select to wait for either a read ready or an except to happen */ + int nread = select(maxFd + 1, &read_fds, NULL, NULL, NULL); + if (nread < 0) { + return -1; + } + + if(FD_ISSET(intr_reading_fd, &read_fds)) { + /* read termination request happened, return */ + int buf[sizeof(intr_reading_signal)]; + nread = read(intr_reading_fd, buf, sizeof(intr_reading_signal)); + if ((nread == sizeof(intr_reading_signal)) && (buf[0] == intr_reading_signal)) { + return -1; + } + } + else if(FD_ISSET(fd, &read_fds)) { + /* a read ready triggered the select to return. call the + * read function with the number of bytes max_bytes */ + nread = read(fd, buf, max_bytes); + } + return nread; +} + /* Use the ESC [6n escape sequence to query the horizontal cursor position * and return it. On error -1 is returned, on success the position of the * cursor. */ @@ -273,7 +308,7 @@ static int getCursorPosition(void) { /* Keep using unistd's functions. Here, using `read` instead of `fgets` * or `fgets` guarantees us that we we can read a byte regardless on * whether the sender sent end of line character(s) (CR, CRLF, LF). */ - if (read(in_fd, buf + i, 1) != 1 || buf[i] == 'R') { + if (linenoiseReadBytes(in_fd, buf + i, 1) != 1 || buf[i] == 'R') { /* If we couldn't read a byte from STDIN or if 'R' was received, * the transmission is finished. */ break; @@ -413,7 +448,7 @@ static int completeLine(struct linenoiseState *ls) { refreshLine(ls); } - nread = read(in_fd, &c, 1); + nread = linenoiseReadBytes(in_fd, &c, 1); if (nread <= 0) { freeCompletions(&lc); return -1; @@ -456,6 +491,12 @@ void linenoiseSetHintsCallback(linenoiseHintsCallback *fn) { hintsCallback = fn; } +void linenoiseSetInterruptReadingData(int fd, uint64_t data) +{ + intr_reading_fd = fd; + intr_reading_signal = data; +} + /* Register a function to free the hints returned by the hints callback * registered with linenoiseSetHintsCallback(). */ void linenoiseSetFreeHintsCallback(linenoiseFreeHintsCallback *fn) { @@ -890,8 +931,6 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt) while(1) { char c; - int nread; - char seq[3]; /** * To determine whether the user is pasting data or typing itself, we @@ -903,8 +942,10 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt) * about 40ms (or even more) */ t1 = getMillis(); - nread = read(in_fd, &c, 1); - if (nread <= 0) return l.len; + int nread = linenoiseReadBytes(in_fd, &c, 1); + if (nread <= 0) { + return l.len; + } if ( (getMillis() - t1) < LINENOISE_PASTE_KEY_DELAY && c != ENTER) { /* Pasting data, insert characters without formatting. @@ -946,7 +987,7 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt) errno = EAGAIN; return -1; case BACKSPACE: /* backspace */ - case 8: /* ctrl-h */ + case CTRL_H: /* ctrl-h */ linenoiseEditBackspace(&l); break; case CTRL_D: /* ctrl-d, remove char at right of cursor, or if the @@ -980,18 +1021,42 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt) case CTRL_N: /* ctrl-n */ linenoiseEditHistoryNext(&l, LINENOISE_HISTORY_NEXT); break; - case ESC: /* escape sequence */ - /* Read the next two bytes representing the escape sequence. */ - if (read(in_fd, seq, 2) < 2) { - break; - } - + case CTRL_U: /* Ctrl+u, delete the whole line. */ + buf[0] = '\0'; + l.pos = l.len = 0; + refreshLine(&l); + break; + case CTRL_K: /* Ctrl+k, delete from current to end of line. */ + buf[l.pos] = '\0'; + l.len = l.pos; + refreshLine(&l); + break; + case CTRL_A: /* Ctrl+a, go to the start of the line */ + linenoiseEditMoveHome(&l); + break; + case CTRL_E: /* ctrl+e, go to the end of the line */ + linenoiseEditMoveEnd(&l); + break; + case CTRL_L: /* ctrl+l, clear screen */ + linenoiseClearScreen(); + refreshLine(&l); + break; + case CTRL_W: /* ctrl+w, delete previous word */ + linenoiseEditDeletePrevWord(&l); + break; + case ESC: { /* escape sequence */ /* ESC [ sequences. */ + char seq[3]; + int r = linenoiseReadBytes(in_fd, seq, 2); + if (r != 2) { + return -1; + } if (seq[0] == '[') { if (seq[1] >= '0' && seq[1] <= '9') { /* Extended escape, read additional byte. */ - if (read(in_fd, seq+2, 1) == -1) { - break; + r = linenoiseReadBytes(in_fd, seq + 2, 1); + if (r != 1) { + return -1; } if (seq[2] == '~') { switch(seq[1]) { @@ -1023,7 +1088,6 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt) } } } - /* ESC O sequences. */ else if (seq[0] == 'O') { switch(seq[1]) { @@ -1036,32 +1100,10 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt) } } break; + } default: if (linenoiseEditInsert(&l,c)) return -1; break; - case CTRL_U: /* Ctrl+u, delete the whole line. */ - buf[0] = '\0'; - l.pos = l.len = 0; - refreshLine(&l); - break; - case CTRL_K: /* Ctrl+k, delete from current to end of line. */ - buf[l.pos] = '\0'; - l.len = l.pos; - refreshLine(&l); - break; - case CTRL_A: /* Ctrl+a, go to the start of the line */ - linenoiseEditMoveHome(&l); - break; - case CTRL_E: /* ctrl+e, go to the end of the line */ - linenoiseEditMoveEnd(&l); - break; - case CTRL_L: /* ctrl+l, clear screen */ - linenoiseClearScreen(); - refreshLine(&l); - break; - case CTRL_W: /* ctrl+w, delete previous word */ - linenoiseEditDeletePrevWord(&l); - break; } flushWrite(); } @@ -1103,12 +1145,7 @@ int linenoiseProbe(void) { } read_bytes += cb; } - /* Restore old mode */ - flags &= ~O_NONBLOCK; - res = fcntl(stdin_fileno, F_SETFL, flags); - if (res != 0) { - return -1; - } + if (read_bytes < 4) { return -2; } @@ -1133,9 +1170,17 @@ static int linenoiseDumb(char* buf, size_t buflen, const char* prompt) { /* dumb terminal, fall back to fgets */ fputs(prompt, stdout); flushWrite(); + size_t count = 0; + const int in_fd = fileno(stdin); + char c = 'c'; + while (count < buflen) { - int c = fgetc(stdin); + + int nread = linenoiseReadBytes(in_fd, &c, 1); + if (nread < 0) { + return nread; + } if (c == '\n') { break; } else if (c == BACKSPACE || c == CTRL_H) { diff --git a/components/console/linenoise/linenoise.h b/components/console/linenoise/linenoise.h index 98876104aa..37cbf4c978 100644 --- a/components/console/linenoise/linenoise.h +++ b/components/console/linenoise/linenoise.h @@ -45,6 +45,7 @@ extern "C" { #include #include +#include typedef struct linenoiseCompletions { size_t len; @@ -57,6 +58,7 @@ typedef void(linenoiseFreeHintsCallback)(void *); void linenoiseSetCompletionCallback(linenoiseCompletionCallback *); void linenoiseSetHintsCallback(linenoiseHintsCallback *); void linenoiseSetFreeHintsCallback(linenoiseFreeHintsCallback *); +void linenoiseSetInterruptReadingData(int, uint64_t); void linenoiseAddCompletion(linenoiseCompletions *, const char *); int linenoiseProbe(void); diff --git a/components/console/private_include/console_private.h b/components/console/private_include/console_private.h index c21144efcb..f95bcfb82b 100644 --- a/components/console/private_include/console_private.h +++ b/components/console/private_include/console_private.h @@ -11,6 +11,7 @@ #include "sdkconfig.h" #include "freertos/FreeRTOS.h" #include "freertos/task.h" +#include "freertos/semphr.h" #include "esp_console.h" #define CONSOLE_PROMPT_MAX_LEN (32) @@ -32,6 +33,7 @@ typedef struct { esp_console_repl_t repl_core; // base class char prompt[CONSOLE_PROMPT_MAX_LEN]; // Prompt to be printed before each line repl_state_t state; + SemaphoreHandle_t state_mux; const char *history_save_path; TaskHandle_t task_hdl; // REPL task handle size_t max_cmdline_length; // Maximum length of a command line. If 0, default value will be used. @@ -49,3 +51,4 @@ esp_err_t esp_console_setup_prompt(const char *prompt, esp_console_repl_com_t *r esp_err_t esp_console_setup_history(const char *history_path, uint32_t max_history_len, esp_console_repl_com_t *repl_com); +esp_err_t esp_console_interrupt_reading(void); diff --git a/components/console/test_apps/console/main/test_console.c b/components/console/test_apps/console/main/test_console.c index 1e56db049c..034b3b7362 100644 --- a/components/console/test_apps/console/main/test_console.c +++ b/components/console/test_apps/console/main/test_console.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -127,8 +127,7 @@ TEST_CASE("esp console init/deinit with context test", "[console]") static int do_cmd_quit(int argc, char **argv) { printf("ByeBye\r\n"); - s_repl->del(s_repl); - + TEST_ESP_OK(s_repl->del(s_repl)); linenoiseHistoryFree(); // Free up memory return 0; @@ -155,6 +154,27 @@ TEST_CASE("esp console repl test", "[console][ignore]") vTaskDelay(pdMS_TO_TICKS(2000)); } +TEST_CASE("esp console repl deinit", "[console][ignore]") +{ + esp_console_repl_config_t repl_config = ESP_CONSOLE_REPL_CONFIG_DEFAULT(); + esp_console_dev_uart_config_t uart_config = ESP_CONSOLE_DEV_UART_CONFIG_DEFAULT(); + TEST_ESP_OK(esp_console_new_repl_uart(&uart_config, &repl_config, &s_repl)); + + /* start the repl task */ + TEST_ESP_OK(esp_console_start_repl(s_repl)); + + /* wait to make sure the task reaches linenoiseEdit function + * and gets stuck in the select */ + vTaskDelay(pdMS_TO_TICKS(500)); + + /* call the delete function, this returns only when the repl task terminated */ + const esp_err_t res = s_repl->del(s_repl); + + /* if this point is reached, the repl environment has been deleted successfully */ + TEST_ASSERT(res == ESP_OK); + printf("-------------- %p\n", s_repl); +} + static const esp_console_cmd_t cmd_a = { .command = "aaa", .help = "should appear first in help", diff --git a/components/console/test_apps/console/pytest_console.py b/components/console/test_apps/console/pytest_console.py index cac89e155d..df7901ff36 100644 --- a/components/console/test_apps/console/pytest_console.py +++ b/components/console/test_apps/console/pytest_console.py @@ -12,6 +12,11 @@ def do_test_quit(dut: Dut) -> None: dut.confirm_write('quit', expect_str='ByeBye') +def do_test_repl_deinit(dut: Dut) -> None: + dut.expect_exact('Press ENTER to see the list of tests') + dut.confirm_write('"esp console repl deinit"', expect_str='esp>') + + def do_test_help_generic(dut: Dut, registration_order: str) -> None: dut.expect_exact('Press ENTER to see the list of tests') dut.confirm_write('"esp console help command - {} registration"'.format(registration_order), expect_str='esp>')