Correcting GRUB_CONSOLE_KEY_NPAGE/PPAGE

PATCH In review Carles Pina i Estany 3 participants 10 messages Aug 24, 2008 – Sep 24, 2008

lore

Share & tools

lore

Carles Pina i Estany

author

2008-08-24 13:02 UTC #0 lore raw

PATCH 1/1 Correcting GRUB_CONSOLE_KEY_NPAGE/PPAGE

Hello,

I send a patch that corrects the GRUB_CONSOLE_KEY_NPAGE/PPAGE constants.

ChangeLog:
--------
2008-08-24 Carles Pina i Estany <[email protected]>

* include/grub/powerpc/ieee1275/console.h (GRUB_TERM_NPAGE):
Changed to 0x5100.
(GRUB_TERM_PPAGE): Changed to 0x4900.

* include/grub/sparc64/ieee1275/console.h: Likewise.

* include/grub/i386/pc/console.h: Likewise.

* include/grub/efi/console.h: Likewise.
--------

(this new values are the same used in Grub and also the same that one can find googling for PGUP and PGDN key codes).

If everything is correct, could someone please commit it?

Cheers,

Signature · 2 lines

Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

grub/powerpc/ieee1275/console.h +2 -2

Index: include/grub/powerpc/ieee1275/console.h
===================================================================
@@ -29,8 +29,8 @@
 #define GRUB_CONSOLE_KEY_BACKSPACE	0x0008
 #define GRUB_CONSOLE_KEY_HOME		0x4700
 #define GRUB_CONSOLE_KEY_END		0x4F00
-#define GRUB_CONSOLE_KEY_NPAGE		0x4900
-#define GRUB_CONSOLE_KEY_PPAGE		0x5100
+#define GRUB_CONSOLE_KEY_NPAGE		0x5100
+#define GRUB_CONSOLE_KEY_PPAGE		0x4900
 
 /* Initialize the console system.  */
 void grub_console_init (void);

grub/sparc64/ieee1275/console.h +2 -2

Index: include/grub/sparc64/ieee1275/console.h
===================================================================
@@ -30,8 +30,8 @@
 #define GRUB_CONSOLE_KEY_BACKSPACE	0x0008
 #define GRUB_CONSOLE_KEY_HOME		0x4700
 #define GRUB_CONSOLE_KEY_END		0x4F00
-#define GRUB_CONSOLE_KEY_NPAGE		0x4900
-#define GRUB_CONSOLE_KEY_PPAGE		0x5100
+#define GRUB_CONSOLE_KEY_NPAGE		0x5100
+#define GRUB_CONSOLE_KEY_PPAGE		0x4900
 
 /* Initialize the console system.  */
 void grub_console_init (void);

grub/i386/pc/console.h +2 -2

Index: include/grub/i386/pc/console.h
===================================================================
@@ -29,8 +29,8 @@
 #define GRUB_CONSOLE_KEY_BACKSPACE	0x0008
 #define GRUB_CONSOLE_KEY_HOME		0x4700
 #define GRUB_CONSOLE_KEY_END		0x4F00
-#define GRUB_CONSOLE_KEY_NPAGE		0x4900
-#define GRUB_CONSOLE_KEY_PPAGE		0x5100
+#define GRUB_CONSOLE_KEY_NPAGE		0x5100
+#define GRUB_CONSOLE_KEY_PPAGE		0x4900
 
 #ifndef ASM_FILE

grub/efi/console.h +2 -2

Index: include/grub/efi/console.h
===================================================================
@@ -29,8 +29,8 @@
 #define GRUB_CONSOLE_KEY_BACKSPACE	0x0008
 #define GRUB_CONSOLE_KEY_HOME		0x4700
 #define GRUB_CONSOLE_KEY_END		0x4F00
-#define GRUB_CONSOLE_KEY_NPAGE		0x4900
-#define GRUB_CONSOLE_KEY_PPAGE		0x5100
+#define GRUB_CONSOLE_KEY_NPAGE		0x5100
+#define GRUB_CONSOLE_KEY_PPAGE		0x4900
 
 #include <grub/types.h>
 #include <grub/symbol.h>

Carles Pina i Estany

author

2008-08-24 14:30 UTC #1 lore raw

Hi,

I was just thinking now...

Quoted 9 lines · Carles Pina i Estany

* include/grub/powerpc/ieee1275/console.h (GRUB_TERM_NPAGE):
Changed to 0x5100.
(GRUB_TERM_PPAGE): Changed to 0x4900.
* include/grub/sparc64/ieee1275/console.h: Likewise.
* include/grub/i386/pc/console.h: Likewise.
* include/grub/efi/console.h: Likewise.

I was just thinking... is there any reason that we have the same macros in 4 different files?

Why not having just a grub/console_scancodes.h:

#ifndef GRUB_CONSOLE_SCAN_CODES

