HID: wacom: Fix generic multitouch device handling
Subsystems (2)
cover HID: wacom: Fix generic multitouch device handling
This set of patches addresses the problems that I had getting Benjamin's "HID generic handling" patches. Each of the patches addresses a seprate issue I encountered:
* Patch 1/3: The devices that I'm using have some usages outside of a logical or physical collection container which prevents the reports from being sent. I'm not sure if keying on the application collection is ideal, but it does do the job.
* Patch 2/3: The devices that I'm using put their X and Y usages after the finger count, which prevents the (compatibility) ABS_X and ABS_Y axes from being initialized. This patch just guarantees that they'll be initialized.
* Patch 3/3: Benjamin's original code seemed to only report data about the last finger in a multitouch report since information about "earlier" fingers would be overwritten. This patch changes the logic so that we immediately report field values whenever we can (there are some cases that we can't -- the tip/range bits appear before the contact identifier, so we have to store their state for a short time).
PATCH 1/3 HID: wacom: Consult the application usage when determining field type
It is not necessarily sufficient to look only at the physical and logical usages when determining if a field is for the pen or touch. Some fields are not contained in a sub-collection and thus only have an application usage. Not checking the application usage in such cases causes us to ignore the field entirely, which may lead to incorrect behavior.
--- drivers/hid/wacom_wac.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/hid/wacom_wac.c +4 -2
@@ -1484,9 +1484,11 @@ static void wacom_wac_finger_report(struct hid_device *hdev, }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \ - ((f)->physical == HID_DG_STYLUS)) + ((f)->physical == HID_DG_STYLUS) || \ + ((f)->application == HID_DG_PEN))#define WACOM_FINGER_FIELD (f) (((f)->logical == HID_DG_FINGER) || \ - ((f)->physical == HID_DG_FINGER)) + ((f)->physical == HID_DG_FINGER) || \ + ((f)->application == HID_DG_TOUCHSCREEN))void wacom_wac_usage_mapping (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage)
Signature · 1 line
2.1.3
Re: [PATCH 1/3] HID: wacom: Consult the application usage when determining field type
Quoted 8 lines · Jason Gerecke
It is not necessarily sufficient to look only at the physical and logical usages when determining if a field is for the pen or touch. Some fields are not contained in a sub-collection and thus only have an application usage. Not checking the application usage in such cases causes us to ignore the field entirely, which may lead to incorrect behavior.
Signed-off-by Jason Gerecke <[email protected]>---
This one is Rev-by: Benjamin Tissoires <[email protected]>
Cheers,
Benjamin
Quoted 24 lines
drivers/hid/wacom_wac.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/hid/wacom_wac.c +4
-3@@ -1484,9 +1484,11 @@ static void wacom_wac_finger_report(struct hid_device *hdev, }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \ - ((f)->physical == HID_DG_STYLUS)) + ((f)->physical == HID_DG_STYLUS) || \ + ((f)->application == HID_DG_PEN))#define WACOM_FINGER_FIELD (f) (((f)->logical == HID_DG_FINGER) || \ - ((f)->physical == HID_DG_FINGER)) + ((f)->physical == HID_DG_FINGER) || \ + ((f)->application == HID_DG_TOUCHSCREEN))void wacom_wac_usage_mapping (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage) --2.1.3
Jiri Kosina
AppliedRe: [PATCH 1/3] HID: wacom: Consult the application usage when determining field type
In reply to Jason GereckeQuoted 7 lines · Jason Gerecke
It is not necessarily sufficient to look only at the physical and logical usages when determining if a field is for the pen or touch. Some fields are not contained in a sub-collection and thus only have an application usage. Not checking the application usage in such cases causes us to ignore the field entirely, which may lead to incorrect behavior.
Signed-off-by Jason Gerecke <[email protected]>
Applied.
Signature · 2 lines
Jiri Kosina SUSE Labs
PATCH 2/3 HID: wacom: Manually declare ABS_{X,Y} for pointer emulation
In reply to Jason GereckeIf a HID descriptor places HID_DG_CONTACTID before HID_DG_X and HID_DG_Y then the ABS_X and ABS_Y will not be automatically initialized by the call to input_mt_init_slots. Here we move the setup of those axes outside of the 'if' statement so that they're always present if the associated HID field is.
--- drivers/hid/wacom_wac.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/hid/wacom_wac.c +4 -6
@@ -1382,16 +1382,14 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,switch (usage->hid) {case HID_GD_X : -if (touch_max ==1 ) -wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 ); -else +wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 ); +if (touch_max >1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_MT_POSITION_X ,4 );break ;case HID_GD_Y : -if (touch_max ==1 ) -wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_Y ,4 ); -else +wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_Y ,4 ); +if (touch_max >1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_MT_POSITION_Y ,4 );break ;
Signature · 1 line
2.1.3
Re: [PATCH 2/3] HID: wacom: Manually declare ABS_{X,Y} for pointer emulation
Quoted 5 lines · Jason Gerecke
If a HID descriptor places
HID_DG_CONTACTIDbeforeHID_DG_XandHID_DG_Ythen theABS_XandABS_Ywill not be automatically initialized by the call toinput_mt_init_slots. Here we move the setup of those axes outside of the 'if' statement so that they're always present if the associated HID field is.
No, I am afraid, that this does not work. The solution is to call input_mt_init_slot once the mapping has been done.
I am complaining here because input_mt_init_slot will set the fuzz to 0 for ABS_X and 4 for ABS_MT_POSITION_X. If both have a fuzz of 4, there will be a double de-fuzzing while sending out the single touch emulation.
Likewise, the pressure should be initialized with ABS_MT_PRESSURE only (not implemented right now, I know).
At the end of wacom_parse_hid in wacom_sys, there should be a call to a function which would finalize the initialization. And then, input_mt_init_slot will be called after ABS_MT_*, no matter what the order of the fields is.
Cheers,
Benjamin
Quoted 34 lines
Signed-off-by Jason Gerecke <[email protected]>--- drivers/hid/wacom_wac.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)drivers/hid/wacom_wac.c +4
-7@@ -1382,16 +1382,14 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,switch (usage->hid) {case HID_GD_X : -if (touch_max ==1 ) -wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 ); -else +wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 ); +if (touch_max >1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_MT_POSITION_X ,4 );break ;case HID_GD_Y : -if (touch_max ==1 ) -wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_Y ,4 ); -else +wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_Y ,4 ); +if (touch_max >1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_MT_POSITION_Y ,4 );break ; --2.1.3
PATCH 3/3 HID: wacom: Report input events immediately upon receipt
In reply to Jason GereckeMultitouch tablets cannot work properly if wacom_wac_finger_event simply stores the event details, since details about former fingers will be overwritten by later ones (causing wacom_wac_finger_report to effectively only report the state of the *last* finger in the packet).
This patch modifies the logic so that events are generated as soon as possible in response to events. We do temporarily store HID_DG_TIPSWITCH value since the value of HID_DG_CONTACTID is (at least in for the tablets I've tested) not known until shortly afterwards.
--- drivers/hid/wacom_wac.c | 77 ++++++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 33 deletions(-)
drivers/hid/wacom_wac.c +44 -33
@@ -1412,19 +1412,40 @@ static int wacom_wac_finger_event(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct hid_data *data = &wacom_wac->hid_data; +unsigned mt = wacom_wac->features.touch_max >1 ; +struct input_dev *input = wacom_wac->input; +bool prox = data->tipswitch && !wacom_wac->shared->stylus_in_proximity; +int slot;switch (usage->hid) {case HID_GD_X : - wacom_wac->hid_data.x = value; +if (prox) { +input_report_abs (input, + mt ?ABS_MT_POSITION_X :ABS_X , + value); + }break ;case HID_GD_Y : - wacom_wac->hid_data.y = value; +if (prox) { +input_report_abs (input, + mt ?ABS_MT_POSITION_Y :ABS_Y , + value); + }break ;case HID_DG_CONTACTID : - wacom_wac->hid_data.id = value; + slot =input_mt_get_slot_by_key (input, value); + +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox);break ;case HID_DG_TIPSWITCH : - wacom_wac->hid_data.tipswitch = value; + data->tipswitch = value; +if (!mt) { + prox = data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; +input_report_key (input,BTN_TOUCH , prox); + }break ; }
@@ -1432,33 +1453,26 @@ static int wacom_wac_finger_event(struct hid_device *hdev,return 0 ; } -static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_touches (struct hid_device *hdev) { -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; - - slot =input_mt_get_slot_by_key (input, hid_data->id); - -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); - } -input_mt_sync_frame (input); -} +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; +if (touch_max ==1 ) +return (wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity); -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,
@@ -1467,18 +1481,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity;unsigned touch_max = wacom_wac->features.touch_max;if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); -else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +input_mt_sync_frame (input); +input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \
Signature · 1 line
2.1.3
Re: [PATCH 3/3] HID: wacom: Report input events immediately upon receipt
Hi Jason,
Quoted 10 lines · Jason Gerecke
Multitouch tablets cannot work properly if
wacom_wac_finger_eventsimply stores the event details, since details about former fingers will be overwritten by later ones (causingwacom_wac_finger_reportto effectively only report the state of the *last* finger in the packet).This patch modifies the logic so that events are generated as soon as possible in response to events. We do temporarily store
HID_DG_TIPSWITCHvalue since the value ofHID_DG_CONTACTIDis (at least in for the tablets I've tested) not known until shortly afterwards.
OK, so, I don't like this one either :-(
I perfectly understand that the previous code worked in the only case where there is only one contact per report. But this patch assumes that the contact ID will come right after the tip switch if I understood correctly. This assumption may strike back in the future (like mine did), so we should be smarter.
In hid-multitouch, we count the total number of mt fields, then divide by the number of time we see ContactID. This gives us the number of fields per touch, and we then have a hook called when we reach this number of fields while processing the report.
It's not perfect, but it's generic.
If you have a better idea, I am all ears, but unless the FW guys can give us some guarantees regarding the HID descriptor*, I think we should go for the most generic possible solution.
BTW, what I dislike here is that I am pretty sure that if we take this one, we will have to revert part of it to have a 2-steps processing. Relying on the order of the hid fields is simply not possible in the long term.
Cheers,
Benjamin
* even if they give some guarantees, I am not sure I will trust them :)
Quoted 129 lines
Signed-off-by Jason Gerecke <[email protected]>--- drivers/hid/wacom_wac.c | 77 ++++++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 33 deletions(-)drivers/hid/wacom_wac.c +44
-34@@ -1412,19 +1412,40 @@ static int wacom_wac_finger_event(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct hid_data *data = &wacom_wac->hid_data; +unsigned mt = wacom_wac->features.touch_max >1 ; +struct input_dev *input = wacom_wac->input; +bool prox = data->tipswitch && !wacom_wac->shared->stylus_in_proximity; +int slot;switch (usage->hid) {case HID_GD_X : - wacom_wac->hid_data.x = value; +if (prox) { +input_report_abs (input, + mt ?ABS_MT_POSITION_X :ABS_X , + value); + }break ;case HID_GD_Y : - wacom_wac->hid_data.y = value; +if (prox) { +input_report_abs (input, + mt ?ABS_MT_POSITION_Y :ABS_Y , + value); + }break ;case HID_DG_CONTACTID : - wacom_wac->hid_data.id = value; + slot =input_mt_get_slot_by_key (input, value); + +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox);break ;case HID_DG_TIPSWITCH : - wacom_wac->hid_data.tipswitch = value; + data->tipswitch = value; +if (!mt) { + prox = data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; +input_report_key (input,BTN_TOUCH , prox); + }break ; }@@ -1432,33 +1453,26 @@ static int wacom_wac_finger_event(struct hid_device *hdev,return 0 ; } -static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_touches (struct hid_device *hdev) { -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; - - slot =input_mt_get_slot_by_key (input, hid_data->id); - -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); - } -input_mt_sync_frame (input); -} +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; +if (touch_max ==1 ) +return (wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity); -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,@@ -1467,18 +1481,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity;unsigned touch_max = wacom_wac->features.touch_max;if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); -else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +input_mt_sync_frame (input); +input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \ --2.1.3