[PATCH] Detect key modifier status in 'sleep --interruptible'

[PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

The attached patch arranges to make use of key modifier status in 'sleep
--interruptible', so that you can interrupt the sleep by pressing Shift.
The benefit of this is that on some architectures it makes it possible
and even sometimes reasonable to use an extremely short sleep for a
hiddenmenu-type arrangement - even a zero sleep - because on some
architectures you can query key modifier state asynchronously without
having to wait for make scancodes by way of key repeat or whatever, so
all the user needs to do is hold Shift as they come out of the BIOS.
While a zero sleep probably isn't a reasonable default for GRUB in
general, it's very useful if you're building single-boot out-of-the-box
systems where you just want to get on with booting the one installed OS.

This is based on previous work in GRUB Legacy by Kristian and Peter
(CCed). Peter tells me that Red Hat has an assignment already on file,
and it looks like mine is finally making some progress ...

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

[-- Attachment #2: keystatus.diff --]
[-- Type: text/x-diff, Size: 10318 bytes --]

diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/commands/sleep.c grub2-1.96+20090725.new/commands/sleep.c
--- grub2-1.96+20090725/commands/sleep.c	2009-03-21 08:39:59.000000000 +0000
+++ grub2-1.96+20090725.new/commands/sleep.c	2009-08-12 16:34:00.000000000 +0100
@@ -43,6 +43,22 @@
   grub_printf ("%d    ", n);
 }
 
+static int
+grub_check_keyboard (void)
+{
+  int mods = grub_keystatus ();
+  if (mods >= 0 &&
+      (mods & (GRUB_TERM_STATUS_LEFT_SHIFT |
+	       GRUB_TERM_STATUS_RIGHT_SHIFT)) != 0)
+    return 1;
+
+  if (grub_checkkey () >= 0 &&
+      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    return 1;
+
+  return 0;
+}
+
 /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
 static int
 grub_interruptible_millisleep (grub_uint32_t ms)
@@ -52,8 +68,7 @@
   start = grub_get_time_ms ();
 
   while (grub_get_time_ms () - start < ms)
-    if (grub_checkkey () >= 0 &&
-	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    if (grub_check_keyboard ())
       return 1;
 
   return 0;
@@ -74,7 +89,7 @@
   if (n == 0)
     {
       /* Either `0' or broken input.  */
-      return 0;
+      return grub_check_keyboard ();
     }
 
   xy = grub_getxy ();
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/include/grub/i386/pc/console.h grub2-1.96+20090725.new/include/grub/i386/pc/console.h
--- grub2-1.96+20090725/include/grub/i386/pc/console.h	2008-11-12 17:43:39.000000000 +0000
+++ grub2-1.96+20090725.new/include/grub/i386/pc/console.h	2009-08-12 16:32:21.000000000 +0100
@@ -42,6 +42,7 @@
 /* These are global to share code between C and asm.  */
 int grub_console_checkkey (void);
 int grub_console_getkey (void);
+int grub_console_keystatus (void);
 grub_uint16_t grub_console_getxy (void);
 void grub_console_gotoxy (grub_uint8_t x, grub_uint8_t y);
 void grub_console_cls (void);
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/include/grub/term.h grub2-1.96+20090725.new/include/grub/term.h
--- grub2-1.96+20090725/include/grub/term.h	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/include/grub/term.h	2009-08-12 16:32:21.000000000 +0100
@@ -72,6 +72,13 @@
 #define GRUB_TERM_NEED_INIT	(1 << 16)
 
 
+/* Bitmasks for modifier keys returned by grub_keystatus.  */
+#define GRUB_TERM_STATUS_ALT		(1 << 3)
+#define GRUB_TERM_STATUS_CTRL		(1 << 2)
+#define GRUB_TERM_STATUS_LEFT_SHIFT	(1 << 1)
+#define GRUB_TERM_STATUS_RIGHT_SHIFT	(1 << 0)
+
+
 /* Unicode characters for fancy graphics.  */
 #define GRUB_TERM_DISP_LEFT	0x2190
 #define GRUB_TERM_DISP_UP	0x2191
@@ -157,6 +164,9 @@
 
   /* Get a character.  */
   int (*getkey) (void);
+
+  /* Get keyboard modifier status.  */
+  int (*keystatus) (void);
 };
 typedef struct grub_term_input *grub_term_input_t;
 
@@ -275,6 +285,7 @@
 grub_ssize_t EXPORT_FUNC(grub_getcharwidth) (grub_uint32_t code);
 int EXPORT_FUNC(grub_getkey) (void);
 int EXPORT_FUNC(grub_checkkey) (void);
+int EXPORT_FUNC(grub_keystatus) (void);
 grub_uint16_t EXPORT_FUNC(grub_getwh) (void);
 grub_uint16_t EXPORT_FUNC(grub_getxy) (void);
 void EXPORT_FUNC(grub_gotoxy) (grub_uint8_t x, grub_uint8_t y);
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/kern/i386/pc/startup.S grub2-1.96+20090725.new/kern/i386/pc/startup.S
--- grub2-1.96+20090725/kern/i386/pc/startup.S	2009-08-12 16:32:18.000000000 +0100
+++ grub2-1.96+20090725.new/kern/i386/pc/startup.S	2009-08-12 16:32:21.000000000 +0100
@@ -1285,6 +1285,38 @@
 
 
 /*
+ * int grub_console_keystatus (void)
+ * BIOS call "INT 16H Function 12H" to get keyboard modifier status
+ *	Call with	%ah = 0x12
+ *	Return:		%al = keyboard state:
+ *				bit 3: alt key down
+ *				bit 2: ctrl key down
+ *				bit 1: left shift key down
+ *				bit 0: right shift key down
+ */
+FUNCTION(grub_console_keystatus)
+	pushl	%ebp
+
+	call	prot_to_real	/* enter real mode */
+	.code16
+
+	movb	$0x12, %ah
+	int	$0x16
+	movw	%ax, %dx
+
+	DATA32	call	real_to_prot
+	.code32
+
+	movw	%dx, %ax
+
+	/* Mask out numlock, capslock and insert state. */
+	andl	$0x0f0f, %eax
+
+	popl	%ebp
+	ret
+
+
+/*
  * grub_uint16_t grub_console_getxy (void)
  * BIOS call "INT 10H Function 03h" to get cursor position
  *	Call with	%ah = 0x03
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/kern/term.c grub2-1.96+20090725.new/kern/term.c
--- grub2-1.96+20090725/kern/term.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/kern/term.c	2009-08-12 16:32:21.000000000 +0100
@@ -140,6 +140,12 @@
   return (grub_cur_term_input->checkkey) ();
 }
 
+int
+grub_keystatus (void)
+{
+  return (grub_cur_term_input->keystatus) ();
+}
+
 grub_uint16_t
 grub_getxy (void)
 {
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/efi/console.c grub2-1.96+20090725.new/term/efi/console.c
--- grub2-1.96+20090725/term/efi/console.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/efi/console.c	2009-08-12 16:32:21.000000000 +0100
@@ -206,6 +206,13 @@
 }
 
 static int
+grub_console_keystatus (void)
+{
+  /* EFI does not support reading modifier key status.  */
+  return 0;
+}
+
+static int
 grub_console_getkey (void)
 {
   grub_efi_simple_input_interface_t *i;
@@ -337,6 +344,7 @@
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .keystatus = grub_console_keystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/at_keyboard.c grub2-1.96+20090725.new/term/i386/pc/at_keyboard.c
--- grub2-1.96+20090725/term/i386/pc/at_keyboard.c	2009-04-14 19:12:14.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/at_keyboard.c	2009-08-12 16:32:21.000000000 +0100
@@ -200,6 +200,14 @@
   return key;
 }
 
+static int
+grub_at_keyboard_keystatus (void)
+{
+  /* FIXME: I don't know if getting key modifier status is possible without
+   * BIOS help.  */
+  return 0;
+}
+
 static grub_err_t
 grub_keyboard_controller_init (void)
 {
@@ -222,6 +230,7 @@
     .fini = grub_keyboard_controller_fini,
     .checkkey = grub_at_keyboard_checkkey,
     .getkey = grub_at_keyboard_getkey,
+    .keystatus = grub_at_keyboard_keystatus,
   };
 
 GRUB_MOD_INIT(at_keyboard)
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/console.c grub2-1.96+20090725.new/term/i386/pc/console.c
--- grub2-1.96+20090725/term/i386/pc/console.c	2009-04-14 19:12:14.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/console.c	2009-08-12 16:32:21.000000000 +0100
@@ -25,6 +25,7 @@
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .keystatus = grub_console_keystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/serial.c grub2-1.96+20090725.new/term/i386/pc/serial.c
--- grub2-1.96+20090725/term/i386/pc/serial.c	2009-06-11 17:17:45.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/serial.c	2009-08-12 16:32:21.000000000 +0100
@@ -259,6 +259,13 @@
   return c;
 }
 
+static int
+grub_serial_keystatus (void)
+{
+  /* We can't get key modifier status on serial consoles.  */
+  return 0;
+}
+
 /* Initialize a serial device. PORT is the port number for a serial device.
    SPEED is a DTE-DTE speed which must be one of these: 2400, 4800, 9600,
    19200, 38400, 57600 and 115200. WORD_LEN is the word length to be used
@@ -465,6 +472,7 @@
   .name = "serial",
   .checkkey = grub_serial_checkkey,
   .getkey = grub_serial_getkey,
+  .keystatus = grub_serial_keystatus,
 };
 
 static struct grub_term_output grub_serial_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/ieee1275/ofconsole.c grub2-1.96+20090725.new/term/ieee1275/ofconsole.c
--- grub2-1.96+20090725/term/ieee1275/ofconsole.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/ieee1275/ofconsole.c	2009-08-12 16:32:21.000000000 +0100
@@ -227,6 +227,13 @@
   return key;
 }
 
+static int
+grub_ofconsole_keystatus (void)
+{
+  /* No attempt to support this for Open Firmware yet. */
+  return 0;
+}
+
 static grub_uint16_t
 grub_ofconsole_getxy (void)
 {
@@ -396,6 +403,7 @@
     .fini = grub_ofconsole_fini,
     .checkkey = grub_ofconsole_checkkey,
     .getkey = grub_ofconsole_getkey,
+    .keystatus = grub_ofconsole_keystatus,
   };
 
 static struct grub_term_output grub_ofconsole_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/usb_keyboard.c grub2-1.96+20090725.new/term/usb_keyboard.c
--- grub2-1.96+20090725/term/usb_keyboard.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/usb_keyboard.c	2009-08-12 16:32:21.000000000 +0100
@@ -234,6 +234,47 @@
   return key;
 }
 
+static int
+grub_usb_keyboard_keystatus (void)
+{
+  unsigned char data[8];
+  int mods = 0;
+  int i;
+  grub_err_t err;
+
+  for (i = 0; i < 50; i++)
+    {
+      /* Get_Report.  */
+      err = grub_usb_keyboard_getreport (usbdev, data);
+
+      if (! err)
+	break;
+    }
+
+  if (err)
+    return -1;
+
+  grub_dprintf ("usb_keyboard",
+		"report: 0x%02x 0x%02x 0x%02x 0x%02x"
+		" 0x%02x 0x%02x 0x%02x 0x%02x\n",
+		data[0], data[1], data[2], data[3],
+		data[4], data[5], data[6], data[7]);
+
+  /* Check Shift, Control, and Alt status.  */
+  if (data[0] & 0x02)
+    mods |= GRUB_TERM_STATUS_LEFT_SHIFT;
+  if (data[0] & 0x20)
+    mods |= GRUB_TERM_STATUS_RIGHT_SHIFT;
+  if (data[0] & 0x01 || data[0] & 0x10)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (data[0] & 0x04 || data[0] & 0x40)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  grub_errno = GRUB_ERR_NONE;
+
+  return mods;
+}
+
 static struct grub_term_input grub_usb_keyboard_term =
   {
     .name = "usb_keyboard",
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/util/console.c grub2-1.96+20090725.new/util/console.c
--- grub2-1.96+20090725/util/console.c	2009-07-07 21:03:03.000000000 +0100
+++ grub2-1.96+20090725.new/util/console.c	2009-08-12 16:32:21.000000000 +0100
@@ -258,6 +258,13 @@
   return c;
 }
 
+static int
+grub_ncurses_keystatus (void)
+{
+  /* ncurses can't tell us keyboard modifier status.  */
+  return 0;
+}
+
 static grub_uint16_t
 grub_ncurses_getxy (void)
 {
@@ -350,6 +357,7 @@
     .name = "console",
     .checkkey = grub_ncurses_checkkey,
     .getkey = grub_ncurses_getkey,
+    .keystatus = grub_ncurses_keystatus,
   };
 
 static struct grub_term_output grub_ncurses_term_output =

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Vladimir 'phcoder' Serbinenko]

> This is based on previous work in GRUB Legacy by Kristian and Peter
> (CCed). Peter tells me that Red Hat has an assignment already on file,
> and it looks like mine is finally making some progress ...
We would need RedHat and Kristian to sign some papers.

Constants are i386-pc specific. In particular left and right shift are
distinguished but not ctrl or alt. We should either distinguish them
all or none.
Going to real mode to retrieve keyboard status is absolutely
unnecessary: same info is available in BDA. Perhaps our console driver
should switch to BDA altogether but I'm not sure that this will work
since grub2 disables interrupts (tests needed) but it definitly works
with keyboard status. Driver should return a consistent value even if
it doesn't support asynchronous query unless driver ignores modifier
keys altogether (EFI, sigh) or has no way to know if they are pressed
unless they are pressed together with another key. In the case of
at_keyboard it specifically means to return at_keyboard_status
transformed to correct format.
>
> Thanks,
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Vladimir 'phcoder' Serbinenko]

On Wed, Aug 12, 2009 at 6:10 PM, Vladimir 'phcoder'
Serbinenko<phcoder@gmail.com> wrote:
>> This is based on previous work in GRUB Legacy by Kristian and Peter
>> (CCed). Peter tells me that Red Hat has an assignment already on file,
>> and it looks like mine is finally making some progress ...
> We would need RedHat and Kristian to sign some papers.
>
> Constants are i386-pc specific. In particular left and right shift are
> distinguished but not ctrl or alt. We should either distinguish them
> all or none.
Another possible problem is about configuration - configuration tool
has to know if 0 timeout is permissible. It depends on both platform
and used driver. But since this probably won't ever be a default
config for mainline it's not our headache ;)

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Wed, Aug 12, 2009 at 06:10:18PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Colin Watson wrote:
> > This is based on previous work in GRUB Legacy by Kristian and Peter
> > (CCed). Peter tells me that Red Hat has an assignment already on file,
> > and it looks like mine is finally making some progress ...
> 
> We would need RedHat and Kristian to sign some papers.

Peter said they were already done, but if that's the case then fine.
Peter/Kristian, can you double-check this?

> Constants are i386-pc specific. In particular left and right shift are
> distinguished but not ctrl or alt. We should either distinguish them
> all or none.

The values I picked for the constants were convenient for i386-pc, but
that's because it's the only architecture that currently needs an
assembly implementation and it seemed to make sense to me to put the
harder work of transforming values around into C code.

I'm not sure what you mean by "distinguish" here though. I included
constants for control and alt that are currently unused, which may
actually have been a mistake since the original Red Hat patch to GRUB 1
honoured those too in its hiddenmenu handling. Is that what you mean?

> Going to real mode to retrieve keyboard status is absolutely
> unnecessary: same info is available in BDA. Perhaps our console driver
> should switch to BDA altogether but I'm not sure that this will work
> since grub2 disables interrupts (tests needed) but it definitly works
> with keyboard status.

