| Message ID | 20190330133735.14681-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: Rename extra mouse buttons defines to match their actual usage | expand |
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. 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. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > 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. 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? 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 >
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@redhat.com> >> --- >> 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 >>
On Mon, Apr 01, 2019 at 09:16:22AM +0200, Hans de Goede wrote: > 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. ratbagd doesn't expose this but maybe some debug messages, same for libinput and evemu. weston doesn't use that function, so it's good. evdev/synaptics are fine too. This is less of an issue for the primary users of evdev (they have the code after all) but the secondary users. For example, the libinput-gestures project parses libinput debug-events output (against my advice I would add). That output contains the button names. libinput-gestures doesn't care about button names but tools like it can break if the button name changes. I'm not aware of any tools that care about those specific button numbers. Anyway, this is more an explanation that there *may* be side-effects but we probably won't know until we run into them, if ever. > 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@redhat.com> > > > --- > > > 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. good, but not enough. the kernel is easiest here because you could literally just add a script to a test suite to make sure it doesn't use BTN_FORWARD. It's all the userspace that'll introduce typos. > > 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." BTN_APP_FORWARD and BTN_APP_BACK then? seems obvious enough and is a lot easier to visually identify than a missing A in FORWRD. > 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. I noticed you left BTN_TASK in the #defines, any specific reason for that over using BTN_EXTRA3 for that one? 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 > > >
Hi, On 01-04-19 12:34, Peter Hutterer wrote: > On Mon, Apr 01, 2019 at 09:16:22AM +0200, Hans de Goede wrote: >> 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. > > ratbagd doesn't expose this but maybe some debug messages, same for > libinput and evemu. weston doesn't use that function, so it's good. > evdev/synaptics are fine too. This is less of an issue for the primary users > of evdev (they have the code after all) but the secondary users. > > For example, the libinput-gestures project parses libinput debug-events > output (against my advice I would add). That output contains the button > names. libinput-gestures doesn't care about button names but tools like it > can break if the button name changes. I'm not aware of any tools that care > about those specific button numbers. > > Anyway, this is more an explanation that there *may* be side-effects but we > probably won't know until we run into them, if ever. Ok. >> 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@redhat.com> >>>> --- >>>> 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. > > good, but not enough. the kernel is easiest here because you could literally > just add a script to a test suite to make sure it doesn't use BTN_FORWARD. > It's all the userspace that'll introduce typos. > >>> 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." > > BTN_APP_FORWARD and BTN_APP_BACK then? seems obvious enough and is a lot > easier to visually identify than a missing A in FORWRD. Yes I was thinking the same about 5 minutes after sending the mail you replied to, so I think we agree on this, will fix for the the first non RFC version. >> 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. > > I noticed you left BTN_TASK in the #defines, any specific reason for that > over using BTN_EXTRA3 for that one? My main focus was making 0x113 some form of back and 0x114 some form of forward and making 0x115/0x116 NOT have back / forward in their names BTN_TASK is 0x117 so I left it alone. But I'm fine with making it BTN_EXTRA3, I agree that would be more consistent. Regards, Hans >>>> 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 >>>>
On Mon, Apr 1, 2019 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 01-04-19 12:34, Peter Hutterer wrote: > > On Mon, Apr 01, 2019 at 09:16:22AM +0200, Hans de Goede wrote: > >> 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. > > > > ratbagd doesn't expose this but maybe some debug messages, same for > > libinput and evemu. weston doesn't use that function, so it's good. > > evdev/synaptics are fine too. This is less of an issue for the primary users > > of evdev (they have the code after all) but the secondary users. > > > > For example, the libinput-gestures project parses libinput debug-events > > output (against my advice I would add). That output contains the button > > names. libinput-gestures doesn't care about button names but tools like it > > can break if the button name changes. I'm not aware of any tools that care > > about those specific button numbers. > > > > Anyway, this is more an explanation that there *may* be side-effects but we > > probably won't know until we run into them, if ever. > > Ok. > > > >> 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@redhat.com> > >>>> --- > >>>> 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. > > > > good, but not enough. the kernel is easiest here because you could literally > > just add a script to a test suite to make sure it doesn't use BTN_FORWARD. > > It's all the userspace that'll introduce typos. > > > >>> 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." > > > > BTN_APP_FORWARD and BTN_APP_BACK then? seems obvious enough and is a lot > > easier to visually identify than a missing A in FORWRD. > > Yes I was thinking the same about 5 minutes after sending the mail you > replied to, so I think we agree on this, will fix for the the first non > RFC version. For the record, I also agree with BTN_APP_FORWARD and BTN_APP_BACK. We made some archeology this morning with Peter. So now is story time, gather around children: BTN_SIDE and BTN_EXTRA were added in 2.3.36 with patch-2.3.36.bz2 that can be found at https://mirrors.edge.kernel.org/pub/linux/kernel/v2.3/. It's actually the big switch from Vojtech from a custom USB mouse boot protocol to an actual HID bus. It's also the creation of the evdev protocol. And those BTN_* were added to suport mice with extra buttons. I guess it was pretty uncommon for such mice in Jan 2000 and the buttons were not commonly used like today as back and forward. Note that it's actually easy to go back to 2.4.0 by using: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/?ofs=63400 So, my fears that the buttons would have a meaning is relieved, and I would Ack such a series (I haven't fully read it so it's an ACK on the principle). Cheers, Benjamin > > >> 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. > > > > I noticed you left BTN_TASK in the #defines, any specific reason for that > > over using BTN_EXTRA3 for that one? > > My main focus was making 0x113 some form of back and 0x114 some form of forward > and making 0x115/0x116 NOT have back / forward in their names BTN_TASK is 0x117 > so I left it alone. But I'm fine with making it BTN_EXTRA3, I agree that would be > more consistent. > > Regards, > > Hans > > > >>>> 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 > >>>>
Hi, On 03-04-19 16:12, Benjamin Tissoires wrote: > On Mon, Apr 1, 2019 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 01-04-19 12:34, Peter Hutterer wrote: >>> On Mon, Apr 01, 2019 at 09:16:22AM +0200, Hans de Goede wrote: >>>> 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. >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>> --- >>>>>> 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. >>> >>> good, but not enough. the kernel is easiest here because you could literally >>> just add a script to a test suite to make sure it doesn't use BTN_FORWARD. >>> It's all the userspace that'll introduce typos. >>> >>>>> 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." >>> >>> BTN_APP_FORWARD and BTN_APP_BACK then? seems obvious enough and is a lot >>> easier to visually identify than a missing A in FORWRD. >> >> Yes I was thinking the same about 5 minutes after sending the mail you >> replied to, so I think we agree on this, will fix for the the first non >> RFC version. > > For the record, I also agree with BTN_APP_FORWARD and BTN_APP_BACK. > > We made some archeology this morning with Peter. > So now is story time, gather around children: > BTN_SIDE and BTN_EXTRA were added in 2.3.36 with patch-2.3.36.bz2 that > can be found at > https://mirrors.edge.kernel.org/pub/linux/kernel/v2.3/. > > It's actually the big switch from Vojtech from a custom USB mouse boot > protocol to an actual HID bus. It's also the creation of the evdev > protocol. > > And those BTN_* were added to suport mice with extra buttons. I guess > it was pretty uncommon for such mice in Jan 2000 and the buttons were > not commonly used like today as back and forward. > > Note that it's actually easy to go back to 2.4.0 by using: > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/?ofs=63400 > > So, my fears that the buttons would have a meaning is relieved, and I > would Ack such a series (I haven't fully read it so it's an ACK on the > principle). Ok, so before I send a v1 of this patch-set, Dmitry do you have any remarks? Regards, Hans
Hi, On 01-04-19 13:11, Hans de Goede wrote: > Hi, > > On 01-04-19 12:34, Peter Hutterer wrote: <snip> >> I noticed you left BTN_TASK in the #defines, any specific reason for that >> over using BTN_EXTRA3 for that one? > > My main focus was making 0x113 some form of back and 0x114 some form of forward > and making 0x115/0x116 NOT have back / forward in their names BTN_TASK is 0x117 > so I left it alone. But I'm fine with making it BTN_EXTRA3, I agree that would be > more consistent. Peter, any preference for keeping BTN_TASK as is (less churn) vs changing it to BTN_EXTRA3 (more consistent) ? Regards, Hans
On Wed, Apr 03, 2019 at 05:40:45PM +0200, Hans de Goede wrote: > Hi, > > On 01-04-19 13:11, Hans de Goede wrote: > > Hi, > > > > On 01-04-19 12:34, Peter Hutterer wrote: > > <snip> > > > > I noticed you left BTN_TASK in the #defines, any specific reason for that > > > over using BTN_EXTRA3 for that one? > > > > My main focus was making 0x113 some form of back and 0x114 some form of forward > > and making 0x115/0x116 NOT have back / forward in their names BTN_TASK is 0x117 > > so I left it alone. But I'm fine with making it BTN_EXTRA3, I agree that would be > > more consistent. > > Peter, any preference for keeping BTN_TASK as is (less churn) vs changing it > to BTN_EXTRA3 (more consistent) ? I'd vote for BTN_EXTRA3 for consistency, doubly so because does *anything* actually use BTN_TASK for uhm... "task"? whatever that means in this context :) Cheers, Peter
On Thu, Apr 04, 2019 at 08:32:55AM +1000, Peter Hutterer wrote: > On Wed, Apr 03, 2019 at 05:40:45PM +0200, Hans de Goede wrote: > > Hi, > > > > On 01-04-19 13:11, Hans de Goede wrote: > > > Hi, > > > > > > On 01-04-19 12:34, Peter Hutterer wrote: > > > > <snip> > > > > > > I noticed you left BTN_TASK in the #defines, any specific reason for that > > > > over using BTN_EXTRA3 for that one? > > > > > > My main focus was making 0x113 some form of back and 0x114 some form of forward > > > and making 0x115/0x116 NOT have back / forward in their names BTN_TASK is 0x117 > > > so I left it alone. But I'm fine with making it BTN_EXTRA3, I agree that would be > > > more consistent. > > > > Peter, any preference for keeping BTN_TASK as is (less churn) vs changing it > > to BTN_EXTRA3 (more consistent) ? > > I'd vote for BTN_EXTRA3 for consistency, doubly so because does *anything* > actually use BTN_TASK for uhm... "task"? whatever that means in this context :) I believe this came from Logitech which had mice with "Task Switch" button. I guess windows driver could convert it to Alt-Tab or similar. http://oyvind.servehttp.com/logitech_mx700.htm Thanks.
Hi Hans, On Wed, Apr 03, 2019 at 05:37:11PM +0200, Hans de Goede wrote: > Hi, > > On 03-04-19 16:12, Benjamin Tissoires wrote: > > On Mon, Apr 1, 2019 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > Hi, > > > > > > On 01-04-19 12:34, Peter Hutterer wrote: > > > > On Mon, Apr 01, 2019 at 09:16:22AM +0200, Hans de Goede wrote: > > > > > 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. > > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > > > --- > > > > > > > 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. > > > > > > > > good, but not enough. the kernel is easiest here because you could literally > > > > just add a script to a test suite to make sure it doesn't use BTN_FORWARD. > > > > It's all the userspace that'll introduce typos. > > > > > > > > > > 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." > > > > > > > > BTN_APP_FORWARD and BTN_APP_BACK then? seems obvious enough and is a lot > > > > easier to visually identify than a missing A in FORWRD. > > > > > > Yes I was thinking the same about 5 minutes after sending the mail you > > > replied to, so I think we agree on this, will fix for the the first non > > > RFC version. > > > > For the record, I also agree with BTN_APP_FORWARD and BTN_APP_BACK. > > > > We made some archeology this morning with Peter. > > So now is story time, gather around children: > > BTN_SIDE and BTN_EXTRA were added in 2.3.36 with patch-2.3.36.bz2 that > > can be found at > > https://mirrors.edge.kernel.org/pub/linux/kernel/v2.3/. > > > > It's actually the big switch from Vojtech from a custom USB mouse boot > > protocol to an actual HID bus. It's also the creation of the evdev > > protocol. > > > > And those BTN_* were added to suport mice with extra buttons. I guess > > it was pretty uncommon for such mice in Jan 2000 and the buttons were > > not commonly used like today as back and forward. > > > > Note that it's actually easy to go back to 2.4.0 by using: > > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/?ofs=63400 > > > > So, my fears that the buttons would have a meaning is relieved, and I > > would Ack such a series (I haven't fully read it so it's an ACK on the > > principle). > > Ok, so before I send a v1 of this patch-set, Dmitry do you have any remarks? I played with my Evoluent Vertical mouse and I found that it reports BTN_EXTRA for button under scroll wheel, BTN_SIDE for the button on the side of the mouse and BTN_FORWARD for the 2nd button on the side. This means that not all mice have these buttons in forward/back positions... Thanks.
Hi Dmitry, On 04-04-19 03:00, Dmitry Torokhov wrote: > Hi Hans, > > On Wed, Apr 03, 2019 at 05:37:11PM +0200, Hans de Goede wrote: >> Hi, >> >> On 03-04-19 16:12, Benjamin Tissoires wrote: >>> On Mon, Apr 1, 2019 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Hi, >>>> >>>> On 01-04-19 12:34, Peter Hutterer wrote: >>>>> On Mon, Apr 01, 2019 at 09:16:22AM +0200, Hans de Goede wrote: >>>>>> 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. >>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>> --- >>>>>>>> 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. >>>>> >>>>> good, but not enough. the kernel is easiest here because you could literally >>>>> just add a script to a test suite to make sure it doesn't use BTN_FORWARD. >>>>> It's all the userspace that'll introduce typos. >>>>> >>>>>>> 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." >>>>> >>>>> BTN_APP_FORWARD and BTN_APP_BACK then? seems obvious enough and is a lot >>>>> easier to visually identify than a missing A in FORWRD. >>>> >>>> Yes I was thinking the same about 5 minutes after sending the mail you >>>> replied to, so I think we agree on this, will fix for the the first non >>>> RFC version. >>> >>> For the record, I also agree with BTN_APP_FORWARD and BTN_APP_BACK. >>> >>> We made some archeology this morning with Peter. >>> So now is story time, gather around children: >>> BTN_SIDE and BTN_EXTRA were added in 2.3.36 with patch-2.3.36.bz2 that >>> can be found at >>> https://mirrors.edge.kernel.org/pub/linux/kernel/v2.3/. >>> >>> It's actually the big switch from Vojtech from a custom USB mouse boot >>> protocol to an actual HID bus. It's also the creation of the evdev >>> protocol. >>> >>> And those BTN_* were added to suport mice with extra buttons. I guess >>> it was pretty uncommon for such mice in Jan 2000 and the buttons were >>> not commonly used like today as back and forward. >>> >>> Note that it's actually easy to go back to 2.4.0 by using: >>> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/?ofs=63400 >>> >>> So, my fears that the buttons would have a meaning is relieved, and I >>> would Ack such a series (I haven't fully read it so it's an ACK on the >>> principle). >> >> Ok, so before I send a v1 of this patch-set, Dmitry do you have any remarks? > > I played with my Evoluent Vertical mouse and I found that it reports > BTN_EXTRA for button under scroll wheel, BTN_SIDE for the button on the > side of the mouse and BTN_FORWARD for the 2nd button on the side. This > means that not all mice have these buttons in forward/back positions... Well that mouse seems to be quite special, most mice with more then 3 buttons have the back/forward side buttons as buttons 4 and 5. Although I myself in the meantime have found a no-name bluetooth mouse which has button 4 forward button 5 back (so swapped compared to what userspace expects). I will try to borrow some mice with more then 3 buttons from people at my local hackerspace and see if there is some consistency there, at least the microsoft page I pointed to suggests that what my logitech mice are doing is the default Windows behavior. And I just plugged the receiver for these Logitech mice into a win10 machine and without installing any logitech drivers, the buttons correctly do back / forward in the webbrowser there. I also tried the bt mouse with the swapped buttons with win10 and indeed the buttons work in the wrong direction there, so that seems to be a hardware-misfeature of that mouse. So it still seems to me that the default Linux userspace behavior (at least in Firefox) with button 4 is back and button 5 is forward is correct. Still I find the existing BTN_FORWARD and BTN_BACK defines misleading and as e.g. the m560 code in drivers/hid/hid-logitech-hidpp.c shows various drivers to try to use them as if userspace will actually interpret them the way the name suggests, which it typically will not. So maybe just rename all 5 defines for buttons 4-8 to BTN_EXTRA1 - BTN_EXTA5? With maybe a comment that userspace may interpret BTN_EXTRA4 as back and BTN_EXTRA5 as forward (or maybe even BTN_APP_FORWARD and BTN_APP_BACK aliases?) ? And then as Peter suggested in another thread about other extra mouse buttons (on which you were not Cc-ed) we really need userspace to have a mapping UI for special / broken cases. Regards, Hans
On Thu, Apr 04, 2019 at 10:00:38AM +0200, Hans de Goede wrote: > > I played with my Evoluent Vertical mouse and I found that it reports > > BTN_EXTRA for button under scroll wheel, BTN_SIDE for the button on the > > side of the mouse and BTN_FORWARD for the 2nd button on the side. This > > means that not all mice have these buttons in forward/back positions... > > Well that mouse seems to be quite special, most mice with more then 3 > buttons have the back/forward side buttons as buttons 4 and 5. > > Although I myself in the meantime have found a no-name bluetooth mouse > which has button 4 forward button 5 back (so swapped compared to what > userspace expects). > > I will try to borrow some mice with more then 3 buttons from people at > my local hackerspace and see if there is some consistency there, > at least the microsoft page I pointed to suggests that what my logitech > mice are doing is the default Windows behavior. > > And I just plugged the receiver for these Logitech mice into a win10 > machine and without installing any logitech drivers, the buttons > correctly do back / forward in the webbrowser there. > > I also tried the bt mouse with the swapped buttons with win10 and > indeed the buttons work in the wrong direction there, so that seems > to be a hardware-misfeature of that mouse. > > So it still seems to me that the default Linux userspace behavior > (at least in Firefox) with button 4 is back and button 5 is forward > is correct. > > Still I find the existing BTN_FORWARD and BTN_BACK defines misleading > and as e.g. the m560 code in drivers/hid/hid-logitech-hidpp.c shows > various drivers to try to use them as if userspace will actually > interpret them the way the name suggests, which it typically will not. > > So maybe just rename all 5 defines for buttons 4-8 to BTN_EXTRA1 - BTN_EXTA5? > > With maybe a comment that userspace may interpret BTN_EXTRA4 as back and > BTN_EXTRA5 as forward (or maybe even BTN_APP_FORWARD and BTN_APP_BACK aliases?) ? might be better, because both xorg-evdev and libinput enumerate the buttons as they are available on the device. So in the niche case that you have only BTN_APP_FORWARD but not BACKWARD, that button will be button number 8 (i.e. back) and BTN_EXTRA1 would be 9 (i.e. forward) . Cheers, Peter > > And then as Peter suggested in another thread about other extra mouse buttons > (on which you were not Cc-ed) we really need userspace to have a mapping UI > for special / broken cases. > > Regards, > > Hans
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 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
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- 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(-)