Skip to contentSkip to conversation

Fix grub-emu curses KEY_* mapping

PATCH In review Christian Franke 3 participants 5 messages Nov 2, 2007 – Nov 18, 2007

lore
Share & tools
lore

Christian Franke

author
#0 lore raw

PATCH 1/1 Fix grub-emu curses KEY_* mapping

Curses function keys do not work in grub-emu, this patch fixes this in grub_ncurses_getkey().

Another approach would be to remove the scancode translation in grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* in grub_cmdline_get() instead. The GRUB_CONSOLE_KEY_* definitions are already platform specific.

Christian

2007-11-02 Christian Franke <[email protected]>

* util/console.c (grub_ncurses_getkey): Change curses KEY_* mapping, now return control chars instead of GRUB_CONSOLE_KEY_* constants. This fixes the problem that function keys did not work in grub-emu.

util/console.c +17 -11
@@ -161,53 +161,59 @@ grub_ncurses_getkey (void)
       c = getch ();
     }
 
+  /* XXX: return GRUB_CONSOLE_KEY_* does not work here,
+     grub_cmdline_get() does not check for these constants.
+     At least on i386-pc, GRUB_CONSOLE_KEY_* are in fact keyboard
+     scancodes which are converted into control chars by
+     grub_console_getkey(). */
+
   switch (c)
     {
     case KEY_LEFT:
-      c = GRUB_CONSOLE_KEY_LEFT;
+      c = 2; /*GRUB_CONSOLE_KEY_LEFT*/
       break;
 
     case KEY_RIGHT:
-      c = GRUB_CONSOLE_KEY_RIGHT;
+      c = 6; /*GRUB_CONSOLE_KEY_RIGHT*/
       break;
       
     case KEY_UP:
-      c = GRUB_CONSOLE_KEY_UP;
+      c = 16; /*GRUB_CONSOLE_KEY_UP*/
       break;
 
     case KEY_DOWN:
-      c = GRUB_CONSOLE_KEY_DOWN;
+      c = 14; /*GRUB_CONSOLE_KEY_DOWN*/
       break;
 
     case KEY_IC:
-      c = GRUB_CONSOLE_KEY_IC;
+      c = 24; /*GRUB_CONSOLE_KEY_IC*/
       break;
 
     case KEY_DC:
-      c = GRUB_CONSOLE_KEY_DC;
+      c = 4; /*GRUB_CONSOLE_KEY_DC*/
       break;
 
     case KEY_BACKSPACE:
       /* XXX: For some reason ncurses on xterm does not return
 	 KEY_BACKSPACE.  */
     case 127: 
-      c = GRUB_CONSOLE_KEY_BACKSPACE;
+      c = 8; /*GRUB_CONSOLE_KEY_BACKSPACE*/;
       break;
 
     case KEY_HOME:
-      c = GRUB_CONSOLE_KEY_HOME;
+      c = 1; /*GRUB_CONSOLE_KEY_HOME*/
       break;
 
     case KEY_END:
-      c = GRUB_CONSOLE_KEY_END;
+      c = 5; /*GRUB_CONSOLE_KEY_END*/
       break;
 
     case KEY_NPAGE:
-      c = GRUB_CONSOLE_KEY_NPAGE;
+      c = 3; /*GRUB_CONSOLE_KEY_NPAGE*/
       break;
 
     case KEY_PPAGE:
-      c = GRUB_CONSOLE_KEY_PPAGE;
+      c = 7; /*GRUB_CONSOLE_KEY_PPAGE*/
       break;
     }
#1 lore raw
Quoted 2 lines · Christian Franke

Curses function keys do not work in grub-emu, this patch fixes this in grub_ncurses_getkey().

Ha! I once noticed this and forgot to document this bug :(

Quoted 4 lines

Another approach would be to remove the scancode translation in grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* in grub_cmdline_get() instead. The GRUB_CONSOLE_KEY_* definitions are already platform specific.

Yes, this is really weird! All archs define these macros and expect for i386-pc none of them are used. I actually wonder if any of those work :-)

Quoted 12 lines

Christian

2007-11-02 Christian Franke <[email protected]>

