Re: [PATCH v3 02/25] cli: Move readline character-processing to a state machine

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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