Re: [PATCH] hid-magicmouse: Correct parsing of large X and Y motions.

[Michael Poole <mdpoole@troilus.org>]

Chase Douglas writes:

> On Tue, 2010-07-06 at 03:54 +0800, Ping Cheng wrote:
>> On Mon, Jul 5, 2010 at 10:50 PM, Michael Poole <mdpoole@troilus.org> wrote:
>> > The X and Y values have two more significant bits in the same byte
>> > that contains click status.  Include these in the reported value.
>> > Thanks to Iain Hibbert of NetBSD for pointing this out.
>> >
>> > Signed-off-by: Michael Poole <mdpoole@troilus.org>
>> > ---
>> >  drivers/hid/hid-magicmouse.c |    4 ++--
>> >  1 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
>> > index 0b89c1c..7cdda23 100644
>> > --- a/drivers/hid/hid-magicmouse.c
>> > +++ b/drivers/hid/hid-magicmouse.c
>> > @@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>> >                 * to have the current touch information before
>> >                 * generating a click event.
>> >                 */
>> > -               x = (signed char)data[1];
>> > -               y = (signed char)data[2];
>> > +               x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
>> > +               y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
>> 
>> Will the following give us the same result?
>> 
>>  +               x = (int)(((data[3] & 0x0c) << 6) | data[1]);
>>  +               y = (int)(((data[3] & 0x30) << 4) | data[2]);
>
> I thought about this too, but there's a sign extension issue. If the X
> coordinate is -1 (these are relative coordinates), then you will end up
> with (int)(1023), which is no longer -1.
>
> When you shift to the right, the bits are sign-extended. Thus, shifting
> everything to the most significant bit of an integer, and then shifting
> everything back to the least significant will properly sign extend.
>
> (I actually wrote a test program just to verify this)
>
> That's just a reason for the shifts. There very well could be a more
> elegant solution.

C99 says that the result of right-shifting a negative value is
compiler-defined.  gcc documents that it ensures sign extension.  Other
parts of hid-magicmouse.c use this idiom already.  The corresponding
idiom in hid-core.c (see the snto32() function) would look something
like this:

      x = ((data[3] & 0x0c) << 6) | data[1];
      x |= (x & (1 << 9)) ? (-1 << 10) : 0;

Which do people find more readable?  Is the more portable behavior of
this idiom preferred over requiring sign extension by the compiler?

Michael Poole