| Message ID | 1475636338-3779-6-git-send-email-roderick@gaikai.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote: > From: Roderick Colenbrander <roderick.colenbrander@sony.com> > > Read the touch history field in the HID descriptor and use this value > to determine how many touch events to read from the report. As part > of this patch, we did a first attempt of making the offset calculation > code less magical. > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com> > --- > drivers/hid/hid-sony.c | 76 +++++++++++++++++++++++++++++++++----------------- > 1 file changed, 50 insertions(+), 26 deletions(-) > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index 24f7d19..2387aaf 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -1030,6 +1030,12 @@ struct motion_output_report_02 { > #define SIXAXIS_REPORT_0xF5_SIZE 8 > #define MOTION_REPORT_0x02_SIZE 49 > > +/* Offsets relative to USB input report (0x1). Bluetooth (0x11) requires an > + * additional +2. > + */ > +#define DS4_INPUT_REPORT_BATTERY_OFFSET 30 > +#define DS4_INPUT_REPORT_TOUCHPAD_OFFSET 33 > + > static DEFINE_SPINLOCK(sony_dev_list_lock); > static LIST_HEAD(sony_device_list); > static DEFINE_IDA(sony_device_id_allocator); > @@ -1226,19 +1232,17 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size) > struct hid_input, list); > struct input_dev *input_dev = hidinput->input; > unsigned long flags; > - int n, offset; > + int n, m, offset, num_touch_data, max_touch_data; > u8 cable_state, battery_capacity, battery_charging; > > - /* > - * Battery and touchpad data starts at byte 30 in the USB report and > - * 32 in Bluetooth report. > - */ > - offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 30 : 32; > + /* When using Bluetooth the header is 2 bytes longer, so skip these. */ > + int data_offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 0 : 2; > > /* > - * The lower 4 bits of byte 30 contain the battery level > + * The lower 4 bits of byte 30 (or 32 for BT) contain the battery level > * and the 5th bit contains the USB cable state. > */ > + offset = data_offset + DS4_INPUT_REPORT_BATTERY_OFFSET; > cable_state = (rd[offset] >> 4) & 0x01; > battery_capacity = rd[offset] & 0x0F; > > @@ -1265,30 +1269,50 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size) > sc->battery_charging = battery_charging; > spin_unlock_irqrestore(&sc->lock, flags); > > - offset += 5; > - > /* > - * The Dualshock 4 multi-touch trackpad data starts at offset 35 on USB > - * and 37 on Bluetooth. > - * The first 7 bits of the first byte is a counter and bit 8 is a touch > - * indicator that is 0 when pressed and 1 when not pressed. > - * The next 3 bytes are two 12 bit touch coordinates, X and Y. > - * The data for the second touch is in the same format and immediatly > - * follows the data for the first. > + * The Dualshock 4 multi-touch trackpad data starts at offset 33 on USB > + * and 35 on Bluetooth. > + * The first byte indicates the number of touch data in the report. > + * Trackpad data starts 2 bytes later (e.g. 35 for USB). > */ > - for (n = 0; n < 2; n++) { > - u16 x, y; > + offset = data_offset + DS4_INPUT_REPORT_TOUCHPAD_OFFSET; > + max_touch_data = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 3 : 4; > + if (rd[offset] > 0 && rd[offset] <= max_touch_data) > + num_touch_data = rd[offset]; > + else > + num_touch_data = 1; > + offset += 1; > > - x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8); > - y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4); > + for (m = 0; m < num_touch_data; m++) { > + /* Skip past timestamp */ > + offset += 1; > > - input_mt_slot(input_dev, n); > - input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, > - !(rd[offset] >> 7)); > - input_report_abs(input_dev, ABS_MT_POSITION_X, x); > - input_report_abs(input_dev, ABS_MT_POSITION_Y, y); > + /* > + * The first 7 bits of the first byte is a counter and bit 8 is > + * a touch indicator that is 0 when pressed and 1 when not > + * pressed. > + * The next 3 bytes are two 12 bit touch coordinates, X and Y. > + * The data for the second touch is in the same format and > + * immediately follows the data for the first. > + */ > + for (n = 0; n < 2; n++) { > + u16 x, y; > + bool active; > + > + x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8); > + y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4); > + > + active = !(rd[offset] >> 7); > + input_mt_slot(input_dev, n); Just to be sure, the device reports 2 touches only, and the "num_touch_data" chunks are just the history of these 2 touches, the last chunk being the last known touches? If so, then why aren't you simply jumping to the last valid value? If not, then you probably need to report (n+m*2) slot, and change the input_mt_init_slot parameter as well. Cheers, Benjamin > + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, active); > > - offset += 4; > + if (active) { > + input_report_abs(input_dev, ABS_MT_POSITION_X, x); > + input_report_abs(input_dev, ABS_MT_POSITION_Y, y); > + } > + > + offset += 4; > + } > } > } > > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, October 5, 2016 2:35 am, Benjamin Tissoires wrote: > On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote: >> + /* >> + * The first 7 bits of the first byte is a counter and bit 8 is >> + * a touch indicator that is 0 when pressed and 1 when not >> + * pressed. >> + * The next 3 bytes are two 12 bit touch coordinates, X and Y. >> + * The data for the second touch is in the same format and >> + * immediately follows the data for the first. >> + */ >> + for (n = 0; n < 2; n++) { >> + u16 x, y; >> + bool active; >> + >> + x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8); >> + y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4); >> + >> + active = !(rd[offset] >> 7); >> + input_mt_slot(input_dev, n); >> > > Just to be sure, the device reports 2 touches only, and the > "num_touch_data" chunks are just the history of these 2 touches, the > last chunk being the last known touches? FYI - Community knowledge/understanding... http://www.psdevwiki.com/ps4/DS4-BT#HID_INPUT_reports Simon -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 5, 2016 at 8:29 AM, Simon Wood <simon@mungewell.org> wrote: > On Wed, October 5, 2016 2:35 am, Benjamin Tissoires wrote: >> On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote: > >>> + /* >>> + * The first 7 bits of the first byte is a counter and bit 8 is >>> + * a touch indicator that is 0 when pressed and 1 when not >>> + * pressed. >>> + * The next 3 bytes are two 12 bit touch coordinates, X and Y. >>> + * The data for the second touch is in the same format and >>> + * immediately follows the data for the first. >>> + */ >>> + for (n = 0; n < 2; n++) { >>> + u16 x, y; >>> + bool active; >>> + >>> + x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8); >>> + y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4); >>> + >>> + active = !(rd[offset] >> 7); >>> + input_mt_slot(input_dev, n); >>> >> >> Just to be sure, the device reports 2 touches only, and the >> "num_touch_data" chunks are just the history of these 2 touches, the >> last chunk being the last known touches? > > FYI - Community knowledge/understanding... > http://www.psdevwiki.com/ps4/DS4-BT#HID_INPUT_reports > > Simon > Hi Ben and Simon, Correct, the DS4 sends a touch history. The last element is the latest. An input report can contain multiple touch samples, because the device internally samples at a rate which is higher than at which it generates HID reports. On USB you can easily see multiple touch events and it is even easier on Bluetooth (especially if it is set to a low frequency). The extra history is important for gestures. Thanks, Roderick -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 05 2016 or thereabouts, Roderick Colenbrander wrote: > On Wed, Oct 5, 2016 at 8:29 AM, Simon Wood <simon@mungewell.org> wrote: > > On Wed, October 5, 2016 2:35 am, Benjamin Tissoires wrote: > >> On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote: > > > >>> + /* > >>> + * The first 7 bits of the first byte is a counter and bit 8 is > >>> + * a touch indicator that is 0 when pressed and 1 when not > >>> + * pressed. > >>> + * The next 3 bytes are two 12 bit touch coordinates, X and Y. > >>> + * The data for the second touch is in the same format and > >>> + * immediately follows the data for the first. > >>> + */ > >>> + for (n = 0; n < 2; n++) { > >>> + u16 x, y; > >>> + bool active; > >>> + > >>> + x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8); > >>> + y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4); > >>> + > >>> + active = !(rd[offset] >> 7); > >>> + input_mt_slot(input_dev, n); > >>> > >> > >> Just to be sure, the device reports 2 touches only, and the > >> "num_touch_data" chunks are just the history of these 2 touches, the > >> last chunk being the last known touches? > > > > FYI - Community knowledge/understanding... > > http://www.psdevwiki.com/ps4/DS4-BT#HID_INPUT_reports > > > > Simon > > > > Hi Ben and Simon, > > Correct, the DS4 sends a touch history. The last element is the > latest. An input report can contain multiple touch samples, because > the device internally samples at a rate which is higher than at which > it generates HID reports. On USB you can easily see multiple touch > events and it is even easier on Bluetooth (especially if it is set to > a low frequency). The extra history is important for gestures. OK, so if it's important, you need to actually send it by adding input_mt_sync_frame() and input_sync() calls. Otherwise, the values will just get mangled by the kernel in evdev and everything happens as if you just sent the last pair in the history. The first 4 patches are IMO mergeable, but this one will need a little bit more polish to actually forward the events. Cheers, Benjamin > > Thanks, > Roderick -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Correct, the DS4 sends a touch history. The last element is the > > latest. An input report can contain multiple touch samples, because > > the device internally samples at a rate which is higher than at which > > it generates HID reports. On USB you can easily see multiple touch > > events and it is even easier on Bluetooth (especially if it is set to > > a low frequency). The extra history is important for gestures. > > OK, so if it's important, you need to actually send it by adding > input_mt_sync_frame() and input_sync() calls. Otherwise, the values will > just get mangled by the kernel in evdev and everything happens as if you > just sent the last pair in the history. > > The first 4 patches are IMO mergeable, but this one will need a little > bit more polish to actually forward the events. > > Cheers, > Benjamin > > > > > Thanks, > > Roderick Hi Ben, A kind of follow-up to this patch feedback which made it into the final patch. The recommendation was to use input_mt_sync_frame, which was a good recommendation. There is one oversight, which I'm not sure how we need to handle. Basically input_mt_sync_frame triggers pointer emulation, which is a problem for the Dualshock 4 since it already provides an ABS_X / ABS_Y, so the pointer emulation affects the analog stick values. The hid-sony driver registers the touchpad through input_mt_init_slots with the flags parameter '0', which was intended not to request such emulation I think. I'm not too familiar with the other input code, but should input_mt_sync_frame gate pointer emulation based on INPUT_MT_POINTER / INPUT_MT_DIRECT flags or something like that? Thanks, Roderick -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roderick, [sorry for the late reply, I was on PTO until today] On Oct 18 2016 or thereabouts, Roderick Colenbrander wrote: > > > > > Correct, the DS4 sends a touch history. The last element is the > > > latest. An input report can contain multiple touch samples, because > > > the device internally samples at a rate which is higher than at which > > > it generates HID reports. On USB you can easily see multiple touch > > > events and it is even easier on Bluetooth (especially if it is set to > > > a low frequency). The extra history is important for gestures. > > > > OK, so if it's important, you need to actually send it by adding > > input_mt_sync_frame() and input_sync() calls. Otherwise, the values will > > just get mangled by the kernel in evdev and everything happens as if you > > just sent the last pair in the history. > > > > The first 4 patches are IMO mergeable, but this one will need a little > > bit more polish to actually forward the events. > > > > Cheers, > > Benjamin > > > > > > > > Thanks, > > > Roderick > > Hi Ben, > > A kind of follow-up to this patch feedback which made it into the > final patch. The recommendation was to use input_mt_sync_frame, which > was a good recommendation. There is one oversight, which I'm not sure > how we need to handle. Basically input_mt_sync_frame triggers pointer > emulation, which is a problem for the Dualshock 4 since it already > provides an ABS_X / ABS_Y, so the pointer emulation affects the analog > stick values. Ouch, sorry, I did not realize this :( > > The hid-sony driver registers the touchpad through input_mt_init_slots > with the flags parameter '0', which was intended not to request such > emulation I think. I'm not too familiar with the other input code, but > should input_mt_sync_frame gate pointer emulation based on > INPUT_MT_POINTER / INPUT_MT_DIRECT flags or something like that? I'd say so, though I need to check the other thread first. Cheers, Benjamin > > Thanks, > Roderick -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index 24f7d19..2387aaf 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -1030,6 +1030,12 @@ struct motion_output_report_02 { #define SIXAXIS_REPORT_0xF5_SIZE 8 #define MOTION_REPORT_0x02_SIZE 49 +/* Offsets relative to USB input report (0x1). Bluetooth (0x11) requires an + * additional +2. + */ +#define DS4_INPUT_REPORT_BATTERY_OFFSET 30 +#define DS4_INPUT_REPORT_TOUCHPAD_OFFSET 33 + static DEFINE_SPINLOCK(sony_dev_list_lock); static LIST_HEAD(sony_device_list); static DEFINE_IDA(sony_device_id_allocator); @@ -1226,19 +1232,17 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size) struct hid_input, list); struct input_dev *input_dev = hidinput->input; unsigned long flags; - int n, offset; + int n, m, offset, num_touch_data, max_touch_data; u8 cable_state, battery_capacity, battery_charging; - /* - * Battery and touchpad data starts at byte 30 in the USB report and - * 32 in Bluetooth report. - */ - offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 30 : 32; + /* When using Bluetooth the header is 2 bytes longer, so skip these. */ + int data_offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 0 : 2; /* - * The lower 4 bits of byte 30 contain the battery level + * The lower 4 bits of byte 30 (or 32 for BT) contain the battery level * and the 5th bit contains the USB cable state. */ + offset = data_offset + DS4_INPUT_REPORT_BATTERY_OFFSET; cable_state = (rd[offset] >> 4) & 0x01; battery_capacity = rd[offset] & 0x0F; @@ -1265,30 +1269,50 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size) sc->battery_charging = battery_charging; spin_unlock_irqrestore(&sc->lock, flags); - offset += 5; - /* - * The Dualshock 4 multi-touch trackpad data starts at offset 35 on USB - * and 37 on Bluetooth. - * The first 7 bits of the first byte is a counter and bit 8 is a touch - * indicator that is 0 when pressed and 1 when not pressed. - * The next 3 bytes are two 12 bit touch coordinates, X and Y. - * The data for the second touch is in the same format and immediatly - * follows the data for the first. + * The Dualshock 4 multi-touch trackpad data starts at offset 33 on USB + * and 35 on Bluetooth. + * The first byte indicates the number of touch data in the report. + * Trackpad data starts 2 bytes later (e.g. 35 for USB). */ - for (n = 0; n < 2; n++) { - u16 x, y; + offset = data_offset + DS4_INPUT_REPORT_TOUCHPAD_OFFSET; + max_touch_data = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 3 : 4; + if (rd[offset] > 0 && rd[offset] <= max_touch_data) + num_touch_data = rd[offset]; + else + num_touch_data = 1; + offset += 1; - x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8); - y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4); + for (m = 0; m < num_touch_data; m++) { + /* Skip past timestamp */ + offset += 1; - input_mt_slot(input_dev, n); - input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, - !(rd[offset] >> 7)); - input_report_abs(input_dev, ABS_MT_POSITION_X, x); - input_report_abs(input_dev, ABS_MT_POSITION_Y, y); + /* + * The first 7 bits of the first byte is a counter and bit 8 is + * a touch indicator that is 0 when pressed and 1 when not + * pressed. + * The next 3 bytes are two 12 bit touch coordinates, X and Y. + * The data for the second touch is in the same format and + * immediately follows the data for the first. + */ + for (n = 0; n < 2; n++) { + u16 x, y; + bool active; + + x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8); + y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4); + + active = !(rd[offset] >> 7); + input_mt_slot(input_dev, n); + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, active); - offset += 4; + if (active) { + input_report_abs(input_dev, ABS_MT_POSITION_X, x); + input_report_abs(input_dev, ABS_MT_POSITION_Y, y); + } + + offset += 4; + } } }