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  <address@hidden>

        * 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                                       address@hidden