[PATCH v3 02/25] cli: Move readline character-processing to a state machine
Simon Glass
sjg at chromium.org
Sat Jan 7 01:13:43 CET 2023
Hi Heinrich,
On Fri, 6 Jan 2023 at 08:50, Heinrich Schuchardt <xypron.glpk at 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 at 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 at 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
More information about the U-Boot
mailing list