OK, fair enough. The attached updated patch makes this change.

> Driver should return a consistent value even if it doesn't support
> asynchronous query unless driver ignores modifier keys altogether
> (EFI, sigh) or has no way to know if they are pressed unless they are
> pressed together with another key.

I honestly don't see much value in this. Think about what it's being
used for; users are not going to know or guess that Shift only works to
interrupt the boot if you press some non-modifier key too. On the basis
of not including code we aren't going to need, I think it's better to
not attempt to support independent queries of modifier key status on
such drivers at all, and just pretend that no modifiers are pressed.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

[-- Attachment #2: keystatus.diff --]
[-- Type: text/x-diff, Size: 10123 bytes --]

diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/commands/sleep.c grub2-1.96+20090725.new/commands/sleep.c
--- grub2-1.96+20090725/commands/sleep.c	2009-03-21 08:39:59.000000000 +0000
+++ grub2-1.96+20090725.new/commands/sleep.c	2009-08-12 18:27:38.000000000 +0100
@@ -43,6 +43,22 @@
   grub_printf ("%d    ", n);
 }
 
+static int
+grub_check_keyboard (void)
+{
+  int mods = grub_keystatus ();
+  if (mods >= 0 &&
+      (mods & (GRUB_TERM_STATUS_LEFT_SHIFT |
+	       GRUB_TERM_STATUS_RIGHT_SHIFT)) != 0)
+    return 1;
+
+  if (grub_checkkey () >= 0 &&
+      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    return 1;
+
+  return 0;
+}
+
 /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
 static int
 grub_interruptible_millisleep (grub_uint32_t ms)
@@ -52,8 +68,7 @@
   start = grub_get_time_ms ();
 
   while (grub_get_time_ms () - start < ms)
-    if (grub_checkkey () >= 0 &&
-	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    if (grub_check_keyboard ())
       return 1;
 
   return 0;
@@ -74,7 +89,7 @@
   if (n == 0)
     {
       /* Either `0' or broken input.  */
-      return 0;
+      return grub_check_keyboard ();
     }
 
   xy = grub_getxy ();
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/include/grub/i386/pc/console.h grub2-1.96+20090725.new/include/grub/i386/pc/console.h
--- grub2-1.96+20090725/include/grub/i386/pc/console.h	2008-11-12 17:43:39.000000000 +0000
+++ grub2-1.96+20090725.new/include/grub/i386/pc/console.h	2009-08-12 18:27:38.000000000 +0100
@@ -42,6 +42,7 @@
 /* These are global to share code between C and asm.  */
 int grub_console_checkkey (void);
 int grub_console_getkey (void);
+int grub_console_keystatus (void);
 grub_uint16_t grub_console_getxy (void);
 void grub_console_gotoxy (grub_uint8_t x, grub_uint8_t y);
 void grub_console_cls (void);
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/include/grub/term.h grub2-1.96+20090725.new/include/grub/term.h
--- grub2-1.96+20090725/include/grub/term.h	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/include/grub/term.h	2009-08-12 18:27:38.000000000 +0100
@@ -72,6 +72,13 @@
 #define GRUB_TERM_NEED_INIT	(1 << 16)
 
 
+/* Bitmasks for modifier keys returned by grub_keystatus.  */
+#define GRUB_TERM_STATUS_ALT		(1 << 3)
+#define GRUB_TERM_STATUS_CTRL		(1 << 2)
+#define GRUB_TERM_STATUS_LEFT_SHIFT	(1 << 1)
+#define GRUB_TERM_STATUS_RIGHT_SHIFT	(1 << 0)
+
+
 /* Unicode characters for fancy graphics.  */
 #define GRUB_TERM_DISP_LEFT	0x2190
 #define GRUB_TERM_DISP_UP	0x2191
@@ -157,6 +164,9 @@
 
   /* Get a character.  */
   int (*getkey) (void);
+
+  /* Get keyboard modifier status.  */
+  int (*keystatus) (void);
 };
 typedef struct grub_term_input *grub_term_input_t;
 
@@ -275,6 +285,7 @@
 grub_ssize_t EXPORT_FUNC(grub_getcharwidth) (grub_uint32_t code);
 int EXPORT_FUNC(grub_getkey) (void);
 int EXPORT_FUNC(grub_checkkey) (void);
+int EXPORT_FUNC(grub_keystatus) (void);
 grub_uint16_t EXPORT_FUNC(grub_getwh) (void);
 grub_uint16_t EXPORT_FUNC(grub_getxy) (void);
 void EXPORT_FUNC(grub_gotoxy) (grub_uint8_t x, grub_uint8_t y);
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/kern/i386/pc/startup.S grub2-1.96+20090725.new/kern/i386/pc/startup.S
--- grub2-1.96+20090725/kern/i386/pc/startup.S	2009-08-12 18:27:36.000000000 +0100
+++ grub2-1.96+20090725.new/kern/i386/pc/startup.S	2009-08-12 18:33:21.000000000 +0100
@@ -1285,6 +1285,25 @@
 
 
 /*
+ * int grub_console_keystatus (void)
+ * Return keyboard modifier status in %al:
+ *	bit 3: alt key down
+ *	bit 2: ctrl key down
+ *	bit 1: left shift key down
+ *	bit 0: right shift key down
+ */
+FUNCTION(grub_console_keystatus)
+	/* keyboard status bits in BIOS Data Area */
+	movw	$0x0417, %bx
+	movb	(%bx), %al
+
+	/* Mask out everything but Shift, Ctrl, and Alt. */
+	andl	$0x000f, %eax
+
+	ret
+
+
+/*
  * grub_uint16_t grub_console_getxy (void)
  * BIOS call "INT 10H Function 03h" to get cursor position
  *	Call with	%ah = 0x03
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/kern/term.c grub2-1.96+20090725.new/kern/term.c
--- grub2-1.96+20090725/kern/term.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/kern/term.c	2009-08-12 18:27:38.000000000 +0100
@@ -140,6 +140,12 @@
   return (grub_cur_term_input->checkkey) ();
 }
 
+int
+grub_keystatus (void)
+{
+  return (grub_cur_term_input->keystatus) ();
+}
+
 grub_uint16_t
 grub_getxy (void)
 {
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/efi/console.c grub2-1.96+20090725.new/term/efi/console.c
--- grub2-1.96+20090725/term/efi/console.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/efi/console.c	2009-08-12 18:27:38.000000000 +0100
@@ -206,6 +206,13 @@
 }
 
 static int
+grub_console_keystatus (void)
+{
+  /* EFI does not support reading modifier key status.  */
+  return 0;
+}
+
+static int
 grub_console_getkey (void)
 {
   grub_efi_simple_input_interface_t *i;
@@ -337,6 +344,7 @@
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .keystatus = grub_console_keystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/at_keyboard.c grub2-1.96+20090725.new/term/i386/pc/at_keyboard.c
--- grub2-1.96+20090725/term/i386/pc/at_keyboard.c	2009-04-14 19:12:14.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/at_keyboard.c	2009-08-12 18:27:38.000000000 +0100
@@ -200,6 +200,14 @@
   return key;
 }
 
+static int
+grub_at_keyboard_keystatus (void)
+{
+  /* FIXME: I don't know if getting key modifier status is possible without
+   * BIOS help.  */
+  return 0;
+}
+
 static grub_err_t
 grub_keyboard_controller_init (void)
 {
@@ -222,6 +230,7 @@
     .fini = grub_keyboard_controller_fini,
     .checkkey = grub_at_keyboard_checkkey,
     .getkey = grub_at_keyboard_getkey,
+    .keystatus = grub_at_keyboard_keystatus,
   };
 
 GRUB_MOD_INIT(at_keyboard)
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/console.c grub2-1.96+20090725.new/term/i386/pc/console.c
--- grub2-1.96+20090725/term/i386/pc/console.c	2009-04-14 19:12:14.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/console.c	2009-08-12 18:27:38.000000000 +0100
@@ -25,6 +25,7 @@
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .keystatus = grub_console_keystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/serial.c grub2-1.96+20090725.new/term/i386/pc/serial.c
--- grub2-1.96+20090725/term/i386/pc/serial.c	2009-06-11 17:17:45.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/serial.c	2009-08-12 18:27:38.000000000 +0100
@@ -259,6 +259,13 @@
   return c;
 }
 
+static int
+grub_serial_keystatus (void)
+{
+  /* We can't get key modifier status on serial consoles.  */
+  return 0;
+}
+
 /* Initialize a serial device. PORT is the port number for a serial device.
    SPEED is a DTE-DTE speed which must be one of these: 2400, 4800, 9600,
    19200, 38400, 57600 and 115200. WORD_LEN is the word length to be used
@@ -465,6 +472,7 @@
   .name = "serial",
   .checkkey = grub_serial_checkkey,
   .getkey = grub_serial_getkey,
+  .keystatus = grub_serial_keystatus,
 };
 
 static struct grub_term_output grub_serial_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/ieee1275/ofconsole.c grub2-1.96+20090725.new/term/ieee1275/ofconsole.c
--- grub2-1.96+20090725/term/ieee1275/ofconsole.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/ieee1275/ofconsole.c	2009-08-12 18:27:38.000000000 +0100
@@ -227,6 +227,13 @@
   return key;
 }
 
+static int
+grub_ofconsole_keystatus (void)
+{
+  /* No attempt to support this for Open Firmware yet. */
+  return 0;
+}
+
 static grub_uint16_t
 grub_ofconsole_getxy (void)
 {
@@ -396,6 +403,7 @@
     .fini = grub_ofconsole_fini,
     .checkkey = grub_ofconsole_checkkey,
     .getkey = grub_ofconsole_getkey,
+    .keystatus = grub_ofconsole_keystatus,
   };
 
 static struct grub_term_output grub_ofconsole_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/usb_keyboard.c grub2-1.96+20090725.new/term/usb_keyboard.c
--- grub2-1.96+20090725/term/usb_keyboard.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/usb_keyboard.c	2009-08-12 18:27:38.000000000 +0100
@@ -234,6 +234,47 @@
   return key;
 }
 
+static int
+grub_usb_keyboard_keystatus (void)
+{
+  unsigned char data[8];
+  int mods = 0;
+  int i;
+  grub_err_t err;
+
+  for (i = 0; i < 50; i++)
+    {
+      /* Get_Report.  */
+      err = grub_usb_keyboard_getreport (usbdev, data);
+
+      if (! err)
+	break;
+    }
+
+  if (err)
+    return -1;
+
+  grub_dprintf ("usb_keyboard",
+		"report: 0x%02x 0x%02x 0x%02x 0x%02x"
+		" 0x%02x 0x%02x 0x%02x 0x%02x\n",
+		data[0], data[1], data[2], data[3],
+		data[4], data[5], data[6], data[7]);
+
+  /* Check Shift, Control, and Alt status.  */
+  if (data[0] & 0x02)
+    mods |= GRUB_TERM_STATUS_LEFT_SHIFT;
+  if (data[0] & 0x20)
+    mods |= GRUB_TERM_STATUS_RIGHT_SHIFT;
+  if (data[0] & 0x01 || data[0] & 0x10)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (data[0] & 0x04 || data[0] & 0x40)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  grub_errno = GRUB_ERR_NONE;
+
+  return mods;
+}
+
 static struct grub_term_input grub_usb_keyboard_term =
   {
     .name = "usb_keyboard",
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/util/console.c grub2-1.96+20090725.new/util/console.c
--- grub2-1.96+20090725/util/console.c	2009-07-07 21:03:03.000000000 +0100
+++ grub2-1.96+20090725.new/util/console.c	2009-08-12 18:27:38.000000000 +0100
@@ -258,6 +258,13 @@
   return c;
 }
 
+static int
+grub_ncurses_keystatus (void)
+{
+  /* ncurses can't tell us keyboard modifier status.  */
+  return 0;
+}
+
 static grub_uint16_t
 grub_ncurses_getxy (void)
 {
@@ -350,6 +357,7 @@
     .name = "console",
     .checkkey = grub_ncurses_checkkey,
     .getkey = grub_ncurses_getkey,
+    .keystatus = grub_ncurses_keystatus,
   };
 
 static struct grub_term_output grub_ncurses_term_output =

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

Oh, I didn't know there was a BIOS callback for this.  Disregard what
I said about at_keyboard then...

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Wed, Aug 12, 2009 at 11:14:31PM +0100, Colin Watson wrote:
> On Wed, Aug 12, 2009 at 06:10:18PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > Colin Watson wrote:
> > > This is based on previous work in GRUB Legacy by Kristian and Peter
> > > (CCed). Peter tells me that Red Hat has an assignment already on file,
> > > and it looks like mine is finally making some progress ...
> > 
> > We would need RedHat and Kristian to sign some papers.
> 
> Peter said they were already done, but if that's the case then fine.
> Peter/Kristian, can you double-check this?

We don't have any assignment from any Kristian, nor Red Hat, but we do
have one from a Peter.

Which Peter were you talking about?  I don't see him in CC.

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Thu, Aug 13, 2009 at 10:52:05PM +0200, Robert Millan wrote:
> On Wed, Aug 12, 2009 at 11:14:31PM +0100, Colin Watson wrote:
> > On Wed, Aug 12, 2009 at 06:10:18PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > > Colin Watson wrote:
> > > > This is based on previous work in GRUB Legacy by Kristian and Peter
> > > > (CCed). Peter tells me that Red Hat has an assignment already on file,
> > > > and it looks like mine is finally making some progress ...
> > > 
> > > We would need RedHat and Kristian to sign some papers.
> > 
> > Peter said they were already done, but if that's the case then fine.
> > Peter/Kristian, can you double-check this?
> 
> We don't have any assignment from any Kristian, nor Red Hat, but we do
> have one from a Peter.
> 
> Which Peter were you talking about?  I don't see him in CC.

Peter Jones, pjones at RH. I definitely CCed him in my mail but the
mailing list seems to have eaten the CC for some reason.

The CVS history suggests that Kristian was the original author, though.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Wed, Aug 12, 2009 at 04:35:21PM +0100, Colin Watson wrote:
> +static int
> +grub_at_keyboard_keystatus (void)
> +{
> +  /* FIXME: I don't know if getting key modifier status is possible without
> +   * BIOS help.  */
> +  return 0;
> +}
> +

What we do in these cases is leave the function pointer undefined, which
means NULL value (and check this in the caller of course).

But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
or SHIFT keys).

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Vladimir 'phcoder' Serbinenko]

On Mon, Aug 24, 2009 at 12:27 AM, Robert Millan<rmh@aybabtu.com> wrote:
> On Wed, Aug 12, 2009 at 04:35:21PM +0100, Colin Watson wrote:
>> +static int
>> +grub_at_keyboard_keystatus (void)
>> +{
>> +  /* FIXME: I don't know if getting key modifier status is possible without
>> +   * BIOS help.  */
>> +  return 0;
>> +}
>> +
>
> What we do in these cases is leave the function pointer undefined, which
> means NULL value (and check this in the caller of course).
>
> But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
> or SHIFT keys).
>
Exactly what I meant
> --
> 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."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Vladimir 'phcoder' Serbinenko]

