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 Oct 18, 2022
1 parent 716ac55 commit d0e12a6
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 31 deletions.
37 changes: 36 additions & 1 deletion 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 @@ -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
18 changes: 9 additions & 9 deletions docs/en/api-guides/usb-serial-jtag-console.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,26 @@ The USB Serial/JTAG Controller is able to put the {IDF_TARGET_NAME} into downloa
Limitations
===========

There are several limitations to the USB console feature. These may or may not be significant, depending on the type of application being developed, and the development workflow.
There are several limitations to the USB Serial/JTAG console feature. These may or may not be significant, depending on the type of application being developed, and the development workflow.

{IDF_TARGET_BOOT_PIN:default = "Not Updated!", esp32c3 = "GPIO9", esp32s3 = "GPIO0"}

1. If the application accidentally reconfigures the USB peripheral pins, or disables the USB Serial/JTAG Controller, the device will disappear from the system. After fixing the issue in the application, you will need to manually put the {IDF_TARGET_NAME} into download mode by pulling low {IDF_TARGET_BOOT_PIN} and resetting the chip.

2. If the application enters deep sleep mode, USB CDC device will disappear from the system.
2. If the application enters deep sleep mode, the USB Serial/JTAG device will disappear from the system.

3. The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different if the ESP-IDF application does not listen for incoming bytes. An USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will block until the application reads the bytes. This can lead to a non-responsive looking terminal program.
3. (Prior to ESP-IDF v5.0) The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different if the ESP-IDF application does not listen for incoming bytes. An USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will block until the application reads the bytes. This can lead to a non-responsive looking terminal program.

4. The USB CDC device won't work in sleep modes as normal due to the lack of APB clock in sleep modes. This includes deep-sleep, light-sleep (automataic light-sleep as well).
4. The USB Serial/JTAG device won't work in sleep modes as normal due to the lack of APB clock in sleep modes. This includes deep-sleep, light-sleep (automataic light-sleep as well).

5. The power consumption in sleep modes will be higher if the USB CDC device is in use.
5. The power consumption in sleep modes will be higher if the USB Serial/JTAG device is in use.

This is because we want to keep the USB CDC device alive during software reset by default.
This is because we want to keep the USB Serial/JTAG device alive during software reset by default.

However there is an issue that this might also increase the power consumption in sleep modes. This is because the software keeps a clock source on during the reset to keep the USB CDC device alive. As a side-effect, the clock is also kept on during sleep modes. There is one exception: the clock will only be kept on when your USB CDC port is really in use (like data transaction), therefore, if your USB CDC is connected to power bank or battery, etc., instead of a valid USB host (for example, a PC), the power consumption will not increase.
However there is an issue that this might also increase the power consumption in sleep modes. This is because the software keeps a clock source on during the reset to keep the USB Serial/JTAG device alive. As a side-effect, the clock is also kept on during sleep modes. There is one exception: the clock will only be kept on when your USB Serial/JTAG port is really in use (like data transaction), therefore, if your USB Serial/JTAG is connected to power bank or battery, etc., instead of a valid USB host (for example, a PC), the power consumption will not increase.

If you still want to keep low power consumption in sleep modes:

1. If you are not using the USB CDC port, you don't need to do anything. Software will detect if the CDC device is connected to a valid host before going to sleep, and keep the clocks only when the host is connected. Otherwise the clocks will be turned off as normal.
1. If you are not using the USB Serial/JTAG port, you don't need to do anything. Software will detect if the USB Serial/JTAG is connected to a valid host before going to sleep, and keep the clocks only when the host is connected. Otherwise the clocks will be turned off as normal.

2. If you are using the USB CDC port, please disable the menuconfig option ``CONFIG_RTC_CLOCK_BBPLL_POWER_ON_WITH_USB``. The clock will be switched off as normal during software reset and in sleep modes. In these cases, the USB CDC device may be unplugged from the host.
2. If you are using the USB Serial/JTAG port, please disable the menuconfig option ``CONFIG_RTC_CLOCK_BBPLL_POWER_ON_WITH_USB``. The clock will be switched off as normal during software reset and in sleep modes. In these cases, the USB Serial/JTAG device may be unplugged from the host.

0 comments on commit d0e12a6

Please sign in to comment.