Enable mapping button events on keyboard #6099

hramrach · 2017-06-07T17:31:06Z

allow mapping BTN_* events on keyboard
detect pointer devices with non-first button (eg mice with only right and middle button)
detect mice without any axis as mice
detect device input capability based on (after) remapping in evdev hwdb

-> hid_mac can be replaced with evdev hwdb rule

fix command given in hwd help text which does not work correctly

keszybz · 2017-06-09T23:04:13Z

Meson update is missing. Looks good in general… @hramrach can you explain a bit more why this is needed (the first patch)?

hramrach · 2017-06-11T10:15:06Z

The kernel is capable of mapping a BTN_* event on a scancode but the hwdb syntax cannot represent that limiting the possible mappings. With this patch you can replace hid_mac (mac mouse emulatiion) legacy driver with hwdb mappings.

martinpitt

Seems okay, but did you actually test that this is sufficient? I'm not convinced that src/udev/udev-builtin-keyboard.c will accept and actually set BTN_* constants in maps without further changes:

                key = udev_list_entry_get_name(entry);
                if (startswith(key, "KEYBOARD_KEY_")) {

and there is no corresponding check for BTN_*.

Please fix the commit message:

Drop the period at the end of the short description and start with lower case
"Keyboards support only keys, not buttons." is not a description of the change, but the current state, which is confusing - that commit does not make keyboards support only keys. Just drop that sentence.

martinpitt · 2017-06-12T09:30:21Z

Makefile.am


 src/udev/keyboard-keys-from-name.gperf: src/udev/keyboard-keys-list.txt
-	$(AM_V_GEN)$(AWK) 'BEGIN{ print "struct key_name { const char* name; unsigned short id; };"; print "%null-strings"; print "%%";} { print tolower(substr($$1 ,5)) ", " $$1 }' < $< > $@
+	$(AM_V_GEN)$(AWK) 'BEGIN{ print "struct key_name { const char* name; unsigned short id; };"; print "%null-strings"; print "%%";} /^KEY_/{ print tolower(substr($$1 ,5)) ", " $$1 } !/^KEY_/{ print tolower($$1) ", " $$1 }' < $< > $@


So KEY_*gets that prefix dropped, but anything else keeps the BTN_ prefix. Not exactly consistent, but necessary for backwards compatibility, and dropping all prefixes is too prone to collisions.

Could change this to also accept key_ prefix. Since there is some fancy processing of the data this should probably not result in significant processing time or binary size increase.

hramrach · 2017-06-12T13:40:47Z

It is sufficient to map the buttons but for libinput to pick up the events input classes must be assigned based on the remapped events. The extra two commits should make that happen.

martinpitt

Please clarify the intent in the commit message of "fix detection of pads" to describe the change, not the status quo ("are not assigned any input class").

martinpitt · 2017-06-12T14:58:42Z

src/udev/udev-builtin-input_id.c

+        if (has_mouse_button)
+                if (has_rel_coordinates)
+                        is_mouse = true;
+                if (!has_abs_coordinates) /* exclude vmmouse and joysticks */


Did you mean to drop the ! here? I'd certainly define a "pad" to be a device with mouse buttons and absolute coordinates, not the absence of the latter.

And what is a device with buttons and no axis? Nothing? Mouse? Pad?

My pad has no axis, only buttons. Based on that I classify such devices as pads.

Ah, like a game controller, not a touchpad. Indeed we call the latter ID_INPUT_TOUCHPAD. Can you please clarify that in a comment, or maybe rename is_pad to is_game_controller or perhaps more precisely is_buttonsonly?

I am actually missing a { here

martinpitt · 2017-06-12T15:01:03Z

src/udev/udev-builtin-input_id.c


        if (is_pointing_stick)
                udev_builtin_add_property(dev, test, "ID_INPUT_POINTINGSTICK", "1");
        if (is_mouse)
                udev_builtin_add_property(dev, test, "ID_INPUT_MOUSE", "1");
+        if (is_pad)
+                udev_builtin_add_property(dev, test, "ID_INPUT_MOUSE", "1");


TBH I don't understand this. The net effect of this patch should be exactly the same as we have now, except for the case where there is a device without relative nor absolute axes: your patch would currently label it a a mouse (which sounds completely wrong), and with dropping the ! above it would behave it as right now (which then makes little sense). And the is_<device> really ought to coincide with the ID_INPUT_<DEVICE> variables, please don't mix up pads and mouses.

Unfortunately, if you classify something as tablet-pad libinput implies wacom-tablet-pad and if the device is not compatible with libwacom it will not work. Hence the mouse capability. I say that in the commit message.

hmm, the two if checks above have the same body, hence they should be combined as if (is_mouse || is_pad) …

With the above rationale and classification of "only buttons", setting INPUT_MOUSE here makes even less sense IMHO. These are most probably for games, so we'd rather want the more widely accessible ID_INPUT_JOYSTICK permissions and thus either classify them as joysticks or introduce a new ID_INPUT_GAMEPAD or so. What kind of device do you actually have?

Nope. It has mouse buttons so it is a mouse. If we make it a joystick libinput will ignore it generating no windowing system events.

hramrach · 2017-06-14T16:25:06Z

ok, updated the patch for detecting devices with buttons to exclude any mention of pads in the code since that seems to be misleading.

The pad I am referring to here is the subdevice of a Wacom tablet that has the mouse buttons and possibly (but not always) some scroll wheel axis. When you map some mouse buttons on a device that does not have mouse features otherwise it looks much like this. But unlike actual Wacom tablet subdevice it is not compatible with libwacom so it should be assigned the mouse class.

hramrach · 2017-06-14T18:20:36Z

duh, this works for me when backported to systemd 228 but not 232. The other difference is usb vs ps/2 keyboard. Needs some more debugging.

... and the discrepancy seems to be due to udevadm test showing capabilities of the actual device state and not of the device state that would result from applying the tested rules.

So WorksForMe now.

hm, was missing button aliases and had quite a few superfluous event names.

keszybz

Please update the PR title — those certainly are not "small" fixups.

keszybz · 2017-06-15T23:14:34Z

src/udev/udev-builtin-input_id.c

+                if (has_rel_coordinates)
+                        is_mouse = true;
+                else if (!has_abs_coordinates) /* mouse buttons and no axis */
+                        is_mouse = true;


This can be written as:

if (has_mouse_button && (has_rel_coordinates || !has_abs_coordinates /* mouse_buttons and no axis */)) is_mouse = true;

Also a note on the commit message: the first sentence says "Assign input class...", but I think it'd be clearer to be explicit and say "Label devices which have mouse buttons and no axis as ID_INPUT_MOUSE=1.".

keszybz · 2017-06-15T23:17:55Z

src/udev/udev-builtin-input_id.c

@@ -158,10 +158,12 @@ static bool test_pointers(struct udev_device *dev,
                          const unsigned long* bitmask_rel,
                          const unsigned long* bitmask_props,
                          bool test) {
+        int btn;


No abbrevations like this please.

keszybz · 2017-06-15T23:19:08Z

src/udev/udev-builtin-input_id.c

@@ -193,7 +195,7 @@ static bool test_pointers(struct udev_device *dev,
        is_pointing_stick = test_bit(INPUT_PROP_POINTING_STICK, bitmask_props);
        stylus_or_pen = test_bit(BTN_STYLUS, 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);
-        has_mouse_button = test_bit(BTN_LEFT, bitmask_key);
+        for (btn = BTN_LEFT; btn < BTN_JOYSTICK && !(has_mouse_button = test_bit(btn, bitmask_key)); btn++);


Please add a comment above the loop saying what it does. It's not entirely obvious at first glance. And an empty line before the comment and after the loop.

The for-loop style is a bit unusual, but is seems to work here better then alternatives, so OK.

keszybz · 2017-06-15T23:20:00Z

src/udev/udev-builtin-input_id.c

-                                       test_bit(ABS_GAS, bitmask_abs) ||
-                                       test_bit(ABS_BRAKE, bitmask_abs);
+        for (btn = BTN_JOYSTICK; btn < BTN_DIGI && !(has_joystick_buttons = test_bit(btn, bitmask_key)); btn++);
+        for (btn = ABS_RX; btn < ABS_PRESSURE && !(has_joystick_axes = test_bit(btn, bitmask_abs)); btn++);


keszybz · 2017-06-15T23:24:54Z

Makefile.am

@@ -3922,7 +3922,7 @@ noinst_LTLIBRARIES += \

 src/udev/keyboard-keys-list.txt:
 	$(AM_V_at)$(MKDIR_P) $(dir $@)
-	$(AM_V_GEN)$(CPP) $(CFLAGS) $(AM_CPPFLAGS) $(CPPFLAGS) -dM -include linux/input.h - < /dev/null | $(AWK) '/^#define[ \t]+(KEY|BTN)_[^ ]+[ \t]+[0-9BK]/ { if ($$2 != "KEY_MAX") { print $$2 } }' > $@
+	$(AM_V_GEN)$(CPP) $(CFLAGS) $(AM_CPPFLAGS) $(CPPFLAGS) -dM -include linux/input.h - < /dev/null | grep -vEe '\<(KEY_(MAX|MIN_INTERESTING))|(BTN_(MISC|MOUSE|JOYSTICK|GAMEPAD|DIGI|WHEEL|TRIGGER_HAPPY))\>'| $(AWK) '/^#define[ \t]+(KEY|BTN)_[^ ]+[ \t]+[0-9BK]/ { print $$2 }' > $@


No need to call grep, if awk is already being called. Use next or something (`awk is not my thing) [https://www.gnu.org/software/gawk/manual/html_node/Next-Statement.html].

awk docs say it uses grep extended regexps so just moving the regexp inside the awk call should work. Might not be so easy when the supported regexp variant happens to differ

hramrach · 2017-06-20T07:11:52Z

It seems this s390x test fails randomly. I did not touch networking so I do not see how networkd would be affected.

poettering · 2017-06-22T13:29:16Z

src/udev/udev-builtin-input_id.c

-        has_mouse_button = test_bit(BTN_LEFT, bitmask_key);
+
+        /* Set has_mouse_button if any button in [BTN_MOUSE, BTN_JOYSTICK) is found */
+        for (button = BTN_MOUSE; button < BTN_JOYSTICK && !(has_mouse_button = test_bit(button, bitmask_key)); button++);


can you please follow coding style and use ; on a separate line if you do for loops with no body?

Also, I really don't appreciate this overly condensed style I must say, this doesn't help readability. Instead of cramming the entire logic with variable assignment and everything into the second part of the for description, why not just place it in the body of the for loop and make it actually readable?

This is not a contest in writing the shortest most unreadable code, but quite the opposite: the focus is on readability.

poettering · 2017-06-22T13:29:59Z

src/udev/udev-builtin-input_id.c

+         * Set has_joystick_buttons if any button in [BTN_JOYSTICK, BTN_DIGI) is found
+         * and has_joystick_axes if any axis in [ABS_RX, ABS_PRESSURE) is found */
+        for (button = BTN_JOYSTICK; button < BTN_DIGI && !(has_joystick_buttons = test_bit(button, bitmask_key)); button++);
+        for (axis = ABS_RX; axis < ABS_PRESSURE && !(has_joystick_axes = test_bit(axis, bitmask_abs)); axis++);


the same here.

The old code I could understand, the new code looks like Perl.

poettering · 2017-06-22T13:31:31Z

This needs rebasing after #6170 got merged.

Also, @whot, can you comment on this one? you're the input guru!

whot

A few nitpicks, the only real blocker here are the missing meson bits and I really think the PR needs to be renamed to something more expressive. Main goal here is to "Support button events from remapped keyboards" so that would be better than just "fixups".

Rest are nitpicks, the code itself looks good.

whot · 2017-06-26T02:43:16Z

rules/60-evdev.rules

@@ -8,7 +8,7 @@ IMPORT{builtin}="hwdb --subsystem=input --lookup-prefix=evdev:", \
  RUN{builtin}+="keyboard", GOTO="evdev_end"

 # AT keyboard matching by the machine's DMI data
-ENV{ID_INPUT_KEY}=="?*", DRIVERS=="atkbd", \


The second sentence of the commit message is typoed, should read "because it is not yet assigned at this point"

I think "not yet assigned" and "now not assigned (at this point)" are the same thing. "now not" is not as easy to parse, but it's good enough.

whot · 2017-06-26T02:44:57Z

rules/60-input-id.rules

+
+SUBSYSTEM=="input", ENV{ID_INPUT}=="", IMPORT{builtin}="input_id"
+
+LABEL="default_end"


please rename the label "input_id_end" so it doesn't look like a copy/paste error

whot · 2017-06-26T02:57:22Z

Makefile.am

@@ -3924,10 +3924,10 @@ noinst_LTLIBRARIES += \

 src/udev/keyboard-keys-list.txt:
 	$(AM_V_at)$(MKDIR_P) $(dir $@)
-	$(AM_V_GEN)$(CPP) $(CFLAGS) $(AM_CPPFLAGS) $(CPPFLAGS) -dM -include linux/input.h - < /dev/null | $(AWK) '/^#define[ \t]+KEY_[^ ]+[ \t]+[0-9K]/ { if ($$2 != "KEY_MAX") { print $$2 } }' > $@


this is missing the matching change to src/udev/generate-keyboard-keys-list.sh and the one below to src/udev/generate-keyboard-gperf.py for meson. I'd say the best option here would be to have a separate patch to Makefile.am to use the scripts as well so we only have one place to fix this up.

hramrach · 2017-06-26T17:14:39Z

hm, pushed an update and gihub is not picking it up.

Updated the Makefile and meson to use same script and while it is verified no-op for Makefile I have no idea about meson.

whot

move this commit to the first one in the set please, otherwise we'll have known broken ones - that's usually frowned upon.

For meson testing, all you need is "meson builddir; ninja -C builddir" and that builds it. Heaps of documentation out there, start with mesonbuild.org

Support BTN_* codes with btn_ prefix and keys with KEY_ prefix optionally removed.

udevadm trigger /dev/input/event* does not work use udevadm trigger --verbose --sysname-match="event*"

This places the input_id call after the evdev hwdb calls. With this the hwdb fixups in evdev can affect the device capabilities assigned in input_id. Remove the ID_INPUT_KEY dependency in atkbd rule because it is now not assigned at this point.

Assign ID_INPUT_MOUSE property to devices with mouse buttons and no axis. Libinput tries to use libwacom on devices with tablet-pad capability which are detected by ID_INPUT_TABLET_PAD=1 property so assign pointer class by setting ID_INPUT_MOUSE=1 to devices with mouse buttons and let libwacom override the class for Wacom pads.

Due to remapping some devices might not have the first button. Check whole button range.

The defines KEY_MAX KEY_CNT KEY_MIN_INTERESTING BTN_MISC BTN_MOUSE BTN_JOYSTICK BTN_GAMEPAD BTN_DIGI BTN_WHEEL BTN_TRIGGER_HAPPY mark start/end of key blocks and do not designate events. Exclude them from the list of recognized key events.

hramrach · 2017-06-27T11:34:46Z

They would not be broken. The results for meson and makefile would diverge, though.

They diverge slightly anyway because the cpp invocation differs between makefile and meson resulting in different order of the key event defines in the gperf file and probably other oddities.

keszybz · 2017-06-27T23:07:13Z

FTR, after sorting, I get the following difference:

@@ -1,3 +1,114 @@
+BTN_0
+BTN_1
+BTN_2
+BTN_3
+BTN_4
+BTN_5
+BTN_6
+BTN_7
+BTN_8
+BTN_9
+BTN_A
+BTN_B
+BTN_BACK
+BTN_BASE
+BTN_BASE2
+BTN_BASE3
+BTN_BASE4
+BTN_BASE5
+BTN_BASE6
+BTN_C
+BTN_DEAD
+BTN_DPAD_DOWN
+BTN_DPAD_LEFT
+BTN_DPAD_RIGHT
+BTN_DPAD_UP
+BTN_EAST
+BTN_EXTRA
+BTN_FORWARD
+BTN_GEAR_DOWN
+BTN_GEAR_UP
+BTN_LEFT
+BTN_MIDDLE
+BTN_MODE
+BTN_NORTH
+BTN_PINKIE
+BTN_RIGHT
+BTN_SELECT
+BTN_SIDE
+BTN_SOUTH
+BTN_START
+BTN_STYLUS
+BTN_STYLUS2
+BTN_TASK
+BTN_THUMB
+BTN_THUMB2
+BTN_THUMBL
+BTN_THUMBR
+BTN_TL
+BTN_TL2
+BTN_TOOL_AIRBRUSH
+BTN_TOOL_BRUSH
+BTN_TOOL_DOUBLETAP
+BTN_TOOL_FINGER
+BTN_TOOL_LENS
+BTN_TOOL_MOUSE
+BTN_TOOL_PEN
+BTN_TOOL_PENCIL
+BTN_TOOL_QUADTAP
+BTN_TOOL_QUINTTAP
+BTN_TOOL_RUBBER
+BTN_TOOL_TRIPLETAP
+BTN_TOP
+BTN_TOP2
+BTN_TOUCH
+BTN_TR
+BTN_TR2
+BTN_TRIGGER
+BTN_TRIGGER_HAPPY1
+BTN_TRIGGER_HAPPY10
+BTN_TRIGGER_HAPPY11
+BTN_TRIGGER_HAPPY12
+BTN_TRIGGER_HAPPY13
+BTN_TRIGGER_HAPPY14
+BTN_TRIGGER_HAPPY15
+BTN_TRIGGER_HAPPY16
+BTN_TRIGGER_HAPPY17
+BTN_TRIGGER_HAPPY18
+BTN_TRIGGER_HAPPY19
+BTN_TRIGGER_HAPPY2
+BTN_TRIGGER_HAPPY20
+BTN_TRIGGER_HAPPY21
+BTN_TRIGGER_HAPPY22
+BTN_TRIGGER_HAPPY23
+BTN_TRIGGER_HAPPY24
+BTN_TRIGGER_HAPPY25
+BTN_TRIGGER_HAPPY26
+BTN_TRIGGER_HAPPY27
+BTN_TRIGGER_HAPPY28
+BTN_TRIGGER_HAPPY29
+BTN_TRIGGER_HAPPY3
+BTN_TRIGGER_HAPPY30
+BTN_TRIGGER_HAPPY31
+BTN_TRIGGER_HAPPY32
+BTN_TRIGGER_HAPPY33
+BTN_TRIGGER_HAPPY34
+BTN_TRIGGER_HAPPY35
+BTN_TRIGGER_HAPPY36
+BTN_TRIGGER_HAPPY37
+BTN_TRIGGER_HAPPY38
+BTN_TRIGGER_HAPPY39
+BTN_TRIGGER_HAPPY4
+BTN_TRIGGER_HAPPY40
+BTN_TRIGGER_HAPPY5
+BTN_TRIGGER_HAPPY6
+BTN_TRIGGER_HAPPY7
+BTN_TRIGGER_HAPPY8
+BTN_TRIGGER_HAPPY9
+BTN_WEST
+BTN_X
+BTN_Y
+BTN_Z
 KEY_0
 KEY_1
 KEY_102ND
@@ -264,7 +375,6 @@
 KEY_MESSENGER
 KEY_MHP
 KEY_MICMUTE
-KEY_MIN_INTERESTING
 KEY_MINUS
 KEY_MODE
 KEY_MOVE

Looks good. I think @whot's comments have been addressed.

yuwata · 2017-06-28T05:54:29Z

This PR causes #6219.

keszybz · 2017-06-29T03:02:40Z

@nrlewis117 please also look this over.

hramrach · 2017-06-29T07:26:51Z

hm, BTN_DPAD_* are outside of any BTN block so they should be added to the joystick test I guess.

keszybz requested a review from martinpitt June 9, 2017 23:02

martinpitt reviewed Jun 12, 2017

View reviewed changes

martinpitt added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 12, 2017

hramrach changed the title ~~small fixups for keyboard handling~~ small fixups for input handling Jun 12, 2017

martinpitt requested changes Jun 12, 2017

View reviewed changes

poettering added the udev label Jun 13, 2017

keszybz requested changes Jun 15, 2017

View reviewed changes

hramrach changed the title ~~small fixups for input handling~~ fixups for input handling Jun 16, 2017

poettering reviewed Jun 22, 2017

View reviewed changes

whot requested changes Jun 26, 2017

View reviewed changes

hramrach changed the title ~~fixups for input handling~~ Enable mapping buttons on keyboard Jun 26, 2017

hramrach changed the title ~~Enable mapping buttons on keyboard~~ Enable mapping button events on keyboard Jun 26, 2017

whot requested changes Jun 27, 2017

View reviewed changes

hramrach added 7 commits June 27, 2017 13:27

Use same script to build key list in Makefile and meson

1b83323

hwdb: support pressing buttons on a keyboard

c6dce24

Support BTN_* codes with btn_ prefix and keys with KEY_ prefix optionally removed.

hwdb: fix help text in 60-keyboard.hwdb

3ee4e61

udevadm trigger /dev/input/event* does not work use udevadm trigger --verbose --sysname-match="event*"

input_id: fix button detection

22811ad

Due to remapping some devices might not have the first button. Check whole button range.

keszybz merged commit f62c9e5 into systemd:master Jun 27, 2017

yuwata mentioned this pull request Jun 28, 2017

lightdm crashes and cannot start graphical session #6219

Closed

hramrach mentioned this pull request Jun 29, 2017

Include dpad buttons in joystick detection #6240

Merged


		SUBSYSTEM=="input", ENV{ID_INPUT}=="", IMPORT{builtin}="input_id"

		LABEL="default_end"

Enable mapping button events on keyboard #6099

Enable mapping button events on keyboard #6099

Conversation

hramrach commented Jun 7, 2017 • edited Loading

keszybz commented Jun 9, 2017

hramrach commented Jun 11, 2017

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hramrach commented Jun 12, 2017

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hramrach commented Jun 14, 2017

hramrach commented Jun 14, 2017 • edited Loading

keszybz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hramrach commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Jun 22, 2017 • edited Loading

whot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hramrach commented Jun 26, 2017

whot left a comment

Choose a reason for hiding this comment

hramrach commented Jun 27, 2017

keszybz commented Jun 27, 2017

yuwata commented Jun 28, 2017

keszybz commented Jun 29, 2017

hramrach commented Jun 29, 2017

hramrach commented Jun 7, 2017 •

edited

Loading

martinpitt Jun 12, 2017 •

edited

Loading

hramrach commented Jun 14, 2017 •

edited

Loading

poettering commented Jun 22, 2017 •

edited

Loading