Skip to content

Commit

Permalink
[Console Repl] fix deinit deadlock, by unblocking reads
Browse files Browse the repository at this point in the history
  • Loading branch information
@chipweinberger
chipweinberger committed Nov 2, 2022
1 parent 5f610a0 commit e96ca62
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 23 deletions.
39 changes: 37 additions & 2 deletions components/console/esp_console_repl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ typedef enum {
CONSOLE_REPL_STATE_DEINIT,
CONSOLE_REPL_STATE_INIT,
CONSOLE_REPL_STATE_START,
CONSOLE_REPL_STATE_REPL_STOP_REQUESTED,
CONSOLE_REPL_STATE_REPL_TASK_ENDED,
} repl_state_t;

typedef struct {
Expand Down Expand Up @@ -162,7 +164,7 @@ esp_err_t esp_console_new_repl_usb_serial_jtag(const esp_console_dev_usb_serial_
goto _exit;
}

/* Tell vfs to use usb-serial-jtag driver */
/* Tell vfs to use usb-serial-jtag driver*/
esp_vfs_usb_serial_jtag_use_driver();

// setup history
Expand Down Expand Up @@ -409,13 +411,25 @@ static esp_err_t esp_console_repl_uart_delete(esp_console_repl_t *repl)
esp_err_t ret = ESP_OK;
esp_console_repl_com_t *repl_com = __containerof(repl, esp_console_repl_com_t, repl_core);
esp_console_repl_universal_t *uart_repl = __containerof(repl_com, esp_console_repl_universal_t, repl_com);

// check if already de-initialized
if (repl_com->state == CONSOLE_REPL_STATE_DEINIT) {
ESP_LOGE(TAG, "already de-initialized");
ret = ESP_ERR_INVALID_STATE;
goto _exit;
}

// wait for repl thread to stop, if it is running
if (repl_com->state == CONSOLE_REPL_STATE_START) {
repl_com->state = CONSOLE_REPL_STATE_REPL_STOP_REQUESTED;
while (repl_com->state != CONSOLE_REPL_STATE_REPL_TASK_ENDED) {
uart_unblock_reads();
vTaskDelay(20 / portTICK_PERIOD_MS);
}
}

repl_com->state = CONSOLE_REPL_STATE_DEINIT;

esp_console_deinit();
esp_vfs_dev_uart_use_nonblocking(uart_repl->uart_channel);
uart_driver_delete(uart_repl->uart_channel);
Expand All @@ -429,13 +443,21 @@ static esp_err_t esp_console_repl_usb_cdc_delete(esp_console_repl_t *repl)
esp_err_t ret = ESP_OK;
esp_console_repl_com_t *repl_com = __containerof(repl, esp_console_repl_com_t, repl_core);
esp_console_repl_universal_t *cdc_repl = __containerof(repl_com, esp_console_repl_universal_t, repl_com);

// check if already de-initialized
if (repl_com->state == CONSOLE_REPL_STATE_DEINIT) {
ESP_LOGE(TAG, "already de-initialized");
ESP_LOGE(TAG, "already de-initialized");s
ret = ESP_ERR_INVALID_STATE;
goto _exit;
}

// TODO: wait for repl thread to stop, if it is running.
// Need to implement a USB CDC driver, and a
// corresponding usb_cdc_unblock_reads() function.
// See other esp_console_repl_X_delete() functions for reference.

repl_com->state = CONSOLE_REPL_STATE_DEINIT;

esp_console_deinit();
free(cdc_repl);
_exit:
Expand All @@ -454,7 +476,18 @@ static esp_err_t esp_console_repl_usb_serial_jtag_delete(esp_console_repl_t *rep
ret = ESP_ERR_INVALID_STATE;
goto _exit;
}

// wait for repl thread to stop, if it is running
if (repl_com->state == CONSOLE_REPL_STATE_START) {
repl_com->state = CONSOLE_REPL_STATE_REPL_STOP_REQUESTED;
while (repl_com->state != CONSOLE_REPL_STATE_REPL_TASK_ENDED) {
usb_serial_jtag_unblock_reads();
vTaskDelay(20 / portTICK_PERIOD_MS);
}
}

repl_com->state = CONSOLE_REPL_STATE_DEINIT;

esp_console_deinit();
esp_vfs_usb_serial_jtag_use_nonblocking();
usb_serial_jtag_driver_uninstall();
Expand Down Expand Up @@ -535,6 +568,8 @@ static void esp_console_repl_task(void *args)
/* linenoise allocates line buffer on the heap, so need to free it */
linenoiseFree(line);
}

repl_com->state = CONSOLE_REPL_STATE_REPL_TASK_ENDED;
ESP_LOGD(TAG, "The End");
vTaskDelete(NULL);
}
61 changes: 40 additions & 21 deletions components/console/linenoise/linenoise.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,8 @@ static void flushWrite(void) {
}

