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
_______________________________________________
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
