Hi,
On 01-04-19 05:05, Peter Hutterer wrote:
On Sat, Mar 30, 2019 at 02:37:30PM +0100, Hans de Goede wrote:
Almost all modern mice use HID. For buttons 1 - 3 the order of left /
right / middle specified in the HUT matches with the BTN_LEFT, BTN_RIGHT
and BTN_MIDDLE order of the evdev code defines since the HUT does not
specify any meaning for button usages >= 4 the HID input driver simply
maps buttons to BTN_LEFT + (number - 1).
In practice on most mice which have more then 3 buttons, usage 4 is
used for the back(ward) button and usage 5 for the forward button.
This means that these buttons generate an input_event code of 0x113 resp.
0x114, which looking at the old defines maps to BTN_SIDE and BTN_EXTRA.
Under X these are mapped to buttons 8 resp. 9; and under wayland the
input_event code is passed through unmodified. Apps, e.g. Firefox both
when running as Wayland client under GNOME3 and when running under Xorg,
correctly interpret these as back and forward.
So we've back and forward buttons generating BTN_SIDE and BTN_EXTRA,
which the apps then interpret as BTN_BACK and BTN_FORWARD, rather then as
BTN_SIDE and BTN_EXTRA (which have no clear meaning).
I find this all very confusing, this commit tries to remove the confusion
by deprecating the old defines and adding new defines which assign labels
to the 0x113 - 0x116 input_event codes which match how they are actually
used today.
Note there are no functional changes here, after this userspace will
see the exact same input_event codes as before, this purely about
assigning a human-readable label to these codes which matches with how
they are actually being used.
there is a small functional change, primarily seen by debugging tools: where
button names are resolved to strings, only one name is returned (at least by
libevdev). So things like libevdev_event_code_get_name(EV_KEY, BTN_EXTRA)
will now return "BTN_FORWRD" instead of "BTN_EXTRA". This has a potential to
break whatever is consuming those strings.
True, but that is actually intentional. I believe the main consumer of these
strings are humans and the whole reason to fix the string is to stop confusing
humans, starting with myself. I'm not fully specialized on input, but I've
done my fair share of input work, so if this confuses me it will certainly
confuse others.
Not sure what the impact of this will be though. It's already the case for
other buttons and I've mostly shrugged it off as niche case not to worry
about. But we're talking about buttons that almost every modern mouse has,
so it's slightly less niche now.
True, but I believe most consumers of these events will either be X-clients
or Wayland clients. I'm pretty sure X-clients are not seeing these strings,
I would also not expect Wayland clients to somehow end up using the
libevdev_event_code_get_name() output, but I'm slightly less sure there:
[hans@shalem ~]$ sudo dnf repoquery --whatrequires 'libevdev.so.2()(64bit)'
dolphin-emu-0:5.0-27.fc30.x86_64
dolphin-emu-nogui-0:5.0-27.fc30.x86_64
evemu-0:2.7.0-9.fc30.x86_64
evemu-libs-0:2.7.0-9.fc30.x86_64
gnome-battery-bench-0:3.15.4-10.fc30.x86_64
libevdev-devel-0:1.6.0-2.fc30.x86_64
libevdev-utils-0:1.6.0-2.fc30.x86_64
libinput-0:1.12.6-3.fc30.x86_64
libinput-utils-0:1.12.6-3.fc30.x86_64
libmanette-0:0.2.2-1.fc30.x86_64
libratbag-ratbagd-0:0.9.904-2.fc30.x86_64
weston-0:5.0.91-1.fc30.x86_64
weston-0:6.0.0-1.fc30.x86_64
xorg-x11-drv-evdev-0:2.10.6-4.fc30.x86_64
xorg-x11-drv-synaptics-legacy-0:1.9.1-3.fc30.x86_64
I do not believe that either libinput or the 2 xorg-x11-drv
pkgs pass along strings to clients.
I will leave judging if this is a problem for ratbagd up to you.
As for the other I assume dolphin-emu is using this
for joysticks. Which just leaves weston as a potential problem
which IMHO itself is a bit of a niche-case.
So I do not think this will cause much of a problem.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
Documentation/input/event-codes.rst | 2 +-
drivers/hid/hid-debug.c | 6 +++---
drivers/input/mousedev.c | 8 ++++----
include/uapi/linux/input-event-codes.h | 13 ++++++++++++-
4 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
index b24b5343f5eb..db52b96e7a83 100644
--- a/Documentation/input/event-codes.rst
+++ b/Documentation/input/event-codes.rst
@@ -422,7 +422,7 @@ should be used to report when the tool is in contact with the tablet.
BTN_{STYLUS,STYLUS2} should be used to report buttons on the tool itself. Any
button may be used for buttons on the tablet except BTN_{MOUSE,LEFT}.
BTN_{0,1,2,etc} are good generic codes for unlabeled buttons. Do not use
-meaningful buttons, like BTN_FORWARD, unless the button is labeled for that
+meaningful buttons, like BTN_FORWRD, unless the button is labeled for that
I haven't thought of a better name yet, but using BTN_FORWRD is bound to
introduce bugs, especially when BTN_FORWARD still resolves. I don't think
that name is a good idea but I don't have a better suggestion yet.
Good point about the typos my plan for the kernel is to add #ifndef __KERNEL__
around the compat defines once all users are moved over.
Thought of the day: if HID doesn't have any meaning, is it really worth
changing these names based on what you hope a client interprets it as?
would a more generic naming scheme that admits this is just button 4 be
better? BTN_HID_4 or something?
The HUT spec may not say anything about this (note I did not check the
gazillion errata I really wish they would just fold those into an
updated version), but Microsoft writes the following about this and it
is reasonable to expect mice vendors to follow this (and in practice they do):
https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/keyboard-and-mouse-hid-client-drivers
"Buttons 4 & 5 on are mapped to WM_APPCOMMAND messages and correspond to App_Back and App_Forward."
All in all I think this will be worth the trouble. As the commit messages
of the patches moving over in kernel users of the old defines show, there
are already several in kernel drivers which are likely using the old defines
wrong, as they assume that BTN_FORWARD and BTN_BACK actually work as advertised
and I almost made the same mistake myself, so IMHO we really need to fix this
and the sooner we fix this the better.
Regards,
Hans
Cheers,
Peter
purpose on the device.
For new hardware, both INPUT_PROP_DIRECT and INPUT_PROP_POINTER should be set.
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index ac9fda1b5a72..45b56f933fd1 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -855,9 +855,9 @@ static const char *keys[KEY_MAX + 1] = {
[BTN_6] = "Btn6", [BTN_7] = "Btn7",
[BTN_8] = "Btn8", [BTN_9] = "Btn9",
[BTN_LEFT] = "LeftBtn", [BTN_RIGHT] = "RightBtn",
- [BTN_MIDDLE] = "MiddleBtn", [BTN_SIDE] = "SideBtn",
- [BTN_EXTRA] = "ExtraBtn", [BTN_FORWARD] = "ForwardBtn",
- [BTN_BACK] = "BackBtn", [BTN_TASK] = "TaskBtn",
+ [BTN_MIDDLE] = "MiddleBtn", [BTN_BACKWRD] = "BackwardBtn",
+ [BTN_FORWRD] = "ForwardBtn", [BTN_EXTRA1] = "ExtraBtn1",
+ [BTN_EXTRA2] = "ExtraBtn2", [BTN_TASK] = "TaskBtn",
[BTN_TRIGGER] = "Trigger", [BTN_THUMB] = "ThumbBtn",
[BTN_THUMB2] = "ThumbBtn2", [BTN_TOP] = "TopBtn",
[BTN_TOP2] = "TopBtn2", [BTN_PINKIE] = "PinkieBtn",
diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index 412fa71245af..7504e854900f 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -238,16 +238,16 @@ static void mousedev_key_event(struct mousedev *mousedev,
case BTN_RIGHT: index = 1; break;
case BTN_2:
- case BTN_FORWARD:
+ case BTN_EXTRA1:
case BTN_STYLUS2:
case BTN_MIDDLE: index = 2; break;
case BTN_3:
- case BTN_BACK:
- case BTN_SIDE: index = 3; break;
+ case BTN_EXTRA2:
+ case BTN_BACKWRD: index = 3; break;
case BTN_4:
- case BTN_EXTRA: index = 4; break;
+ case BTN_FORWRD: index = 4; break;
default: return;
}
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 7f14d4a66c28..f11ce5d2c228 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -355,11 +355,22 @@
#define BTN_LEFT 0x110
#define BTN_RIGHT 0x111
#define BTN_MIDDLE 0x112
+#define BTN_BACKWRD 0x113
+#define BTN_FORWRD 0x114
+#define BTN_EXTRA1 0x115
+#define BTN_EXTRA2 0x116
+#define BTN_TASK 0x117
+
+/*
+ * DEPRECATED: Do not use!
+ * These old defines are to not break the compilation of user code ONLY.
+ * Over time they have grown to be incorrect. Almost all modern mice with
+ * back / forward buttons generate 0x113 for back and 0x114 for forward.
+ */
#define BTN_SIDE 0x113
#define BTN_EXTRA 0x114
#define BTN_FORWARD 0x115
#define BTN_BACK 0x116
-#define BTN_TASK 0x117
#define BTN_JOYSTICK 0x120
#define BTN_TRIGGER 0x120
--
2.21.0