#define GRUB_CONSOLE_KEY_LEFT           0x4B00
#define GRUB_CONSOLE_KEY_RIGHT          0x4D00
#define GRUB_CONSOLE_KEY_UP             0x4800
#define GRUB_CONSOLE_KEY_DOWN           0x5000
#define GRUB_CONSOLE_KEY_IC             0x5200
#define GRUB_CONSOLE_KEY_DC             0x5300
#define GRUB_CONSOLE_KEY_BACKSPACE      0x0008
#define GRUB_CONSOLE_KEY_HOME           0x4700
#define GRUB_CONSOLE_KEY_END            0x4F00
#define GRUB_CONSOLE_KEY_NPAGE          0x5100
#define GRUB_CONSOLE_KEY_PPAGE          0x4900

#endif

and include this file from the other 4 files?

Thanks,

Signature · 2 lines

Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

Vesa Jääskeläinen

2008-08-24 16:09 UTC #2 lore raw

Quoted 19 lines · Carles Pina i Estany

I was just thinking... is there any reason that we have the same macros in 4 different files?

Why not having just a grub/console_scancodes.h:

#ifndef GRUB_CONSOLE_SCAN_CODES

#define GRUB_CONSOLE_KEY_LEFT           0x4B00
#define GRUB_CONSOLE_KEY_RIGHT          0x4D00
#define GRUB_CONSOLE_KEY_UP             0x4800
#define GRUB_CONSOLE_KEY_DOWN           0x5000
#define GRUB_CONSOLE_KEY_IC             0x5200
#define GRUB_CONSOLE_KEY_DC             0x5300
#define GRUB_CONSOLE_KEY_BACKSPACE      0x0008
#define GRUB_CONSOLE_KEY_HOME           0x4700
#define GRUB_CONSOLE_KEY_END            0x4F00
#define GRUB_CONSOLE_KEY_NPAGE          0x5100
#define GRUB_CONSOLE_KEY_PPAGE          0x4900

Quoted 3 lines

#endif
and include this file from the other 4 files?

Sounds fine. Why do we have same thing with TERM and CONSOLE?

Thanks,
Vesa Jääskeläinen

Carles Pina i Estany

author

2008-08-24 18:11 UTC #3 lore raw

Hi,

Quoted 9 lines · Vesa Jääskeläinen

Quoted 8 lines · Carles Pina i Estany
I was just thinking... is there any reason that we have the same macros in 4 different files?
Why not having just a grub/console_scancodes.h:
#ifndef GRUB_CONSOLE_SCAN_CODES
#define GRUB_CONSOLE_KEY_LEFT 0x4B00

[...]

Quoted 7 lines

Quoted 5 lines
#define GRUB_CONSOLE_KEY_PPAGE 0x4900
#endif
and include this file from the other 4 files?
Sounds fine. Why do we have same thing with TERM and CONSOLE?

Right now, these macros are in these files:
include/grub/powerpc/ieee1275/console.h
include/grub/sparc64/ieee1275/console.h
include/grub/i386/pc/console.h
include/grub/efi/console.h

In sparc64 there is a FIXME above the macros:
/* FIXME (sparc64). */