>> Constants are i386-pc specific. In particular left and right shift are
>> distinguished but not ctrl or alt. We should either distinguish them
>> all or none.
>
> The values I picked for the constants were convenient for i386-pc, but
> that's because it's the only architecture that currently needs an
> assembly implementation and it seemed to make sense to me to put the
> harder work of transforming values around into C code.
>
> I'm not sure what you mean by "distinguish" here though. I included
> constants for control and alt that are currently unused, which may
> actually have been a mistake since the original Red Hat patch to GRUB 1
> honoured those too in its hiddenmenu handling. Is that what you mean?
I mean that upper layer code knows whether it was left or right shift
but it doesn't know whether it was left or right alt.
>
>> Going to real mode to retrieve keyboard status is absolutely
>> unnecessary: same info is available in BDA. Perhaps our console driver
>> should switch to BDA altogether but I'm not sure that this will work
>> since grub2 disables interrupts (tests needed) but it definitly works
>> with keyboard status.
>
> OK, fair enough. The attached updated patch makes this change.
>
Why is it still asm? It can be done easily in C.
Actually here we have 2 extremes: nice code (C, transform to nice
flags (only one shift or 2 conrols and alts)) or small code (asm,
crude value). Where we choose the compromise is a question for
maintainer, not me.
Another possibility is to put this code outside of kernel altogether.
It saves bytes but may make approach ugly.

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 12:46:29AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Why is it still asm? It can be done easily in C.
> Actually here we have 2 extremes: nice code (C, transform to nice
> flags (only one shift or 2 conrols and alts)) or small code (asm,
> crude value). Where we choose the compromise is a question for
> maintainer, not me.

Nice code, please.  We're pressed for space, but not *that* much.

Besides, we can't use this asm code as-is, unless we get word from the
people who wrote it (Colin wrote most of the patch, but not this).

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 12:41:38AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Mon, Aug 24, 2009 at 12:27 AM, Robert Millan<rmh@aybabtu.com> wrote:
> > On Wed, Aug 12, 2009 at 04:35:21PM +0100, Colin Watson wrote:
> >> +static int
> >> +grub_at_keyboard_keystatus (void)
> >> +{
> >> +  /* FIXME: I don't know if getting key modifier status is possible without
> >> +   * BIOS help.  */
> >> +  return 0;
> >> +}
> >> +
> >
> > What we do in these cases is leave the function pointer undefined, which
> > means NULL value (and check this in the caller of course).
> >
> > But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
> > or SHIFT keys).
> >
> Exactly what I meant

Oh, I'm sorry, I sometimes miss things.  Anyway, I hope it's clearer now that
I repeated the same thing :-)

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Vladimir 'phcoder' Serbinenko]

On Mon, Aug 24, 2009 at 12:56 AM, Robert Millan<rmh@aybabtu.com> wrote:
> On Mon, Aug 24, 2009 at 12:46:29AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> Why is it still asm? It can be done easily in C.
>> Actually here we have 2 extremes: nice code (C, transform to nice
>> flags (only one shift or 2 conrols and alts)) or small code (asm,
>> crude value). Where we choose the compromise is a question for
>> maintainer, not me.
>
> Nice code, please.  We're pressed for space, but not *that* much.
>
> Besides, we can't use this asm code as-is, unless we get word from the
> people who wrote it (Colin wrote most of the patch, but not this).
>
In this case we don't need asm at all.
> --
> 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."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 01:00:06AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Mon, Aug 24, 2009 at 12:56 AM, Robert Millan<rmh@aybabtu.com> wrote:
> > On Mon, Aug 24, 2009 at 12:46:29AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> Why is it still asm? It can be done easily in C.
> >> Actually here we have 2 extremes: nice code (C, transform to nice
> >> flags (only one shift or 2 conrols and alts)) or small code (asm,
> >> crude value). Where we choose the compromise is a question for
> >> maintainer, not me.
> >
> > Nice code, please.  We're pressed for space, but not *that* much.
> >
> > Besides, we can't use this asm code as-is, unless we get word from the
> > people who wrote it (Colin wrote most of the patch, but not this).
> >
> In this case we don't need asm at all.

I didn't look much into this.  I thought there was a new BIOS call being
introduced?

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Vladimir 'phcoder' Serbinenko]

On Mon, Aug 24, 2009 at 1:03 AM, Robert Millan<rmh@aybabtu.com> wrote:
> On Mon, Aug 24, 2009 at 01:00:06AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> On Mon, Aug 24, 2009 at 12:56 AM, Robert Millan<rmh@aybabtu.com> wrote:
>> > On Mon, Aug 24, 2009 at 12:46:29AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> >> Why is it still asm? It can be done easily in C.
>> >> Actually here we have 2 extremes: nice code (C, transform to nice
>> >> flags (only one shift or 2 conrols and alts)) or small code (asm,
>> >> crude value). Where we choose the compromise is a question for
>> >> maintainer, not me.
>> >
>> > Nice code, please.  We're pressed for space, but not *that* much.
>> >
>> > Besides, we can't use this asm code as-is, unless we get word from the
>> > people who wrote it (Colin wrote most of the patch, but not this).
>> >
>> In this case we don't need asm at all.
>
> I didn't look much into this.  I thought there was a new BIOS call being
> introduced?
>
In first version of patch yes but in the second version just checks a
value in memory


-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Mon, Aug 24, 2009 at 12:27:45AM +0200, Robert Millan wrote:
> On Wed, Aug 12, 2009 at 04:35:21PM +0100, Colin Watson wrote:
> > +static int
> > +grub_at_keyboard_keystatus (void)
> > +{
> > +  /* FIXME: I don't know if getting key modifier status is possible without
> > +   * BIOS help.  */
> > +  return 0;
> > +}
> > +
> 
> What we do in these cases is leave the function pointer undefined, which
> means NULL value (and check this in the caller of course).

OK.

> But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
> or SHIFT keys).

No, that code only spots make scan codes arriving after GRUB's terminal
starts up. AFAICS it has no way to tell whether e.g. Shift was held down
already when GRUB started (except through the vagaries of key repeat),
which is important here.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On Mon, Aug 24, 2009 at 12:46:29AM +0200, Vladimir 'phcoder' Serbinenko wrote:

[Could you please preserve attributions in your replies? I've reinserted
one here so that it's clear who wrote what.]

> Colin Watson wrote:
> > The values I picked for the constants were convenient for i386-pc, but
> > that's because it's the only architecture that currently needs an
> > assembly implementation and it seemed to make sense to me to put the
> > harder work of transforming values around into C code.
> >
> > I'm not sure what you mean by "distinguish" here though. I included
> > constants for control and alt that are currently unused, which may
> > actually have been a mistake since the original Red Hat patch to GRUB 1
> > honoured those too in its hiddenmenu handling. Is that what you mean?
> 
> I mean that upper layer code knows whether it was left or right shift
> but it doesn't know whether it was left or right alt.

Oh, I see. There's no real need to distinguish here, so the attached
patch removes the distinction. Note that this should NOT be applied
as-is, as I have not updated the assembly implementation for this;
Robert is going to clean-room this in C (which should take all of a
minute or so :-) ), and I hope he can deal with the bit-banging at the
same time.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

[-- Attachment #2: keystatus.diff --]
[-- Type: text/x-diff, Size: 9136 bytes --]

diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/commands/sleep.c grub2-1.96+20090725.new/commands/sleep.c
--- grub2-1.96+20090725/commands/sleep.c	2009-03-21 08:39:59.000000000 +0000
+++ grub2-1.96+20090725.new/commands/sleep.c	2009-08-24 10:17:58.000000000 +0100
@@ -43,6 +43,20 @@
   grub_printf ("%d    ", n);
 }
 
+static int
+grub_check_keyboard (void)
+{
+  int mods = grub_keystatus ();
+  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
+    return 1;
+
+  if (grub_checkkey () >= 0 &&
+      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    return 1;
+
+  return 0;
+}
+
 /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
 static int
 grub_interruptible_millisleep (grub_uint32_t ms)
@@ -52,8 +66,7 @@
   start = grub_get_time_ms ();
 
   while (grub_get_time_ms () - start < ms)
-    if (grub_checkkey () >= 0 &&
-	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    if (grub_check_keyboard ())
       return 1;
 
   return 0;
@@ -74,7 +87,7 @@
   if (n == 0)
     {
       /* Either `0' or broken input.  */
-      return 0;
+      return grub_check_keyboard ();
     }
 
   xy = grub_getxy ();
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/include/grub/i386/pc/console.h grub2-1.96+20090725.new/include/grub/i386/pc/console.h
--- grub2-1.96+20090725/include/grub/i386/pc/console.h	2008-11-12 17:43:39.000000000 +0000
+++ grub2-1.96+20090725.new/include/grub/i386/pc/console.h	2009-08-24 10:17:58.000000000 +0100
@@ -42,6 +42,7 @@
 /* These are global to share code between C and asm.  */
 int grub_console_checkkey (void);
 int grub_console_getkey (void);
+int grub_console_keystatus (void);
 grub_uint16_t grub_console_getxy (void);
 void grub_console_gotoxy (grub_uint8_t x, grub_uint8_t y);
 void grub_console_cls (void);
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/include/grub/term.h grub2-1.96+20090725.new/include/grub/term.h
--- grub2-1.96+20090725/include/grub/term.h	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/include/grub/term.h	2009-08-24 10:17:58.000000000 +0100
@@ -72,6 +72,12 @@
 #define GRUB_TERM_NEED_INIT	(1 << 16)
 
 
+/* Bitmasks for modifier keys returned by grub_keystatus.  */
+#define GRUB_TERM_STATUS_SHIFT	(1 << 0)
+#define GRUB_TERM_STATUS_CTRL	(1 << 1)
+#define GRUB_TERM_STATUS_ALT	(1 << 2)
+
+
 /* Unicode characters for fancy graphics.  */
 #define GRUB_TERM_DISP_LEFT	0x2190
 #define GRUB_TERM_DISP_UP	0x2191
@@ -157,6 +163,9 @@
 
   /* Get a character.  */
   int (*getkey) (void);
+
+  /* Get keyboard modifier status.  */
+  int (*keystatus) (void);
 };
 typedef struct grub_term_input *grub_term_input_t;
 
@@ -275,6 +284,7 @@
 grub_ssize_t EXPORT_FUNC(grub_getcharwidth) (grub_uint32_t code);
 int EXPORT_FUNC(grub_getkey) (void);
 int EXPORT_FUNC(grub_checkkey) (void);
+int EXPORT_FUNC(grub_keystatus) (void);
 grub_uint16_t EXPORT_FUNC(grub_getwh) (void);
 grub_uint16_t EXPORT_FUNC(grub_getxy) (void);
 void EXPORT_FUNC(grub_gotoxy) (grub_uint8_t x, grub_uint8_t y);
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/kern/i386/pc/startup.S grub2-1.96+20090725.new/kern/i386/pc/startup.S
--- grub2-1.96+20090725/kern/i386/pc/startup.S	2009-08-24 10:17:56.000000000 +0100
+++ grub2-1.96+20090725.new/kern/i386/pc/startup.S	2009-08-24 10:17:58.000000000 +0100
@@ -1285,6 +1285,25 @@
 
 
 /*
+ * int grub_console_keystatus (void)
+ * Return keyboard modifier status in %al:
+ *	bit 3: alt key down
+ *	bit 2: ctrl key down
+ *	bit 1: left shift key down
+ *	bit 0: right shift key down
+ */
+FUNCTION(grub_console_keystatus)
+	/* keyboard status bits in BIOS Data Area */
+	movw	$0x0417, %bx
+	movb	(%bx), %al
+
+	/* Mask out everything but Shift, Ctrl, and Alt. */
+	andl	$0x000f, %eax
+
+	ret
+
+
+/*
  * grub_uint16_t grub_console_getxy (void)
  * BIOS call "INT 10H Function 03h" to get cursor position
  *	Call with	%ah = 0x03
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/kern/term.c grub2-1.96+20090725.new/kern/term.c
--- grub2-1.96+20090725/kern/term.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/kern/term.c	2009-08-24 10:17:58.000000000 +0100
@@ -140,6 +140,15 @@
   return (grub_cur_term_input->checkkey) ();
 }
 
+int
+grub_keystatus (void)
+{
+  if (grub_cur_term_input->keystatus)
+    return (grub_cur_term_input->keystatus) ();
+  else
+    return 0;
+}
+
 grub_uint16_t
 grub_getxy (void)
 {
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/efi/console.c grub2-1.96+20090725.new/term/efi/console.c
--- grub2-1.96+20090725/term/efi/console.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/efi/console.c	2009-08-24 10:17:58.000000000 +0100
@@ -337,6 +337,8 @@
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    /* EFI does not support reading modifier key status.  */
+    .keystatus = NULL,
   };
 
 static struct grub_term_output grub_console_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/at_keyboard.c grub2-1.96+20090725.new/term/i386/pc/at_keyboard.c
--- grub2-1.96+20090725/term/i386/pc/at_keyboard.c	2009-04-14 19:12:14.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/at_keyboard.c	2009-08-24 10:17:58.000000000 +0100
@@ -222,6 +222,9 @@
     .fini = grub_keyboard_controller_fini,
     .checkkey = grub_at_keyboard_checkkey,
     .getkey = grub_at_keyboard_getkey,
+    /* Getting key modifier status is not possible without BIOS help; all we
+     * can do is spot when keys are pressed.  */
+    .keystatus = NULL,
   };
 
 GRUB_MOD_INIT(at_keyboard)
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/console.c grub2-1.96+20090725.new/term/i386/pc/console.c
--- grub2-1.96+20090725/term/i386/pc/console.c	2009-04-14 19:12:14.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/console.c	2009-08-24 10:17:58.000000000 +0100
@@ -25,6 +25,7 @@
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .keystatus = grub_console_keystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/i386/pc/serial.c grub2-1.96+20090725.new/term/i386/pc/serial.c
--- grub2-1.96+20090725/term/i386/pc/serial.c	2009-06-11 17:17:45.000000000 +0100
+++ grub2-1.96+20090725.new/term/i386/pc/serial.c	2009-08-24 10:17:58.000000000 +0100
@@ -465,6 +465,8 @@
   .name = "serial",
   .checkkey = grub_serial_checkkey,
   .getkey = grub_serial_getkey,
+  /* We can't get key modifier status on serial consoles.  */
+  .keystatus = NULL,
 };
 
 static struct grub_term_output grub_serial_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/ieee1275/ofconsole.c grub2-1.96+20090725.new/term/ieee1275/ofconsole.c
--- grub2-1.96+20090725/term/ieee1275/ofconsole.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/ieee1275/ofconsole.c	2009-08-24 10:17:58.000000000 +0100
@@ -396,6 +396,8 @@
     .fini = grub_ofconsole_fini,
     .checkkey = grub_ofconsole_checkkey,
     .getkey = grub_ofconsole_getkey,
+    /* No attempt to support this for Open Firmware yet. */
+    .keystatus = NULL,
   };
 
 static struct grub_term_output grub_ofconsole_term_output =
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/usb_keyboard.c grub2-1.96+20090725.new/term/usb_keyboard.c
--- grub2-1.96+20090725/term/usb_keyboard.c	2009-06-10 22:04:23.000000000 +0100
+++ grub2-1.96+20090725.new/term/usb_keyboard.c	2009-08-24 10:18:08.000000000 +0100
@@ -234,11 +234,51 @@
   return key;
 }
 
