[PATCH v3] cli: Consume invalid escape sequences early

[PATCH v3] cli: Consume invalid escape sequences early

[Yurii Monakov]

Unexpected 'Esc' key presses are accumulated internally, even if it is
already clear that the current escape sequence is invalid. This results
in weird behaviour. For example, the next character after 'Esc' key
simply disappears from input and 'Unknown command' is printed
after 'Enter'.

This commit fixes some issues with extra 'Esc' keys entered by user:

1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
=>
nknown command 'ry 'help'
=>
2. Sequence <Esc><p><r><i><Enter> gives:
=> ri
Unknown command 'ri' - try 'help'
=>
3. Extra 'Esc' key presses break backspace functionality.

Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
---
Changes for v2:
- add tests and reword commit message
Changes for v3:
- fix indentation

 common/cli_getch.c  |  2 ++
 test/common/cread.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/common/cli_getch.c b/common/cli_getch.c
index 61d4cb261b..0ee7908777 100644
--- a/common/cli_getch.c
+++ b/common/cli_getch.c
@@ -46,6 +46,8 @@ static int cli_ch_esc(struct cli_ch_state *cch, int
ichar,
 	case 1:
 		if (ichar == '[' || ichar == 'O')
 			act = ESC_SAVE;
+		else
+			act = ESC_CONVERTED;
 		break;
 	case 2:
 		switch (ichar) {
diff --git a/test/common/cread.c b/test/common/cread.c
index 2fdd29a265..4edc773960 100644
--- a/test/common/cread.c
+++ b/test/common/cread.c
@@ -43,6 +43,12 @@ static int cli_ch_test(struct unit_test_state *uts)
 	ut_asserteq('a', cli_ch_process(cch, 'a'));
 	ut_asserteq(0, cli_ch_process(cch, 0));
 
+	/* unexpected 'Esc' */
+	ut_asserteq('a', cli_ch_process(cch, 'a'));
+	ut_asserteq(0, cli_ch_process(cch, '\e'));
+	ut_asserteq('b', cli_ch_process(cch, 'b'));
+	ut_asserteq(0, cli_ch_process(cch, 0));
+
 	return 0;
 }
 COMMON_TEST(cli_ch_test, 0);
@@ -80,6 +86,12 @@ static int cread_test(struct unit_test_state *uts)
 	ut_asserteq(7, cli_readline_into_buffer("-> ", buf, 1));
 	ut_asserteq_str("abc\e[Xx", buf);
 
+	/* unexpected 'Esc' */
+	*buf = '\0';
+	ut_asserteq(7, console_in_puts("abc\eXx\n"));
+	ut_asserteq(5, cli_readline_into_buffer("-> ", buf, 1));
+	ut_asserteq_str("abcXx", buf);
+
 	/* check timeout, should be between 1000 and 1050ms */
 	start = get_timer(0);
 	*buf = '\0';
-- 
2.34.1

Re: [PATCH v3] cli: Consume invalid escape sequences early

[Simon Glass]

On Tue, 10 Oct 2023 at 02:16, Yurii Monakov <monakov.y@gmail.com> wrote:
>
> Unexpected 'Esc' key presses are accumulated internally, even if it is
> already clear that the current escape sequence is invalid. This results
> in weird behaviour. For example, the next character after 'Esc' key
> simply disappears from input and 'Unknown command' is printed
> after 'Enter'.
>
> This commit fixes some issues with extra 'Esc' keys entered by user:
>
> 1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
> =>
> nknown command 'ry 'help'
> =>
> 2. Sequence <Esc><p><r><i><Enter> gives:
> => ri
> Unknown command 'ri' - try 'help'
> =>
> 3. Extra 'Esc' key presses break backspace functionality.
>
> Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> ---
> Changes for v2:
> - add tests and reword commit message
> Changes for v3:
> - fix indentation
>
>  common/cli_getch.c  |  2 ++
>  test/common/cread.c | 12 ++++++++++++
>  2 files changed, 14 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Unfortunately this shows a design flaw, one which is hard to fix in
general. But this does improve it.

The flaw is that we assume that character sequences have a time gap between
them, which allows figuring out when a sequence has finished.

But when starting up there may be buffered output with no gaps. I don't
think there is a general fix for this problem. One option is to have a
special mode at the start, where escape sequences are ignored. But the user
may press an arrow key on startup.

So I don't have anything much to suggest here. Let's see how this fix goes.
Perhaps it is enough.

Regards,
Simon

Re: [PATCH v3] cli: Consume invalid escape sequences early

[Yurii Monakov]

Simon,

On Tue, Oct 10, 2023 at 5:58 PM Simon Glass <sjg@google.com> wrote:
>
> On Tue, 10 Oct 2023 at 02:16, Yurii Monakov <monakov.y@gmail.com> wrote:
> >
> > Unexpected 'Esc' key presses are accumulated internally, even if it is
> > already clear that the current escape sequence is invalid. This results
> > in weird behaviour. For example, the next character after 'Esc' key
> > simply disappears from input and 'Unknown command' is printed
> > after 'Enter'.
> >
> > This commit fixes some issues with extra 'Esc' keys entered by user:
> >
> > 1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
> > =>
> > nknown command 'ry 'help'
> > =>
> > 2. Sequence <Esc><p><r><i><Enter> gives:
> > => ri
> > Unknown command 'ri' - try 'help'
> > =>
> > 3. Extra 'Esc' key presses break backspace functionality.
> >
> > Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> > ---
> > Changes for v2:
> > - add tests and reword commit message
> > Changes for v3:
> > - fix indentation
> >
> >  common/cli_getch.c  |  2 ++
> >  test/common/cread.c | 12 ++++++++++++
> >  2 files changed, 14 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Unfortunately this shows a design flaw, one which is hard to fix in general. But this does improve it.
>
> The flaw is that we assume that character sequences have a time gap between them, which allows figuring out when a sequence has finished.
>
> But when starting up there may be buffered output with no gaps. I don't think there is a general fix for this problem. One option is to have a special mode at the start, where escape sequences are ignored. But the user may press an arrow key on startup.
>
> So I don't have anything much to suggest here. Let's see how this fix goes. Perhaps it is enough.
>
> Regards,
> Simon

> One option is to have a special mode at the start, where escape sequences are ignored
I think that there is no reason to emit parts of invalid escape
sequences, as current code does.
There is a very little chance (at least for a human being) to input
such sequences by intent.

> But the user may press an arrow key on startup.
As a fix, autoboot code can check for 'Esc' and keep it as a part of
input string.

Best Regards,
Yurii

Re: [PATCH v3] cli: Consume invalid escape sequences early

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

On Tue, Oct 10, 2023 at 11:16:39AM +0300, Yurii Monakov wrote:

> Unexpected 'Esc' key presses are accumulated internally, even if it is
> already clear that the current escape sequence is invalid. This results
> in weird behaviour. For example, the next character after 'Esc' key
> simply disappears from input and 'Unknown command' is printed
> after 'Enter'.
> 
> This commit fixes some issues with extra 'Esc' keys entered by user:
> 
> 1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
> =>
> nknown command 'ry 'help'
> =>
> 2. Sequence <Esc><p><r><i><Enter> gives:
> => ri
> Unknown command 'ri' - try 'help'
> =>
> 3. Extra 'Esc' key presses break backspace functionality.
> 
> Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3] cli: Consume invalid escape sequences early

[Rasmus Villemoes]

On 10/10/2023 10.16, Yurii Monakov wrote:
> Unexpected 'Esc' key presses are accumulated internally, even if it is
> already clear that the current escape sequence is invalid. This results
> in weird behaviour. For example, the next character after 'Esc' key
> simply disappears from input and 'Unknown command' is printed
> after 'Enter'.
> 
> This commit fixes some issues with extra 'Esc' keys entered by user:
> 
> 1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
> =>
> nknown command 'ry 'help'
> =>
> 2. Sequence <Esc><p><r><i><Enter> gives:
> => ri
> Unknown command 'ri' - try 'help'
> =>
> 3. Extra 'Esc' key presses break backspace functionality.

Thank you! This has been bugging me for years, since we have

CONFIG_AUTOBOOT_DELAY_STR=" "
CONFIG_AUTOBOOT_STOP_STR="\x1b"

and pressing <esc> to stop autoboot did have that side-effect of
swallowing the first following char. But I never found the time to dig
into why or if it was even fixable.

Tom has already applied this, but nevertheless

Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Rasmus