hid-magicmouse: Correct parsing of large X and Y motions.
PATCH 1/1 hid-magicmouse: Correct parsing of large X and Y motions.
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.
--- drivers/hid/hid-magicmouse.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/hid/hid-magicmouse.c +2 -2
@@ -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 ; clicks = data[3 ];break ;case 0x20 :/* Theoretically battery status (0-100), but I have
Signature · 1 line
1.7.1
Quoted 14 lines · Michael Poole
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 <[email protected]>--- drivers/hid/hid-magicmouse.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/hid/hid-magicmouse.c +0
-0
drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device *hdev, · line 274
Quoted 10 lines
drivers/hid/hid-magicmouse.c +2
-2@@ -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 ; clicks = data[3 ];break ;case 0x20 :/* Theoretically battery status (0-100), but I have
Looks right to me.
-- Chase
Quoted 28 lines · Chase Douglas
Quoted 14 lines · Michael Poole
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 <[email protected]>--- drivers/hid/hid-magicmouse.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/hid/hid-magicmouse.c +0
-0drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device *hdev, · line 274
Quoted 10 lines
drivers/hid/hid-magicmouse.c +2
-2@@ -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 ; clicks = data[3 ];break ;case 0x20 :/* Theoretically battery status (0-100), but I have Looks right to me.
Quoted 1 line
Acked-by Chase Douglas <[email protected]>
Thanks. Applied to magicmouse branch now.
Signature · 2 lines
Jiri Kosina SUSE Labs, Novell Inc.
Quoted 14 lines · Michael Poole
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 <[email protected]>---
drivers/hid/hid-magicmouse.c | 4 ++--1 files changed, 2 insertions(+), 2 deletions(-)drivers/hid/hid-magicmouse.c +0
-0
drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device *hdev, · line 271
Quoted 7 lines
drivers/hid/hid-magicmouse.c +2
-2@@ -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]);
Ping
Quoted 10 lines
drivers/hid/hid-magicmouse.c +0
-0clicks = data[3]; break;case
0x20: /* Theoretically battery status (0-100), but I have --
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 25 lines · Ping Cheng
Quoted 14 lines · Michael Poole
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 <[email protected]>--- drivers/hid/hid-magicmouse.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/hid/hid-magicmouse.c +0
-0drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device *hdev, · line 271
Quoted 7 lines
drivers/hid/hid-magicmouse.c +2
-2@@ -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?
Quoted 2 lines
+ 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.
-- Chase
Quoted 33 lines · Chase Douglas
Quoted 25 lines · Ping Cheng
Quoted 14 lines · Michael Poole
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 <[email protected]>--- drivers/hid/hid-magicmouse.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/hid/hid-magicmouse.c +0
-0drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device *hdev, · line 271
Quoted 7 lines
drivers/hid/hid-magicmouse.c +2
-2@@ -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?
Quoted 2 lines
+ 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.
Quoted 8 lines
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
Quoted 8 lines · Michael Poole
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.cuse this idiom already. The corresponding idiom inhid-core.c(see thesnto32()function) would look something like this:x = ((data[3] & 0x0c) << 6) | data[1]; x |= (x & (1 << 9)) ? (-1 << 10) : 0;
snto32() seems like something we should be using in hid-magicmouse.c? On further thought, it actually seems like something that should be a macro in linux/kernel.h. I would think there could be utility for it in many places of the kernel.
-- Chase
Quoted 7 lines · Chase Douglas
Quoted 6 lines · Michael Poole
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.cuse this idiom already. The corresponding idiom inhid-core.c(see thesnto32()function) would look something like this:
Quoted 7 lines
Quoted 2 lines
x = ((data[3] & 0x0c) << 6) | data[1]; x |= (x & (1 << 9)) ? (-1 << 10) : 0;
snto32()seems like something we should be using inhid-magicmouse.c? On further thought, it actually seems like something that should be a macro inlinux/kernel.h. I would think there could be utility for it in many places of the kernel.
Probably so. The patch below is a proof of concept, which probably is not worth including until we decide the right place to put snto32(), but I think does show that using snto32() is more readable than the current code.
Note the bitwise-and for the ABS_MT_POSITION_X value: I missed that the first time I tried this, to rather undesirable effect. I am not sure whether snto32() should automatically do that kind of masking: It is only needed in one out of the four sites here, but it is a fairly easy mistake to make.
Michael Poole
drivers/hid/hid-magicmouse.c +16 -4
@@ -163,14 +163,26 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) msc->scroll_accel =SCROLL_ACCEL_DEFAULT ; } +/* Function shamelessly borrowed from hid-core.c. */ + +static s32 snto32 (__u32 value,unsigned n) +{ +switch (n) { +case 8 :return ((__s8 )value); +case 16 :return ((__s16 )value); +case 32 :return ((__s32 )value); + } +return value & (1 << (n -1 )) ? value | (-1 << n) : value; +} +static void magicmouse_emit_touch (struct magicmouse_sc *msc,int raw_id,u8 *tdata) {struct input_dev *input = msc->input;__s32 x_y = tdata[0 ] <<8 | tdata[1 ] <<16 | tdata[2 ] <<24 ;int misc = tdata[5 ] | tdata[6 ] <<8 ;int id = (misc >>6 ) &15 ; -int x = x_y <<12 >>20 ; -int y = -(x_y >>20 ); +int x =snto32 ((x_y >>8 ) &4095 ,12 ); +int y =snto32 (x_y >>20 ,12 );int down = (tdata[7 ] &TOUCH_STATE_MASK ) !=TOUCH_STATE_NONE ;/* Store tracking ID and other fields. */
@@ -285,8 +297,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,* to have the current touch information before * generating a click event. */ - x = (int )(((data[3 ] &0x0c ) <<28 ) | (data[1 ] <<22 )) >>22 ; - y = (int )(((data[3 ] &0x30 ) <<26 ) | (data[2 ] <<22 )) >>22 ; + x =snto32 (((data[3 ] &0x0c ) <<6 ) | data[1 ],10 ); + y =snto32 (((data[3 ] &0x30 ) <<4 ) | data[2 ],10 ); clicks = data[3 ];break ;case 0x20 :/* Theoretically battery status (0-100), but I have
Quoted 40 lines · Michael Poole
Quoted 4 lines · Chase Douglas
snto32()seems like something we should be using inhid-magicmouse.c? On further thought, it actually seems like something that should be a macro inlinux/kernel.h. I would think there could be utility for it in many places of the kernel.Probably so. The patch below is a proof of concept, which probably is not worth including until we decide the right place to put
snto32(), but I think does show that usingsnto32()is more readable than the current code.Note the bitwise-and for the
ABS_MT_POSITION_Xvalue: I missed that the first time I tried this, to rather undesirable effect. I am not sure whethersnto32()should automatically do that kind of masking: It is only needed in one out of the four sites here, but it is a fairly easy mistake to make.Michael Poole
drivers/hid/hid-magicmouse.c +12
-0@@ -163,14 +163,26 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) msc->scroll_accel =SCROLL_ACCEL_DEFAULT ; } +/* Function shamelessly borrowed from hid-core.c. */ + +static s32 snto32 (__u32 value,unsigned n) +{ +switch (n) { +case 8 :return ((__s8 )value); +case 16 :return ((__s16 )value); +case 32 :return ((__s32 )value); + } +return value & (1 << (n -1 )) ? value | (-1 << n) : value; +} +static void magicmouse_emit_touch (struct magicmouse_sc *msc,int raw_id,u8 *tdata) {struct input_dev *input = msc->input;
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 188
Quoted 10 lines
drivers/hid/hid-magicmouse.c +2
-2__s32 x_y = tdata[0 ] <<8 | tdata[1 ] <<16 | tdata[2 ] <<24 ;int misc = tdata[5 ] | tdata[6 ] <<8 ;int id = (misc >>6 ) &15 ; -int x = x_y <<12 >>20 ; -int y = -(x_y >>20 ); +int x =snto32 ((x_y >>8 ) &4095 ,12 ); +int y =snto32 (x_y >>20 ,12 );int down = (tdata[7 ] &TOUCH_STATE_MASK ) !=TOUCH_STATE_NONE ;/* Store tracking ID and other fields. */
Instead of having this intermediate x_y that's shifted 8 bits to the left, why not do:
int x = snto32((tdata[1] << 8 | tdata[0]) & 0xFFF, 12); int y = snto32(tdata[2] << 4 | tdata[1] >> 4, 12);
P.S.: I much prefer hex when doing masks because it's easier to count the bits.
-- Chase
Arsebiscuits, forced to use webmail, sloppily prefixing every line with '>' manually ...
________________________________________
From: [email protected] [[email protected]] On Behalf Of ext Michael Poole [[email protected]]
Sent: Tuesday, July 06, 2010 12:08 AM
To: Chase Douglas
Cc: Ping Cheng; Jiri Kosina; [email protected]
Subject: Re: [PATCH] hid-magicmouse: Correct parsing of large X and Y motions.
Quoted 34 lines
Quoted 7 lines · Chase Douglas
Quoted 6 lines · Michael Poole
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.cuse this idiom already. The corresponding idiom inhid-core.c(see thesnto32()function) would look something like this:Quoted 7 lines
Quoted 2 lines
x = ((data[3] & 0x0c) << 6) | data[1]; x |= (x & (1 << 9)) ? (-1 << 10) : 0;
snto32()seems like something we should be using inhid-magicmouse.c? On further thought, it actually seems like something that should be a macro inlinux/kernel.h. I would think there could be utility for it in many places of the kernel.Probably so. The patch below is a proof of concept, which probably is not worth including until we decide the right place to put
snto32(), but I think does show that usingsnto32()is more readable than the current code.Note the bitwise-and for the
ABS_MT_POSITION_Xvalue: I missed that the first time I tried this, to rather undesirable effect. I am not sure whethersnto32()should automatically do that kind of masking: It is only needed in one out of the four sites here, but it is a fairly easy mistake to make.Michael Poole
drivers/hid/hid-magicmouse.c +0
-0@@ -163,14 +163,26 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) msc->scroll_accel =SCROLL_ACCEL_DEFAULT ;
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 173
Quoted 10 lines
drivers/hid/hid-magicmouse.c +8
-0} +/* Function shamelessly borrowed from hid-core.c. */ + +static s32 snto32 (__u32 value,unsigned n) +{ +switch (n) { +case 8 :return ((__s8 )value); +case 16 :return ((__s16 )value); +case 32 :return ((__s32 )value);
Beware - this relies on sane behaviour from the current and future compilers, as the C standard doesn't mandate what should happen when shortening out of range signed values. [n869.txt: 6.3.1.1.#3]
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 175
Quoted 2 lines
drivers/hid/hid-magicmouse.c +2
-0+ } +return value & (1 << (n -1 )) ? value | (-1 << n) : value;
That signed left shift, however, is good old fashioned undefined behaviour. [n869.txt 6.5.7.#4]
Quoted 26 lines
drivers/hid/hid-magicmouse.c +6
-4+} +static void magicmouse_emit_touch (struct magicmouse_sc *msc,int raw_id,u8 *tdata) {struct input_dev *input = msc->input;__s32 x_y = tdata[0 ] <<8 | tdata[1 ] <<16 | tdata[2 ] <<24 ;int misc = tdata[5 ] | tdata[6 ] <<8 ;int id = (misc >>6 ) &15 ; -int x = x_y <<12 >>20 ; -int y = -(x_y >>20 ); +int x =snto32 ((x_y >>8 ) &4095 ,12 ); +int y =snto32 (x_y >>20 ,12 );int down = (tdata[7 ] &TOUCH_STATE_MASK ) !=TOUCH_STATE_NONE ;/* Store tracking ID and other fields. */ @@ -285,8 +297,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,* to have the current touch information before * generating a click event. */ - x = (int )(((data[3 ] &0x0c ) <<28 ) | (data[1 ] <<22 )) >>22 ; - y = (int )(((data[3 ] &0x30 ) <<26 ) | (data[2 ] <<22 )) >>22 ; + x =snto32 (((data[3 ] &0x0c ) <<6 ) | data[1 ],10 ); + y =snto32 (((data[3 ] &0x30 ) <<4 ) | data[2 ],10 ); clicks = data[3 ];break ;case 0x20 :/* Theoretically battery status (0-100), but I have
Quoted 64 lines
-----Original Message-----
From: [email protected] [mailto:linux-input- [email protected]] On Behalf Of [email protected]
Sent: Tuesday, July 06, 2010 4:53 PM
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH] hid-magicmouse: Correct parsing of large X and Y motions.Arsebiscuits, forced to use webmail, sloppily prefixing every line with '>' manually ...
________________________________________
From: [email protected] [linux-input- [email protected]] On Behalf Of ext Michael Poole [[email protected]]
Sent: Tuesday, July 06, 2010 12:08 AM
To: Chase Douglas
Cc: Ping Cheng; Jiri Kosina; [email protected]
Subject: Re: [PATCH] hid-magicmouse: Correct parsing of large X and Y motions.Quoted 5 lines
Quoted 3 lines · Chase Douglas
Quoted 2 lines · Michael Poole
C99 says that the result of right-shifting a negative value is compiler-defined. gcc documents that it ensures sign extension.
Other
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
parts of
hid-magicmouse.cuse this idiom already. The corresponding idiom inhid-core.c(see thesnto32()function) would look something like this:x = ((data[3] & 0x0c) << 6) | data[1]; x |= (x & (1 << 9)) ? (-1 << 10) : 0;
snto32()seems like something we should be using inhid-magicmouse.c?On
Quoted 1 line
Quoted 1 line
further thought, it actually seems like something that should be a
macro
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct
Quoted 19 lines
Quoted 2 lines
in
linux/kernel.h. I would think there could be utility for it in many places of the kernel.Probably so. The patch below is a proof of concept, which probably is not worth including until we decide the right place to put
snto32(), but I think does show that usingsnto32()is more readable than the current code.Note the bitwise-and for the
ABS_MT_POSITION_Xvalue: I missed that the first time I tried this, to rather undesirable effect. I am not sure whethersnto32()should automatically do that kind of masking: It is only needed in one out of the four sites here, but it is a fairly easy mistake to make.Michael Poole
drivers/hid/hid-magicmouse.c +0
-0@@ -163,14 +163,26 @@ static void magicmouse_emit_buttons(struct
magicmouse_sc*msc, int state)Quoted 3 lines
drivers/hid/hid-magicmouse.c +0
-0msc->scroll_accel =SCROLL_ACCEL_DEFAULT ; }
Quoted 12 lines
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 173
Quoted 8 lines
drivers/hid/hid-magicmouse.c +8
-0+/* Function shamelessly borrowed from hid-core.c. */ + +static s32 snto32 (__u32 value,unsigned n) +{ +switch (n) { +case 8 :return ((__s8 )value); +case 16 :return ((__s16 )value); +case 32 :return ((__s32 )value);Beware - this relies on sane behaviour from the current and future compilers, as the C standard doesn't mandate what should happen when shortening out of range signed values. [n869.txt: 6.3.1.1.#3]
Any reason of choosing __s32/ s32 ?
Quoted 41 lines
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 175
Quoted 3 lines
drivers/hid/hid-magicmouse.c +2
-0+ } +return value & (1 << (n -1 )) ? value | (-1 << n) : value;That signed left shift, however, is good old fashioned undefined behaviour. [n869.txt 6.5.7.#4]
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
Quoted 3 lines
drivers/hid/hid-magicmouse.c +2
-0+} + static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id,u8 *tdata)
drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device
Quoted 13 lines
drivers/hid/hid-magicmouse.c +2
-2{struct input_dev *input = msc->input;__s32 x_y = tdata[0 ] <<8 | tdata[1 ] <<16 | tdata[2 ] <<24 ;int misc = tdata[5 ] | tdata[6 ] <<8 ;int id = (misc >>6 ) &15 ; -int x = x_y <<12 >>20 ; -int y = -(x_y >>20 ); +int x =snto32 ((x_y >>8 ) &4095 ,12 ); +int y =snto32 (x_y >>20 ,12 );int down = (tdata[7 ] &TOUCH_STATE_MASK ) !=TOUCH_STATE_NONE ;/* Store tracking ID and other fields. */ @@ -285,8 +297,8 @@ static int magicmouse_raw_event(struct hid_device*hdev,
drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device *hdev,
Quoted 4 lines
drivers/hid/hid-magicmouse.c +0
-1* to have the current touch information before * generating a click event. */ - x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >>22;
drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device · line 289
Quoted 1 line
drivers/hid/hid-magicmouse.c +0
-1- y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >>22;
drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct hid_device *hdev, · line 304
Quoted 5 lines
drivers/hid/hid-magicmouse.c +2
-0+ x =snto32 (((data[3 ] &0x0c ) <<6 ) | data[1 ],10 ); + y =snto32 (((data[3 ] &0x30 ) <<4 ) | data[2 ],10 ); clicks = data[3 ];break ;case 0x20 :/* Theoretically battery status (0-100), but I have --
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[email protected] writes:
[snip]
Quoted 10 lines
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 173
Quoted 8 lines
drivers/hid/hid-magicmouse.c +8
-0+/* Function shamelessly borrowed from hid-core.c. */ + +static s32 snto32 (__u32 value,unsigned n) +{ +switch (n) { +case 8 :return ((__s8 )value); +case 16 :return ((__s16 )value); +case 32 :return ((__s32 )value);Beware - this relies on sane behaviour from the current and future compilers, as the C standard doesn't mandate what should happen when shortening out of range signed values. [n869.txt: 6.3.1.1.#3]
That's good to know. There's probably some much more complicated, but portable, way to write the operation -- I suspect we'll continue to rely on the compiler to be sane.
Quoted 4 lines
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 175
Quoted 2 lines
drivers/hid/hid-magicmouse.c +2
-0+ } +return value & (1 << (n -1 )) ? value | (-1 << n) : value;That signed left shift, however, is good old fashioned undefined behaviour. [n869.txt 6.5.7.#4]
Which version is that from? The copy of C99 I have says the resulting value is -1 * (2 ** n) if that value can be represented. (A 6 May 2005 committee draft of TC2 says the same thing.) Until n > 31, the behavior should be standard-defined.
Michael
From: ext Michael Poole [[email protected]]
Quoted 18 lines
[email protected] writes:
[snip]
Quoted 10 lines
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 173
Quoted 8 lines
drivers/hid/hid-magicmouse.c +8
-0+/* Function shamelessly borrowed from hid-core.c. */ + +static s32 snto32 (__u32 value,unsigned n) +{ +switch (n) { +case 8 :return ((__s8 )value); +case 16 :return ((__s16 )value); +case 32 :return ((__s32 )value);Beware - this relies on sane behaviour from the current and future compilers, as the C standard doesn't mandate what should happen when shortening out of range signed values. [n869.txt: 6.3.1.1.#3]
That's good to know. There's probably some much more complicated, but portable, way to write the operation -- I suspect we'll continue to rely on the compiler to be sane.
Quoted 6 lines
Quoted 4 lines
drivers/hid/hid-magicmouse.c static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state) · line 175
Quoted 2 lines
drivers/hid/hid-magicmouse.c +2
-0+ } +return value & (1 << (n -1 )) ? value | (-1 << n) : value;That signed left shift, however, is good old fashioned undefined behaviour. [n869.txt 6.5.7.#4]
Which version is that from?
A text version of n869, which looks like it suffered a little in conversion to not-quite-ASCII.
Quoted 4 lines
The copy of C99 I have says the resulting
value is -1 * (2 ** n) if that value can be represented. (A 6 May 2005 committee draft of TC2 says the same thing.) Until n > 31, the behavior should be standard-defined.
Looking at the pdfs, agreed, My mistake. I shall have to 'fix' the non-ASCII in my text version, as I much prefer greppable text to pdfs.
Phil
Quoted 28 lines · Ping Cheng
Quoted 14 lines · Michael Poole
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 <[email protected]>--- drivers/hid/hid-magicmouse.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/hid/hid-magicmouse.c +0
-0drivers/hid/hid-magicmouse.c static int magicmouse_raw_event(struct · line 274
Quoted 10 lines
drivers/hid/hid-magicmouse.c +2
-2@@ -267,8 +267,8 @@ static int magicmouse_raw_event(structhid_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?
Review
Quoted 2 lines
(quoted diff) +2
-0+ x = (int)(((data[3] & 0x0c) << 6) | data[1]); + y = (int)(((data[3] & 0x30) << 4) | data[2]);
I'd think you'd run into endianness issues.
Signature · 1 line
Dmitry