+static int
+grub_usb_keyboard_keystatus (void)
+{
+  unsigned char data[8];
+  int mods = 0;
+  int i;
+  grub_err_t err;
+
+  for (i = 0; i < 50; i++)
+    {
+      /* Get_Report.  */
+      err = grub_usb_keyboard_getreport (usbdev, data);
+
+      if (! err)
+	break;
+    }
+
+  if (err)
+    return -1;
+
+  grub_dprintf ("usb_keyboard",
+		"report: 0x%02x 0x%02x 0x%02x 0x%02x"
+		" 0x%02x 0x%02x 0x%02x 0x%02x\n",
+		data[0], data[1], data[2], data[3],
+		data[4], data[5], data[6], data[7]);
+
+  /* Check Shift, Control, and Alt status.  */
+  if (data[0] & 0x02 || data[0] & 0x20)
+    mods |= GRUB_TERM_STATUS_SHIFT;
+  if (data[0] & 0x01 || data[0] & 0x10)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (data[0] & 0x04 || data[0] & 0x40)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  grub_errno = GRUB_ERR_NONE;
+
+  return mods;
+}
+
 static struct grub_term_input grub_usb_keyboard_term =
   {
     .name = "usb_keyboard",
     .checkkey = grub_usb_keyboard_checkkey,
     .getkey = grub_usb_keyboard_getkey,
+    .keystatus = grub_usb_keyboard_keystatus,
     .next = 0
   };
 
diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/util/console.c grub2-1.96+20090725.new/util/console.c
--- grub2-1.96+20090725/util/console.c	2009-07-07 21:03:03.000000000 +0100
+++ grub2-1.96+20090725.new/util/console.c	2009-08-24 10:17:58.000000000 +0100
@@ -350,6 +350,8 @@
     .name = "console",
     .checkkey = grub_ncurses_checkkey,
     .getkey = grub_ncurses_getkey,
+    /* ncurses can't tell us keyboard modifier status.  */
+    .keystatus = NULL,
   };
 
 static struct grub_term_output grub_ncurses_term_output =

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 10:11:10AM +0100, Colin Watson wrote:
> > But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
> > or SHIFT keys).
> 
> No, that code only spots make scan codes arriving after GRUB's terminal
> starts up. AFAICS it has no way to tell whether e.g. Shift was held down
> already when GRUB started

Ah, you're right on this..

> (except through the vagaries of key repeat),

..but on this too.  Why not check for key repeat?  The controller generates
them for all keys AFAIK.

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

On Mon, Aug 24, 2009 at 10:24:06AM +0100, Colin Watson wrote:
> On Mon, Aug 24, 2009 at 12:46:29AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> 
> [Could you please preserve attributions in your replies? I've reinserted
> one here so that it's clear who wrote what.]
> 
> > Colin Watson wrote:
> > > The values I picked for the constants were convenient for i386-pc, but
> > > that's because it's the only architecture that currently needs an
> > > assembly implementation and it seemed to make sense to me to put the
> > > harder work of transforming values around into C code.
> > >
> > > I'm not sure what you mean by "distinguish" here though. I included
> > > constants for control and alt that are currently unused, which may
> > > actually have been a mistake since the original Red Hat patch to GRUB 1
> > > honoured those too in its hiddenmenu handling. Is that what you mean?
> > 
> > I mean that upper layer code knows whether it was left or right shift
> > but it doesn't know whether it was left or right alt.
> 
> Oh, I see. There's no real need to distinguish here, so the attached
> patch removes the distinction. Note that this should NOT be applied
> as-is, as I have not updated the assembly implementation for this;
> Robert is going to clean-room this in C (which should take all of a
> minute or so :-) ), and I hope he can deal with the bit-banging at the
> same time.

Hi,

I don't have time to test it, but what we want is roughly something like
this.  If you would please test and confirm it's working the way you expected,
it can be committed.

Also, please remember to include a ChangeLog entry.

I ommitted the USB part of your patch, because I had some comments.  Once
that's fixed it can be added as well:

> +static int
> +grub_usb_keyboard_keystatus (void)
> +{
> +  unsigned char data[8];

Please use grub_uint8_t.

> +  for (i = 0; i < 50; i++)
> +    {
> +      /* Get_Report.  */
> +      err = grub_usb_keyboard_getreport (usbdev, data);
> +
> +      if (! err)
> +	break;
> +    }

If the 50 is a "timeout", it should be using grub_get_time_ms() instead;
if it's a number given by spec (e.g. number of times an I/O operation must
be performed), then please macroify it to indicate what it is.

-- 
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."

[-- Attachment #2: getkeystatus.diff --]
[-- Type: text/x-diff, Size: 5935 bytes --]

Index: kern/term.c
===================================================================
--- kern/term.c	(revision 2520)
+++ kern/term.c	(working copy)
@@ -140,6 +140,15 @@ grub_checkkey (void)
   return (grub_cur_term_input->checkkey) ();
 }
 
+int
+grub_getkeystatus (void)
+{
+  if (grub_cur_term_input->getkeystatus)
+    return (grub_cur_term_input->getkeystatus) ();
+  else
+    return 0;
+}
+
 grub_uint16_t
 grub_getxy (void)
 {
Index: include/grub/term.h
===================================================================
--- include/grub/term.h	(revision 2520)
+++ include/grub/term.h	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -72,6 +72,12 @@ grub_term_color_state;
 #define GRUB_TERM_NEED_INIT	(1 << 16)
 
 
+/* Bitmasks for modifier keys returned by grub_getkeystatus.  */
+#define GRUB_TERM_STATUS_SHIFT	(1 << 0)
+#define GRUB_TERM_STATUS_CTRL	(1 << 1)
+#define GRUB_TERM_STATUS_ALT	(1 << 2)
+
+
 /* Unicode characters for fancy graphics.  */
 #define GRUB_TERM_DISP_LEFT	0x2190
 #define GRUB_TERM_DISP_UP	0x2191
@@ -157,6 +163,9 @@ struct grub_term_input
 
   /* Get a character.  */
   int (*getkey) (void);
+
+  /* Get keyboard modifier status.  */
+  int (*getkeystatus) (void);
 };
 typedef struct grub_term_input *grub_term_input_t;
 
@@ -275,6 +284,7 @@ void EXPORT_FUNC(grub_putcode) (grub_uin
 grub_ssize_t EXPORT_FUNC(grub_getcharwidth) (grub_uint32_t code);
 int EXPORT_FUNC(grub_getkey) (void);
 int EXPORT_FUNC(grub_checkkey) (void);
+int EXPORT_FUNC(grub_getkeystatus) (void);
 grub_uint16_t EXPORT_FUNC(grub_getwh) (void);
 grub_uint16_t EXPORT_FUNC(grub_getxy) (void);
 void EXPORT_FUNC(grub_gotoxy) (grub_uint8_t x, grub_uint8_t y);
Index: include/grub/i386/pc/console.h
===================================================================
--- include/grub/i386/pc/console.h	(revision 2520)
+++ include/grub/i386/pc/console.h	(working copy)
@@ -42,6 +42,7 @@
 /* These are global to share code between C and asm.  */
 int grub_console_checkkey (void);
 int grub_console_getkey (void);
+int grub_console_getkeystatus (void);
 grub_uint16_t grub_console_getxy (void);
 void grub_console_gotoxy (grub_uint8_t x, grub_uint8_t y);
 void grub_console_cls (void);
Index: include/grub/i386/pc/memory.h
===================================================================
--- include/grub/i386/pc/memory.h	(revision 2520)
+++ include/grub/i386/pc/memory.h	(working copy)
@@ -78,6 +78,15 @@
 /* The data segment of the pseudo real mode.  */
 #define GRUB_MEMORY_MACHINE_PSEUDO_REAL_DSEG	0x20
 
+#define GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR	0x400
+
+static struct grub_machine_bios_data_area
+{
+  grub_uint8_t unused1[0x17];
+  grub_uint16_t keyboard_status; /* 0x17 */ 
+  grub_uint8_t unused2[0xf0 - 0x19];
+}
+
 #ifndef ASM_FILE
 
 struct grub_machine_mmap_entry
Index: commands/sleep.c
===================================================================
--- commands/sleep.c	(revision 2520)
+++ commands/sleep.c	(working copy)
@@ -1,7 +1,7 @@
 /* sleep.c - Command to wait a specified number of seconds.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2006,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2006,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -43,6 +43,20 @@ do_print (int n)
   grub_printf ("%d    ", n);
 }
 
+static int
+grub_check_keyboard (void)
+{
+  int mods = grub_getkeystatus ();
+  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
+    return 1;
+
+  if (grub_checkkey () >= 0 &&
+      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    return 1;
+
+  return 0;
+}
+
 /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
 static int
 grub_interruptible_millisleep (grub_uint32_t ms)
@@ -52,8 +66,7 @@ grub_interruptible_millisleep (grub_uint
   start = grub_get_time_ms ();
 
   while (grub_get_time_ms () - start < ms)
-    if (grub_checkkey () >= 0 &&
-	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    if (grub_check_keyboard ())
       return 1;
 
   return 0;
@@ -74,7 +87,7 @@ grub_cmd_sleep (grub_extcmd_t cmd, int a
   if (n == 0)
     {
       /* Either `0' or broken input.  */
-      return 0;
+      return grub_check_keyboard ();
     }
 
   xy = grub_getxy ();
Index: term/i386/pc/console.c
===================================================================
--- term/i386/pc/console.c	(revision 2520)
+++ term/i386/pc/console.c	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -16,15 +16,25 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <grub/machine/memory.h>
 #include <grub/machine/console.h>
 #include <grub/term.h>
 #include <grub/types.h>
 
+static const struct grub_machine_bios_data_area *bios_data_area = GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR;
+
+static grub_uint16_t
+grub_console_getkeystatus (void)
+{
+  return bios_data_area->keyboard_status;
+}
+
 static struct grub_term_input grub_console_term_input =
   {
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .getkeystatus = grub_console_getkeystatus,
   };
 
 static struct grub_term_output grub_console_term_output =

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 02:23:52PM +0200, Robert Millan wrote:
> Index: include/grub/i386/pc/memory.h
> ===================================================================
> --- include/grub/i386/pc/memory.h	(revision 2520)
> +++ include/grub/i386/pc/memory.h	(working copy)
> @@ -78,6 +78,15 @@
>  /* The data segment of the pseudo real mode.  */
>  #define GRUB_MEMORY_MACHINE_PSEUDO_REAL_DSEG	0x20
>  
> +#define GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR	0x400
> +
> +static struct grub_machine_bios_data_area
> +{
> +  grub_uint8_t unused1[0x17];
> +  grub_uint16_t keyboard_status; /* 0x17 */ 
> +  grub_uint8_t unused2[0xf0 - 0x19];
> +}
> +

Btw, Colin pointed at this URL:

  http://heim.ifi.uio.no/~stanisls/helppc/bios_data_area.html

I suppose it'd be a good idea to include it in a comment next to the struct
definition.

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Vladimir 'phcoder' Serbinenko]

On Mon, Aug 24, 2009 at 1:57 PM, Robert Millan<rmh@aybabtu.com> wrote:
> On Mon, Aug 24, 2009 at 10:11:10AM +0100, Colin Watson wrote:
>> > But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
>> > or SHIFT keys).
>>
>> No, that code only spots make scan codes arriving after GRUB's terminal
>> starts up. AFAICS it has no way to tell whether e.g. Shift was held down
>> already when GRUB started
>
> Ah, you're right on this..
>
In this particular case yes. But I can imagine other cases when it
doesn't matter. E.g. if a distro wants to show a notice before booting
single user entry and user wants to press shift to bypass this notice.
>> (except through the vagaries of key repeat),
>
> ..but on this too.  Why not check for key repeat?  The controller generates
> them for all keys AFAIK.
>
> --
> 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."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 10:24:06AM +0100, Colin Watson wrote:
> diff -Nur -x '*.orig' -x '*~' grub2-1.96+20090725/term/ieee1275/ofconsole.c grub2-1.96+20090725.new/term/ieee1275/ofconsole.c
> --- grub2-1.96+20090725/term/ieee1275/ofconsole.c	2009-06-10 22:04:23.000000000 +0100
> +++ grub2-1.96+20090725.new/term/ieee1275/ofconsole.c	2009-08-24 10:17:58.000000000 +0100
> @@ -396,6 +396,8 @@
>      .fini = grub_ofconsole_fini,
>      .checkkey = grub_ofconsole_checkkey,
>      .getkey = grub_ofconsole_getkey,
> +    /* No attempt to support this for Open Firmware yet. */
> +    .keystatus = NULL,
>    };

Btw, I ommitted this from the last patch I sent, as it is unnecessary.  The
BSS is zero'ed before grub_main() so all uninitialized variables are
implicitly 0.  In this kind of structs, when a member is optional and we
don't implement it, we just skip it.

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Michal Suchanek]

2009/8/24 Robert Millan <rmh@aybabtu.com>:
> On Mon, Aug 24, 2009 at 10:11:10AM +0100, Colin Watson wrote:
>> > But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
>> > or SHIFT keys).
>>
>> No, that code only spots make scan codes arriving after GRUB's terminal
>> starts up. AFAICS it has no way to tell whether e.g. Shift was held down
>> already when GRUB started
>
> Ah, you're right on this..
>
>> (except through the vagaries of key repeat),
>
> ..but on this too.  Why not check for key repeat?  The controller generates
> them for all keys AFAIK.

..unless turned off in the BIOS (which some seem to allow).

Thanks

Michal

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Mon, Aug 24, 2009 at 01:57:26PM +0200, Robert Millan wrote:
> On Mon, Aug 24, 2009 at 10:11:10AM +0100, Colin Watson wrote:
> > No, that code only spots make scan codes arriving after GRUB's terminal
> > starts up. AFAICS it has no way to tell whether e.g. Shift was held down
> > already when GRUB started
> 
> Ah, you're right on this..
> 
> > (except through the vagaries of key repeat),
> 
> ..but on this too.  Why not check for key repeat?  The controller generates
> them for all keys AFAIK.

If you want a delay of zero (i.e. "is Shift pressed? no? OK, boot
immediately with no delay"), how long should you wait for key repeat? If
there's a sensible answer to that question (IIRC key repeat speed is
sometimes configurable in the BIOS) then we could do that ...

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Mon, Aug 24, 2009 at 02:23:52PM +0200, Robert Millan wrote:
> I ommitted the USB part of your patch, because I had some comments.  Once
> that's fixed it can be added as well:
> 
> > +static int
> > +grub_usb_keyboard_keystatus (void)
> > +{
> > +  unsigned char data[8];
> 
> Please use grub_uint8_t.
> 
> > +  for (i = 0; i < 50; i++)
> > +    {
> > +      /* Get_Report.  */
> > +      err = grub_usb_keyboard_getreport (usbdev, data);
> > +
> > +      if (! err)
> > +	break;
> > +    }
> 
> If the 50 is a "timeout", it should be using grub_get_time_ms() instead;
> if it's a number given by spec (e.g. number of times an I/O operation must
> be performed), then please macroify it to indicate what it is.

Perhaps we can apply the following patch first, then? I was following
existing style, so the other code should be updated too.

I don't see anything in the USB HID spec about the number of times one
must attempt to get a keyboard report, so I think it must simply be a
timeout. USB frames are 1ms so waiting for 50ms should do.

