[PATCH v3 02/25] cli: Move readline character-processing to a state machine
Heinrich Schuchardt
xypron.glpk at gmx.de
Sat Jan 7 23:31:21 CET 2023
On 1/7/23 01:13, Simon Glass wrote:
> 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?
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
More information about the U-Boot
mailing list