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

[Andreas Pokorny]

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]

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?


> [...]
> > +                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?


> [...]
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150529/19ec728b/attachment-0001.html>