2009-08-24  Colin Watson  <cjwatson@ubuntu.com>

	* term/usb_keyboard.c (grub_usb_keyboard_getreport): Make
	`report' grub_uint8_t *.
	(grub_usb_keyboard_checkkey): Make `data' elements grub_uint8_t.
	Use a 50-millisecond timeout rather than just repeating
	grub_usb_keyboard_getreport 50 times.
	(grub_usb_keyboard_getkey): Make `data' elements grub_uint8_t.

Index: term/usb_keyboard.c
===================================================================
--- term/usb_keyboard.c	(revision 2522)
+++ term/usb_keyboard.c	(working copy)
@@ -97,7 +97,7 @@
 }
 
 static grub_err_t
-grub_usb_keyboard_getreport (grub_usb_device_t dev, unsigned char *report)
+grub_usb_keyboard_getreport (grub_usb_device_t dev, grub_uint8_t *report)
 {
   return grub_usb_control_msg (dev, (1 << 7) | (1 << 5) | 1, 0x01, 0, 0,
 			       8, (char *) report);
@@ -108,20 +108,24 @@
 static int
 grub_usb_keyboard_checkkey (void)
 {
-  unsigned char data[8];
+  grub_uint8_t data[8];
   int key;
-  int i;
   grub_err_t err;
+  grub_uint64_t currtime;
+  int timeout = 50;
 
   data[2] = 0;
-  for (i = 0; i < 50; i++)
+  currtime = grub_get_time_ms ();
+  do
     {
       /* Get_Report.  */
       err = grub_usb_keyboard_getreport (usbdev, data);
 
-      if (! err && data[2])
+      /* Implement a timeout.  */
+      if (grub_get_time_ms () > currtime + timeout)
 	break;
     }
+  while (err || !data[2]);
 
   if (err || !data[2])
     return -1;
@@ -174,7 +178,7 @@
 {
   int key;
   grub_err_t err;
-  unsigned char data[8];
+  grub_uint8_t data[8];
   grub_uint64_t currtime;
   int timeout;
   static grub_usb_keyboard_repeat_t repeat = GRUB_HIDBOOT_REPEAT_NONE;

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 01:57:26PM +0200, Robert Millan wrote:
> On Mon, Aug 24, 2009 at 10:11:10AM +0100, Colin Watson wrote:
> > > But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
> > > or SHIFT keys).
> > 
> > No, that code only spots make scan codes arriving after GRUB's terminal
> > starts up. AFAICS it has no way to tell whether e.g. Shift was held down
> > already when GRUB started
> 
> Ah, you're right on this..
> 
> > (except through the vagaries of key repeat),
> 
> ..but on this too.  Why not check for key repeat?  The controller generates
> them for all keys AFAIK.

Or we could just make at_keyboard query the startup console for initial
state and then update it afterwards.

But this has the problem that in a multi-keyboard scenario we might be
using fake information.

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 02:27:07PM +0100, Colin Watson wrote:
> 
> Perhaps we can apply the following patch first, then? I was following
> existing style, so the other code should be updated too.

Ah, sorry I didn't know that.  But thanks for fixing anyway :-)

Please go ahead (I assume you've tested it, at least with QEMU's USB
keyboard).

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 02:58:50PM +0200, Michal Suchanek wrote:
> 2009/8/24 Robert Millan <rmh@aybabtu.com>:
> > On Mon, Aug 24, 2009 at 10:11:10AM +0100, Colin Watson wrote:
> >> > But in at_keyboard it's definitely possible (check how we handle e.g. CTRL
> >> > or SHIFT keys).
> >>
> >> No, that code only spots make scan codes arriving after GRUB's terminal
> >> starts up. AFAICS it has no way to tell whether e.g. Shift was held down
> >> already when GRUB started
> >
> > Ah, you're right on this..
> >
> >> (except through the vagaries of key repeat),
> >
> > ..but on this too.  Why not check for key repeat?  The controller generates
> > them for all keys AFAIK.
> 
> ..unless turned off in the BIOS (which some seem to allow).

It's still hardware, so we can turn it back on.  I guess we should be?

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Mon, Aug 24, 2009 at 04:28:00PM +0200, Robert Millan wrote:
> Please go ahead (I assume you've tested it, at least with QEMU's USB
> keyboard).

Hmm. Does GRUB actually work with that right now? Even without my patch
applied, this just gives me a prompt I can't type at:

  ./grub-mkrescue --modules='uhci usb_keyboard' --pkglibdir=. --grub-mkimage=./grub-mkimage ../t/disk
  qemu -boot d -cdrom ../t/disk -usb -usbdevice keyboard

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Mon, Aug 24, 2009 at 05:23:00PM +0100, Colin Watson wrote:
> On Mon, Aug 24, 2009 at 04:28:00PM +0200, Robert Millan wrote:
> > Please go ahead (I assume you've tested it, at least with QEMU's USB
> > keyboard).
> 
> Hmm. Does GRUB actually work with that right now? Even without my patch
> applied, this just gives me a prompt I can't type at:
> 
>   ./grub-mkrescue --modules='uhci usb_keyboard' --pkglibdir=. --grub-mkimage=./grub-mkimage ../t/disk
>   qemu -boot d -cdrom ../t/disk -usb -usbdevice keyboard

Ah, Felix pointed me at adding 'terminal_input.usb_keyboard' to
grub.cfg. I'm working on testing this now.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Mon, Aug 24, 2009 at 04:28:00PM +0200, Robert Millan wrote:
> On Mon, Aug 24, 2009 at 02:27:07PM +0100, Colin Watson wrote:
> > Perhaps we can apply the following patch first, then? I was following
> > existing style, so the other code should be updated too.
> 
> Ah, sorry I didn't know that.  But thanks for fixing anyway :-)
> 
> Please go ahead (I assume you've tested it, at least with QEMU's USB
> keyboard).

Committed this cleanup patch.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Mon, Aug 24, 2009 at 02:23:52PM +0200, Robert Millan wrote:
> I don't have time to test it, but what we want is roughly something like
> this.  If you would please test and confirm it's working the way you expected,
> it can be committed.

OK, here's a cleanup that (a) fixes some compilation errors in your
patch and (b) adds USB keyboard support back in. How does this look?

2009-08-24  Colin Watson  <cjwatson@ubuntu.com>
2009-08-24  Robert Millan  <rmh.grub@aybabtu.com>

	Add `getkeystatus' terminal method.  Use it in `sleep' to detect
	Shift being held down.

	* include/grub/term.h (GRUB_TERM_STATUS_SHIFT,
	GRUB_TERM_STATUS_CTRL, GRUB_TERM_STATUS_ALT): Definitions for
	modifier key bitmasks.
	(struct grub_term_input): Add `getkeystatus' member.
	(grub_getkeystatus): Add prototype.
	* kern/term.c (grub_getkeystatus): New function.

	* include/grub/i386/pc/memory.h
	(GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR): New macro.
	(struct grub_machine_bios_data_area): Define necessary parts of BIOS
	Data Area layout.
	* term/i386/pc/console.c (grub_console_getkeystatus): New function.
	(grub_console_term_input): Set `getkeystatus' member.
	* term/usb_keyboard.c (grub_usb_keyboard_getkeystatus): New
	function.
	(grub_usb_keyboard_term): Set `getkeystatus' member.

	* commands/sleep.c (grub_check_keyboard): New function.
	(grub_interruptible_millisleep, grub_cmd_sleep): Use
	grub_check_keyboard, checking whether Shift is pressed in addition
	to checking for Escape.

Index: kern/term.c
===================================================================
--- kern/term.c	(revision 2525)
+++ kern/term.c	(working copy)
@@ -140,6 +140,15 @@
   return (grub_cur_term_input->checkkey) ();
 }
 
+int
+grub_getkeystatus (void)
+{
+  if (grub_cur_term_input->getkeystatus)
+    return (grub_cur_term_input->getkeystatus) ();
+  else
+    return 0;
+}
+
 grub_uint16_t
 grub_getxy (void)
 {
Index: include/grub/term.h
===================================================================
--- include/grub/term.h	(revision 2525)
+++ include/grub/term.h	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -72,6 +72,12 @@
 #define GRUB_TERM_NEED_INIT	(1 << 16)
 
 
+/* Bitmasks for modifier keys returned by grub_getkeystatus.  */
+#define GRUB_TERM_STATUS_SHIFT	(1 << 0)
+#define GRUB_TERM_STATUS_CTRL	(1 << 1)
+#define GRUB_TERM_STATUS_ALT	(1 << 2)
+
+
 /* Unicode characters for fancy graphics.  */
 #define GRUB_TERM_DISP_LEFT	0x2190
 #define GRUB_TERM_DISP_UP	0x2191
@@ -157,6 +163,9 @@
 
   /* Get a character.  */
   int (*getkey) (void);
+
+  /* Get keyboard modifier status.  */
+  int (*getkeystatus) (void);
 };
 typedef struct grub_term_input *grub_term_input_t;
 
@@ -275,6 +284,7 @@
 grub_ssize_t EXPORT_FUNC(grub_getcharwidth) (grub_uint32_t code);
 int EXPORT_FUNC(grub_getkey) (void);
 int EXPORT_FUNC(grub_checkkey) (void);
+int EXPORT_FUNC(grub_getkeystatus) (void);
 grub_uint16_t EXPORT_FUNC(grub_getwh) (void);
 grub_uint16_t EXPORT_FUNC(grub_getxy) (void);
 void EXPORT_FUNC(grub_gotoxy) (grub_uint8_t x, grub_uint8_t y);
Index: include/grub/i386/pc/memory.h
===================================================================
--- include/grub/i386/pc/memory.h	(revision 2525)
+++ include/grub/i386/pc/memory.h	(working copy)
@@ -78,8 +78,20 @@
 /* The data segment of the pseudo real mode.  */
 #define GRUB_MEMORY_MACHINE_PSEUDO_REAL_DSEG	0x20
 
+#define GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR	0x400
+
 #ifndef ASM_FILE
 
+/* See http://heim.ifi.uio.no/~stanisls/helppc/bios_data_area.html for a
+   description of the BIOS Data Area layout.  */
+struct grub_machine_bios_data_area
+{
+  grub_uint8_t unused1[0x17];
+  grub_uint8_t keyboard_flag_lower; /* 0x17 */ 
+  grub_uint8_t keyboard_flag_upper; /* 0x17 */ 
+  grub_uint8_t unused2[0xf0 - 0x19];
+};
+
 struct grub_machine_mmap_entry
 {
   grub_uint32_t size;
Index: commands/sleep.c
===================================================================
--- commands/sleep.c	(revision 2525)
+++ commands/sleep.c	(working copy)
@@ -1,7 +1,7 @@
 /* sleep.c - Command to wait a specified number of seconds.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2006,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2006,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -43,6 +43,20 @@
   grub_printf ("%d    ", n);
 }
 
+static int
+grub_check_keyboard (void)
+{
+  int mods = grub_getkeystatus ();
+  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
+    return 1;
+
+  if (grub_checkkey () >= 0 &&
+      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    return 1;
+
+  return 0;
+}
+
 /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
 static int
 grub_interruptible_millisleep (grub_uint32_t ms)
@@ -52,8 +66,7 @@
   start = grub_get_time_ms ();
 
   while (grub_get_time_ms () - start < ms)
-    if (grub_checkkey () >= 0 &&
-	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    if (grub_check_keyboard ())
       return 1;
 
   return 0;
@@ -74,7 +87,7 @@
   if (n == 0)
     {
       /* Either `0' or broken input.  */
-      return 0;
+      return grub_check_keyboard ();
     }
 
   xy = grub_getxy ();
Index: term/i386/pc/console.c
===================================================================
--- term/i386/pc/console.c	(revision 2525)
+++ term/i386/pc/console.c	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -16,15 +16,35 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <grub/machine/memory.h>
 #include <grub/machine/console.h>
 #include <grub/term.h>
 #include <grub/types.h>
 
+static const struct grub_machine_bios_data_area *bios_data_area = GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR;
+
+static int
+grub_console_getkeystatus (void)
+{
+  grub_uint8_t status = bios_data_area->keyboard_flag_lower;
+  int mods = 0;
+
+  if (status & 0x03)
+    mods |= GRUB_TERM_STATUS_SHIFT;
+  if (status & 0x04)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (status & 0x08)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  return mods;
+}
+
 static struct grub_term_input grub_console_term_input =
   {
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .getkeystatus = grub_console_getkeystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
Index: term/usb_keyboard.c
===================================================================
--- term/usb_keyboard.c	(revision 2526)
+++ term/usb_keyboard.c	(working copy)
@@ -238,11 +238,55 @@
   return key;
 }
 
+static int
+grub_usb_keyboard_getkeystatus (void)
+{
+  grub_uint8_t data[8];
+  int mods = 0;
+  grub_err_t err;
+  grub_uint64_t currtime;
+  int timeout = 50;
+
+  currtime = grub_get_time_ms ();
+  do
+    {
+      /* Get_Report.  */
+      err = grub_usb_keyboard_getreport (usbdev, data);
+
+      /* Implement a timeout.  */
+      if (grub_get_time_ms () > currtime + timeout)
+	break;
+    }
+  while (err);
+
+  if (err)
+    return -1;
+
+  grub_dprintf ("usb_keyboard",
+		"report: 0x%02x 0x%02x 0x%02x 0x%02x"
+		" 0x%02x 0x%02x 0x%02x 0x%02x\n",
+		data[0], data[1], data[2], data[3],
+		data[4], data[5], data[6], data[7]);
+
+  /* Check Shift, Control, and Alt status.  */
+  if (data[0] & 0x02 || data[0] & 0x20)
+    mods |= GRUB_TERM_STATUS_SHIFT;
+  if (data[0] & 0x01 || data[0] & 0x10)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (data[0] & 0x04 || data[0] & 0x40)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  grub_errno = GRUB_ERR_NONE;
+
+  return mods;
+}
+
 static struct grub_term_input grub_usb_keyboard_term =
   {
     .name = "usb_keyboard",
     .checkkey = grub_usb_keyboard_checkkey,
     .getkey = grub_usb_keyboard_getkey,
+    .getkeystatus = grub_usb_keyboard_getkeystatus,
     .next = 0
   };
 

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 11:04:19PM +0100, Colin Watson wrote:
> +/* See http://heim.ifi.uio.no/~stanisls/helppc/bios_data_area.html for a
> +   description of the BIOS Data Area layout.  */
> +struct grub_machine_bios_data_area
> +{
> +  grub_uint8_t unused1[0x17];
> +  grub_uint8_t keyboard_flag_lower; /* 0x17 */ 
> +  grub_uint8_t keyboard_flag_upper; /* 0x17 */ 
> +  grub_uint8_t unused2[0xf0 - 0x19];
> +};

Why split the keyboard field in upper/lower?  We have working 16-bit types :-)

> +static int
> +grub_check_keyboard (void)
> +{
> +  int mods = grub_getkeystatus ();
> +  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
> +    return 1;
> +
> +  if (grub_checkkey () >= 0 &&
> +      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
> +    return 1;
> +
> +  return 0;
> +}

I'm not sure if hardcoding ESC and SHIFT here makes this a bit too ad-hoc.

IIRC you had a more generic approach in mind?

What does everyone think about this?

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Tue, Aug 25, 2009 at 12:41:20AM +0200, Robert Millan wrote:
> On Mon, Aug 24, 2009 at 11:04:19PM +0100, Colin Watson wrote:
> > +/* See http://heim.ifi.uio.no/~stanisls/helppc/bios_data_area.html for a
> > +   description of the BIOS Data Area layout.  */
> > +struct grub_machine_bios_data_area
> > +{
> > +  grub_uint8_t unused1[0x17];
> > +  grub_uint8_t keyboard_flag_lower; /* 0x17 */ 
> > +  grub_uint8_t keyboard_flag_upper; /* 0x17 */ 
> > +  grub_uint8_t unused2[0xf0 - 0x19];
> > +};
> 
> Why split the keyboard field in upper/lower?  We have working 16-bit types :-)

We only need the lower one and counterintuitively (for a little-endian
system) I seemed to have to look at bits 8-11 of the 16-bit type rather
than bits 0-3. Life was too short to worry about what was wrong and I
decided to only fetch the byte we needed. I suppose we could delete the
declaration of keyboard_flag_upper.

