Re: [PATCH v3] cli: Consume invalid escape sequences early
Simon, On Tue, Oct 10, 2023 at 5:58 PM Simon Glass wrote: > > On Tue, 10 Oct 2023 at 02: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 right after autoboot stop gives: > > => > > nknown command 'ry 'help' > > => > > 2. Sequence gives: > > => ri > > Unknown command 'ri' - try 'help' > > => > > 3. Extra 'Esc' key presses break backspace functionality. > > > > Signed-off-by: Yurii Monakov > > --- > > 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 > > 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
[PATCH v3] cli: Consume invalid escape sequences early
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 right after autoboot stop gives:
=>
nknown command 'ry 'help'
=>
2. Sequence gives:
=> ri
Unknown command 'ri' - try 'help'
=>
3. Extra 'Esc' key presses break backspace functionality.
Signed-off-by: Yurii Monakov
---
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
[PATCH v2] cli: Consume invalid escape sequences early
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 right after autoboot stop gives:
=>
nknown command 'ry 'help'
=>
2. Sequence gives:
=> ri
Unknown command 'ri' - try 'help'
=>
3. Extra 'Esc' key presses break backspace functionality.
Signed-off-by: Yurii Monakov
---
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
[PATCH] cli: Consume invalid escape sequences early
Hi Simon,
On Sun, Oct 8, 2023 at 2:10=E2=80=AFAM Simon Glass wrote:
>
> Hi Yurii,
>
> On Fri, 6 Oct 2023 at 15:32, Yurii Monakov wrote:
> >
> > This commit fixes some issues with extra 'Esc' keys entered by user:
> >
> > 1. Sequence right after autoboot stop gives:
> > =3D>
> > nknown command 'ry 'help'
> > =3D>
> > 2. Sequence gives:
> > =3D> ri
> > Unknown command 'ri' - try 'help'
> > =3D>
> > 3. Extra 'Esc' key presses break backspace functionality.
> >
> > Signed-off-by: Yurii Monakov
> > ---
> > common/cli_getch.c | 2 ++
> > 1 file changed, 2 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 i=
char,
> > case 1:
> > if (ichar =3D=3D '[' || ichar =3D=3D 'O')
> > act =3D ESC_SAVE;
> > + else
> > + act =3D ESC_CONVERTED;
> > break;
> > case 2:
> > switch (ichar) {
> > --
> > 2.34.1
> >
>
> This is a bit unfortunate. Basically the problem (as I understand it)
> is that characters are built up for processing so the expected ichar
> =3D=3D 0 is never passed in.
>
> I think the fix is reasonable. Does it work as well if use:
>
>else if (ichar =3D=3D '\e')
>act =3D ESC_CONVERTED;
>
> ?
>
> Also please can you add a test case o test/common/cread.c
>
> https://u-boot.readthedocs.io/en/latest/develop/tests_sandbox.html
>
> Regards,
> Simon
> This is a bit unfortunate. Basically the problem (as I understand it)
The main problem is that extra 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' printed after 'Enter'.
Googling "u-boot 'nknown' ry help" gives plenty of such examples. This ofte=
n
happens when user stops autoboot pressing 'Esc' key.
> expected ichar =3D=3D 0 is never passed in
IIUYC, this is already checked in the calling function (cli_ch_process).
> Does it work as well if use:
It was my first fix attempt. But unfortunately it does not work, because if
the next character is not valid for the escape sequence it gets swallowed.
> Also please can you add a test case o test/common/cread.c
It will be quite easy. I've tested this in the sandbox (and with qemu) and
was not able to break something with outstanding 'Esc' key presses.
Should I resend this patch?
Best Regards,
Yurii
[PATCH] cli: Consume invalid escape sequences early
This commit fixes some issues with extra 'Esc' keys entered by user:
1. Sequence right after autoboot stop gives:
=>
nknown command 'ry 'help'
=>
2. Sequence gives:
=> ri
Unknown command 'ri' - try 'help'
=>
3. Extra 'Esc' key presses break backspace functionality.
Signed-off-by: Yurii Monakov
---
common/cli_getch.c | 2 ++
1 file changed, 2 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) {
--
2.34.1
