| Message ID | 1499626491-22592-2-git-send-email-floe@butterbrot.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jul 09, 2017 at 08:54:51PM +0200, Florian Echtler wrote: > The SUR40 labels all reported blobs as touch, token, or generic blob. > Previously, all blobs were reported as touch regardless of type, causing > lots of false positives. Present patch fixes this. > > Signed-off-by: Martin Kaltenbrunner <modin@yuri.at> > Signed-off-by: Florian Echtler <floe@butterbrot.org> > --- > drivers/input/touchscreen/sur40.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c > index 12bdee9..fbd4010 100644 > --- a/drivers/input/touchscreen/sur40.c > +++ b/drivers/input/touchscreen/sur40.c > @@ -309,6 +309,8 @@ static void sur40_report_blob(struct sur40_blob *blob, struct input_dev *input) > int slotnum = input_mt_get_slot_by_key(input, blob->blob_id); > if (slotnum < 0 || slotnum >= MAX_CONTACTS) > return; > + if (blob->type != SUR40_TOUCH) > + return; I think we should be checking blob type before trying to get slot number. > > input_mt_slot(input, slotnum); > input_mt_report_slot_state(input, MT_TOOL_FINGER, 1); > -- > 2.7.4 > Thanks.
Good point, I'll send an updated patch ASAP. Related question: we first attempted to label non-touch objects as MT_TOOL_PALM, but it looks like userspace (Xorg in particular) doesn't actually distinguish between MT_TOOL_* types; is that correct? Best, Florian On 09.07.2017 23:41, Dmitry Torokhov wrote: > On Sun, Jul 09, 2017 at 08:54:51PM +0200, Florian Echtler wrote: >> The SUR40 labels all reported blobs as touch, token, or generic blob. >> Previously, all blobs were reported as touch regardless of type, causing >> lots of false positives. Present patch fixes this. >> >> Signed-off-by: Martin Kaltenbrunner <modin@yuri.at> >> Signed-off-by: Florian Echtler <floe@butterbrot.org> >> --- >> drivers/input/touchscreen/sur40.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c >> index 12bdee9..fbd4010 100644 >> --- a/drivers/input/touchscreen/sur40.c >> +++ b/drivers/input/touchscreen/sur40.c >> @@ -309,6 +309,8 @@ static void sur40_report_blob(struct sur40_blob *blob, struct input_dev *input) >> int slotnum = input_mt_get_slot_by_key(input, blob->blob_id); >> if (slotnum < 0 || slotnum >= MAX_CONTACTS) >> return; >> + if (blob->type != SUR40_TOUCH) >> + return; > > I think we should be checking blob type before trying to get slot > number. > >> >> input_mt_slot(input, slotnum); >> input_mt_report_slot_state(input, MT_TOOL_FINGER, 1); >> -- >> 2.7.4 >> > > Thanks. >
On Mon, Jul 10, 2017 at 09:11:53AM +0200, Florian Echtler wrote: > Good point, I'll send an updated patch ASAP. > > Related question: we first attempted to label non-touch objects as MT_TOOL_PALM, > but it looks like userspace (Xorg in particular) doesn't actually distinguish > between MT_TOOL_* types; is that correct? It really should, but I think Peter never got around implementing this. Also, I think it is a good idea to set touch major to max in this case. I believe that that will help clients that do no understand MT_TOOL_PALM to still do palm rejection. Peter? > > Best, Florian > > On 09.07.2017 23:41, Dmitry Torokhov wrote: > > On Sun, Jul 09, 2017 at 08:54:51PM +0200, Florian Echtler wrote: > >> The SUR40 labels all reported blobs as touch, token, or generic blob. > >> Previously, all blobs were reported as touch regardless of type, causing > >> lots of false positives. Present patch fixes this. > >> > >> Signed-off-by: Martin Kaltenbrunner <modin@yuri.at> > >> Signed-off-by: Florian Echtler <floe@butterbrot.org> > >> --- > >> drivers/input/touchscreen/sur40.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c > >> index 12bdee9..fbd4010 100644 > >> --- a/drivers/input/touchscreen/sur40.c > >> +++ b/drivers/input/touchscreen/sur40.c > >> @@ -309,6 +309,8 @@ static void sur40_report_blob(struct sur40_blob *blob, struct input_dev *input) > >> int slotnum = input_mt_get_slot_by_key(input, blob->blob_id); > >> if (slotnum < 0 || slotnum >= MAX_CONTACTS) > >> return; > >> + if (blob->type != SUR40_TOUCH) > >> + return; > > > > I think we should be checking blob type before trying to get slot > > number. > > > >> > >> input_mt_slot(input, slotnum); > >> input_mt_report_slot_state(input, MT_TOOL_FINGER, 1); > >> -- > >> 2.7.4 > >> > > > > Thanks. > > > > > -- > SENT FROM MY DEC VT50 TERMINAL >
On Mon, 10 Jul 2017, Dmitry Torokhov wrote: > On Mon, Jul 10, 2017 at 09:11:53AM +0200, Florian Echtler wrote: >> >> Related question: we first attempted to label non-touch objects as MT_TOOL_PALM, >> but it looks like userspace (Xorg in particular) doesn't actually distinguish >> between MT_TOOL_* types; is that correct? > > It really should, but I think Peter never got around implementing this. > > Also, I think it is a good idea to set touch major to max in this case. > I believe that that will help clients that do no understand MT_TOOL_PALM > to still do palm rejection. > > Peter? Would you consider merging v2 of the patch regardless of the Xorg situation? Right now, it's a useful bugfix in any case, and we can deal with how to represent blobs/palms/tokens later on. Best, Florian
On Fri, Jul 14, 2017 at 09:54:03AM +0200, Florian Echtler wrote: > On Mon, 10 Jul 2017, Dmitry Torokhov wrote: > > On Mon, Jul 10, 2017 at 09:11:53AM +0200, Florian Echtler wrote: > > > > > > Related question: we first attempted to label non-touch objects as MT_TOOL_PALM, > > > but it looks like userspace (Xorg in particular) doesn't actually distinguish > > > between MT_TOOL_* types; is that correct? > > > > It really should, but I think Peter never got around implementing this. > > > > Also, I think it is a good idea to set touch major to max in this case. > > I believe that that will help clients that do no understand MT_TOOL_PALM > > to still do palm rejection. > > > > Peter? > > Would you consider merging v2 of the patch regardless of the Xorg situation? > Right now, it's a useful bugfix in any case, and we can deal with how to > represent blobs/palms/tokens later on. sorry about the delay, bit chaotic here. libinput 1.8 was released a week or so ago and it supports MT_TOOL_PALM, so consider userspace ready for that. I also have patches to use major/minor for palm detection where appropriate which will hit git master (my) tonight. That's the libinput situation sorted I think, don't expect synaptics to catch up with that though (unless someone ends up writing the patches). Cheers, Peter -- 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 14.07.2017 10:24, Peter Hutterer wrote: > On Fri, Jul 14, 2017 at 09:54:03AM +0200, Florian Echtler wrote: >> On Mon, 10 Jul 2017, Dmitry Torokhov wrote: >>> On Mon, Jul 10, 2017 at 09:11:53AM +0200, Florian Echtler wrote: >>>> >>>> Related question: we first attempted to label non-touch objects as MT_TOOL_PALM, >>>> but it looks like userspace (Xorg in particular) doesn't actually distinguish >>>> between MT_TOOL_* types; is that correct? >>> >>> It really should, but I think Peter never got around implementing this. >>> >>> Also, I think it is a good idea to set touch major to max in this case. >>> I believe that that will help clients that do no understand MT_TOOL_PALM >>> to still do palm rejection. >>> >>> Peter? >> >> Would you consider merging v2 of the patch regardless of the Xorg situation? >> Right now, it's a useful bugfix in any case, and we can deal with how to >> represent blobs/palms/tokens later on. > > sorry about the delay, bit chaotic here. libinput 1.8 was released a week or > so ago and it supports MT_TOOL_PALM, so consider userspace ready for that. I > also have patches to use major/minor for palm detection where appropriate > which will hit git master (my) tonight. Nevermind :-) Just to clarify, I can now set MT_TOOL_PALM on generic blob objects, and they will still be available in userspace "on request", but will not be considered as touch points? Do you know how "legacy" xserver-xorg-input-evdev will handle this case? And final question: the SUR40 also is able to identify specific patterns as tokens (so-called "bytetags", see https://github.com/floe/surface-2.0/blob/master/bytetag/bytetag.pdf ). What would be a sensible way to expose these to userspace, too? Add another MT_TOOL_TOKEN type? Thanks & best regards, Florian
On 14/07/2017 19:28 , Florian Echtler wrote: > On 14.07.2017 10:24, Peter Hutterer wrote: >> On Fri, Jul 14, 2017 at 09:54:03AM +0200, Florian Echtler wrote: >>> On Mon, 10 Jul 2017, Dmitry Torokhov wrote: >>>> On Mon, Jul 10, 2017 at 09:11:53AM +0200, Florian Echtler wrote: >>>>> >>>>> Related question: we first attempted to label non-touch objects as MT_TOOL_PALM, >>>>> but it looks like userspace (Xorg in particular) doesn't actually distinguish >>>>> between MT_TOOL_* types; is that correct? >>>> >>>> It really should, but I think Peter never got around implementing this. >>>> >>>> Also, I think it is a good idea to set touch major to max in this case. >>>> I believe that that will help clients that do no understand MT_TOOL_PALM >>>> to still do palm rejection. >>>> >>>> Peter? >>> >>> Would you consider merging v2 of the patch regardless of the Xorg situation? >>> Right now, it's a useful bugfix in any case, and we can deal with how to >>> represent blobs/palms/tokens later on. >> >> sorry about the delay, bit chaotic here. libinput 1.8 was released a week or >> so ago and it supports MT_TOOL_PALM, so consider userspace ready for that. I >> also have patches to use major/minor for palm detection where appropriate >> which will hit git master (my) tonight. > > Nevermind :-) Just to clarify, I can now set MT_TOOL_PALM on generic blob > objects, and they will still be available in userspace "on request", but will > not be considered as touch points? > > Do you know how "legacy" xserver-xorg-input-evdev will handle this case? sorry, I just went back to read the whole thread and realised this is a touchscreen, not a touchpad. Short answer: we don't have tool type handling in touchscreens because no-one asked for it yet. I can probably get this done by the end of next week. File a bug against libinput and assign it to me please. That's for libinput. evdev is in maintenance mode, it may be possible to add that bit but it largely depends on when the tool type is set. If it's set in the first event already that's trivial to add (I think). > And final question: the SUR40 also is able to identify specific patterns as > tokens (so-called "bytetags", see > https://github.com/floe/surface-2.0/blob/master/bytetag/bytetag.pdf ). What > would be a sensible way to expose these to userspace, too? Add another > MT_TOOL_TOKEN type? You'd have to add one for each token here, but that may not be too bad, beyond adding a lot of #defines. The tool type is the value field, so we have 32 bits before we run out. 64 tokens won't make a dent there. Maybe use MT_TOKEN_MIN and MT_TOKEN_MAX to keep the defines in check, though I'd recommend considering more specific naming and lots of documentation to detail what the tokens stand for. And consider whether we want to use something overly specific, userspace won't be happy if we have thirty different vendor-specific token ranges. Cheers, Peter -- 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/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c index 12bdee9..fbd4010 100644 --- a/drivers/input/touchscreen/sur40.c +++ b/drivers/input/touchscreen/sur40.c @@ -309,6 +309,8 @@ static void sur40_report_blob(struct sur40_blob *blob, struct input_dev *input) int slotnum = input_mt_get_slot_by_key(input, blob->blob_id); if (slotnum < 0 || slotnum >= MAX_CONTACTS) return; + if (blob->type != SUR40_TOUCH) + return; input_mt_slot(input, slotnum); input_mt_report_slot_state(input, MT_TOOL_FINGER, 1);