> > +static int
> > +grub_check_keyboard (void)
> > +{
> > +  int mods = grub_getkeystatus ();
> > +  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
> > +    return 1;
> > +
> > +  if (grub_checkkey () >= 0 &&
> > +      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
> > +    return 1;
> > +
> > +  return 0;
> > +}
> 
> I'm not sure if hardcoding ESC and SHIFT here makes this a bit too ad-hoc.

Maybe not much more than the previous hardcoding of Escape alone?

> IIRC you had a more generic approach in mind?

We talked on IRC about adding a magic "keystatus" environment variable
that user code could inspect. I'm not sure whether this will actually
make things any simpler at the moment, though - I don't think there's a
way to do bitwise operations in the shell-like scripting interface (you
can use 'test' to test greater/less than, but nothing like "&"). Would
three magic variables, keystatus_shift, keystatus_ctrl, and
keystatus_alt, be too inelegant?

I'm open to other ideas. 'sleep --interruptible 0' is not an entirely
natural way to express what I'm looking for, so I'm not particularly
attached to it. 'if sleep --interruptible 0; then set timeout=0; fi' is
suboptimal because it means that if either (a) core.img is too old to
have getkeystatus support or (b) the current console driver does not
support getkeystatus then the menu will be skipped without a way to
access it at run-time. I'd much rather have something that allowed
zero-timeout-with-Shift-to-access-menu if and only if we have a new
enough core.img and the console driver has getkeystatus support, and
otherwise degraded to something else sensible.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Mon, Aug 24, 2009 at 12:56:23AM +0200, Robert Millan wrote:
> 
> Besides, we can't use this asm code as-is, unless we get word from the
> people who wrote it (Colin wrote most of the patch, but not this).

I've been pointed out that Red Hat has a blanket copyright assignment contract
that covers all FSF software.

Therefore this is not a problem at all (though, we've rewritten it in C
anyway for technical reasons).

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Tue, Aug 25, 2009 at 12:02:17AM +0100, Colin Watson wrote:
> I'm open to other ideas. 'sleep --interruptible 0' is not an entirely
> natural way to express what I'm looking for, so I'm not particularly
> attached to it. 'if sleep --interruptible 0; then set timeout=0; fi' is
> suboptimal because it means that if either (a) core.img is too old to
> have getkeystatus support or (b) the current console driver does not
> support getkeystatus then the menu will be skipped without a way to
> access it at run-time. I'd much rather have something that allowed
> zero-timeout-with-Shift-to-access-menu if and only if we have a new
> enough core.img and the console driver has getkeystatus support, and
> otherwise degraded to something else sensible.

Here's yet another version, following a discussion with Robert on IRC. I
think that this now satisfies Vladimir's concerns about not breaking
platforms that don't have a usable getkeystatus, and my concerns about
backward compatibility with old core.img files. The new keystatus
command is, I think, also a rather clearer way to express this.

I expect to use this something like this:

  set timeout=10
  if keystatus; then
    if keystatus --shift; then
      true
    else
      set timeout=0
    fi
  fi

I left part of the sleep change in for some degree of consistency, but
removed the change to the case where the sleep period is zero. I also
adjusted the USB keyboard driver to tweak the idle rate temporarily to
make sure that we get events even if a modifier isn't pressed or
released during the timeout.

2009-08-24  Colin Watson  <cjwatson@ubuntu.com>
2009-08-24  Robert Millan  <rmh.grub@aybabtu.com>

	Add `getkeystatus' terminal method.  Use it in `sleep' to detect
	Shift being held down.  Add a new `keystatus' command to query
	it.

	* include/grub/term.h (GRUB_TERM_STATUS_SHIFT,
	GRUB_TERM_STATUS_CTRL, GRUB_TERM_STATUS_ALT): Definitions for
	modifier key bitmasks.
	(struct grub_term_input): Add `getkeystatus' member.
	(grub_getkeystatus): Add prototype.
	* kern/term.c (grub_getkeystatus): New function.

	* include/grub/i386/pc/memory.h
	(GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR): New macro.
	(struct grub_machine_bios_data_area): Define necessary parts of BIOS
	Data Area layout.
	* term/i386/pc/console.c (grub_console_getkeystatus): New function.
	(grub_console_term_input): Set `getkeystatus' member.
	* term/usb_keyboard.c (grub_usb_keyboard_getkeystatus): New
	function.
	(grub_usb_keyboard_term): Set `getkeystatus' member.

	* commands/sleep.c (grub_check_keyboard): New function.
	(grub_interruptible_millisleep): Use grub_check_keyboard,
	checking whether Shift is pressed in addition to checking for
	Escape.

	* commands/keystatus.c: New file.
	* conf/common.rmk (pkglib_MODULES): Add keystatus.mod.
	(keystatus_mod_SOURCES): New variable.
	(keystatus_mod_CFLAGS): Likewise.
	(keystatus_mod_LDFLAGS): Likewise.
	* conf/i386-coreboot.rmk (grub_emu_SOURCES): Add
	commands/keystatus.c.
	* conf/i386-efi.rmk (grub_emu_SOURCES): Likewise.
	* conf/i386-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/i386-pc.rmk (grub_emu_SOURCES): Likewise.
	* conf/powerpc-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/sparc64-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/x86_64-efi.rmk (grub_emu_SOURCES): Likewise.
	* DISTLIST: Add commands/keystatus.c.

Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(revision 2535)
+++ conf/common.rmk	(working copy)
@@ -363,7 +363,8 @@
 	terminfo.mod test.mod blocklist.mod hexdump.mod		\
 	read.mod sleep.mod loadenv.mod crc.mod parttool.mod	\
 	msdospart.mod memrw.mod normal.mod sh.mod lua.mod	\
-	gptsync.mod true.mod probe.mod password.mod
+	gptsync.mod true.mod probe.mod password.mod		\
+	keystatus.mod
 
 # For password.mod.
 password_mod_SOURCES = commands/password.c
@@ -510,6 +511,11 @@
 probe_mod_CFLAGS = $(COMMON_CFLAGS)
 probe_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
+# For keystatus.mod.
+keystatus_mod_SOURCES = commands/keystatus.c
+keystatus_mod_CFLAGS = $(COMMON_CFLAGS)
+keystatus_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For normal.mod.
 normal_mod_SOURCES = normal/main.c normal/cmdline.c normal/dyncmd.c \
 	normal/auth.c normal/autofs.c normal/handler.c \
Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 2535)
+++ conf/i386-pc.rmk	(working copy)
@@ -123,7 +123,7 @@
 	lib/hexdump.c commands/i386/pc/halt.c commands/reboot.c		\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
 	commands/i386/cpuid.c	\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/host.c disk/loopback.c disk/scsi.c				\
 	fs/fshelp.c 	\
 	\
Index: conf/i386-efi.rmk
===================================================================
--- conf/i386-efi.rmk	(revision 2535)
+++ conf/i386-efi.rmk	(working copy)
@@ -37,7 +37,7 @@
 	commands/configfile.c commands/help.c				\
 	commands/handler.c commands/ls.c commands/test.c 		\
 	commands/search.c commands/hexdump.c lib/hexdump.c		\
-	commands/halt.c commands/reboot.c				\
+	commands/halt.c commands/reboot.c commands/keystatus.c		\
 	commands/i386/cpuid.c						\
 	commands/password.c						\
 	disk/loopback.c							\
Index: conf/i386-ieee1275.rmk
===================================================================
--- conf/i386-ieee1275.rmk	(revision 2535)
+++ conf/i386-ieee1275.rmk	(working copy)
@@ -66,7 +66,7 @@
 	lib/hexdump.c commands/halt.c commands/reboot.c			\
 	commands/gptsync.c commands/probe.c  commands/xnu_uuid.c	\
 	commands/i386/cpuid.c	\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/host.c disk/loopback.c					\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: conf/x86_64-efi.rmk
===================================================================
--- conf/x86_64-efi.rmk	(revision 2535)
+++ conf/x86_64-efi.rmk	(working copy)
@@ -37,7 +37,7 @@
 	commands/search.c commands/hexdump.c lib/hexdump.c		\
 	commands/halt.c commands/reboot.c				\
 	commands/i386/cpuid.c						\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/loopback.c							\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: conf/i386-coreboot.rmk
===================================================================
--- conf/i386-coreboot.rmk	(revision 2535)
+++ conf/i386-coreboot.rmk	(working copy)
@@ -110,7 +110,7 @@
 	commands/handler.c commands/ls.c commands/test.c 		\
 	commands/search.c commands/blocklist.c commands/hexdump.c	\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	lib/hexdump.c commands/i386/cpuid.c				\
 	disk/host.c disk/loopback.c					\
 	\
Index: conf/powerpc-ieee1275.rmk
===================================================================
--- conf/powerpc-ieee1275.rmk	(revision 2535)
+++ conf/powerpc-ieee1275.rmk	(working copy)
@@ -46,7 +46,7 @@
 	commands/ls.c commands/blocklist.c commands/hexdump.c		\
 	lib/hexdump.c commands/halt.c commands/reboot.c 		\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/loopback.c							\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: conf/sparc64-ieee1275.rmk
===================================================================
--- conf/sparc64-ieee1275.rmk	(revision 2535)
+++ conf/sparc64-ieee1275.rmk	(working copy)
@@ -103,7 +103,7 @@
 	commands/ls.c commands/blocklist.c commands/hexdump.c		\
 	lib/hexdump.c commands/halt.c commands/reboot.c 		\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/loopback.c							\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: kern/term.c
===================================================================
--- kern/term.c	(revision 2535)
+++ kern/term.c	(working copy)
@@ -140,6 +140,15 @@
   return (grub_cur_term_input->checkkey) ();
 }
 
+int
+grub_getkeystatus (void)
+{
+  if (grub_cur_term_input->getkeystatus)
+    return (grub_cur_term_input->getkeystatus) ();
+  else
+    return 0;
+}
+
 grub_uint16_t
 grub_getxy (void)
 {
Index: include/grub/term.h
===================================================================
--- include/grub/term.h	(revision 2535)
+++ include/grub/term.h	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -72,6 +72,12 @@
 #define GRUB_TERM_NEED_INIT	(1 << 16)
 
 
+/* Bitmasks for modifier keys returned by grub_getkeystatus.  */
+#define GRUB_TERM_STATUS_SHIFT	(1 << 0)
+#define GRUB_TERM_STATUS_CTRL	(1 << 1)
+#define GRUB_TERM_STATUS_ALT	(1 << 2)
+
+
 /* Unicode characters for fancy graphics.  */
 #define GRUB_TERM_DISP_LEFT	0x2190
 #define GRUB_TERM_DISP_UP	0x2191
@@ -157,6 +163,9 @@
 
   /* Get a character.  */
   int (*getkey) (void);
+
+  /* Get keyboard modifier status.  */
+  int (*getkeystatus) (void);
 };
 typedef struct grub_term_input *grub_term_input_t;
 
@@ -275,6 +284,7 @@
 grub_ssize_t EXPORT_FUNC(grub_getcharwidth) (grub_uint32_t code);
 int EXPORT_FUNC(grub_getkey) (void);
 int EXPORT_FUNC(grub_checkkey) (void);
+int EXPORT_FUNC(grub_getkeystatus) (void);
 grub_uint16_t EXPORT_FUNC(grub_getwh) (void);
 grub_uint16_t EXPORT_FUNC(grub_getxy) (void);
 void EXPORT_FUNC(grub_gotoxy) (grub_uint8_t x, grub_uint8_t y);
Index: include/grub/i386/pc/memory.h
===================================================================
--- include/grub/i386/pc/memory.h	(revision 2535)
+++ include/grub/i386/pc/memory.h	(working copy)
@@ -78,8 +78,19 @@
 /* The data segment of the pseudo real mode.  */
 #define GRUB_MEMORY_MACHINE_PSEUDO_REAL_DSEG	0x20
 
+#define GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR	0x400
+
 #ifndef ASM_FILE
 
+/* See http://heim.ifi.uio.no/~stanisls/helppc/bios_data_area.html for a
+   description of the BIOS Data Area layout.  */
+struct grub_machine_bios_data_area
+{
+  grub_uint8_t unused1[0x17];
+  grub_uint8_t keyboard_flag_lower; /* 0x17 */ 
+  grub_uint8_t unused2[0xf0 - 0x18];
+};
+
 struct grub_machine_mmap_entry
 {
   grub_uint32_t size;
Index: commands/sleep.c
===================================================================
--- commands/sleep.c	(revision 2535)
+++ commands/sleep.c	(working copy)
@@ -1,7 +1,7 @@
 /* sleep.c - Command to wait a specified number of seconds.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2006,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2006,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -43,6 +43,20 @@
   grub_printf ("%d    ", n);
 }
 
+static int
+grub_check_keyboard (void)
+{
+  int mods = grub_getkeystatus ();
+  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
+    return 1;
+
+  if (grub_checkkey () >= 0 &&
+      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    return 1;
+
+  return 0;
+}
+
 /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
 static int
 grub_interruptible_millisleep (grub_uint32_t ms)
@@ -52,8 +66,7 @@
   start = grub_get_time_ms ();
 
   while (grub_get_time_ms () - start < ms)
-    if (grub_checkkey () >= 0 &&
-	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
+    if (grub_check_keyboard ())
       return 1;
 
   return 0;
Index: commands/keystatus.c
===================================================================
--- commands/keystatus.c	(revision 0)
+++ commands/keystatus.c	(revision 0)
@@ -0,0 +1,81 @@
+/* keystatus.c - Command to check key modifier status.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/dl.h>
+#include <grub/misc.h>
+#include <grub/extcmd.h>
+#include <grub/term.h>
+
+static const struct grub_arg_option options[] =
+  {
+    {"shift", 's', 0, "check Shift key", 0, 0},
+    {"ctrl", 'c', 0, "check Control key", 0, 0},
+    {"alt", 'a', 0, "check Alt key", 0, 0},
+    {0, 0, 0, 0, 0, 0}
+  };
+
+#define grub_cur_term_input	grub_term_get_current_input ()
+
+static grub_err_t
+grub_cmd_keystatus (grub_extcmd_t cmd,
+		    int argc __attribute__ ((unused)),
+		    char **args __attribute__ ((unused)))
+{
+  struct grub_arg_list *state = cmd->state;
+  int expect_mods = 0;
+  int mods;
+
+  if (state[0].set)
+    expect_mods |= GRUB_TERM_STATUS_SHIFT;
+  if (state[1].set)
+    expect_mods |= GRUB_TERM_STATUS_CTRL;
+  if (state[2].set)
+    expect_mods |= GRUB_TERM_STATUS_ALT;
+
+  /* Without arguments, just check whether getkeystatus is supported at
+     all.  */
+  if (!grub_cur_term_input->getkeystatus)
+    return grub_error (GRUB_ERR_TEST_FAILURE, "false");
+  grub_dprintf ("keystatus", "expect_mods: %d\n", expect_mods);
+  if (!expect_mods)
+    return 0;
+
+  mods = grub_getkeystatus ();
+  grub_dprintf ("keystatus", "mods: %d\n", mods);
+  if (mods >= 0 && (mods & expect_mods) != 0)
+    return 0;
+  else
+    return grub_error (GRUB_ERR_TEST_FAILURE, "false");
+}
+
+static grub_extcmd_t cmd;
+\f
+GRUB_MOD_INIT(keystatus)
+{
+  cmd = grub_register_extcmd ("keystatus", grub_cmd_keystatus,
+			      GRUB_COMMAND_FLAG_BOTH,
+			      "keystatus [--shift|--ctrl|--alt]",
+			      "Check key modifier status",
+			      options);
+}
+
+GRUB_MOD_FINI(keystatus)
+{
+  grub_unregister_extcmd (cmd);
+}
Index: term/i386/pc/console.c
===================================================================
--- term/i386/pc/console.c	(revision 2535)
+++ term/i386/pc/console.c	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -16,15 +16,35 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <grub/machine/memory.h>
 #include <grub/machine/console.h>
 #include <grub/term.h>
 #include <grub/types.h>
 
