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

[Robert Millan]

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

getkeystatus.diff
Description: Text Data