| Message ID | 20211022232837.18988-1-ping.cheng@wacom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: input: fix the incorrectly reported BTN_TOOL_RUBBER/PEN tools | expand |
Hi @Dmitry Torokhov and @Benjamin Tissoires, Do you have any comments about this patch? The issue and the logic behind the fix has been explained in the commit comment and in the code. Jiri is probably waiting for an acknowledgment from one of you... Thank you, Ping On Fri, Oct 22, 2021 at 4:29 PM Ping Cheng <pinglinux@gmail.com> wrote: > > The HID_QUIRK_INVERT caused BTN_TOOL_RUBBER events were reported at the > same time as events for BTN_TOOL_PEN/PENCIL/etc, if HID_QUIRK_INVERT > was set by a stylus' sideswitch. The reality is that a pen can only be > a stylus (writing/drawing) or an eraser, but not both at the same time. > This patch makes that logic correct. > > CC: stable@vger.kernel.org # 2.4+ > Signed-off-by: Ping Cheng <ping.cheng@wacom.com> > Reviewed-by: Jason Gerecke <killertofu@gmail.com> > Tested-by: Tatsunosuke Tobita <junkpainting@gmail.com> > --- > drivers/hid/hid-input.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 4b5ebeacd283..85741a2d828d 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -1344,12 +1344,28 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct > } > > if (usage->hid == HID_DG_INRANGE) { > + /* when HID_QUIRK_INVERT is set by a stylus sideswitch, HID_DG_INRANGE could be > + * for stylus or eraser. Make sure events are only posted to the current valid tool: > + * BTN_TOOL_RUBBER vs BTN_TOOL_PEN/BTN_TOOL_PENCIL/BTN_TOOL_BRUSH/etc since a pen > + * can not be used as a stylus (to draw/write) and an erasaer at the same time > + */ > + static unsigned int last_code = 0; > + unsigned int code = (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code; > if (value) { > - input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1); > - return; > + if (code != last_code) { > + /* send the last tool out before allow the new one in */ > + if (last_code) > + input_event(input, usage->type, last_code, 0); > + input_event(input, usage->type, code, 1); > + } > + last_code = code; > + } else { > + /* only send the last valid tool out */ > + if (last_code) > + input_event(input, usage->type, last_code, 0); > + /* reset tool for next cycle */ > + last_code = 0; > } > - input_event(input, usage->type, usage->code, 0); > - input_event(input, usage->type, BTN_TOOL_RUBBER, 0); > return; > } > > -- > 2.25.1 >
Ok, I know why no one follow-up with my patch. Life wasn't as simple as I wished for :). The patch missed one important fact: repeated events don't go into userland. They are filtered! We need to initiate another input_dev to hold all the past events for the coming tool, whether it is PEN or RUBBER. So, it is more like a simplified multi-pen case, right? What's your suggestions? Do we claim two tools for this particular use case? On Wed, Oct 27, 2021 at 12:06 PM Ping Cheng <pinglinux@gmail.com> wrote: > > Hi @Dmitry Torokhov and @Benjamin Tissoires, > > Do you have any comments about this patch? The issue and the logic > behind the fix has been explained in the commit comment and in the > code. Jiri is probably waiting for an acknowledgment from one of > you... > > Thank you, > Ping > > On Fri, Oct 22, 2021 at 4:29 PM Ping Cheng <pinglinux@gmail.com> wrote: > > > > The HID_QUIRK_INVERT caused BTN_TOOL_RUBBER events were reported at the > > same time as events for BTN_TOOL_PEN/PENCIL/etc, if HID_QUIRK_INVERT > > was set by a stylus' sideswitch. The reality is that a pen can only be > > a stylus (writing/drawing) or an eraser, but not both at the same time. > > This patch makes that logic correct. > > > > CC: stable@vger.kernel.org # 2.4+ > > Signed-off-by: Ping Cheng <ping.cheng@wacom.com> > > Reviewed-by: Jason Gerecke <killertofu@gmail.com> > > Tested-by: Tatsunosuke Tobita <junkpainting@gmail.com> > > --- > > drivers/hid/hid-input.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > > index 4b5ebeacd283..85741a2d828d 100644 > > --- a/drivers/hid/hid-input.c > > +++ b/drivers/hid/hid-input.c > > @@ -1344,12 +1344,28 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct > > } > > > > if (usage->hid == HID_DG_INRANGE) { > > + /* when HID_QUIRK_INVERT is set by a stylus sideswitch, HID_DG_INRANGE could be > > + * for stylus or eraser. Make sure events are only posted to the current valid tool: > > + * BTN_TOOL_RUBBER vs BTN_TOOL_PEN/BTN_TOOL_PENCIL/BTN_TOOL_BRUSH/etc since a pen > > + * can not be used as a stylus (to draw/write) and an erasaer at the same time > > + */ > > + static unsigned int last_code = 0; > > + unsigned int code = (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code; > > if (value) { > > - input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1); > > - return; > > + if (code != last_code) { > > + /* send the last tool out before allow the new one in */ > > + if (last_code) > > + input_event(input, usage->type, last_code, 0); > > + input_event(input, usage->type, code, 1); > > + } > > + last_code = code; > > + } else { > > + /* only send the last valid tool out */ > > + if (last_code) > > + input_event(input, usage->type, last_code, 0); > > + /* reset tool for next cycle */ > > + last_code = 0; > > } > > - input_event(input, usage->type, usage->code, 0); > > - input_event(input, usage->type, BTN_TOOL_RUBBER, 0); > > return; > > } > > > > -- > > 2.25.1 > >
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 4b5ebeacd283..85741a2d828d 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1344,12 +1344,28 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct } if (usage->hid == HID_DG_INRANGE) { + /* when HID_QUIRK_INVERT is set by a stylus sideswitch, HID_DG_INRANGE could be + * for stylus or eraser. Make sure events are only posted to the current valid tool: + * BTN_TOOL_RUBBER vs BTN_TOOL_PEN/BTN_TOOL_PENCIL/BTN_TOOL_BRUSH/etc since a pen + * can not be used as a stylus (to draw/write) and an erasaer at the same time + */ + static unsigned int last_code = 0; + unsigned int code = (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code; if (value) { - input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1); - return; + if (code != last_code) { + /* send the last tool out before allow the new one in */ + if (last_code) + input_event(input, usage->type, last_code, 0); + input_event(input, usage->type, code, 1); + } + last_code = code; + } else { + /* only send the last valid tool out */ + if (last_code) + input_event(input, usage->type, last_code, 0); + /* reset tool for next cycle */ + last_code = 0; } - input_event(input, usage->type, usage->code, 0); - input_event(input, usage->type, BTN_TOOL_RUBBER, 0); return; }