* util/console.c (grub_ncurses_getkey): Change curses KEY_* mapping, now return control chars instead of GRUB_CONSOLE_KEY_* constants. This fixes the problem that function keys did not work in grub-emu.

util/console.c grub_ncurses_getkey (void) · line 169

Quoted 9 lines
util/console.c +6 -0
@@ -161,53 +161,59 @@ grub_ncurses_getkey (void)
       c = getch ();
     }
 
+  /* XXX: return GRUB_CONSOLE_KEY_* does not work here,
+     grub_cmdline_get() does not check for these constants.
+     At least on i386-pc, GRUB_CONSOLE_KEY_* are in fact keyboard
+     scancodes which are converted into control chars by
+     grub_console_getkey(). */
+

I do not think this comment is needed. You explained it in the email. I do not like comments that explain why we aren't doing some things, it is usually better to explain why we do something if it is a hack or not obvious. In this case I prefer no comment.

util/console.c grub_ncurses_getkey (void) · line 174

Quoted 6 lines
util/console.c +1 -1
   switch (c)
     {
     case KEY_LEFT:
-      c = GRUB_CONSOLE_KEY_LEFT;
+      c = 2; /*GRUB_CONSOLE_KEY_LEFT*/
       break;

Please just remove the comment. As this appears to be wrong anyways :-)

Thanks,
Marco

author
#2 lore raw
Quoted 15 lines · Marco Gerards
Quoted 3 lines · Christian Franke

Curses function keys do not work in grub-emu, this patch fixes this in grub_ncurses_getkey().

Ha! I once noticed this and forgot to document this bug :(

Quoted 5 lines

Another approach would be to remove the scancode translation in grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* in grub_cmdline_get() instead. The GRUB_CONSOLE_KEY_* definitions are already platform specific.

Quoted 5 lines

Yes, this is really weird! All archs define these macros and expect for i386-pc none of them are used. I actually wonder if any of those work :-)

Even on i386-pc, GRUB_CONSOLE_KEY_* are only used once in the translation table of grub_console_getkey(). The defines are not part of the console interface definition.

I would suggest to remove the defines from all include/grub/PLATFORM/console.h. The GRUB_CONSOLE_KEY_* in i386/pc/startup.S can be replaced with the constants itself.

Quoted 24 lines

Review

Quoted 9 lines

...

(quoted diff) +6 -0
+  /* XXX: return GRUB_CONSOLE_KEY_* does not work here,
+     grub_cmdline_get() does not check for these constants.
+     At least on i386-pc, GRUB_CONSOLE_KEY_* are in fact keyboard
+     scancodes which are converted into control chars by
+     grub_console_getkey(). */
+

I do not think this comment is needed. You explained it in the email. I do not like comments that explain why we aren't doing some things, it is usually better to explain why we do something if it is a hack or not obvious. In this case I prefer no comment.

Review

Quoted 7 lines
   switch (c)
     {
     case KEY_LEFT:
(quoted diff) +1 -1
-      c = GRUB_CONSOLE_KEY_LEFT;
+      c = 2; /*GRUB_CONSOLE_KEY_LEFT*/
       break;
Quoted 4 lines

Please just remove the comment. As this appears to be wrong anyways :-)

Done.

Christian

2007-11-10 Christian Franke <[email protected]>

* util/console.c (grub_ncurses_getkey): Change curses KEY_* mapping, now return control chars instead of GRUB_CONSOLE_KEY_* constants. This fixes the problem that function keys did not work in grub-emu.