/* 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. */
* and return it. On read error -1 is returned, On cursor error, -2 is returned.
* On success the position of the cursor. */
static int getCursorPosition(void) {
char buf[LINENOISE_COMMAND_MAX_LEN] = { 0 };
int cols = 0;
Expand All @@ -248,10 +248,14 @@ static int getCursorPosition(void) {
* Stop right before the last character of the buffer, to be able to NULL
* terminate it. */
while (i < sizeof(buf)-1) {

int nread = read(in_fd, buf + i, 1);
if (nread == -1 && errno == EWOULDBLOCK) {return -1;} // read error

/* 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 (nread != 1 || buf[i] == 'R') {
/* If we couldn't read a byte from STDIN or if 'R' was received,
* the transmission is finished. */
break;
Expand All @@ -270,7 +274,8 @@ static int getCursorPosition(void) {

/* Parse the received data to get the position of the cursor. */
if (buf[0] != ESC || buf[1] != '[' || sscanf(buf+2,"%d;%d",&rows,&cols) != 2) {
return -1;
// cursor error
return -2;
}
return cols;
}
Expand All @@ -294,9 +299,8 @@ static int getColumns(void) {

/* Get the initial position so we can restore it later. */
start = getCursorPosition();
if (start == -1) {
goto failed;
}
if (start == -1 && errno == EWOULDBLOCK) {return -1;}
if (start == -2) {goto failed;}

/* Send the command to go to right margin. Use `write` function instead of
* `fwrite` for the same reasons explained in `getCursorPosition()` */
Expand All @@ -308,9 +312,8 @@ static int getColumns(void) {
/* After sending this command, we can get the new position of the cursor,
* we'd get the size, in columns, of the opened TTY. */
cols = getCursorPosition();
if (cols == -1) {
goto failed;
}
if (cols == -1 && errno == EWOULDBLOCK) {return -1;}
if (cols == -2) {goto failed;}

/* Restore the position of the cursor back. */
if (cols > start) {
Expand Down Expand Up @@ -819,6 +822,9 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt)
int out_fd = fileno(stdout);
int in_fd = fileno(stdin);

int cols = getColumns();
if (cols == -1 && errno == EWOULDBLOCK) {return -1;}

/* Populate the linenoise state that we pass to functions implementing
* specific editing functionalities. */
l.buf = buf;
Expand All @@ -827,9 +833,9 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt)
l.plen = strlen(prompt);
l.oldpos = l.pos = 0;
l.len = 0;
l.cols = getColumns();
l.maxrows = 0;
l.history_index = 0;
l.cols = cols;

/* Buffer starts empty. */
l.buf[0] = '\0';
Expand All @@ -840,11 +846,16 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt)
linenoiseHistoryAdd("");

int pos1 = getCursorPosition();
if (pos1 == -1 && errno == EWOULDBLOCK){return -1;}

if (write(out_fd, prompt,l.plen) == -1) {
return -1;
}
flushWrite();

int pos2 = getCursorPosition();
if (pos2 == -1 && errno == EWOULDBLOCK){return -1;}

if (pos1 >= 0 && pos2 >= 0) {
l.plen = pos2 - pos1;
}
Expand All @@ -864,7 +875,8 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt)
*/
t1 = getMillis();
nread = read(in_fd, &c, 1);
if (nread <= 0) return l.len;
if (nread == -1 && errno == EWOULDBLOCK) {return -1;}
if (nread <= 0){return l.len;}

if ( (getMillis() - t1) < LINENOISE_PASTE_KEY_DELAY && c != ENTER) {
/* Pasting data, insert characters without formatting.
Expand Down Expand Up @@ -940,19 +952,19 @@ 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 */
case ESC:{ /* escape sequence */
/* Read the next two bytes representing the escape sequence. */
if (read(in_fd, seq, 2) < 2) {
break;
}
int n = read(in_fd, seq, 2);
if (n == -1 && errno == EWOULDBLOCK) {return -1;}
if (n < 2) {break;}

/* ESC [ sequences. */
if (seq[0] == '[') {
if (seq[1] >= '0' && seq[1] <= '9') {
/* Extended escape, read additional byte. */
if (read(in_fd, seq+2, 1) == -1) {
break;
}
int n = read(in_fd, seq+2, 1);
if (n == -1 && errno == EWOULDBLOCK) {return -1;}
if (n == -1) {break;}
if (seq[2] == '~') {
switch(seq[1]) {
case '3': /* Delete key. */
Expand Down Expand Up @@ -996,6 +1008,7 @@ static int linenoiseEdit(char *buf, size_t buflen, const char *prompt)
}
}
break;
}
default:
if (linenoiseEditInsert(&l,c)) return -1;
break;
Expand Down Expand Up @@ -1054,6 +1067,8 @@ int linenoiseProbe(void) {
timeout_ms -= retry_ms;
char c;
int cb = read(stdin_fileno, &c, 1);
// Note! Due to the fcntl call above, this is read in non-blocking mode!
// So, nread == -1 && errno == EWOULDBLOCK are expected here!
if (cb < 0) {
continue;
}
Expand Down Expand Up @@ -1084,8 +1099,10 @@ static int linenoiseRaw(char *buf, size_t buflen, const char *prompt) {
}

count = linenoiseEdit(buf, buflen, prompt);
fputc('\n', stdout);
flushWrite();
if (count != -1){
fputc('\n', stdout);
flushWrite();
}
return count;
}

Expand All @@ -1095,6 +1112,8 @@ static int linenoiseDumb(char* buf, size_t buflen, const char* prompt) {
size_t count = 0;
while (count < buflen) {
int c = fgetc(stdin);
if (c == -1 && errno == EWOULDBLOCK) {return -1;}

if (c == '\n') {
break;
} else if (c >= 0x1c && c <= 0x1f){
Expand Down

0 comments on commit e96ca62

Please sign in to comment.