I would merge these macros in a file and include this file if all architectures uses the same codes. But maybe next architecture (I mean, right know it's powerpc, sparc64, i386 and efi) will use a different codes.

What are your opinions, merging to a single file (grub/console_scancodes.h?) or just leave in different files because one of the files has a FIXME and maybe our next architecture will not share these codes?

Sorry for so many questions. I guess that you have had the same question in other areas in Grub.

Signature · 2 lines

Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

Robert Millan

2008-09-24 10:25 UTC #4 lore raw

In reply to Vesa Jääskeläinen

Quoted 2 lines · Vesa Jääskeläinen

Sounds fine. Why do we have same thing with TERM and CONSOLE?

TERM is for GRUB's internal, arch-independant, key codes. CONSOLE is for key codes as provided by i386-pc BIOS.

Signature · 5 lines

Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Robert Millan

2008-08-30 11:51 UTC #5 lore raw

In reply to Carles Pina i Estany

Quoted 11 lines · Carles Pina i Estany

Hi,
I was just thinking now...
Quoted 4 lines · Carles Pina i Estany
	* include/grub/powerpc/ieee1275/console.h (GRUB_TERM_NPAGE):
	Changed to 0x5100.
	(GRUB_TERM_PPAGE): Changed to 0x4900.

Quoted 8 lines

Quoted 5 lines
* include/grub/sparc64/ieee1275/console.h: Likewise.
* include/grub/i386/pc/console.h: Likewise.
* include/grub/efi/console.h: Likewise.
I was just thinking... is there any reason that we have the same macros in 4 different files?

This would need to be checked carefully, but a quick peek at GRUB_CONSOLE_KEY_LEFT:

./kern/i386/pc/startup.S: .word GRUB_CONSOLE_KEY_LEFT, GRUB_TERM_LEFT
./include/grub/i386/pc/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00
./include/grub/efi/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00
./include/grub/powerpc/ieee1275/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00
./include/grub/sparc64/ieee1275/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00

seems to indicate these macros are part of the BIOS console interface, and therefore only used on i386-pc.

If you can confirm this, I'd recommend just removing them from the other console.h files.

Signature · 5 lines

Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Carles Pina i Estany

author

2008-09-05 17:00 UTC #6 lore raw

Hi,

Quoted 30 lines · Robert Millan

Quoted 11 lines · Carles Pina i Estany
Hi,
I was just thinking now...
Quoted 4 lines · Carles Pina i Estany
	* include/grub/powerpc/ieee1275/console.h (GRUB_TERM_NPAGE):
	Changed to 0x5100.
	(GRUB_TERM_PPAGE): Changed to 0x4900.
Quoted 8 lines
Quoted 5 lines
* include/grub/sparc64/ieee1275/console.h: Likewise.
* include/grub/i386/pc/console.h: Likewise.
* include/grub/efi/console.h: Likewise.
I was just thinking... is there any reason that we have the same macros in 4 different files?
This would need to be checked carefully, but a quick peek at GRUB_CONSOLE_KEY_LEFT:
./kern/i386/pc/startup.S: .word GRUB_CONSOLE_KEY_LEFT, GRUB_TERM_LEFT
./include/grub/i386/pc/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00
./include/grub/efi/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00
./include/grub/powerpc/ieee1275/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00
./include/grub/sparc64/ieee1275/console.h:#define GRUB_CONSOLE_KEY_LEFT 0x4B00

Quoted 5 lines

seems to indicate these macros are part of the BIOS console interface, and therefore only used on i386-pc.
If you can confirm this, I'd recommend just removing them from the other console.h files.

I will also check it and confirm (or not confirm). I will do it on next days.

Signature · 2 lines

Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

Carles Pina i Estany

author

2008-08-24 18:34 UTC #7 lore raw

Re: [PATCH] Correcting GRUB_CONSOLE_KEY_NPAGE/PPAGE (summary)

In reply to Carles Pina i Estany

Hello,

Quoted 12 lines · Carles Pina i Estany

ChangeLog:
--------
2008-08-24 Carles Pina i Estany <[email protected]>
* include/grub/powerpc/ieee1275/console.h (GRUB_TERM_NPAGE):
Changed to 0x5100.
(GRUB_TERM_PPAGE): Changed to 0x4900.
* include/grub/sparc64/ieee1275/console.h: Likewise.
* include/grub/i386/pc/console.h: Likewise.

Quoted 7 lines

* include/grub/efi/console.h: Likewise.
--------
(this new values are the same used in Grub and also the same that one can find googling for PGUP and PGDN key codes).
If everything is correct, could someone please commit it?

summarizing all discussions about this topic (there is three different issues):

a) This patch fix the incorrect values of GRUB_TERM_PPAGE/NPAGE. In my opinion, can be committed (so, feel free to commit if you think so).

b) The name of PPAGE/NPAGE/IC/... are ncurses based and not very intuitive. Discussing if we should change these names in the same thread.

c) If it's save merging all places where these macros appears (including a new file with the values). Discussing in the same thread.

I would go patching or not patching these issues one by one or we will discuss it forever :-)

Signature · 2 lines

Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

Robert Millan

2008-09-24 10:27 UTC #8 lore raw

Re: [PATCH] Correcting GRUB_CONSOLE_KEY_NPAGE/PPAGE (summary)

Quoted 6 lines · Carles Pina i Estany

summarizing all discussions about this topic (there is three different issues):
a) This patch fix the incorrect values of GRUB_TERM_PPAGE/NPAGE. In my opinion, can be committed (so, feel free to commit if you think so).

Done.

Quoted 3 lines

b) The name of PPAGE/NPAGE/IC/... are ncurses based and not very intuitive. Discussing if we should change these names in the same thread.

I'm not concerned about this. But if Vesa wants to change them, I have no objection either.

Quoted 2 lines

c) If it's save merging all places where these macros appears (including a new file with the values). Discussing in the same thread.

The non-i386 instances of these macros have been removed.

Signature · 5 lines

Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Robert Millan

2008-09-24 10:24 UTC #9 lore raw

In reply to Carles Pina i Estany

Quoted 17 lines · Carles Pina i Estany

Hello,
I send a patch that corrects the GRUB_CONSOLE_KEY_NPAGE/PPAGE constants.
ChangeLog:
--------
2008-08-24 Carles Pina i Estany <[email protected]>
* include/grub/powerpc/ieee1275/console.h (GRUB_TERM_NPAGE):
Changed to 0x5100.
(GRUB_TERM_PPAGE): Changed to 0x4900.
* include/grub/sparc64/ieee1275/console.h: Likewise.
* include/grub/i386/pc/console.h: Likewise.

Quoted 7 lines

* include/grub/efi/console.h: Likewise.
--------
(this new values are the same used in Grub and also the same that one can find googling for PGUP and PGDN key codes).
If everything is correct, could someone please commit it?

Committed (with some adjustments).

Signature · 5 lines

Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."