util/console.c +11 -11
@@ -164,50 +164,50 @@ grub_ncurses_getkey (void)
   switch (c)
     {
     case KEY_LEFT:
-      c = GRUB_CONSOLE_KEY_LEFT;
+      c = 2;
       break;
 
     case KEY_RIGHT:
-      c = GRUB_CONSOLE_KEY_RIGHT;
+      c = 6;
       break;
       
     case KEY_UP:
-      c = GRUB_CONSOLE_KEY_UP;
+      c = 16;
       break;
 
     case KEY_DOWN:
-      c = GRUB_CONSOLE_KEY_DOWN;
+      c = 14;
       break;
 
     case KEY_IC:
-      c = GRUB_CONSOLE_KEY_IC;
+      c = 24;
       break;
 
     case KEY_DC:
-      c = GRUB_CONSOLE_KEY_DC;
+      c = 4;
       break;
 
     case KEY_BACKSPACE:
       /* XXX: For some reason ncurses on xterm does not return
 	 KEY_BACKSPACE.  */
     case 127: 
-      c = GRUB_CONSOLE_KEY_BACKSPACE;
+      c = 8;
       break;
 
     case KEY_HOME:
-      c = GRUB_CONSOLE_KEY_HOME;
+      c = 1;
       break;
 
     case KEY_END:
-      c = GRUB_CONSOLE_KEY_END;
+      c = 5;
       break;
 
     case KEY_NPAGE:
-      c = GRUB_CONSOLE_KEY_NPAGE;
+      c = 3;
       break;
 
     case KEY_PPAGE:
-      c = GRUB_CONSOLE_KEY_PPAGE;
+      c = 7;
       break;
     }
#3 lore raw
Quoted 23 lines · Christian Franke
Quoted 15 lines · Marco Gerards
Quoted 3 lines · Christian Franke

Curses function keys do not work in grub-emu, this patch fixes this in grub_ncurses_getkey().

Ha! I once noticed this and forgot to document this bug :(

Quoted 5 lines

Another approach would be to remove the scancode translation in grub_console_getkey(), and check for GRUB_CONSOLE_KEY_* in grub_cmdline_get() instead. The GRUB_CONSOLE_KEY_* definitions are already platform specific.

Quoted 5 lines

Yes, this is really weird! All archs define these macros and expect for i386-pc none of them are used. I actually wonder if any of those work :-)

Quoted 7 lines

Even on i386-pc, GRUB_CONSOLE_KEY_* are only used once in the translation table of grub_console_getkey(). The defines are not part of the console interface definition.

I would suggest to remove the defines from all include/grub/PLATFORM/console.h. The GRUB_CONSOLE_KEY_* in i386/pc/startup.S can be replaced with the constants itself.

The other definitions have to be used. I do not like using constant values in asm/C code, instead I like using macros for this because in that case its meaning is obvious.

Quoted 30 lines
Quoted 23 lines

Review

Quoted 8 lines

...
+ /* XXX: return GRUB_CONSOLE_KEY_* does not work here,

(quoted diff) +5 -0
+     grub_cmdline_get() does not check for these constants.
+     At least on i386-pc, GRUB_CONSOLE_KEY_* are in fact keyboard
+     scancodes which are converted into control chars by
+     grub_console_getkey(). */
+

I do not think this comment is needed. You explained it in the email. I do not like comments that explain why we aren't doing some things, it is usually better to explain why we do something if it is a hack or not obvious. In this case I prefer no comment.

util/console.c grub_ncurses_getkey (void) · line 168

Quoted 7 lines
   switch (c)
     {
     case KEY_LEFT:
util/console.c +1 -1
-      c = GRUB_CONSOLE_KEY_LEFT;
+      c = 2; /*GRUB_CONSOLE_KEY_LEFT*/
       break;
Quoted 4 lines

Please just remove the comment. As this appears to be wrong anyways :-)

Done.

Quoted 7 lines

Christian

2007-11-10 Christian Franke <[email protected]>

* util/console.c (grub_ncurses_getkey): Change curses KEY_* mapping, now return control chars instead of GRUB_CONSOLE_KEY_* constants. This fixes the problem that function keys did not work in grub-emu.

This looks fine to me. Now we have to wait for the paperwork :-/

--
Marco

#4 lore raw
Quoted 8 lines · Marco Gerards
Quoted 6 lines

2007-11-10 Christian Franke <[email protected]>

* util/console.c (grub_ncurses_getkey): Change curses KEY_* mapping, now return control chars instead of GRUB_CONSOLE_KEY_* constants. This fixes the problem that function keys did not work in grub-emu.

This looks fine to me. Now we have to wait for the paperwork :-/

Committed.

Signature · 5 lines
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)