[systemd-devel] [PATCH v2] udev: input_id: fix detection of various touchscreens
Peter Hutterer
peter.hutterer at who-t.net
Thu May 28 17:07:48 PDT 2015
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.
> libinput would not treat those as input devices. Additionally there
> is an 'IS_DIRECT' property exposed by most touch screens, that would
> otherwise be detected as touch pads.
> ---
> src/udev/udev-builtin-input_id.c | 145 +++++++++++++++++++++------------------
> 1 file changed, 80 insertions(+), 65 deletions(-)
>
> diff --git a/src/udev/udev-builtin-input_id.c b/src/udev/udev-builtin-input_id.c
> index b14190e..a4cafa0 100644
> --- a/src/udev/udev-builtin-input_id.c
> +++ b/src/udev/udev-builtin-input_id.c
> @@ -133,79 +133,94 @@ static bool test_pointers(struct udev_device *dev,
> const unsigned long* bitmask_rel,
> const unsigned long* bitmask_props,
> bool test) {
> - int is_mouse = 0;
> - int is_touchpad = 0;
> - bool ret = false;
> -
> - if (test_bit(INPUT_PROP_ACCELEROMETER, bitmask_props)) {
> - udev_builtin_add_property(dev, test, "ID_INPUT_ACCELEROMETER", "1");
> - return true;
> - }
> -
> - if (!test_bit(EV_KEY, bitmask_ev)) {
> - if (test_bit(EV_ABS, bitmask_ev) &&
> - test_bit(ABS_X, bitmask_abs) &&
> - test_bit(ABS_Y, bitmask_abs) &&
> - test_bit(ABS_Z, bitmask_abs)) {
> - udev_builtin_add_property(dev, test, "ID_INPUT_ACCELEROMETER", "1");
> - ret = true;
> - }
> - return ret;
> - }
> -
> - if (test_bit(EV_ABS, bitmask_ev) &&
> - test_bit(ABS_X, bitmask_abs) && test_bit(ABS_Y, bitmask_abs)) {
> - if (test_bit(BTN_STYLUS, bitmask_key) || test_bit(BTN_TOOL_PEN, bitmask_key)) {
> - udev_builtin_add_property(dev, test, "ID_INPUT_TABLET", "1");
> - ret = true;
> - } else if (test_bit(BTN_TOOL_FINGER, bitmask_key) && !test_bit(BTN_TOOL_PEN, bitmask_key)) {
> - is_touchpad = 1;
> - } else if (test_bit(BTN_MOUSE, bitmask_key)) {
> + bool is_mouse = false;
> + bool is_touchpad = false;
> + bool is_direct = false;
> + bool has_coordinates = false;
> + bool has_mt_coordinates = false;
> + bool has_joystick_axes_or_buttons = false;
> + bool has_touch = false;
> + bool stylus_or_pen = false;
> + bool finger_but_no_pen = false;
> + bool has_mouse = false;
s/has_mouse/has_mouse_button/ or better has_button_left
> + bool is_touchscreen = false;
> + bool is_tablet = false;
> + bool is_joystick = false;
> + bool is_accelerometer = false;
> + bool is_pointing_stick= false;
> + bool has_3d_coordinates = false;
> + bool has_keys = false;
> +
> + has_keys = test_bit(EV_KEY, bitmask_ev);
> + has_touch = test_bit(BTN_TOUCH, bitmask_key);
> + has_mouse = test_bit(BTN_MOUSE, bitmask_key);
I'd prefer BTN_LEFT here, they're aliases anyway and LEFT is more
expressive.
> + has_coordinates = test_bit(ABS_X, bitmask_abs) && test_bit(ABS_Y, bitmask_abs);
s/has_abs_coordinates/
> + 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.
> + is_direct = test_bit(INPUT_PROP_DIRECT, bitmask_props);
> + is_accelerometer = test_bit(INPUT_PROP_ACCELEROMETER, bitmask_props);
> + is_pointing_stick = test_bit(INPUT_PROP_POINTING_STICK, bitmask_props);
> + stylus_or_pen = test_bit(BTN_STYLUS, bitmask_key) ||
> + test_bit(BTN_STYLUS2, bitmask_key) ||
> + test_bit(BTN_TOOL_PEN, bitmask_key);
> + finger_but_no_pen = test_bit(BTN_TOOL_FINGER, bitmask_key) &&
> + !test_bit(BTN_TOOL_PEN, bitmask_key);
> + /* joysticks don't necessarily have to have buttons; e. g.
s/have to have/have/
> + * rudders/pedals are joystick-like, but buttonless; they have
> + * other fancy axes */
> + has_joystick_axes_or_buttons = test_bit(BTN_TRIGGER, bitmask_key) ||
> + test_bit(BTN_A, bitmask_key) ||
> + test_bit(BTN_1, bitmask_key) ||
> + test_bit(ABS_RX, bitmask_abs) ||
> + test_bit(ABS_RY, bitmask_abs) ||
> + test_bit(ABS_RZ, bitmask_abs) ||
> + test_bit(ABS_THROTTLE, bitmask_abs) ||
> + test_bit(ABS_RUDDER, bitmask_abs) ||
> + test_bit(ABS_WHEEL, bitmask_abs) ||
> + test_bit(ABS_GAS, bitmask_abs) ||
> + test_bit(ABS_BRAKE, bitmask_abs);
> +
> + if (has_coordinates) {
> + if (stylus_or_pen)
> + is_tablet = true;
> + else if (!is_direct && finger_but_no_pen)
> + is_touchpad = true;
> + else if (has_mouse)
> /* This path is taken by VMware's USB mouse, which has
> * absolute axes, but no touch/pressure button. */
> - is_mouse = 1;
> - } else if (test_bit(BTN_TOUCH, bitmask_key)) {
> - udev_builtin_add_property(dev, test, "ID_INPUT_TOUCHSCREEN", "1");
> - ret = true;
> - /* joysticks don't necessarily have to have buttons; e. g.
> - * rudders/pedals are joystick-like, but buttonless; they have
> - * other fancy axes */
> - } else if (test_bit(BTN_TRIGGER, bitmask_key) ||
> - test_bit(BTN_A, bitmask_key) ||
> - test_bit(BTN_1, bitmask_key) ||
> - test_bit(ABS_RX, bitmask_abs) ||
> - test_bit(ABS_RY, bitmask_abs) ||
> - test_bit(ABS_RZ, bitmask_abs) ||
> - test_bit(ABS_THROTTLE, bitmask_abs) ||
> - test_bit(ABS_RUDDER, bitmask_abs) ||
> - test_bit(ABS_WHEEL, bitmask_abs) ||
> - test_bit(ABS_GAS, bitmask_abs) ||
> - test_bit(ABS_BRAKE, bitmask_abs)) {
> - udev_builtin_add_property(dev, test, "ID_INPUT_JOYSTICK", "1");
> - ret = true;
> - }
> + is_mouse = true;
> + else if (has_touch)
> + is_touchscreen = true;
> + else if (has_joystick_axes_or_buttons)
> + is_joystick= true;
> + 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".
> }
> + if (has_mt_coordinates && is_direct)
> + is_touchscreen = true;
>
> - if (test_bit(INPUT_PROP_POINTING_STICK, bitmask_props)) {
> - udev_builtin_add_property(dev, test, "ID_INPUT_POINTINGSTICK", "1");
> - ret = true;
> - }
> -
> - if (test_bit(EV_REL, bitmask_ev) &&
> - test_bit(REL_X, bitmask_rel) && test_bit(REL_Y, bitmask_rel) &&
> - test_bit(BTN_MOUSE, bitmask_key))
> - is_mouse = 1;
> + if (test_bit(EV_REL, bitmask_ev) && test_bit(REL_X, bitmask_rel) &&
> + test_bit(REL_Y, bitmask_rel) && has_mouse)
> + is_mouse = true;
why not test this above too and then have a
if (has_mouse_buttons && has_rel_xy)
is_mouse = true;
>
> - if (is_mouse) {
> + if (is_mouse)
> udev_builtin_add_property(dev, test, "ID_INPUT_MOUSE", "1");
> - ret = true;
> - }
> - if (is_touchpad) {
> + if (is_touchpad)
> udev_builtin_add_property(dev, test, "ID_INPUT_TOUCHPAD", "1");
> - ret = true;
> - }
> + if (is_touchscreen)
> + udev_builtin_add_property(dev, test, "ID_INPUT_TOUCHSCREEN", "1");
> + if (is_tablet)
> + udev_builtin_add_property(dev, test, "ID_INPUT_TABLET", "1");
> + if (is_joystick)
> + udev_builtin_add_property(dev, test, "ID_INPUT_JOYSTICK", "1");
> + if (is_accelerometer)
> + udev_builtin_add_property(dev, test, "ID_INPUT_ACCELEROMETER", "1");
> + if (is_pointing_stick)
> + udev_builtin_add_property(dev, test, "ID_INPUT_POINTINGSTICK", "1");
>
> - return ret;
> + return is_mouse || is_touchpad || is_touchscreen || is_tablet || is_joystick ||
> + is_accelerometer || is_pointing_stick;
> }
>
> /* key like devices */
> --
> 2.1.4
tbh, I'd like to see this patch split up. right now it's a bit
refactoring with minor behaviour changes and that makes it harder to track.
IMO the patches should be:
* rework everything without behaviour changes (includes the unconditional
accelerometer I mentioned, may include the BTN_MOUSE/BTN_LEFT change
because they don't change behaviour)
* patch fot the has_mt + is_direct, which is new behaviour
* patch for the ABS_MT_SLOT-1 behaviour I mentioned
all the other bits go into the first patch. also, please CC me on the next
version, this way I'll see it quicker.
Cheers,
Peter
More information about the systemd-devel
mailing list