| Message ID | 20230106145243.411626-3-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | b08e9d4b6632a72b91306690d552c125b071441e |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | bootstd: Add a boot menu | expand |
On 1/6/23 15:52, Simon Glass wrote: > The current cread_line() function is very long. It handles the escape > processing inline. The menu command does similar processing but at the > character level, so there is some duplication. > > Split the character processing into a new function cli_ch_process() which > processes individual characters and returns the resulting input character, > taking account of escape sequences. It requires the caller to set up and > maintain its state. > > Update cread_line() to use this new function. > > The only intended functional change is that an invalid escape sequence > does not add invalid/control characters into the input buffer, but instead > discards these. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > (no changes since v1) > > common/Makefile | 6 +- > common/cli_getch.c | 204 ++++++++++++++++++++++++++++++++++++++++++ > common/cli_readline.c | 150 +++++-------------------------- > include/cli.h | 72 +++++++++++++++ > 4 files changed, 301 insertions(+), 131 deletions(-) > create mode 100644 common/cli_getch.c > > diff --git a/common/Makefile b/common/Makefile > index 20addfb244c..67485e77a04 100644 > --- a/common/Makefile > +++ b/common/Makefile > @@ -38,7 +38,7 @@ obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o > obj-$(CONFIG_MENU) += menu.o > obj-$(CONFIG_UPDATE_COMMON) += update.o > obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o > -obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o > +obj-$(CONFIG_CMDLINE) += cli_getch.o cli_readline.o cli_simple.o > > endif # !CONFIG_SPL_BUILD > > @@ -93,8 +93,8 @@ obj-y += eeprom/eeprom_field.o eeprom/eeprom_layout.o > endif > > obj-y += cli.o > -obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_simple.o cli_readline.o > -obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_simple.o cli_readline.o > +obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o > +obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o > obj-$(CONFIG_DFU_OVER_USB) += dfu.o > obj-y += command.o > obj-$(CONFIG_$(SPL_TPL_)LOG) += log.o > diff --git a/common/cli_getch.c b/common/cli_getch.c > new file mode 100644 > index 00000000000..9eeea7fef29 > --- /dev/null > +++ b/common/cli_getch.c > @@ -0,0 +1,204 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * (C) Copyright 2000 > + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. > + * > + * Copyright 2022 Google LLC > + */ > + > +#include <common.h> > +#include <cli.h> > + > +/** > + * enum cli_esc_state_t - indicates what to do with an escape character > + * > + * @ESC_REJECT: Invalid escape sequence, so the esc_save[] characters are > + * returned from each subsequent call to cli_ch_esc() > + * @ESC_SAVE: Character should be saved in esc_save until we have another one > + * @ESC_CONVERTED: Escape sequence has been completed and the resulting > + * character is available > + */ > +enum cli_esc_state_t { > + ESC_REJECT, > + ESC_SAVE, > + ESC_CONVERTED > +}; > + > +void cli_ch_init(struct cli_ch_state *cch) > +{ > + memset(cch, '\0', sizeof(*cch)); > +} > + > +/** > + * cli_ch_esc() - Process a character in an ongoing escape sequence > + * > + * @cch: State information > + * @ichar: Character to process > + * @actp: Returns the action to take > + * Returns: Output character if *actp is ESC_CONVERTED, else 0 > + */ > +static int cli_ch_esc(struct cli_ch_state *cch, int ichar, > + enum cli_esc_state_t *actp) > +{ > + enum cli_esc_state_t act = ESC_REJECT; > + > + switch (cch->esc_len) { > + case 1: > + if (ichar == '[' || ichar == 'O') > + act = ESC_SAVE; > + break; > + case 2: > + switch (ichar) { > + case 'D': /* <- key */ > + ichar = CTL_CH('b'); > + act = ESC_CONVERTED; > + break; /* pass off to ^B handler */ > + case 'C': /* -> key */ > + ichar = CTL_CH('f'); > + act = ESC_CONVERTED; > + break; /* pass off to ^F handler */ > + case 'H': /* Home key */ > + ichar = CTL_CH('a'); > + act = ESC_CONVERTED; > + break; /* pass off to ^A handler */ > + case 'F': /* End key */ > + ichar = CTL_CH('e'); > + act = ESC_CONVERTED; > + break; /* pass off to ^E handler */ > + case 'A': /* up arrow */ > + ichar = CTL_CH('p'); > + act = ESC_CONVERTED; > + break; /* pass off to ^P handler */ > + case 'B': /* down arrow */ > + ichar = CTL_CH('n'); > + act = ESC_CONVERTED; > + break; /* pass off to ^N handler */ > + case '1': > + case '2': > + case '3': > + case '4': > + case '7': > + case '8': > + if (cch->esc_save[1] == '[') { > + /* see if next character is ~ */ > + act = ESC_SAVE; > + } > + break; > + } > + break; > + case 3: > + switch (ichar) { > + case '~': > + switch (cch->esc_save[2]) { > + case '3': /* Delete key */ > + ichar = CTL_CH('d'); > + act = ESC_CONVERTED; > + break; /* pass to ^D handler */ > + case '1': /* Home key */ > + case '7': > + ichar = CTL_CH('a'); > + act = ESC_CONVERTED; > + break; /* pass to ^A handler */ > + case '4': /* End key */ > + case '8': > + ichar = CTL_CH('e'); > + act = ESC_CONVERTED; > + break; /* pass to ^E handler */ > + } > + break; > + case '0': > + if (cch->esc_save[2] == '2') > + act = ESC_SAVE; > + break; > + } > + break; > + case 4: > + switch (ichar) { > + case '0': > + case '1': > + act = ESC_SAVE; > + break; /* bracketed paste */ > + } > + break; > + case 5: > + if (ichar == '~') { /* bracketed paste */ > + ichar = 0; > + act = ESC_CONVERTED; > + } > + } > + > + *actp = act; > + > + return act == ESC_CONVERTED ? ichar : 0; > +} > + > +int cli_ch_process(struct cli_ch_state *cch, int ichar) > +{ > + /* > + * ichar=0x0 when error occurs in U-Boot getchar() or when the caller > + * wants to check if there are more characters saved in the escape > + * sequence > + */ > + if (!ichar) { > + if (cch->emit_upto) { > + if (cch->emit_upto < cch->esc_len) > + return cch->esc_save[cch->emit_upto++]; > + cch->emit_upto = 0; > + } > + return 0; > + } else if (ichar == -ETIMEDOUT) { > + /* > + * If we are in an escape sequence but nothing has followed the > + * Escape character, then the user probably just pressed the > + * Escape key. Return it and clear the sequence. > + */ > + if (cch->esc_len) { > + cch->esc_len = 0; > + return '\e'; > + } > + > + /* Otherwise there is nothing to return */ > + return 0; > + } > + > + if (ichar == '\n' || ichar == '\r') > + return '\n'; > + > + /* handle standard linux xterm esc sequences for arrow key, etc. */ > + if (cch->esc_len != 0) { > + enum cli_esc_state_t act; > + > + ichar = cli_ch_esc(cch, ichar, &act); > + > + switch (act) { > + case ESC_SAVE: > + /* save this character and return nothing */ > + cch->esc_save[cch->esc_len++] = ichar; > + return 0; > + case ESC_REJECT: > + /* > + * invalid escape sequence, start returning the > + * characters in it > + */ > + cch->esc_save[cch->esc_len++] = ichar; > + return cch->esc_save[cch->emit_upto++]; > + case ESC_CONVERTED: > + /* valid escape sequence, return the resulting char */ > + cch->esc_len = 0; > + return ichar; > + } > + } > + > + if (ichar == '\e') { > + if (!cch->esc_len) { > + cch->esc_save[cch->esc_len] = ichar; > + cch->esc_len = 1; > + } else { > + puts("impossible condition #876\n"); > + cch->esc_len = 0; > + } > + return 0; > + } > + > + return ichar; > +} > diff --git a/common/cli_readline.c b/common/cli_readline.c > index d6444f5fc1d..709e9c3d38b 100644 > --- a/common/cli_readline.c > +++ b/common/cli_readline.c > @@ -62,7 +62,6 @@ static char *delete_char (char *buffer, char *p, int *colp, int *np, int plen) > > #define putnstr(str, n) printf("%.*s", (int)n, str) > > -#define CTL_CH(c) ((c) - 'a' + 1) > #define CTL_BACKSPACE ('\b') > #define DEL ((char)255) > #define DEL7 ((char)127) > @@ -252,156 +251,53 @@ static void cread_add_str(char *str, int strsize, int insert, > static int cread_line(const char *const prompt, char *buf, unsigned int *len, > int timeout) > { > + struct cli_ch_state s_cch, *cch = &s_cch; > unsigned long num = 0; > unsigned long eol_num = 0; > unsigned long wlen; > char ichar; > int insert = 1; > - int esc_len = 0; > - char esc_save[8]; > int init_len = strlen(buf); > int first = 1; > > + cli_ch_init(cch); > + > if (init_len) > cread_add_str(buf, init_len, 1, &num, &eol_num, buf, *len); > > while (1) { > - if (bootretry_tstc_timeout()) > - return -2; /* timed out */ > - if (first && timeout) { > - uint64_t etime = endtick(timeout); > - > - while (!tstc()) { /* while no incoming data */ > - if (get_ticks() >= etime) > - return -2; /* timed out */ > - schedule(); > + /* Check for saved characters */ > + ichar = cli_ch_process(cch, 0); > + > + if (!ichar) { > + if (bootretry_tstc_timeout()) > + return -2; /* timed out */ > + if (first && timeout) { > + u64 etime = endtick(timeout); > + > + while (!tstc()) { /* while no incoming data */ > + if (get_ticks() >= etime) > + return -2; /* timed out */ > + schedule(); > + } > + first = 0; > } > - first = 0; > + > + ichar = getcmd_getch(); > } > > - ichar = getcmd_getch(); > + ichar = cli_ch_process(cch, ichar); > > /* ichar=0x0 when error occurs in U-Boot getc */ > if (!ichar) > continue; > > - if ((ichar == '\n') || (ichar == '\r')) { > + if (ichar == '\n') { > putc('\n'); > break; > } > > - /* > - * handle standard linux xterm esc sequences for arrow key, etc. > - */ > - if (esc_len != 0) { > - enum { ESC_REJECT, ESC_SAVE, ESC_CONVERTED } act = ESC_REJECT; > - > - if (esc_len == 1) { > - if (ichar == '[' || ichar == 'O') > - act = ESC_SAVE; > - } else if (esc_len == 2) { > - switch (ichar) { > - case 'D': /* <- key */ > - ichar = CTL_CH('b'); > - act = ESC_CONVERTED; > - break; /* pass off to ^B handler */ We have similar code to decode escape sequences and Unicode letters in efi_cin_read_key_stroke_ex(). The big difference in the EFI code is that it does not use dummy control characters but uses a structure: struct efi_key_data { struct efi_input_key key; struct efi_key_state key_state; }; with struct efi_input_key { u16 scan_code; s16 unicode_char; }; The special keys are represented as numbers in scan_code while characters are represented in unicode_char. Separating special keys from characters is much cleaner than creating dummy control characters. Anyway you would be able to first create the structure above and then in a separate step to convert to control characters. The code below misses to interprete some special keys like FN1 - FN12 which could be quite useful for navigating a UI. Best regards Heinrich > - case 'C': /* -> key */ > - ichar = CTL_CH('f'); > - act = ESC_CONVERTED; > - break; /* pass off to ^F handler */ > - case 'H': /* Home key */ > - ichar = CTL_CH('a'); > - act = ESC_CONVERTED; > - break; /* pass off to ^A handler */ > - case 'F': /* End key */ > - ichar = CTL_CH('e'); > - act = ESC_CONVERTED; > - break; /* pass off to ^E handler */ > - case 'A': /* up arrow */ > - ichar = CTL_CH('p'); > - act = ESC_CONVERTED; > - break; /* pass off to ^P handler */ > - case 'B': /* down arrow */ > - ichar = CTL_CH('n'); > - act = ESC_CONVERTED; > - break; /* pass off to ^N handler */ > - case '1': > - case '2': > - case '3': > - case '4': > - case '7': > - case '8': > - if (esc_save[1] == '[') { > - /* see if next character is ~ */ > - act = ESC_SAVE; > - } > - break; > - } > - } else if (esc_len == 3) { > - switch (ichar) { > - case '~': > - switch (esc_save[2]) { > - case '3': /* Delete key */ > - ichar = CTL_CH('d'); > - act = ESC_CONVERTED; > - break; /* pass to ^D handler */ > - case '1': /* Home key */ > - case '7': > - ichar = CTL_CH('a'); > - act = ESC_CONVERTED; > - break; /* pass to ^A handler */ > - case '4': /* End key */ > - case '8': > - ichar = CTL_CH('e'); > - act = ESC_CONVERTED; > - break; /* pass to ^E handler */ > - } > - break; > - case '0': > - if (esc_save[2] == '2') > - act = ESC_SAVE; > - break; > - } > - } else if (esc_len == 4) { > - switch (ichar) { > - case '0': > - case '1': > - act = ESC_SAVE; > - break; /* bracketed paste */ > - } > - } else if (esc_len == 5) { > - if (ichar == '~') { /* bracketed paste */ > - ichar = 0; > - act = ESC_CONVERTED; > - } > - } > - switch (act) { > - case ESC_SAVE: > - esc_save[esc_len++] = ichar; > - continue; > - case ESC_REJECT: > - esc_save[esc_len++] = ichar; > - cread_add_str(esc_save, esc_len, insert, > - &num, &eol_num, buf, *len); > - esc_len = 0; > - continue; > - case ESC_CONVERTED: > - esc_len = 0; > - break; > - } > - } > - > switch (ichar) { > - case 0x1b: > - if (esc_len == 0) { > - esc_save[esc_len] = ichar; > - esc_len = 1; > - } else { > - puts("impossible condition #876\n"); > - esc_len = 0; > - } > - break; > - > case CTL_CH('a'): > BEGINNING_OF_LINE(); > break; > @@ -470,8 +366,6 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, > { > char *hline; > > - esc_len = 0; > - > if (ichar == CTL_CH('p')) > hline = hist_prev(); > else > diff --git a/include/cli.h b/include/cli.h > index ba5b8ebd36e..863519e4b13 100644 > --- a/include/cli.h > +++ b/include/cli.h > @@ -7,6 +7,21 @@ > #ifndef __CLI_H > #define __CLI_H > > +#include <stdbool.h> > + > +/** > + * struct cli_ch_state - state information for reading cmdline characters > + * > + * @esc_len: Number of escape characters read so far > + * @esc_save: Escape characters collected so far > + * @emit_upto: Next character to emit from esc_save (0 if not emitting) > + */ > +struct cli_ch_state { > + int esc_len; > + char esc_save[8]; > + int emit_upto; > +}; > + > /** > * Go into the command loop > * > @@ -154,5 +169,62 @@ void cli_loop(void); > void cli_init(void); > > #define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk()) > +#define CTL_CH(c) ((c) - 'a' + 1) > + > +/** > + * cli_ch_init() - Set up the initial state to process input characters > + * > + * @cch: State to set up > + */ > +void cli_ch_init(struct cli_ch_state *cch); > + > +/** > + * cli_ch_process() - Process an input character > + * > + * When @ichar is 0, this function returns any characters from an invalid escape > + * sequence which are still pending in the buffer > + * > + * Otherwise it processes the input character. If it is an escape character, > + * then an escape sequence is started and the function returns 0. If we are in > + * the middle of an escape sequence, the character is processed and may result > + * in returning 0 (if more characters are needed) or a valid character (if > + * @ichar finishes the sequence). > + * > + * If @ichar is a valid character and there is no escape sequence in progress, > + * then it is returned as is. > + * > + * If the Enter key is pressed, '\n' is returned. > + * > + * Usage should be like this:: > + * > + * struct cli_ch_state cch; > + * > + * cli_ch_init(cch); > + * do > + * { > + * int ichar, ch; > + * > + * ichar = cli_ch_process(cch, 0); > + * if (!ichar) { > + * ch = getchar(); > + * ichar = cli_ch_process(cch, ch); > + * } > + * (handle the ichar character) > + * } while (!done) > + * > + * If tstc() is used to look for keypresses, this function can be called with > + * @ichar set to -ETIMEDOUT if there is no character after 5-10ms. This allows > + * the ambgiuity between the Escape key and the arrow keys (which generate an > + * escape character followed by other characters) to be resolved. > + * > + * @cch: Current state > + * @ichar: Input character to process, or 0 if none, or -ETIMEDOUT if no > + * character has been received within a small number of milliseconds (this > + * cancels any existing escape sequence and allows pressing the Escape key to > + * work) > + * Returns: Resulting input character after processing, 0 if none, '\e' if > + * an existing escape sequence was cancelled > + */ > +int cli_ch_process(struct cli_ch_state *cch, int ichar); > > #endif
Hi Heinrich, On Fri, 6 Jan 2023 at 08:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 1/6/23 15:52, Simon Glass wrote: > > The current cread_line() function is very long. It handles the escape > > processing inline. The menu command does similar processing but at the > > character level, so there is some duplication. > > > > Split the character processing into a new function cli_ch_process() which > > processes individual characters and returns the resulting input character, > > taking account of escape sequences. It requires the caller to set up and > > maintain its state. > > > > Update cread_line() to use this new function. > > > > The only intended functional change is that an invalid escape sequence > > does not add invalid/control characters into the input buffer, but instead > > discards these. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > (no changes since v1) > > > > common/Makefile | 6 +- > > common/cli_getch.c | 204 ++++++++++++++++++++++++++++++++++++++++++ > > common/cli_readline.c | 150 +++++-------------------------- > > include/cli.h | 72 +++++++++++++++ > > 4 files changed, 301 insertions(+), 131 deletions(-) > > create mode 100644 common/cli_getch.c > > > > diff --git a/common/Makefile b/common/Makefile > > index 20addfb244c..67485e77a04 100644 > > --- a/common/Makefile > > +++ b/common/Makefile > > @@ -38,7 +38,7 @@ obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o > > obj-$(CONFIG_MENU) += menu.o > > obj-$(CONFIG_UPDATE_COMMON) += update.o > > obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o > > -obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o > > +obj-$(CONFIG_CMDLINE) += cli_getch.o cli_readline.o cli_simple.o > > > > endif # !CONFIG_SPL_BUILD > > > > @@ -93,8 +93,8 @@ obj-y += eeprom/eeprom_field.o eeprom/eeprom_layout.o > > endif > > > > obj-y += cli.o > > -obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_simple.o cli_readline.o > > -obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_simple.o cli_readline.o > > +obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o > > +obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o > > obj-$(CONFIG_DFU_OVER_USB) += dfu.o > > obj-y += command.o > > obj-$(CONFIG_$(SPL_TPL_)LOG) += log.o > > diff --git a/common/cli_getch.c b/common/cli_getch.c > > new file mode 100644 > > index 00000000000..9eeea7fef29 > > --- /dev/null > > +++ b/common/cli_getch.c > > @@ -0,0 +1,204 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * (C) Copyright 2000 > > + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. > > + * > > + * Copyright 2022 Google LLC > > + */ > > + > > +#include <common.h> > > +#include <cli.h> > > + > > +/** > > + * enum cli_esc_state_t - indicates what to do with an escape character > > + * > > + * @ESC_REJECT: Invalid escape sequence, so the esc_save[] characters are > > + * returned from each subsequent call to cli_ch_esc() > > + * @ESC_SAVE: Character should be saved in esc_save until we have another one > > + * @ESC_CONVERTED: Escape sequence has been completed and the resulting > > + * character is available > > + */ > > +enum cli_esc_state_t { > > + ESC_REJECT, > > + ESC_SAVE, > > + ESC_CONVERTED > > +}; > > + > > +void cli_ch_init(struct cli_ch_state *cch) > > +{ > > + memset(cch, '\0', sizeof(*cch)); > > +} > > + > > +/** > > + * cli_ch_esc() - Process a character in an ongoing escape sequence > > + * > > + * @cch: State information > > + * @ichar: Character to process > > + * @actp: Returns the action to take > > + * Returns: Output character if *actp is ESC_CONVERTED, else 0 > > + */ > > +static int cli_ch_esc(struct cli_ch_state *cch, int ichar, > > + enum cli_esc_state_t *actp) > > +{ > > + enum cli_esc_state_t act = ESC_REJECT; > > + > > + switch (cch->esc_len) { > > + case 1: > > + if (ichar == '[' || ichar == 'O') > > + act = ESC_SAVE; > > + break; > > + case 2: > > + switch (ichar) { > > + case 'D': /* <- key */ > > + ichar = CTL_CH('b'); > > + act = ESC_CONVERTED; > > + break; /* pass off to ^B handler */ > > + case 'C': /* -> key */ > > + ichar = CTL_CH('f'); > > + act = ESC_CONVERTED; > > + break; /* pass off to ^F handler */ > > + case 'H': /* Home key */ > > + ichar = CTL_CH('a'); > > + act = ESC_CONVERTED; > > + break; /* pass off to ^A handler */ > > + case 'F': /* End key */ > > + ichar = CTL_CH('e'); > > + act = ESC_CONVERTED; > > + break; /* pass off to ^E handler */ > > + case 'A': /* up arrow */ > > + ichar = CTL_CH('p'); > > + act = ESC_CONVERTED; > > + break; /* pass off to ^P handler */ > > + case 'B': /* down arrow */ > > + ichar = CTL_CH('n'); > > + act = ESC_CONVERTED; > > + break; /* pass off to ^N handler */ > > + case '1': > > + case '2': > > + case '3': > > + case '4': > > + case '7': > > + case '8': > > + if (cch->esc_save[1] == '[') { > > + /* see if next character is ~ */ > > + act = ESC_SAVE; > > + } > > + break; > > + } > > + break; > > + case 3: > > + switch (ichar) { > > + case '~': > > + switch (cch->esc_save[2]) { > > + case '3': /* Delete key */ > > + ichar = CTL_CH('d'); > > + act = ESC_CONVERTED; > > + break; /* pass to ^D handler */ > > + case '1': /* Home key */ > > + case '7': > > + ichar = CTL_CH('a'); > > + act = ESC_CONVERTED; > > + break; /* pass to ^A handler */ > > + case '4': /* End key */ > > + case '8': > > + ichar = CTL_CH('e'); > > + act = ESC_CONVERTED; > > + break; /* pass to ^E handler */ > > + } > > + break; > > + case '0': > > + if (cch->esc_save[2] == '2') > > + act = ESC_SAVE; > > + break; > > + } > > + break; > > + case 4: > > + switch (ichar) { > > + case '0': > > + case '1': > > + act = ESC_SAVE; > > + break; /* bracketed paste */ > > + } > > + break; > > + case 5: > > + if (ichar == '~') { /* bracketed paste */ > > + ichar = 0; > > + act = ESC_CONVERTED; > > + } > > + } > > + > > + *actp = act; > > + > > + return act == ESC_CONVERTED ? ichar : 0; > > +} > > + > > +int cli_ch_process(struct cli_ch_state *cch, int ichar) > > +{ > > + /* > > + * ichar=0x0 when error occurs in U-Boot getchar() or when the caller > > + * wants to check if there are more characters saved in the escape > > + * sequence > > + */ > > + if (!ichar) { > > + if (cch->emit_upto) { > > + if (cch->emit_upto < cch->esc_len) > > + return cch->esc_save[cch->emit_upto++]; > > + cch->emit_upto = 0; > > + } > > + return 0; > > + } else if (ichar == -ETIMEDOUT) { > > + /* > > + * If we are in an escape sequence but nothing has followed the > > + * Escape character, then the user probably just pressed the > > + * Escape key. Return it and clear the sequence. > > + */ > > + if (cch->esc_len) { > > + cch->esc_len = 0; > > + return '\e'; > > + } > > + > > + /* Otherwise there is nothing to return */ > > + return 0; > > + } > > + > > + if (ichar == '\n' || ichar == '\r') > > + return '\n'; > > + > > + /* handle standard linux xterm esc sequences for arrow key, etc. */ > > + if (cch->esc_len != 0) { > > + enum cli_esc_state_t act; > > + > > + ichar = cli_ch_esc(cch, ichar, &act); > > + > > + switch (act) { > > + case ESC_SAVE: > > + /* save this character and return nothing */ > > + cch->esc_save[cch->esc_len++] = ichar; > > + return 0; > > + case ESC_REJECT: > > + /* > > + * invalid escape sequence, start returning the > > + * characters in it > > + */ > > + cch->esc_save[cch->esc_len++] = ichar; > > + return cch->esc_save[cch->emit_upto++]; > > + case ESC_CONVERTED: > > + /* valid escape sequence, return the resulting char */ > > + cch->esc_len = 0; > > + return ichar; > > + } > > + } > > + > > + if (ichar == '\e') { > > + if (!cch->esc_len) { > > + cch->esc_save[cch->esc_len] = ichar; > > + cch->esc_len = 1; > > + } else { > > + puts("impossible condition #876\n"); > > + cch->esc_len = 0; > > + } > > + return 0; > > + } > > + > > + return ichar; > > +} > > diff --git a/common/cli_readline.c b/common/cli_readline.c > > index d6444f5fc1d..709e9c3d38b 100644 > > --- a/common/cli_readline.c > > +++ b/common/cli_readline.c > > @@ -62,7 +62,6 @@ static char *delete_char (char *buffer, char *p, int *colp, int *np, int plen) > > > > #define putnstr(str, n) printf("%.*s", (int)n, str) > > > > -#define CTL_CH(c) ((c) - 'a' + 1) > > #define CTL_BACKSPACE ('\b') > > #define DEL ((char)255) > > #define DEL7 ((char)127) > > @@ -252,156 +251,53 @@ static void cread_add_str(char *str, int strsize, int insert, > > static int cread_line(const char *const prompt, char *buf, unsigned int *len, > > int timeout) > > { > > + struct cli_ch_state s_cch, *cch = &s_cch; > > unsigned long num = 0; > > unsigned long eol_num = 0; > > unsigned long wlen; > > char ichar; > > int insert = 1; > > - int esc_len = 0; > > - char esc_save[8]; > > int init_len = strlen(buf); > > int first = 1; > > > > + cli_ch_init(cch); > > + > > if (init_len) > > cread_add_str(buf, init_len, 1, &num, &eol_num, buf, *len); > > > > while (1) { > > - if (bootretry_tstc_timeout()) > > - return -2; /* timed out */ > > - if (first && timeout) { > > - uint64_t etime = endtick(timeout); > > - > > - while (!tstc()) { /* while no incoming data */ > > - if (get_ticks() >= etime) > > - return -2; /* timed out */ > > - schedule(); > > + /* Check for saved characters */ > > + ichar = cli_ch_process(cch, 0); > > + > > + if (!ichar) { > > + if (bootretry_tstc_timeout()) > > + return -2; /* timed out */ > > + if (first && timeout) { > > + u64 etime = endtick(timeout); > > + > > + while (!tstc()) { /* while no incoming data */ > > + if (get_ticks() >= etime) > > + return -2; /* timed out */ > > + schedule(); > > + } > > + first = 0; > > } > > - first = 0; > > + > > + ichar = getcmd_getch(); > > } > > > > - ichar = getcmd_getch(); > > + ichar = cli_ch_process(cch, ichar); > > > > /* ichar=0x0 when error occurs in U-Boot getc */ > > if (!ichar) > > continue; > > > > - if ((ichar == '\n') || (ichar == '\r')) { > > + if (ichar == '\n') { > > putc('\n'); > > break; > > } > > > > - /* > > - * handle standard linux xterm esc sequences for arrow key, etc. > > - */ > > - if (esc_len != 0) { > > - enum { ESC_REJECT, ESC_SAVE, ESC_CONVERTED } act = ESC_REJECT; > > - > > - if (esc_len == 1) { > > - if (ichar == '[' || ichar == 'O') > > - act = ESC_SAVE; > > - } else if (esc_len == 2) { > > - switch (ichar) { > > - case 'D': /* <- key */ > > - ichar = CTL_CH('b'); > > - act = ESC_CONVERTED; > > - break; /* pass off to ^B handler */ > > We have similar code to decode escape sequences and Unicode letters in > efi_cin_read_key_stroke_ex(). The big difference in the EFI code is that > it does not use dummy control characters but uses a structure: > > struct efi_key_data { > struct efi_input_key key; > struct efi_key_state key_state; > }; > > with > > struct efi_input_key { > u16 scan_code; > s16 unicode_char; > }; > > The special keys are represented as numbers in scan_code while > characters are represented in unicode_char. > > Separating special keys from characters is much cleaner than creating > dummy control characters. > > Anyway you would be able to first create the structure above and then in > a separate step to convert to control characters. I thought this discussion started in the last version of the series. Do we need to support unicode input? How would that work in U-Boot, since it seems to only be in the EFI layer at present? > > The code below misses to interprete some special keys like FN1 - FN12 > which could be quite useful for navigating a UI. Can we add them later? I don't have any use for them at present. Regards, Simon
On 1/7/23 01:13, Simon Glass wrote: > Hi Heinrich, > > On Fri, 6 Jan 2023 at 08:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 1/6/23 15:52, Simon Glass wrote: >>> The current cread_line() function is very long. It handles the escape >>> processing inline. The menu command does similar processing but at the >>> character level, so there is some duplication. >>> >>> Split the character processing into a new function cli_ch_process() which >>> processes individual characters and returns the resulting input character, >>> taking account of escape sequences. It requires the caller to set up and >>> maintain its state. >>> >>> Update cread_line() to use this new function. >>> >>> The only intended functional change is that an invalid escape sequence >>> does not add invalid/control characters into the input buffer, but instead >>> discards these. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> (no changes since v1) >>> >>> common/Makefile | 6 +- >>> common/cli_getch.c | 204 ++++++++++++++++++++++++++++++++++++++++++ >>> common/cli_readline.c | 150 +++++-------------------------- >>> include/cli.h | 72 +++++++++++++++ >>> 4 files changed, 301 insertions(+), 131 deletions(-) >>> create mode 100644 common/cli_getch.c >>> >>> diff --git a/common/Makefile b/common/Makefile >>> index 20addfb244c..67485e77a04 100644 >>> --- a/common/Makefile >>> +++ b/common/Makefile >>> @@ -38,7 +38,7 @@ obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o >>> obj-$(CONFIG_MENU) += menu.o >>> obj-$(CONFIG_UPDATE_COMMON) += update.o >>> obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o >>> -obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o >>> +obj-$(CONFIG_CMDLINE) += cli_getch.o cli_readline.o cli_simple.o >>> >>> endif # !CONFIG_SPL_BUILD >>> >>> @@ -93,8 +93,8 @@ obj-y += eeprom/eeprom_field.o eeprom/eeprom_layout.o >>> endif >>> >>> obj-y += cli.o >>> -obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_simple.o cli_readline.o >>> -obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_simple.o cli_readline.o >>> +obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o >>> +obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o >>> obj-$(CONFIG_DFU_OVER_USB) += dfu.o >>> obj-y += command.o >>> obj-$(CONFIG_$(SPL_TPL_)LOG) += log.o >>> diff --git a/common/cli_getch.c b/common/cli_getch.c >>> new file mode 100644 >>> index 00000000000..9eeea7fef29 >>> --- /dev/null >>> +++ b/common/cli_getch.c >>> @@ -0,0 +1,204 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * (C) Copyright 2000 >>> + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. >>> + * >>> + * Copyright 2022 Google LLC >>> + */ >>> + >>> +#include <common.h> >>> +#include <cli.h> >>> + >>> +/** >>> + * enum cli_esc_state_t - indicates what to do with an escape character >>> + * >>> + * @ESC_REJECT: Invalid escape sequence, so the esc_save[] characters are >>> + * returned from each subsequent call to cli_ch_esc() >>> + * @ESC_SAVE: Character should be saved in esc_save until we have another one >>> + * @ESC_CONVERTED: Escape sequence has been completed and the resulting >>> + * character is available >>> + */ >>> +enum cli_esc_state_t { >>> + ESC_REJECT, >>> + ESC_SAVE, >>> + ESC_CONVERTED >>> +}; >>> + >>> +void cli_ch_init(struct cli_ch_state *cch) >>> +{ >>> + memset(cch, '\0', sizeof(*cch)); >>> +} >>> + >>> +/** >>> + * cli_ch_esc() - Process a character in an ongoing escape sequence >>> + * >>> + * @cch: State information >>> + * @ichar: Character to process >>> + * @actp: Returns the action to take >>> + * Returns: Output character if *actp is ESC_CONVERTED, else 0 >>> + */ >>> +static int cli_ch_esc(struct cli_ch_state *cch, int ichar, >>> + enum cli_esc_state_t *actp) >>> +{ >>> + enum cli_esc_state_t act = ESC_REJECT; >>> + >>> + switch (cch->esc_len) { >>> + case 1: >>> + if (ichar == '[' || ichar == 'O') >>> + act = ESC_SAVE; >>> + break; >>> + case 2: >>> + switch (ichar) { >>> + case 'D': /* <- key */ >>> + ichar = CTL_CH('b'); >>> + act = ESC_CONVERTED; >>> + break; /* pass off to ^B handler */ >>> + case 'C': /* -> key */ >>> + ichar = CTL_CH('f'); >>> + act = ESC_CONVERTED; >>> + break; /* pass off to ^F handler */ >>> + case 'H': /* Home key */ >>> + ichar = CTL_CH('a'); >>> + act = ESC_CONVERTED; >>> + break; /* pass off to ^A handler */ >>> + case 'F': /* End key */ >>> + ichar = CTL_CH('e'); >>> + act = ESC_CONVERTED; >>> + break; /* pass off to ^E handler */ >>> + case 'A': /* up arrow */ >>> + ichar = CTL_CH('p'); >>> + act = ESC_CONVERTED; >>> + break; /* pass off to ^P handler */ >>> + case 'B': /* down arrow */ >>> + ichar = CTL_CH('n'); >>> + act = ESC_CONVERTED; >>> + break; /* pass off to ^N handler */ >>> + case '1': >>> + case '2': >>> + case '3': >>> + case '4': >>> + case '7': >>> + case '8': >>> + if (cch->esc_save[1] == '[') { >>> + /* see if next character is ~ */ >>> + act = ESC_SAVE; >>> + } >>> + break; >>> + } >>> + break; >>> + case 3: >>> + switch (ichar) { >>> + case '~': >>> + switch (cch->esc_save[2]) { >>> + case '3': /* Delete key */ >>> + ichar = CTL_CH('d'); >>> + act = ESC_CONVERTED; >>> + break; /* pass to ^D handler */ >>> + case '1': /* Home key */ >>> + case '7': >>> + ichar = CTL_CH('a'); >>> + act = ESC_CONVERTED; >>> + break; /* pass to ^A handler */ >>> + case '4': /* End key */ >>> + case '8': >>> + ichar = CTL_CH('e'); >>> + act = ESC_CONVERTED; >>> + break; /* pass to ^E handler */ >>> + } >>> + break; >>> + case '0': >>> + if (cch->esc_save[2] == '2') >>> + act = ESC_SAVE; >>> + break; >>> + } >>> + break; >>> + case 4: >>> + switch (ichar) { >>> + case '0': >>> + case '1': >>> + act = ESC_SAVE; >>> + break; /* bracketed paste */ >>> + } >>> + break; >>> + case 5: >>> + if (ichar == '~') { /* bracketed paste */ >>> + ichar = 0; >>> + act = ESC_CONVERTED; >>> + } >>> + } >>> + >>> + *actp = act; >>> + >>> + return act == ESC_CONVERTED ? ichar : 0; >>> +} >>> + >>> +int cli_ch_process(struct cli_ch_state *cch, int ichar) >>> +{ >>> + /* >>> + * ichar=0x0 when error occurs in U-Boot getchar() or when the caller >>> + * wants to check if there are more characters saved in the escape >>> + * sequence >>> + */ >>> + if (!ichar) { >>> + if (cch->emit_upto) { >>> + if (cch->emit_upto < cch->esc_len) >>> + return cch->esc_save[cch->emit_upto++]; >>> + cch->emit_upto = 0; >>> + } >>> + return 0; >>> + } else if (ichar == -ETIMEDOUT) { >>> + /* >>> + * If we are in an escape sequence but nothing has followed the >>> + * Escape character, then the user probably just pressed the >>> + * Escape key. Return it and clear the sequence. >>> + */ >>> + if (cch->esc_len) { >>> + cch->esc_len = 0; >>> + return '\e'; >>> + } >>> + >>> + /* Otherwise there is nothing to return */ >>> + return 0; >>> + } >>> + >>> + if (ichar == '\n' || ichar == '\r') >>> + return '\n'; >>> + >>> + /* handle standard linux xterm esc sequences for arrow key, etc. */ >>> + if (cch->esc_len != 0) { >>> + enum cli_esc_state_t act; >>> + >>> + ichar = cli_ch_esc(cch, ichar, &act); >>> + >>> + switch (act) { >>> + case ESC_SAVE: >>> + /* save this character and return nothing */ >>> + cch->esc_save[cch->esc_len++] = ichar; >>> + return 0; >>> + case ESC_REJECT: >>> + /* >>> + * invalid escape sequence, start returning the >>> + * characters in it >>> + */ >>> + cch->esc_save[cch->esc_len++] = ichar; >>> + return cch->esc_save[cch->emit_upto++]; >>> + case ESC_CONVERTED: >>> + /* valid escape sequence, return the resulting char */ >>> + cch->esc_len = 0; >>> + return ichar; >>> + } >>> + } >>> + >>> + if (ichar == '\e') { >>> + if (!cch->esc_len) { >>> + cch->esc_save[cch->esc_len] = ichar; >>> + cch->esc_len = 1; >>> + } else { >>> + puts("impossible condition #876\n"); >>> + cch->esc_len = 0; >>> + } >>> + return 0; >>> + } >>> + >>> + return ichar; >>> +} >>> diff --git a/common/cli_readline.c b/common/cli_readline.c >>> index d6444f5fc1d..709e9c3d38b 100644 >>> --- a/common/cli_readline.c >>> +++ b/common/cli_readline.c >>> @@ -62,7 +62,6 @@ static char *delete_char (char *buffer, char *p, int *colp, int *np, int plen) >>> >>> #define putnstr(str, n) printf("%.*s", (int)n, str) >>> >>> -#define CTL_CH(c) ((c) - 'a' + 1) >>> #define CTL_BACKSPACE ('\b') >>> #define DEL ((char)255) >>> #define DEL7 ((char)127) >>> @@ -252,156 +251,53 @@ static void cread_add_str(char *str, int strsize, int insert, >>> static int cread_line(const char *const prompt, char *buf, unsigned int *len, >>> int timeout) >>> { >>> + struct cli_ch_state s_cch, *cch = &s_cch; >>> unsigned long num = 0; >>> unsigned long eol_num = 0; >>> unsigned long wlen; >>> char ichar; >>> int insert = 1; >>> - int esc_len = 0; >>> - char esc_save[8]; >>> int init_len = strlen(buf); >>> int first = 1; >>> >>> + cli_ch_init(cch); >>> + >>> if (init_len) >>> cread_add_str(buf, init_len, 1, &num, &eol_num, buf, *len); >>> >>> while (1) { >>> - if (bootretry_tstc_timeout()) >>> - return -2; /* timed out */ >>> - if (first && timeout) { >>> - uint64_t etime = endtick(timeout); >>> - >>> - while (!tstc()) { /* while no incoming data */ >>> - if (get_ticks() >= etime) >>> - return -2; /* timed out */ >>> - schedule(); >>> + /* Check for saved characters */ >>> + ichar = cli_ch_process(cch, 0); >>> + >>> + if (!ichar) { >>> + if (bootretry_tstc_timeout()) >>> + return -2; /* timed out */ >>> + if (first && timeout) { >>> + u64 etime = endtick(timeout); >>> + >>> + while (!tstc()) { /* while no incoming data */ >>> + if (get_ticks() >= etime) >>> + return -2; /* timed out */ >>> + schedule(); >>> + } >>> + first = 0; >>> } >>> - first = 0; >>> + >>> + ichar = getcmd_getch(); >>> } >>> >>> - ichar = getcmd_getch(); >>> + ichar = cli_ch_process(cch, ichar); >>> >>> /* ichar=0x0 when error occurs in U-Boot getc */ >>> if (!ichar) >>> continue; >>> >>> - if ((ichar == '\n') || (ichar == '\r')) { >>> + if (ichar == '\n') { >>> putc('\n'); >>> break; >>> } >>> >>> - /* >>> - * handle standard linux xterm esc sequences for arrow key, etc. >>> - */ >>> - if (esc_len != 0) { >>> - enum { ESC_REJECT, ESC_SAVE, ESC_CONVERTED } act = ESC_REJECT; >>> - >>> - if (esc_len == 1) { >>> - if (ichar == '[' || ichar == 'O') >>> - act = ESC_SAVE; >>> - } else if (esc_len == 2) { >>> - switch (ichar) { >>> - case 'D': /* <- key */ >>> - ichar = CTL_CH('b'); >>> - act = ESC_CONVERTED; >>> - break; /* pass off to ^B handler */ >> >> We have similar code to decode escape sequences and Unicode letters in >> efi_cin_read_key_stroke_ex(). The big difference in the EFI code is that >> it does not use dummy control characters but uses a structure: >> >> struct efi_key_data { >> struct efi_input_key key; >> struct efi_key_state key_state; >> }; >> >> with >> >> struct efi_input_key { >> u16 scan_code; >> s16 unicode_char; >> }; >> >> The special keys are represented as numbers in scan_code while >> characters are represented in unicode_char. >> >> Separating special keys from characters is much cleaner than creating >> dummy control characters. >> >> Anyway you would be able to first create the structure above and then in >> a separate step to convert to control characters. > > I thought this discussion started in the last version of the series. > Do we need to support unicode input? How would that work in U-Boot, > since it seems to only be in the EFI layer at present? File systems like FAT and EXT4 use Unicode. > >> >> The code below misses to interprete some special keys like FN1 - FN12 >> which could be quite useful for navigating a UI. > > Can we add them later? I don't have any use for them at present. We can work on the unification of those codes later. Best regards Heinrich > > Regards, > Simon
On 1/6/23 15:52, Simon Glass wrote: > The current cread_line() function is very long. It handles the escape > processing inline. The menu command does similar processing but at the > character level, so there is some duplication. > > Split the character processing into a new function cli_ch_process() which > processes individual characters and returns the resulting input character, > taking account of escape sequences. It requires the caller to set up and > maintain its state. > > Update cread_line() to use this new function. > > The only intended functional change is that an invalid escape sequence > does not add invalid/control characters into the input buffer, but instead > discards these. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > Hello Simon in the Change Boot Order menu of the eficonfig command I hit the PAGE-UP or PAGE-DOWN button and get this output: [ ] label0038 [ ] label0039impossible condition #876 impossible condition #876 Press UP/DOWN to move, +/- to change orde Press SPACE to activate or deactivate the entry Select [Save] to complete, ESC/CTRL+C to quit Hitting an unsupported key should not result in output. This is the line providing the output. common/cli_getch.c:201: puts("impossible condition #876\n"); How line be reached by an "impossible condition". That text does not make any sense. This is the patch to blame b08e9d4b6632 ("cli: Move readline character-processing to a state machine") Please, remove that debug message. The conitrace command shows these escape sequences: PAGE-DOWN 1b 5b 36 7e PAGE-UP 1b 5b 35 7e Best regards Heinrich
Hi Heinrich, On Tue, 24 Jan 2023 at 08:19, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 1/6/23 15:52, Simon Glass wrote: > > The current cread_line() function is very long. It handles the escape > > processing inline. The menu command does similar processing but at the > > character level, so there is some duplication. > > > > Split the character processing into a new function cli_ch_process() which > > processes individual characters and returns the resulting input character, > > taking account of escape sequences. It requires the caller to set up and > > maintain its state. > > > > Update cread_line() to use this new function. > > > > The only intended functional change is that an invalid escape sequence > > does not add invalid/control characters into the input buffer, but instead > > discards these. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > Hello Simon > > in the Change Boot Order menu of the eficonfig command I hit the > PAGE-UP or PAGE-DOWN button and get this output: > > [ ] label0038 > [ ] label0039impossible condition #876 > impossible condition #876 > Press UP/DOWN to move, +/- to change orde > Press SPACE to activate or deactivate the entry > Select [Save] to complete, ESC/CTRL+C to quit > > Hitting an unsupported key should not result in output. > > This is the line providing the output. > > common/cli_getch.c:201: > puts("impossible condition #876\n"); > > How line be reached by an "impossible condition". That text does not > make any sense. > > This is the patch to blame > > b08e9d4b6632 ("cli: Move readline character-processing to a state machine") > > Please, remove that debug message. This was already in the code. I just split it out into a separate file. > > The conitrace command shows these escape sequences: > > PAGE-DOWN 1b 5b 36 7e > PAGE-UP 1b 5b 35 7e I believe it indicates that the sequence is not understood. It is therefore rejected and sent out as an escape sequence to the terminal. This works by calling cli_ch_process() with a character of 0, to find out if there is a rejected sequence to be emitted. Only once it returns 0 should getchar() be called. See the detailed function comments for how it works. From what I can see, bootmenu_autoboot_loop() is violating that. Probably that is what is going on. The message is correct in that the condition cannot happen if the caller is behaving correctly. In any case it seems that these keys are not supported, so in addition to fixing the bug you found, we need to add support for them to cli_ch_esc(). > > Best regards > > Heinrich > Regards, Simon
diff --git a/common/Makefile b/common/Makefile index 20addfb244c..67485e77a04 100644 --- a/common/Makefile +++ b/common/Makefile @@ -38,7 +38,7 @@ obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o obj-$(CONFIG_MENU) += menu.o obj-$(CONFIG_UPDATE_COMMON) += update.o obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o -obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o +obj-$(CONFIG_CMDLINE) += cli_getch.o cli_readline.o cli_simple.o endif # !CONFIG_SPL_BUILD @@ -93,8 +93,8 @@ obj-y += eeprom/eeprom_field.o eeprom/eeprom_layout.o endif obj-y += cli.o -obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_simple.o cli_readline.o -obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_simple.o cli_readline.o +obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o +obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o obj-$(CONFIG_DFU_OVER_USB) += dfu.o obj-y += command.o obj-$(CONFIG_$(SPL_TPL_)LOG) += log.o diff --git a/common/cli_getch.c b/common/cli_getch.c new file mode 100644 index 00000000000..9eeea7fef29 --- /dev/null +++ b/common/cli_getch.c @@ -0,0 +1,204 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) Copyright 2000 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * Copyright 2022 Google LLC + */ + +#include <common.h> +#include <cli.h> + +/** + * enum cli_esc_state_t - indicates what to do with an escape character + * + * @ESC_REJECT: Invalid escape sequence, so the esc_save[] characters are + * returned from each subsequent call to cli_ch_esc() + * @ESC_SAVE: Character should be saved in esc_save until we have another one + * @ESC_CONVERTED: Escape sequence has been completed and the resulting + * character is available + */ +enum cli_esc_state_t { + ESC_REJECT, + ESC_SAVE, + ESC_CONVERTED +}; + +void cli_ch_init(struct cli_ch_state *cch) +{ + memset(cch, '\0', sizeof(*cch)); +} + +/** + * cli_ch_esc() - Process a character in an ongoing escape sequence + * + * @cch: State information + * @ichar: Character to process + * @actp: Returns the action to take + * Returns: Output character if *actp is ESC_CONVERTED, else 0 + */ +static int cli_ch_esc(struct cli_ch_state *cch, int ichar, + enum cli_esc_state_t *actp) +{ + enum cli_esc_state_t act = ESC_REJECT; + + switch (cch->esc_len) { + case 1: + if (ichar == '[' || ichar == 'O') + act = ESC_SAVE; + break; + case 2: + switch (ichar) { + case 'D': /* <- key */ + ichar = CTL_CH('b'); + act = ESC_CONVERTED; + break; /* pass off to ^B handler */ + case 'C': /* -> key */ + ichar = CTL_CH('f'); + act = ESC_CONVERTED; + break; /* pass off to ^F handler */ + case 'H': /* Home key */ + ichar = CTL_CH('a'); + act = ESC_CONVERTED; + break; /* pass off to ^A handler */ + case 'F': /* End key */ + ichar = CTL_CH('e'); + act = ESC_CONVERTED; + break; /* pass off to ^E handler */ + case 'A': /* up arrow */ + ichar = CTL_CH('p'); + act = ESC_CONVERTED; + break; /* pass off to ^P handler */ + case 'B': /* down arrow */ + ichar = CTL_CH('n'); + act = ESC_CONVERTED; + break; /* pass off to ^N handler */ + case '1': + case '2': + case '3': + case '4': + case '7': + case '8': + if (cch->esc_save[1] == '[') { + /* see if next character is ~ */ + act = ESC_SAVE; + } + break; + } + break; + case 3: + switch (ichar) { + case '~': + switch (cch->esc_save[2]) { + case '3': /* Delete key */ + ichar = CTL_CH('d'); + act = ESC_CONVERTED; + break; /* pass to ^D handler */ + case '1': /* Home key */ + case '7': + ichar = CTL_CH('a'); + act = ESC_CONVERTED; + break; /* pass to ^A handler */ + case '4': /* End key */ + case '8': + ichar = CTL_CH('e'); + act = ESC_CONVERTED; + break; /* pass to ^E handler */ + } + break; + case '0': + if (cch->esc_save[2] == '2') + act = ESC_SAVE; + break; + } + break; + case 4: + switch (ichar) { + case '0': + case '1': + act = ESC_SAVE; + break; /* bracketed paste */ + } + break; + case 5: + if (ichar == '~') { /* bracketed paste */ + ichar = 0; + act = ESC_CONVERTED; + } + } + + *actp = act; + + return act == ESC_CONVERTED ? ichar : 0; +} + +int cli_ch_process(struct cli_ch_state *cch, int ichar) +{ + /* + * ichar=0x0 when error occurs in U-Boot getchar() or when the caller + * wants to check if there are more characters saved in the escape + * sequence + */ + if (!ichar) { + if (cch->emit_upto) { + if (cch->emit_upto < cch->esc_len) + return cch->esc_save[cch->emit_upto++]; + cch->emit_upto = 0; + } + return 0; + } else if (ichar == -ETIMEDOUT) { + /* + * If we are in an escape sequence but nothing has followed the + * Escape character, then the user probably just pressed the + * Escape key. Return it and clear the sequence. + */ + if (cch->esc_len) { + cch->esc_len = 0; + return '\e'; + } + + /* Otherwise there is nothing to return */ + return 0; + } + + if (ichar == '\n' || ichar == '\r') + return '\n'; + + /* handle standard linux xterm esc sequences for arrow key, etc. */ + if (cch->esc_len != 0) { + enum cli_esc_state_t act; + + ichar = cli_ch_esc(cch, ichar, &act); + + switch (act) { + case ESC_SAVE: + /* save this character and return nothing */ + cch->esc_save[cch->esc_len++] = ichar; + return 0; + case ESC_REJECT: + /* + * invalid escape sequence, start returning the + * characters in it + */ + cch->esc_save[cch->esc_len++] = ichar; + return cch->esc_save[cch->emit_upto++]; + case ESC_CONVERTED: + /* valid escape sequence, return the resulting char */ + cch->esc_len = 0; + return ichar; + } + } + + if (ichar == '\e') { + if (!cch->esc_len) { + cch->esc_save[cch->esc_len] = ichar; + cch->esc_len = 1; + } else { + puts("impossible condition #876\n"); + cch->esc_len = 0; + } + return 0; + } + + return ichar; +} diff --git a/common/cli_readline.c b/common/cli_readline.c index d6444f5fc1d..709e9c3d38b 100644 --- a/common/cli_readline.c +++ b/common/cli_readline.c @@ -62,7 +62,6 @@ static char *delete_char (char *buffer, char *p, int *colp, int *np, int plen) #define putnstr(str, n) printf("%.*s", (int)n, str) -#define CTL_CH(c) ((c) - 'a' + 1) #define CTL_BACKSPACE ('\b') #define DEL ((char)255) #define DEL7 ((char)127) @@ -252,156 +251,53 @@ static void cread_add_str(char *str, int strsize, int insert, static int cread_line(const char *const prompt, char *buf, unsigned int *len, int timeout) { + struct cli_ch_state s_cch, *cch = &s_cch; unsigned long num = 0; unsigned long eol_num = 0; unsigned long wlen; char ichar; int insert = 1; - int esc_len = 0; - char esc_save[8]; int init_len = strlen(buf); int first = 1; + cli_ch_init(cch); + if (init_len) cread_add_str(buf, init_len, 1, &num, &eol_num, buf, *len); while (1) { - if (bootretry_tstc_timeout()) - return -2; /* timed out */ - if (first && timeout) { - uint64_t etime = endtick(timeout); - - while (!tstc()) { /* while no incoming data */ - if (get_ticks() >= etime) - return -2; /* timed out */ - schedule(); + /* Check for saved characters */ + ichar = cli_ch_process(cch, 0); + + if (!ichar) { + if (bootretry_tstc_timeout()) + return -2; /* timed out */ + if (first && timeout) { + u64 etime = endtick(timeout); + + while (!tstc()) { /* while no incoming data */ + if (get_ticks() >= etime) + return -2; /* timed out */ + schedule(); + } + first = 0; } - first = 0; + + ichar = getcmd_getch(); } - ichar = getcmd_getch(); + ichar = cli_ch_process(cch, ichar); /* ichar=0x0 when error occurs in U-Boot getc */ if (!ichar) continue; - if ((ichar == '\n') || (ichar == '\r')) { + if (ichar == '\n') { putc('\n'); break; } - /* - * handle standard linux xterm esc sequences for arrow key, etc. - */ - if (esc_len != 0) { - enum { ESC_REJECT, ESC_SAVE, ESC_CONVERTED } act = ESC_REJECT; - - if (esc_len == 1) { - if (ichar == '[' || ichar == 'O') - act = ESC_SAVE; - } else if (esc_len == 2) { - switch (ichar) { - case 'D': /* <- key */ - ichar = CTL_CH('b'); - act = ESC_CONVERTED; - break; /* pass off to ^B handler */ - case 'C': /* -> key */ - ichar = CTL_CH('f'); - act = ESC_CONVERTED; - break; /* pass off to ^F handler */ - case 'H': /* Home key */ - ichar = CTL_CH('a'); - act = ESC_CONVERTED; - break; /* pass off to ^A handler */ - case 'F': /* End key */ - ichar = CTL_CH('e'); - act = ESC_CONVERTED; - break; /* pass off to ^E handler */ - case 'A': /* up arrow */ - ichar = CTL_CH('p'); - act = ESC_CONVERTED; - break; /* pass off to ^P handler */ - case 'B': /* down arrow */ - ichar = CTL_CH('n'); - act = ESC_CONVERTED; - break; /* pass off to ^N handler */ - case '1': - case '2': - case '3': - case '4': - case '7': - case '8': - if (esc_save[1] == '[') { - /* see if next character is ~ */ - act = ESC_SAVE; - } - break; - } - } else if (esc_len == 3) { - switch (ichar) { - case '~': - switch (esc_save[2]) { - case '3': /* Delete key */ - ichar = CTL_CH('d'); - act = ESC_CONVERTED; - break; /* pass to ^D handler */ - case '1': /* Home key */ - case '7': - ichar = CTL_CH('a'); - act = ESC_CONVERTED; - break; /* pass to ^A handler */ - case '4': /* End key */ - case '8': - ichar = CTL_CH('e'); - act = ESC_CONVERTED; - break; /* pass to ^E handler */ - } - break; - case '0': - if (esc_save[2] == '2') - act = ESC_SAVE; - break; - } - } else if (esc_len == 4) { - switch (ichar) { - case '0': - case '1': - act = ESC_SAVE; - break; /* bracketed paste */ - } - } else if (esc_len == 5) { - if (ichar == '~') { /* bracketed paste */ - ichar = 0; - act = ESC_CONVERTED; - } - } - switch (act) { - case ESC_SAVE: - esc_save[esc_len++] = ichar; - continue; - case ESC_REJECT: - esc_save[esc_len++] = ichar; - cread_add_str(esc_save, esc_len, insert, - &num, &eol_num, buf, *len); - esc_len = 0; - continue; - case ESC_CONVERTED: - esc_len = 0; - break; - } - } - switch (ichar) { - case 0x1b: - if (esc_len == 0) { - esc_save[esc_len] = ichar; - esc_len = 1; - } else { - puts("impossible condition #876\n"); - esc_len = 0; - } - break; - case CTL_CH('a'): BEGINNING_OF_LINE(); break; @@ -470,8 +366,6 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, { char *hline; - esc_len = 0; - if (ichar == CTL_CH('p')) hline = hist_prev(); else diff --git a/include/cli.h b/include/cli.h index ba5b8ebd36e..863519e4b13 100644 --- a/include/cli.h +++ b/include/cli.h @@ -7,6 +7,21 @@ #ifndef __CLI_H #define __CLI_H +#include <stdbool.h> + +/** + * struct cli_ch_state - state information for reading cmdline characters + * + * @esc_len: Number of escape characters read so far + * @esc_save: Escape characters collected so far + * @emit_upto: Next character to emit from esc_save (0 if not emitting) + */ +struct cli_ch_state { + int esc_len; + char esc_save[8]; + int emit_upto; +}; + /** * Go into the command loop * @@ -154,5 +169,62 @@ void cli_loop(void); void cli_init(void); #define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk()) +#define CTL_CH(c) ((c) - 'a' + 1) + +/** + * cli_ch_init() - Set up the initial state to process input characters + * + * @cch: State to set up + */ +void cli_ch_init(struct cli_ch_state *cch); + +/** + * cli_ch_process() - Process an input character + * + * When @ichar is 0, this function returns any characters from an invalid escape + * sequence which are still pending in the buffer + * + * Otherwise it processes the input character. If it is an escape character, + * then an escape sequence is started and the function returns 0. If we are in + * the middle of an escape sequence, the character is processed and may result + * in returning 0 (if more characters are needed) or a valid character (if + * @ichar finishes the sequence). + * + * If @ichar is a valid character and there is no escape sequence in progress, + * then it is returned as is. + * + * If the Enter key is pressed, '\n' is returned. + * + * Usage should be like this:: + * + * struct cli_ch_state cch; + * + * cli_ch_init(cch); + * do + * { + * int ichar, ch; + * + * ichar = cli_ch_process(cch, 0); + * if (!ichar) { + * ch = getchar(); + * ichar = cli_ch_process(cch, ch); + * } + * (handle the ichar character) + * } while (!done) + * + * If tstc() is used to look for keypresses, this function can be called with + * @ichar set to -ETIMEDOUT if there is no character after 5-10ms. This allows + * the ambgiuity between the Escape key and the arrow keys (which generate an + * escape character followed by other characters) to be resolved. + * + * @cch: Current state + * @ichar: Input character to process, or 0 if none, or -ETIMEDOUT if no + * character has been received within a small number of milliseconds (this + * cancels any existing escape sequence and allows pressing the Escape key to + * work) + * Returns: Resulting input character after processing, 0 if none, '\e' if + * an existing escape sequence was cancelled + */ +int cli_ch_process(struct cli_ch_state *cch, int ichar); #endif
The current cread_line() function is very long. It handles the escape processing inline. The menu command does similar processing but at the character level, so there is some duplication. Split the character processing into a new function cli_ch_process() which processes individual characters and returns the resulting input character, taking account of escape sequences. It requires the caller to set up and maintain its state. Update cread_line() to use this new function. The only intended functional change is that an invalid escape sequence does not add invalid/control characters into the input buffer, but instead discards these. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) common/Makefile | 6 +- common/cli_getch.c | 204 ++++++++++++++++++++++++++++++++++++++++++ common/cli_readline.c | 150 +++++-------------------------- include/cli.h | 72 +++++++++++++++ 4 files changed, 301 insertions(+), 131 deletions(-) create mode 100644 common/cli_getch.c