Problem: When using KDB via keyboard it does not react to control
characters which are supported in serial mode.

Example: Chords such as CTRL+A/E/D/P do not work in keyboard mode

Solution: Before disregarding a non-printable key character, check if it
is one of the supported control characters, I have took the control
characters from the switch case upwards in this function that translates
scan codes of arrow keys/backspace/home/.. to the control characters.
I have took them all besides the TAB control character translation (I am
not sure what that maps to on the keyboard)

---
 kernel/debug/kdb/kdb_keyboard.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
index 3c2987f46f6e..2c004abd5375 100644
--- a/kernel/debug/kdb/kdb_keyboard.c
+++ b/kernel/debug/kdb/kdb_keyboard.c
@@ -172,6 +172,9 @@ int kdb_get_kbd_char(void)
 	switch (KTYP(keychar)) {
 	case KT_LETTER:
 	case KT_LATIN:
+		if (keychar == 4 || keychar == 1 || keychar == 5 || keychar == 2 ||
+		    keychar == 16 || keychar == 14 || keychar == 6)
+			return keychar;		/* non-printable supported control characters (e.g. CTRL+A) */
 		if (isprint(keychar))
 			break;		/* printable characters */
 		fallthrough;
-- 
2.39.2

Hi,

On Fri, Nov 8, 2024 at 2:11 PM Nir Lichtman <nir@lichtman.org> wrote:
>
> Problem: When using KDB via keyboard it does not react to control
> characters which are supported in serial mode.
>
> Example: Chords such as CTRL+A/E/D/P do not work in keyboard mode
>
> Solution: Before disregarding a non-printable key character, check if it
> is one of the supported control characters, I have took the control
> characters from the switch case upwards in this function that translates
> scan codes of arrow keys/backspace/home/.. to the control characters.
> I have took them all besides the TAB control character translation (I am
> not sure what that maps to on the keyboard)

Tab translates to 9, so the 9th character. Ctrl-I.


>  kernel/debug/kdb/kdb_keyboard.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
> index 3c2987f46f6e..2c004abd5375 100644
> --- a/kernel/debug/kdb/kdb_keyboard.c
> +++ b/kernel/debug/kdb/kdb_keyboard.c
> @@ -172,6 +172,9 @@ int kdb_get_kbd_char(void)
>         switch (KTYP(keychar)) {
>         case KT_LETTER:
>         case KT_LATIN:
> +               if (keychar == 4 || keychar == 1 || keychar == 5 || keychar == 2 ||
> +                   keychar == 16 || keychar == 14 || keychar == 6)
> +                       return keychar;         /* non-printable supported control characters (e.g. CTRL+A) */

This is probably OK, but IMO readability here (and above) could be
improved. Untested, but I think:

#define CTRL(c) (c - 64)

Then:

/* Allowlist supported control characters */
switch(keychar) {
case CTRL('A'): /* Home */
case CTRL('B'): /* Left */
case CTRL('D'): /* Del */
case CTRL('E'): /* End */
case CTRL('F'): /* Right */
case CTRL('I'): /* Tab */
case CTRL('N'): /* Down */
case CTRL('P'): /* Up */
    return keychar;
}

The code above could also use them, AKA (untested):

/* Translate special keys to equivalent Ctrl characters */
switch (scancode) {
case 0xF: /* Tab */
  return CTRL('I');
case 0x53: /* Del */
  return CTRL('D');
case 0x47: /* Home */
  return CTRL('A');
case 0x4F: /* End */
  return CTRL('E');
case 0x4B: /* Left */
  return CTRL('B');
case 0x48: /* Up */
  return CTRL('P');
case 0x50: /* Down */
  return CTRL('N');
case 0x4D: /* Right */
  return CTRL('F');
}

On Fri, Nov 08, 2024 at 04:31:12PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Fri, Nov 8, 2024 at 2:11 PM Nir Lichtman <nir@lichtman.org> wrote:
> >
> > Problem: When using KDB via keyboard it does not react to control
> > characters which are supported in serial mode.
> >
> > I have took them all besides the TAB control character translation (I am
> > not sure what that maps to on the keyboard)
> 
> Tab translates to 9, so the 9th character. Ctrl-I.

Noted, thanks

> 
> 
> >  kernel/debug/kdb/kdb_keyboard.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
> > index 3c2987f46f6e..2c004abd5375 100644
> > --- a/kernel/debug/kdb/kdb_keyboard.c
> > +++ b/kernel/debug/kdb/kdb_keyboard.c
> > @@ -172,6 +172,9 @@ int kdb_get_kbd_char(void)
> >         switch (KTYP(keychar)) {
> >         case KT_LETTER:
> >         case KT_LATIN:
> > +               if (keychar == 4 || keychar == 1 || keychar == 5 || keychar == 2 ||
> > +                   keychar == 16 || keychar == 14 || keychar == 6)
> > +                       return keychar;         /* non-printable supported control characters (e.g. CTRL+A) */
> 
> This is probably OK, but IMO readability here (and above) could be
> improved. Untested, but I think:
> 
> #define CTRL(c) (c - 64)

That's a good point, I will work on a v2 with this fix.

> 
> Then:
> 
> /* Allowlist supported control characters */
> switch(keychar) {
> case CTRL('A'): /* Home */
[...]

Thanks,
Nir