+static const struct grub_machine_bios_data_area *bios_data_area = GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR;
+
+static int
+grub_console_getkeystatus (void)
+{
+  grub_uint8_t status = bios_data_area->keyboard_flag_lower;
+  int mods = 0;
+
+  if (status & 0x03)
+    mods |= GRUB_TERM_STATUS_SHIFT;
+  if (status & 0x04)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (status & 0x08)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  return mods;
+}
+
 static struct grub_term_input grub_console_term_input =
   {
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .getkeystatus = grub_console_getkeystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
Index: term/usb_keyboard.c
===================================================================
--- term/usb_keyboard.c	(revision 2535)
+++ term/usb_keyboard.c	(working copy)
@@ -238,11 +238,67 @@
   return key;
 }
 
+static int
+grub_usb_keyboard_getkeystatus (void)
+{
+  grub_uint8_t data[8];
+  int mods = 0;
+  grub_err_t err;
+  grub_uint64_t currtime;
+  int timeout = 50;
+
+  /* Set idle time to the minimum offered by the spec (4 milliseconds) so
+     that we can find out the current state.  */
+  grub_usb_control_msg (usbdev, 0x21, 0x0A, 1<<8, 0, 0, 0);
+
+  grub_usb_control_msg (usbdev, (1 << 7) | (1 << 5) | 1, 0x02, 0, 0, 1, (char *) data);
+
+  currtime = grub_get_time_ms ();
+  do
+    {
+      /* Get_Report.  */
+      err = grub_usb_keyboard_getreport (usbdev, data);
+
+      /* Implement a timeout.  */
+      if (grub_get_time_ms () > currtime + timeout)
+	break;
+    }
+  while (err || !data[0]);
+
+  /* Go back to reporting every time an event occurs and not more often than
+     that.  */
+  grub_usb_control_msg (usbdev, 0x21, 0x0A, 0<<8, 0, 0, 0);
+
+  /* We allowed a while for modifiers to show up in the report, but it is
+     not an error if they never did.  */
+  if (err)
+    return -1;
+
+  grub_dprintf ("usb_keyboard",
+		"report: 0x%02x 0x%02x 0x%02x 0x%02x"
+		" 0x%02x 0x%02x 0x%02x 0x%02x\n",
+		data[0], data[1], data[2], data[3],
+		data[4], data[5], data[6], data[7]);
+
+  /* Check Shift, Control, and Alt status.  */
+  if (data[0] & 0x02 || data[0] & 0x20)
+    mods |= GRUB_TERM_STATUS_SHIFT;
+  if (data[0] & 0x01 || data[0] & 0x10)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (data[0] & 0x04 || data[0] & 0x40)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  grub_errno = GRUB_ERR_NONE;
+
+  return mods;
+}
+
 static struct grub_term_input grub_usb_keyboard_term =
   {
     .name = "usb_keyboard",
     .checkkey = grub_usb_keyboard_checkkey,
     .getkey = grub_usb_keyboard_getkey,
+    .getkeystatus = grub_usb_keyboard_getkeystatus,
     .next = 0
   };
 

