[systemd-devel] [PATCH v2] udev: input_id: fix detection of various touchscreens

[Peter Hutterer]

On Fri, May 29, 2015 at 12:32:44PM +0200, Andreas Pokorny wrote:
> Hi,
> 
> On Fri, May 29, 2015 at 2:07 AM, Peter Hutterer <peter.hutterer at who-t.net>
> wrote:
> 
> > On Thu, May 28, 2015 at 02:47:35PM +0200, Andreas Pokorny wrote:
> > > There are touch screens that do not provide 'BTN_TOUCH' and hence no
> > > 'EV_KEY'. Previously those would not get any properties assigned and
> >
> > note: I consider this a kernel bug. From the kernel's documentation:
> > * BTN_TOUCH:
> >   BTN_TOUCH is used for touch contact. While an input tool is determined
> >   to be within meaningful physical contact, the value of this property must
> >   be set to 1.
> >
> 
> Yes and somewhat further down in the text:
> 
> [snip]
> Note: Historically a touch device with BTN_TOOL_FINGER and BTN_TOUCH was
> interpreted as a touchpad by userspace, while a similar device without
> BTN_TOOL_FINGER was interpreted as a touchscreen. For backwards
> compatibility
> with current userspace it is recommended to follow this distinction. In the
> future, this distinction will be deprecated and the device properties ioctl
> EVIOCGPROP, defined in linux/input.h, will be used to convey the device
> type.
> [snip]

hmm, I read this as a comment regarding BTN_TOOL_FINGER, not BTN_TOUCH. so
the property tells us direct/indirect but both still have BTN_TOUCH and a
touchscreen may have BTN_TOOL_FINGER (though not recommended).
 
> I try to ensure that we capture most devices with and without broken
> drivers as possible.
> With the various soc vendors it is hard to fix all kernel source providers.
> As far as I can
> see from the source code I counted three kernel drivers that do not emit
> BTN_TOUCH.
> But I believe I only have access to one of those devices. Then there are
> various other
> that have not been sent upstream. But all of those seemed to set the DIRECT
> property.
> 
> 
> [...]
> >
> > +        has_3d_coordinates = has_coordinates && test_bit(ABS_Z,
> > bitmask_abs);
> > > +        has_mt_coordinates = test_bit(ABS_MT_POSITION_X, bitmask_abs) &&
> > > +                test_bit(ABS_MT_POSITION_Y, bitmask_abs);
> >
> > pretty sure the second line must be aligned with the =, applies for the
> > others too
> >
> > as a follow-up: this test should be updated to check if ABS_MT_SLOT - 1
> > *and* ABS_MT_SLOT are set. If both are set, the device is *not* a
> > touchscreen.
> >
> 
> ACK to all of the above, but ABS_MT_SLOT - 1? I never saw that axis
> anywhere. Oh or is that a joystick device with so so many axis that it
> hits the touch pad/screen range of axis?

yep, there's a few devices that simply set all ABS bits (or just anything
north of ABS_MISC) and interpreting them as touchscreens has interesting
effects.

> > [...]
> > > +                else if (has_3d_coordinates && !has_keys)
> > > +                        is_accelerometer = true;
> >
> > this is a behaviour change: before anything with INPUT_PROP_ACCELEROMETER
> > was tagged as such, now some of these may be tagged as other devices.
> > specifically, the Wacom cintiq 27QHD remote would be tagged as tablet now.
> > BTN_STYLUS + ABS_X/Y/Z + KEY_PROG1/2/3 + INPUT_PROP_ACCELEROMETER
> >
> > I think you should reinstate the "if it says it's an accelerometer,
> > anything
> > else doesn't matter".
> >
> 
> The initial code was similar to
>    if (!keys) {
>           if(has_3d_coordinates) is_accelerometer = true
>           return false;
>    }
> I had to change that part for nexus4 and nexus10 devices, because there
> was no EV_KEY bit set.
> Hence test_pointers returned false. But yes I can reinstate a "if 3d axis +
> no buttons it is an accelerometer and anything else does not matter"...
> But then what about devices like the PS4 controller? As far as I can tell
> it has three rotational axis respective joystick axis and a touchpad. So I
> believe the device should have all three udev properties set?

does that come through as one kernel device?
tbh I don't consider a controller like this as touchpad, setting
ID_INPUT_JOYSTICK is enough. and anything that wants to deal with it can
then see the touchpad on the joystick but trying to use it as a normal
touchpad is IMO outside of the use-cases we should aim for.

Cheers,
   Peter

> > [...]
> > all the other bits go into the first patch. also, please CC me on the next
> > version, this way I'll see it quicker.
> >
> 
> Thank you for the review. I will make a patch series out of that, and send
> it again soon.
> 
> regards
> Andreas Pokorny