Re: [PATCH v3] cli: Consume invalid escape sequences early
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 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. Thank you! This has been bugging me for years, since we have CONFIG_AUTOBOOT_DELAY_STR=" " CONFIG_AUTOBOOT_STOP_STR="\x1b" and pressing 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
Re: [PATCH v3] cli: Consume invalid escape sequences early
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 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 > Reviewed-by: Simon Glass Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
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
Re: [PATCH v3] cli: Consume invalid escape sequences early
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
[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