-- 
Colin Watson                           [cjwatson@chiark.greenend.org.uk]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Wed, Aug 26, 2009 at 04:33:56PM +0100, Colin Watson wrote:
> Index: commands/sleep.c
> ===================================================================
> --- commands/sleep.c	(revision 2535)
> +++ commands/sleep.c	(working copy)
> [...]
>  
> +static int
> +grub_check_keyboard (void)
> +{
> +  int mods = grub_getkeystatus ();
> +  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
> +    return 1;
> +
> +  if (grub_checkkey () >= 0 &&
> +      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
> +    return 1;
> +
> +  return 0;
> +}
> +
>  /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
>  static int
>  grub_interruptible_millisleep (grub_uint32_t ms)
> @@ -52,8 +66,7 @@
>    start = grub_get_time_ms ();
>  
>    while (grub_get_time_ms () - start < ms)
> -    if (grub_checkkey () >= 0 &&
> -	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
> +    if (grub_check_keyboard ())
>        return 1;

I'm still not convinced that we want the sleep bits.  They seem too ad-hoc,
and I'd like to hear what others think first.

But if I understood correctly, you plan on using keystatus command only?

> +static int
> +grub_console_getkeystatus (void)
> +{
> +  grub_uint8_t status = bios_data_area->keyboard_flag_lower;
> +  int mods = 0;
> +
> +  if (status & 0x03)
> +    mods |= GRUB_TERM_STATUS_SHIFT;
> +  if (status & 0x04)
> +    mods |= GRUB_TERM_STATUS_CTRL;
> +  if (status & 0x08)
> +    mods |= GRUB_TERM_STATUS_ALT;
> +
> +  return mods;
> +}

This should be macroified (but for the time being, I have no problem with
our internal representation matching the one in BIOS Data Area, hence no
translation would be needed on i386-pc).

> +  /* Set idle time to the minimum offered by the spec (4 milliseconds) so
> +     that we can find out the current state.  */
> +  grub_usb_control_msg (usbdev, 0x21, 0x0A, 1<<8, 0, 0, 0);
> +
> +  grub_usb_control_msg (usbdev, (1 << 7) | (1 << 5) | 1, 0x02, 0, 0, 1, (char *) data);
> +
> [...]
> +  /* Go back to reporting every time an event occurs and not more often than
> +     that.  */
> +  grub_usb_control_msg (usbdev, 0x21, 0x0A, 0<<8, 0, 0, 0);

If they represent fixed numbers like registers/ports, these hex values should
be macros too.

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Wed, Aug 26, 2009 at 08:39:00PM +0200, Robert Millan wrote:
> On Wed, Aug 26, 2009 at 04:33:56PM +0100, Colin Watson wrote:
> > Index: commands/sleep.c
> > ===================================================================
> > --- commands/sleep.c	(revision 2535)
> > +++ commands/sleep.c	(working copy)
> > [...]
> >  
> > +static int
> > +grub_check_keyboard (void)
> > +{
> > +  int mods = grub_getkeystatus ();
> > +  if (mods >= 0 && (mods & GRUB_TERM_STATUS_SHIFT) != 0)
> > +    return 1;
> > +
> > +  if (grub_checkkey () >= 0 &&
> > +      GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
> > +    return 1;
> > +
> > +  return 0;
> > +}
> > +
> >  /* Based on grub_millisleep() from kern/generic/millisleep.c.  */
> >  static int
> >  grub_interruptible_millisleep (grub_uint32_t ms)
> > @@ -52,8 +66,7 @@
> >    start = grub_get_time_ms ();
> >  
> >    while (grub_get_time_ms () - start < ms)
> > -    if (grub_checkkey () >= 0 &&
> > -	GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
> > +    if (grub_check_keyboard ())
> >        return 1;
> 
> I'm still not convinced that we want the sleep bits.  They seem too ad-hoc,
> and I'd like to hear what others think first.
> 
> But if I understood correctly, you plan on using keystatus command only?

If GRUB_HIDDEN_TIMEOUT is set to a non-zero value, I would use sleep
rather than keystatus, I think. (At least, if I decided I wanted to
support that configuration; I'm not quite sure yet.) In that case I'd
want the sleep to be interruptible by pressing Shift for consistency
with the zero-delay case.

However, since this is essentially an artifact of our configuration and
doesn't introduce any new configuration file words, I could keep this
part as an Ubuntu-specific patch with relatively little trouble.

> > +static int
> > +grub_console_getkeystatus (void)
> > +{
> > +  grub_uint8_t status = bios_data_area->keyboard_flag_lower;
> > +  int mods = 0;
> > +
> > +  if (status & 0x03)
> > +    mods |= GRUB_TERM_STATUS_SHIFT;
> > +  if (status & 0x04)
> > +    mods |= GRUB_TERM_STATUS_CTRL;
> > +  if (status & 0x08)
> > +    mods |= GRUB_TERM_STATUS_ALT;
> > +
> > +  return mods;
> > +}
> 
> This should be macroified (but for the time being, I have no problem with
> our internal representation matching the one in BIOS Data Area, hence no
> translation would be needed on i386-pc).

Vladimir objected to the inconsistency between distinguishing left and
right Shift but not left and right Control or Alt, which is why our
internal representation does not currently match. I agree it's clearer
to macroify things.

> > +  /* Set idle time to the minimum offered by the spec (4 milliseconds) so
> > +     that we can find out the current state.  */
> > +  grub_usb_control_msg (usbdev, 0x21, 0x0A, 1<<8, 0, 0, 0);
> > +
> > +  grub_usb_control_msg (usbdev, (1 << 7) | (1 << 5) | 1, 0x02, 0, 0, 1, (char *) data);
> > +
> > [...]
> > +  /* Go back to reporting every time an event occurs and not more often than
> > +     that.  */
> > +  grub_usb_control_msg (usbdev, 0x21, 0x0A, 0<<8, 0, 0, 0);
> 
> If they represent fixed numbers like registers/ports, these hex values should
> be macros too.

OK, and doing so helped me notice that I'd left a Get_Idle request in
there by mistake from debugging. Updated patch follows. There are a few
changes unrelated to the main body of the patch as a result; do you want
these split out?


2009-08-24  Colin Watson  <cjwatson@ubuntu.com>
2009-08-24  Robert Millan  <rmh.grub@aybabtu.com>

	Add `getkeystatus' terminal method.  Add a new `keystatus' command
	to query it.

	* include/grub/term.h (GRUB_TERM_STATUS_SHIFT,
	GRUB_TERM_STATUS_CTRL, GRUB_TERM_STATUS_ALT): Definitions for
	modifier key bitmasks.
	(struct grub_term_input): Add `getkeystatus' member.
	(grub_getkeystatus): Add prototype.
	* kern/term.c (grub_getkeystatus): New function.

	* include/grub/i386/pc/memory.h
	(GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR): New macro.
	(struct grub_machine_bios_data_area): Define necessary parts of BIOS
	Data Area layout.
	* term/i386/pc/console.c (grub_console_getkeystatus): New function.
	(grub_console_term_input): Set `getkeystatus' member.
	* term/usb_keyboard.c (grub_usb_hid): Macroify HID protocol
	constants.
	(grub_usb_keyboard_getreport): Likewise.
	(grub_usb_keyboard_checkkey): Likewise.
	(grub_usb_keyboard_getkeystatus): New function.
	(grub_usb_keyboard_term): Set `getkeystatus' member.

	* commands/keystatus.c: New file.
	* conf/common.rmk (pkglib_MODULES): Add keystatus.mod.
	(keystatus_mod_SOURCES): New variable.
	(keystatus_mod_CFLAGS): Likewise.
	(keystatus_mod_LDFLAGS): Likewise.
	* conf/i386-coreboot.rmk (grub_emu_SOURCES): Add
	commands/keystatus.c.
	* conf/i386-efi.rmk (grub_emu_SOURCES): Likewise.
	* conf/i386-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/i386-pc.rmk (grub_emu_SOURCES): Likewise.
	* conf/powerpc-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/sparc64-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/x86_64-efi.rmk (grub_emu_SOURCES): Likewise.
	* DISTLIST: Add commands/keystatus.c.

Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(revision 2535)
+++ conf/common.rmk	(working copy)
@@ -363,7 +363,8 @@
 	terminfo.mod test.mod blocklist.mod hexdump.mod		\
 	read.mod sleep.mod loadenv.mod crc.mod parttool.mod	\
 	msdospart.mod memrw.mod normal.mod sh.mod lua.mod	\
-	gptsync.mod true.mod probe.mod password.mod
+	gptsync.mod true.mod probe.mod password.mod		\
+	keystatus.mod
 
 # For password.mod.
 password_mod_SOURCES = commands/password.c
@@ -510,6 +511,11 @@
 probe_mod_CFLAGS = $(COMMON_CFLAGS)
 probe_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
+# For keystatus.mod.
+keystatus_mod_SOURCES = commands/keystatus.c
+keystatus_mod_CFLAGS = $(COMMON_CFLAGS)
+keystatus_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For normal.mod.
 normal_mod_SOURCES = normal/main.c normal/cmdline.c normal/dyncmd.c \
 	normal/auth.c normal/autofs.c normal/handler.c \
Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 2535)
+++ conf/i386-pc.rmk	(working copy)
@@ -123,7 +123,7 @@
 	lib/hexdump.c commands/i386/pc/halt.c commands/reboot.c		\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
 	commands/i386/cpuid.c	\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/host.c disk/loopback.c disk/scsi.c				\
 	fs/fshelp.c 	\
 	\
Index: conf/i386-efi.rmk
===================================================================
--- conf/i386-efi.rmk	(revision 2535)
+++ conf/i386-efi.rmk	(working copy)
@@ -37,7 +37,7 @@
 	commands/configfile.c commands/help.c				\
 	commands/handler.c commands/ls.c commands/test.c 		\
 	commands/search.c commands/hexdump.c lib/hexdump.c		\
-	commands/halt.c commands/reboot.c				\
+	commands/halt.c commands/reboot.c commands/keystatus.c		\
 	commands/i386/cpuid.c						\
 	commands/password.c						\
 	disk/loopback.c							\
Index: conf/i386-ieee1275.rmk
===================================================================
--- conf/i386-ieee1275.rmk	(revision 2535)
+++ conf/i386-ieee1275.rmk	(working copy)
@@ -66,7 +66,7 @@
 	lib/hexdump.c commands/halt.c commands/reboot.c			\
 	commands/gptsync.c commands/probe.c  commands/xnu_uuid.c	\
 	commands/i386/cpuid.c	\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/host.c disk/loopback.c					\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: conf/x86_64-efi.rmk
===================================================================
--- conf/x86_64-efi.rmk	(revision 2535)
+++ conf/x86_64-efi.rmk	(working copy)
@@ -37,7 +37,7 @@
 	commands/search.c commands/hexdump.c lib/hexdump.c		\
 	commands/halt.c commands/reboot.c				\
 	commands/i386/cpuid.c						\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/loopback.c							\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: conf/i386-coreboot.rmk
===================================================================
--- conf/i386-coreboot.rmk	(revision 2535)
+++ conf/i386-coreboot.rmk	(working copy)
@@ -110,7 +110,7 @@
 	commands/handler.c commands/ls.c commands/test.c 		\
 	commands/search.c commands/blocklist.c commands/hexdump.c	\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	lib/hexdump.c commands/i386/cpuid.c				\
 	disk/host.c disk/loopback.c					\
 	\
Index: conf/powerpc-ieee1275.rmk
===================================================================
--- conf/powerpc-ieee1275.rmk	(revision 2535)
+++ conf/powerpc-ieee1275.rmk	(working copy)
@@ -46,7 +46,7 @@
 	commands/ls.c commands/blocklist.c commands/hexdump.c		\
 	lib/hexdump.c commands/halt.c commands/reboot.c 		\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/loopback.c							\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: conf/sparc64-ieee1275.rmk
===================================================================
--- conf/sparc64-ieee1275.rmk	(revision 2535)
+++ conf/sparc64-ieee1275.rmk	(working copy)
@@ -103,7 +103,7 @@
 	commands/ls.c commands/blocklist.c commands/hexdump.c		\
 	lib/hexdump.c commands/halt.c commands/reboot.c 		\
 	commands/gptsync.c commands/probe.c commands/xnu_uuid.c		\
-	commands/password.c						\
+	commands/password.c commands/keystatus.c			\
 	disk/loopback.c							\
 	\
 	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
Index: kern/term.c
===================================================================
--- kern/term.c	(revision 2535)
+++ kern/term.c	(working copy)
@@ -140,6 +140,15 @@
   return (grub_cur_term_input->checkkey) ();
 }
 
+int
+grub_getkeystatus (void)
+{
+  if (grub_cur_term_input->getkeystatus)
+    return (grub_cur_term_input->getkeystatus) ();
+  else
+    return 0;
+}
+
 grub_uint16_t
 grub_getxy (void)
 {
Index: include/grub/term.h
===================================================================
--- include/grub/term.h	(revision 2535)
+++ include/grub/term.h	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -72,6 +72,12 @@
 #define GRUB_TERM_NEED_INIT	(1 << 16)
 
 
+/* Bitmasks for modifier keys returned by grub_getkeystatus.  */
+#define GRUB_TERM_STATUS_SHIFT	(1 << 0)
+#define GRUB_TERM_STATUS_CTRL	(1 << 1)
+#define GRUB_TERM_STATUS_ALT	(1 << 2)
+
+
 /* Unicode characters for fancy graphics.  */
 #define GRUB_TERM_DISP_LEFT	0x2190
 #define GRUB_TERM_DISP_UP	0x2191
@@ -157,6 +163,9 @@
 
   /* Get a character.  */
   int (*getkey) (void);
+
+  /* Get keyboard modifier status.  */
+  int (*getkeystatus) (void);
 };
 typedef struct grub_term_input *grub_term_input_t;
 
@@ -275,6 +284,7 @@
 grub_ssize_t EXPORT_FUNC(grub_getcharwidth) (grub_uint32_t code);
 int EXPORT_FUNC(grub_getkey) (void);
 int EXPORT_FUNC(grub_checkkey) (void);
+int EXPORT_FUNC(grub_getkeystatus) (void);
 grub_uint16_t EXPORT_FUNC(grub_getwh) (void);
 grub_uint16_t EXPORT_FUNC(grub_getxy) (void);
 void EXPORT_FUNC(grub_gotoxy) (grub_uint8_t x, grub_uint8_t y);
Index: include/grub/i386/pc/memory.h
===================================================================
--- include/grub/i386/pc/memory.h	(revision 2535)
+++ include/grub/i386/pc/memory.h	(working copy)
@@ -78,8 +78,19 @@
 /* The data segment of the pseudo real mode.  */
 #define GRUB_MEMORY_MACHINE_PSEUDO_REAL_DSEG	0x20
 
+#define GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR	0x400
+
 #ifndef ASM_FILE
 
+/* See http://heim.ifi.uio.no/~stanisls/helppc/bios_data_area.html for a
+   description of the BIOS Data Area layout.  */
+struct grub_machine_bios_data_area
+{
+  grub_uint8_t unused1[0x17];
+  grub_uint8_t keyboard_flag_lower; /* 0x17 */ 
+  grub_uint8_t unused2[0xf0 - 0x18];
+};
+
 struct grub_machine_mmap_entry
 {
   grub_uint32_t size;
Index: commands/keystatus.c
===================================================================
--- commands/keystatus.c	(revision 0)
+++ commands/keystatus.c	(revision 0)
@@ -0,0 +1,81 @@
+/* keystatus.c - Command to check key modifier status.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/dl.h>
+#include <grub/misc.h>
+#include <grub/extcmd.h>
+#include <grub/term.h>
+
+static const struct grub_arg_option options[] =
+  {
+    {"shift", 's', 0, "check Shift key", 0, 0},
+    {"ctrl", 'c', 0, "check Control key", 0, 0},
+    {"alt", 'a', 0, "check Alt key", 0, 0},
+    {0, 0, 0, 0, 0, 0}
+  };
+
+#define grub_cur_term_input	grub_term_get_current_input ()
+
+static grub_err_t
+grub_cmd_keystatus (grub_extcmd_t cmd,
+		    int argc __attribute__ ((unused)),
+		    char **args __attribute__ ((unused)))
+{
+  struct grub_arg_list *state = cmd->state;
+  int expect_mods = 0;
+  int mods;
+
+  if (state[0].set)
+    expect_mods |= GRUB_TERM_STATUS_SHIFT;
+  if (state[1].set)
+    expect_mods |= GRUB_TERM_STATUS_CTRL;
+  if (state[2].set)
+    expect_mods |= GRUB_TERM_STATUS_ALT;
+
+  /* Without arguments, just check whether getkeystatus is supported at
+     all.  */
+  if (!grub_cur_term_input->getkeystatus)
+    return grub_error (GRUB_ERR_TEST_FAILURE, "false");
+  grub_dprintf ("keystatus", "expect_mods: %d\n", expect_mods);
+  if (!expect_mods)
+    return 0;
+
+  mods = grub_getkeystatus ();
+  grub_dprintf ("keystatus", "mods: %d\n", mods);
+  if (mods >= 0 && (mods & expect_mods) != 0)
+    return 0;
+  else
+    return grub_error (GRUB_ERR_TEST_FAILURE, "false");
+}
+
+static grub_extcmd_t cmd;
+\f
+GRUB_MOD_INIT(keystatus)
+{
+  cmd = grub_register_extcmd ("keystatus", grub_cmd_keystatus,
+			      GRUB_COMMAND_FLAG_BOTH,
+			      "keystatus [--shift|--ctrl|--alt]",
+			      "Check key modifier status",
+			      options);
+}
+
+GRUB_MOD_FINI(keystatus)
+{
+  grub_unregister_extcmd (cmd);
+}
Index: term/i386/pc/console.c
===================================================================
--- term/i386/pc/console.c	(revision 2535)
+++ term/i386/pc/console.c	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -16,15 +16,40 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <grub/machine/memory.h>
 #include <grub/machine/console.h>
 #include <grub/term.h>
 #include <grub/types.h>
 
+static const struct grub_machine_bios_data_area *bios_data_area = GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR;
+
+#define KEYBOARD_LEFT_SHIFT	(1 << 0)
+#define KEYBOARD_RIGHT_SHIFT	(1 << 1)
+#define KEYBOARD_CTRL		(1 << 2)
+#define KEYBOARD_ALT		(1 << 3)
+
+static int
+grub_console_getkeystatus (void)
+{
+  grub_uint8_t status = bios_data_area->keyboard_flag_lower;
+  int mods = 0;
+
+  if (status & (KEYBOARD_LEFT_SHIFT | KEYBOARD_RIGHT_SHIFT))
+    mods |= GRUB_TERM_STATUS_SHIFT;
+  if (status & KEYBOARD_CTRL)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (status & KEYBOARD_ALT)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  return mods;
+}
+
 static struct grub_term_input grub_console_term_input =
   {
     .name = "console",
     .checkkey = grub_console_checkkey,
     .getkey = grub_console_getkey,
+    .getkeystatus = grub_console_getkeystatus,
   };
 
 static struct grub_term_output grub_console_term_output =
Index: term/usb_keyboard.c
===================================================================
--- term/usb_keyboard.c	(revision 2535)
+++ term/usb_keyboard.c	(working copy)
@@ -58,6 +58,19 @@
 
 static grub_usb_device_t usbdev;
 
+/* Valid values for bmRequestType.  See HID definition version 1.11 section
+   7.2.  */
+#define USB_HID_HOST_TO_DEVICE	0x21
+#define USB_HID_DEVICE_TO_HOST	0x61
+
+/* Valid values for bRequest.  See HID definition version 1.11 section 7.2. */
+#define USB_HID_GET_REPORT	0x01
+#define USB_HID_GET_IDLE	0x02
+#define USB_HID_GET_PROTOCOL	0x03
+#define USB_HID_SET_REPORT	0x09
+#define USB_HID_SET_IDLE	0x0A
+#define USB_HID_SET_PROTOCOL	0x0B
+
 static void
 grub_usb_hid (void)
 {
@@ -90,17 +103,19 @@
   grub_usb_iterate (usb_iterate);
 
   /* Place the device in boot mode.  */
-  grub_usb_control_msg (usbdev, 0x21, 0x0B, 0, 0, 0, 0);
+  grub_usb_control_msg (usbdev, USB_HID_HOST_TO_DEVICE, USB_HID_SET_PROTOCOL,
+			0, 0, 0, 0);
 
   /* Reports every time an event occurs and not more often than that.  */
-  grub_usb_control_msg (usbdev, 0x21, 0x0A, 0<<8, 0, 0, 0);
+  grub_usb_control_msg (usbdev, USB_HID_HOST_TO_DEVICE, USB_HID_SET_IDLE,
+			0<<8, 0, 0, 0);
 }
 
 static grub_err_t
 grub_usb_keyboard_getreport (grub_usb_device_t dev, grub_uint8_t *report)
 {
-  return grub_usb_control_msg (dev, (1 << 7) | (1 << 5) | 1, 0x01, 0, 0,
-			       8, (char *) report);
+  return grub_usb_control_msg (dev, USB_HID_DEVICE_TO_HOST, USB_HID_GET_REPORT,
+			       0, 0, 8, (char *) report);
 }
 
 \f
@@ -151,7 +166,8 @@
   /* Wait until the key is released.  */
   while (!err && data[2])
     {
-      err = grub_usb_control_msg (usbdev, (1 << 7) | (1 << 5) | 1, 0x01, 0, 0,
+      err = grub_usb_control_msg (usbdev, USB_HID_DEVICE_TO_HOST,
+				  USB_HID_GET_REPORT, 0, 0,
 				  sizeof (data), (char *) data);
       grub_dprintf ("usb_keyboard",
 		    "report2: 0x%02x 0x%02x 0x%02x 0x%02x"
@@ -238,11 +254,67 @@
   return key;
 }
 
+static int
+grub_usb_keyboard_getkeystatus (void)
+{
+  grub_uint8_t data[8];
+  int mods = 0;
+  grub_err_t err;
+  grub_uint64_t currtime;
+  int timeout = 50;
+
+  /* Set idle time to the minimum offered by the spec (4 milliseconds) so
+     that we can find out the current state.  */
+  grub_usb_control_msg (usbdev, USB_HID_HOST_TO_DEVICE, USB_HID_SET_IDLE,
+			0<<8, 0, 0, 0);
+
+  currtime = grub_get_time_ms ();
+  do
+    {
+      /* Get_Report.  */
+      err = grub_usb_keyboard_getreport (usbdev, data);
+
+      /* Implement a timeout.  */
+      if (grub_get_time_ms () > currtime + timeout)
+	break;
+    }
+  while (err || !data[0]);
+
+  /* Go back to reporting every time an event occurs and not more often than
+     that.  */
+  grub_usb_control_msg (usbdev, USB_HID_HOST_TO_DEVICE, USB_HID_SET_IDLE,
+			0<<8, 0, 0, 0);
+
+  /* We allowed a while for modifiers to show up in the report, but it is
+     not an error if they never did.  */
+  if (err)
+    return -1;
+
+  grub_dprintf ("usb_keyboard",
+		"report: 0x%02x 0x%02x 0x%02x 0x%02x"
+		" 0x%02x 0x%02x 0x%02x 0x%02x\n",
+		data[0], data[1], data[2], data[3],
+		data[4], data[5], data[6], data[7]);
+
+  /* Check Shift, Control, and Alt status.  */
+  if (data[0] & 0x02 || data[0] & 0x20)
+    mods |= GRUB_TERM_STATUS_SHIFT;
+  if (data[0] & 0x01 || data[0] & 0x10)
+    mods |= GRUB_TERM_STATUS_CTRL;
+  if (data[0] & 0x04 || data[0] & 0x40)
+    mods |= GRUB_TERM_STATUS_ALT;
+
+  grub_errno = GRUB_ERR_NONE;
+
+  return mods;
+}
+
 static struct grub_term_input grub_usb_keyboard_term =
   {
     .name = "usb_keyboard",
     .checkkey = grub_usb_keyboard_checkkey,
     .getkey = grub_usb_keyboard_getkey,
+    .getkeystatus = grub_usb_keyboard_getkeystatus,
     .next = 0
   };
 

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Robert Millan]

On Wed, Aug 26, 2009 at 09:26:06PM +0100, Colin Watson wrote:
> > 
> > I'm still not convinced that we want the sleep bits.  They seem too ad-hoc,
> > and I'd like to hear what others think first.
> > 
> > But if I understood correctly, you plan on using keystatus command only?
> 
> If GRUB_HIDDEN_TIMEOUT is set to a non-zero value, I would use sleep
> rather than keystatus, I think. (At least, if I decided I wanted to
> support that configuration; I'm not quite sure yet.) In that case I'd
> want the sleep to be interruptible by pressing Shift for consistency
> with the zero-delay case.
> 
> However, since this is essentially an artifact of our configuration and
> doesn't introduce any new configuration file words, I could keep this
> part as an Ubuntu-specific patch with relatively little trouble.

Please do for now.  I'd rather not rush when discussing this part.

> > > +static int
> > > +grub_console_getkeystatus (void)
> > > +{
> > > +  grub_uint8_t status = bios_data_area->keyboard_flag_lower;
> > > +  int mods = 0;
> > > +
> > > +  if (status & 0x03)
> > > +    mods |= GRUB_TERM_STATUS_SHIFT;
> > > +  if (status & 0x04)
> > > +    mods |= GRUB_TERM_STATUS_CTRL;
> > > +  if (status & 0x08)
> > > +    mods |= GRUB_TERM_STATUS_ALT;
> > > +
> > > +  return mods;
> > > +}
> > 
> > This should be macroified (but for the time being, I have no problem with
> > our internal representation matching the one in BIOS Data Area, hence no
> > translation would be needed on i386-pc).
> 
> Vladimir objected to the inconsistency between distinguishing left and
> right Shift but not left and right Control or Alt, which is why our
> internal representation does not currently match. I agree it's clearer
> to macroify things.

Looks fine now, thanks.

> OK, and doing so helped me notice that I'd left a Get_Idle request in
> there by mistake from debugging. Updated patch follows. There are a few
> changes unrelated to the main body of the patch as a result; do you want
> these split out?

No need, this looks good.  Please commit.

-- 
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."

Re: [PATCH] Detect key modifier status in 'sleep --interruptible'

[Colin Watson]

On Fri, Aug 28, 2009 at 02:44:15PM +0200, Robert Millan wrote:
> On Wed, Aug 26, 2009 at 09:26:06PM +0100, Colin Watson wrote:
> > If GRUB_HIDDEN_TIMEOUT is set to a non-zero value, I would use sleep
> > rather than keystatus, I think. (At least, if I decided I wanted to
> > support that configuration; I'm not quite sure yet.) In that case I'd
> > want the sleep to be interruptible by pressing Shift for consistency
> > with the zero-delay case.
> > 
> > However, since this is essentially an artifact of our configuration and
> > doesn't introduce any new configuration file words, I could keep this
> > part as an Ubuntu-specific patch with relatively little trouble.
> 
> Please do for now.  I'd rather not rush when discussing this part.

Consider it done.

> > OK, and doing so helped me notice that I'd left a Get_Idle request in
> > there by mistake from debugging. Updated patch follows. There are a few
> > changes unrelated to the main body of the patch as a result; do you want
> > these split out?
> 
> No need, this looks good.  Please commit.

Committed, thanks.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]