[PATCH 0/6] nokia_nx51: Attempts to debug keyboard
This series is an attempt to get the keyboard working properly with bootmenu.
It fixes the board's tstc() function, which should be in drivers/input
It also adjusts stopping of the menu autodelay - it should only stop when a whole escape sequence is received, not when the first escape character is received. That is pretty minor, so we could drop that patch.
This series also adds some debugging output. This seems to make things work correctly, suggesting that there is some other problem.
I also see this message fairly often:
cyclic function rx51_watchdog took too long: 10000us vs 1000us max
Simon Glass (6): menu: Re-enable the ANSI codes nokia_rx51: Correct tstc() implementation bootmenu: Cancel delay only when a real key is pressed boobtmenu: Add debugging getch: debugging menu: Add debugging
board/nokia/rx51/rx51.c | 3 +++ cmd/bootmenu.c | 19 ++++++++++++------- common/cli_getch.c | 23 ++++++++++++++++++++--- common/menu.c | 7 ++++++- 4 files changed, 41 insertions(+), 11 deletions(-)
The intent here was to allow ANSI codes to be disabled, since it was proving impoosible to test operation of the menu code when it kept moving the cursor. Unfortunately this ended up in the patch.
Correct this by enabling ANSI again.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Pali Rohár pali@kernel.org Reported-by: Mark Kettenis mark.kettenis@xs4all.nl Reported-by: Frank Wunderlich frank-w@public-files.de Fixes: 32bab0eae51b ("menu: Make use of CLI character processing") Tested-by: Mark Kettenis kettenis@openbsd.org Reviewed-by: Mark Kettenis kettenis@openbsd.org ---
common/menu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/menu.c b/common/menu.c index 94514177e4e9..b55cf7b99967 100644 --- a/common/menu.c +++ b/common/menu.c @@ -15,7 +15,7 @@
#include "menu.h"
-#define ansi 0 +#define ansi 1
/* * Internally, each item in a menu is represented by a struct menu_item.
On Sat, Jun 17, 2023 at 11:49:48AM +0100, Simon Glass wrote:
The intent here was to allow ANSI codes to be disabled, since it was proving impoosible to test operation of the menu code when it kept moving the cursor. Unfortunately this ended up in the patch.
Correct this by enabling ANSI again.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Pali Rohár pali@kernel.org Reported-by: Mark Kettenis mark.kettenis@xs4all.nl Reported-by: Frank Wunderlich frank-w@public-files.de Fixes: 32bab0eae51b ("menu: Make use of CLI character processing") Tested-by: Mark Kettenis kettenis@openbsd.org Reviewed-by: Mark Kettenis kettenis@openbsd.org
Applied to u-boot/master, thanks!
This returns false even when there are keys in the buffer. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org ---
board/nokia/rx51/rx51.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/board/nokia/rx51/rx51.c b/board/nokia/rx51/rx51.c index 238b9637badd..138f5a811eb6 100644 --- a/board/nokia/rx51/rx51.c +++ b/board/nokia/rx51/rx51.c @@ -668,6 +668,9 @@ static int rx51_kp_tstc(struct udevice *dev) u8 intr; u8 mods;
+ if ((KEYBUF_SIZE + keybuf_tail - keybuf_head) % KEYBUF_SIZE) + return 1; + /* localy lock twl4030 i2c bus */ if (test_and_set_bit(0, &twl_i2c_lock)) return 0;
On Saturday 17 June 2023 11:49:49 Simon Glass wrote:
This returns false even when there are keys in the buffer.
This is not truth. After scanning HW buffer it returns the correct information if some key is in SW buffer, even when function did not read anything from HW buffer.
So, NAK.
Fix it.
Signed-off-by: Simon Glass sjg@chromium.org
board/nokia/rx51/rx51.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/board/nokia/rx51/rx51.c b/board/nokia/rx51/rx51.c index 238b9637badd..138f5a811eb6 100644 --- a/board/nokia/rx51/rx51.c +++ b/board/nokia/rx51/rx51.c @@ -668,6 +668,9 @@ static int rx51_kp_tstc(struct udevice *dev) u8 intr; u8 mods;
- if ((KEYBUF_SIZE + keybuf_tail - keybuf_head) % KEYBUF_SIZE)
return 1;- /* localy lock twl4030 i2c bus */ if (test_and_set_bit(0, &twl_i2c_lock)) return 0;
-- 2.41.0.162.gfafddb0af9-goog
Hi Pali,
On Mon, 10 Jul 2023 at 08:01, Pali Rohár pali@kernel.org wrote:
On Saturday 17 June 2023 11:49:49 Simon Glass wrote:
This returns false even when there are keys in the buffer.
This is not truth. After scanning HW buffer it returns the correct information if some key is in SW buffer, even when function did not read anything from HW buffer.
So, NAK.
Why is is reading new chars when it still has some in the buffer?
Do you see i2c errors?
Regards, Simon
Fix it.
Signed-off-by: Simon Glass sjg@chromium.org
board/nokia/rx51/rx51.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/board/nokia/rx51/rx51.c b/board/nokia/rx51/rx51.c index 238b9637badd..138f5a811eb6 100644 --- a/board/nokia/rx51/rx51.c +++ b/board/nokia/rx51/rx51.c @@ -668,6 +668,9 @@ static int rx51_kp_tstc(struct udevice *dev) u8 intr; u8 mods;
if ((KEYBUF_SIZE + keybuf_tail - keybuf_head) % KEYBUF_SIZE)return 1;/* localy lock twl4030 i2c bus */ if (test_and_set_bit(0, &twl_i2c_lock)) return 0;-- 2.41.0.162.gfafddb0af9-goog
We need to decode the input character before deciding if it is a real key, or just part of an escape sequence. Check this.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/menu.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/common/menu.c b/common/menu.c index b55cf7b99967..c81494b3d1f3 100644 --- a/common/menu.c +++ b/common/menu.c @@ -446,11 +446,14 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu, continue; }
- menu->delay = -1; c = getchar();
ichar = cli_ch_process(cch, c);
+ if (!ichar) + continue; + menu->delay = -1; + switch (ichar) { case '\0': key = BKEY_NONE;
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/bootmenu.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c index 6baeedc69f91..b2fb119e70f9 100644 --- a/cmd/bootmenu.c +++ b/cmd/bootmenu.c @@ -18,6 +18,8 @@ #include <linux/delay.h> #include <linux/string.h>
+#define ansi 0 + /* maximum bootmenu entries */ #define MAX_COUNT 99
@@ -101,6 +103,7 @@ static char *bootmenu_choice_entry(void *data) /* Some key was pressed, so autoboot was stopped */ key = bootmenu_loop(menu, cch); } + printf("[got bkey %d]\n", key);
switch (key) { case BKEY_UP: @@ -521,9 +524,11 @@ static enum bootmenu_ret bootmenu_show(int delay) /* Default menu entry is always first */ menu_default_set(menu, "0");
- puts(ANSI_CURSOR_HIDE); - puts(ANSI_CLEAR_CONSOLE); - printf(ANSI_CURSOR_POSITION, 1, 1); + if (ansi) { +// puts(ANSI_CURSOR_HIDE); +// puts(ANSI_CLEAR_CONSOLE); +// printf(ANSI_CURSOR_POSITION, 1, 1); + }
init = 1;
@@ -566,10 +571,10 @@ cleanup: menu_destroy(menu); bootmenu_destroy(bootmenu);
- if (init) { - puts(ANSI_CURSOR_SHOW); - puts(ANSI_CLEAR_CONSOLE); - printf(ANSI_CURSOR_POSITION, 1, 1); + if (init && ansi) { +// puts(ANSI_CURSOR_SHOW); +// puts(ANSI_CLEAR_CONSOLE); +// printf(ANSI_CURSOR_POSITION, 1, 1); }
if (title && command) {
Signed-off-by: Simon Glass sjg@chromium.org ---
common/cli_getch.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/common/cli_getch.c b/common/cli_getch.c index 61d4cb261b81..cd44415a9f67 100644 --- a/common/cli_getch.c +++ b/common/cli_getch.c @@ -6,8 +6,11 @@ * Copyright 2022 Google LLC */
+#define LOG_CATEGORY LOGC_CORE + #include <common.h> #include <cli.h> +#include <log.h>
/** * enum cli_esc_state_t - indicates what to do with an escape character @@ -134,6 +137,8 @@ static int cli_ch_esc(struct cli_ch_state *cch, int ichar,
int cli_ch_process(struct cli_ch_state *cch, int ichar) { + log_debug(" [ichar=%x, esc_len=%d] ", ichar, cch->esc_len); + /* * 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 @@ -141,12 +146,18 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar) */ if (!ichar) { if (cch->emitting) { - if (cch->emit_upto < cch->esc_len) - return cch->esc_save[cch->emit_upto++]; + if (cch->emit_upto < cch->esc_len) { + int ch; + + ch = cch->esc_save[cch->emit_upto++]; + log_debug(" [->%x] ", ch); + return ch; + } cch->emit_upto = 0; cch->emitting = false; cch->esc_len = 0; } + log_debug(" [->0] "); return 0; } else if (ichar == -ETIMEDOUT) { /* @@ -156,15 +167,19 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar) */ if (cch->esc_len) { cch->esc_len = 0; + log_debug(" [->esc] "); return '\e'; }
/* Otherwise there is nothing to return */ + log_debug(" [->0] "); return 0; }
- if (ichar == '\n' || ichar == '\r') + if (ichar == '\n' || ichar == '\r') { + log_debug(" [->nl] "); return '\n'; + }
/* handle standard linux xterm esc sequences for arrow key, etc. */ if (cch->esc_len != 0) { @@ -186,6 +201,7 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar) cch->esc_save[cch->esc_len++] = ichar; ichar = cch->esc_save[cch->emit_upto++]; cch->emitting = true; + log_debug(" [->reject %x] ", ichar); return ichar; case ESC_CONVERTED: /* valid escape sequence, return the resulting char */ @@ -205,5 +221,6 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar) return 0; }
+ log_debug(" [->%x] ", ichar); return ichar; }
Signed-off-by: Simon Glass sjg@chromium.org ---
common/menu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/menu.c b/common/menu.c index c81494b3d1f3..fe27262ebdd0 100644 --- a/common/menu.c +++ b/common/menu.c @@ -15,7 +15,7 @@
#include "menu.h"
-#define ansi 1 +#define ansi 0
/* * Internally, each item in a menu is represented by a struct menu_item. @@ -447,8 +447,10 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu, }
c = getchar(); + printf("\n [c=%x] \n", c);
ichar = cli_ch_process(cch, c); + printf("\n [i=%x %d] \n", ichar, cch->esc_len);
if (!ichar) continue;
On Saturday 17 June 2023 11:49:47 Simon Glass wrote:
This series is an attempt to get the keyboard working properly with bootmenu.
It fixes the board's tstc() function, which should be in drivers/input
It also adjusts stopping of the menu autodelay - it should only stop when a whole escape sequence is received, not when the first escape character is received. That is pretty minor, so we could drop that patch.
This series also adds some debugging output. This seems to make things work correctly, suggesting that there is some other problem.
I also see this message fairly often:
cyclic function rx51_watchdog took too long: 10000us vs 1000us max
Simon Glass (6): menu: Re-enable the ANSI codes nokia_rx51: Correct tstc() implementation bootmenu: Cancel delay only when a real key is pressed boobtmenu: Add debugging getch: debugging menu: Add debugging
board/nokia/rx51/rx51.c | 3 +++ cmd/bootmenu.c | 19 ++++++++++++------- common/cli_getch.c | 23 ++++++++++++++++++++--- common/menu.c | 7 ++++++- 4 files changed, 41 insertions(+), 11 deletions(-)
-- 2.41.0.162.gfafddb0af9-goog
Hello, it looks like that this patch series is not finish yet (contains some commented code, etc.). Anyway, I have tested it and it makes it even worse. For example PAGE DOWN key in bootmenu on emulated UART terminal completely stopped working and do nothing. So this patch series still does not fix this problem.
Hi Pali,
On Sat, 24 Jun 2023 at 09:44, Pali Rohár pali@kernel.org wrote:
On Saturday 17 June 2023 11:49:47 Simon Glass wrote:
This series is an attempt to get the keyboard working properly with bootmenu.
It fixes the board's tstc() function, which should be in drivers/input
It also adjusts stopping of the menu autodelay - it should only stop when a whole escape sequence is received, not when the first escape character is received. That is pretty minor, so we could drop that patch.
This series also adds some debugging output. This seems to make things work correctly, suggesting that there is some other problem.
I also see this message fairly often:
cyclic function rx51_watchdog took too long: 10000us vs 1000us max
Simon Glass (6): menu: Re-enable the ANSI codes nokia_rx51: Correct tstc() implementation bootmenu: Cancel delay only when a real key is pressed boobtmenu: Add debugging getch: debugging menu: Add debugging
board/nokia/rx51/rx51.c | 3 +++ cmd/bootmenu.c | 19 ++++++++++++------- common/cli_getch.c | 23 ++++++++++++++++++++--- common/menu.c | 7 ++++++- 4 files changed, 41 insertions(+), 11 deletions(-)
-- 2.41.0.162.gfafddb0af9-goog
Hello, it looks like that this patch series is not finish yet (contains some commented code, etc.). Anyway, I have tested it and it makes it even worse. For example PAGE DOWN key in bootmenu on emulated UART terminal completely stopped working and do nothing. So this patch series still does not fix this problem.
Indeed, but I was hoping it would help you to find the problem.
The keyboard driver is currently returning 0 from tst() even when characters are available (i.e. a call to getc() will return valid chars but tstc() returns 0. I attempted to fix this, but perhaps you can do a proper fix? With that bug in place, it cannot work properly with the menu.
See the docs for tstc:
/** * tstc() - check if a key is available * * @dev: Device to check * @return 0 if no key is available, 1 if a key is available, -ve on * error */ int (*tstc)(struct udevice *dev);
Regards, Simon
On Monday 26 June 2023 10:07:35 Simon Glass wrote:
Hi Pali,
On Sat, 24 Jun 2023 at 09:44, Pali Rohár pali@kernel.org wrote:
On Saturday 17 June 2023 11:49:47 Simon Glass wrote:
This series is an attempt to get the keyboard working properly with bootmenu.
It fixes the board's tstc() function, which should be in drivers/input
It also adjusts stopping of the menu autodelay - it should only stop when a whole escape sequence is received, not when the first escape character is received. That is pretty minor, so we could drop that patch.
This series also adds some debugging output. This seems to make things work correctly, suggesting that there is some other problem.
I also see this message fairly often:
cyclic function rx51_watchdog took too long: 10000us vs 1000us max
Simon Glass (6): menu: Re-enable the ANSI codes nokia_rx51: Correct tstc() implementation bootmenu: Cancel delay only when a real key is pressed boobtmenu: Add debugging getch: debugging menu: Add debugging
board/nokia/rx51/rx51.c | 3 +++ cmd/bootmenu.c | 19 ++++++++++++------- common/cli_getch.c | 23 ++++++++++++++++++++--- common/menu.c | 7 ++++++- 4 files changed, 41 insertions(+), 11 deletions(-)
-- 2.41.0.162.gfafddb0af9-goog
Hello, it looks like that this patch series is not finish yet (contains some commented code, etc.). Anyway, I have tested it and it makes it even worse. For example PAGE DOWN key in bootmenu on emulated UART terminal completely stopped working and do nothing. So this patch series still does not fix this problem.
Indeed, but I was hoping it would help you to find the problem.
The keyboard driver is currently returning 0 from tst() even when characters are available (i.e. a call to getc() will return valid chars but tstc() returns 0.
No, look again, rx51 tstc is returning 1 if there is something in SW buffer.
I attempted to fix this, but perhaps you can do a proper fix? With that bug in place, it cannot work properly with the menu.
See the docs for tstc:
/**
- tstc() - check if a key is available
- @dev: Device to check
- @return 0 if no key is available, 1 if a key is available, -ve on
- error
*/ int (*tstc)(struct udevice *dev);
Regards, Simon
I think it matches doc; I do not see issue here.
With this patch series applied, pressing DOWN key on UART prints this "garbage" on the UART:
[c=1b]
[i=0 1] P/DOWN to move, ENTER to select, ESC to quit
[c=5b]
[i=0 2]
[c=42]
[i=e 0] [got bkey 0]
And nothing happens.
Pressing it second time prints this "garbage":
[c=1b]
[Press UP/DOWN to move, ENTER to select, ESC to quit
[c=5b]
[i=0 2]
[c=42]
[i=e 0] [got bkey 0] [got bkey 0] [got bkey 0] [got bkey 2]
And now moves cursor down.
Hi Pali,
On Mon, 10 Jul 2023 at 08:08, Pali Rohár pali@kernel.org wrote:
With this patch series applied, pressing DOWN key on UART prints this "garbage" on the UART:
[c=1b]
[i=0 1] P/DOWN to move, ENTER to select, ESC to quit
[c=5b]
[i=0 2]
[c=42]
[i=e 0] [got bkey 0]
And nothing happens.
When I tested it I got an i2c error and saw the tstc function returning ESC followed by \0. Do you see that?
Pressing it second time prints this "garbage":
[c=1b]
[Press UP/DOWN to move, ENTER to select, ESC to quit
[c=5b]
[i=0 2]
[c=42]
[i=e 0] [got bkey 0] [got bkey 0] [got bkey 0] [got bkey 2]
And now moves cursor down.
So has the bug changed in this series? I thought before that it exited?
Regards, Simon
participants (3)
-
Pali Rohár -
Simon Glass -
Tom Rini