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 <cjwat...@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 [cjwat...@ubuntu.com]
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
