atar-axis / xpadneo

Advanced Linux Driver for Xbox One Wireless Controller (shipped with Xbox One S)
https://atar-axis.github.io/xpadneo/
GNU General Public License v3.0
1.92k stars 111 forks source link

KEY_MODE doesn't work in userspace software #301

Closed gudvinr closed 2 years ago

gudvinr commented 3 years ago

Version of xpadneo

hid-xpadneo, 0.9.r78.g4fa679b, 5.12.14-arch1-1, x86_64

Controller Model

Other

Installed Software

Severity / Impact

Describe the Bug

When I hook up XBSC using USB-C cable, it uses standard xpad driver and detected as Generic X-Box pad.
That way Home button (BTN_MODE) works as expected but share button is not exposed at all.

If I connect same gamepad using BT, it gets recognized as two devices: Xbox Wireless Controller and Xbox Wireless Controller Consumer Control. Second one has both KEY_MODE and KEY_RECORD and evtest able to detect key presses but neither steam nor gamepad testers (both online and KCM one) are unable to recognize such events.

Steps to Reproduce

  1. Pair XBSC with PC using Bluetooth
  2. Launch gamepad-tester.com or Steam
  3. Press Home/Guide/Big-X button

Expected Behavior

Keypress gets detected by software (e.g. Steam launches BPM).

Screenshots / GIFs / Videos

System Information

# uname -a
Linux 5.12.14-arch1-1 #1 SMP PREEMPT Thu, 01 Jul 2021 07:26:06 +0000 x86_64 GNU/Linux
# xxd -c20 -g1 /sys/module/hid_xpadneo/drivers/hid:xpadneo/0005:045E:*/report_descriptor | tee >(cksum)
00000000: 05 01 09 05 a1 01 85 01 09 01 a1 00 09 30 09 31 15 00 27 ff  .............0.1..'.
00000014: ff 00 00 95 02 75 10 81 02 c0 09 01 a1 00 09 33 09 34 15 00  .....u.........3.4..
00000028: 27 ff ff 00 00 95 02 75 10 81 02 c0 05 01 09 32 15 00 26 ff  '......u.......2..&.
0000003c: 03 95 01 75 0a 81 02 15 00 25 00 75 06 95 01 81 03 05 01 09  ...u.....%.u........
00000050: 35 15 00 26 ff 03 95 01 75 0a 81 02 15 00 25 00 75 06 95 01  5..&....u.....%.u...
00000064: 81 03 05 01 09 39 15 01 25 08 35 00 46 3b 01 66 14 00 75 04  .....9..%.5.F;.f..u.
00000078: 95 01 81 42 75 04 95 01 15 00 25 00 35 00 45 00 65 00 81 03  ...Bu.....%.5.E.e...
0000008c: 05 09 19 01 29 0c 15 00 25 01 75 01 95 0c 81 02 15 00 25 00  ....)...%.u.......%.
000000a0: 75 01 95 04 81 03 05 0c 0a b2 00 15 00 25 01 95 01 75 01 81  u............%...u..
000000b4: 02 15 00 25 00 75 07 95 01 81 03 05 0f 09 21 85 03 a1 02 09  ...%.u........!.....
000000c8: 97 15 00 25 01 75 04 95 01 91 02 15 00 25 00 75 04 95 01 91  ...%.u.......%.u....
000000dc: 03 09 70 15 00 25 64 75 08 95 04 91 02 09 50 66 01 10 55 0e  ..p..%du......Pf..U.
000000f0: 15 00 26 ff 00 75 08 95 01 91 02 09 a7 15 00 26 ff 00 75 08  ..&..u.........&..u.
00000104: 95 01 91 02 65 00 55 00 09 7c 15 00 26 ff 00 75 08 95 01 91  ....e.U..|..&..u....
00000118: 02 c0 c0                                                     ...
2986910699 1363
kakra commented 3 years ago

The Steam client generally ignores single keyboard events for such hotkeys. It only uses BTN_MODE for this. The Xpad driver will expose it but it confuses some games if that button is present in the gamepad button mapping. MS itself doesn't expose it in the Windows driver: The gamepad has exactly 10 buttons there.

Thus, xpadneo creates a second device which has the buttons not part of the original 10 buttons map.

Currently, there are no immediate plans on finding a better solution, at least not before I implemented remapping and profile emulation. For a while, KEY_HOME or KEY_HOMEPAGE was working to launch Steam BPM but that seems to have been removed.

It's not like userspace isn't able to read these events, after all, evtest shows them, and even xev detects the Share button as Xf86AudioRecord. It's just that Steam ignores it and doesn't want to support that, partially maybe because there's no defined standard button for keyboards that could act as the Steam BPM/overlay hotkey. I tried a few that could work but none did, and a request for telling which keyboard hotkey could be used to do it has been ignored by Valve: https://github.com/ValveSoftware/steam-for-linux/issues/7101

gudvinr commented 3 years ago

@kakra I never had an issue with any other controller, wired or wireless.
Home button on DS4 works and steam recognizes it. SN30Pro+ also correctly recognized by Steam and other software.
Even XBSC works like that when you use it with cable because it hooks up to xpad module.

It's not like userspace isn't able to read these events, after all, evtest shows them

No, it doesn't show these events for joystick device. It shows them for separate input

It's just that Steam ignores it and doesn't want to support that

Is there any gaming-related software or libary that aware of xpadneo behaviour and works with "Consumer Control" device?

I understand motivation behind this decision. But while exposing mode button can hypothetically confuse some games, not exposing it breaks working software right now since it is not how other drivers work (at least with none of controllers that I have).

kakra commented 3 years ago

I understand motivation behind this decision. But while exposing mode button can hypothetically confuse some games, not exposing it breaks working software right now since it is not how other drivers work (at least with none of controllers that I have).

It's not hypothetically, it's a solution to a real problem that appeared during early development when games and SDL just mixed up the buttons due to this. Technically, that button is not a joystick button. It should never have been there in the Linux button map - or it should have been placed at another position.

Is there any gaming-related software or libary that aware of xpadneo behaviour and works with "Consumer Control" device?

Games don't need this button, it's a meta button. And the event is exposed to user-space - it should not matter to which device it belongs, you press a button, software can react to it. Steam, as an example, reacts to keyboard events to bring up the overlay even when you play with the gamepad. It just ignores it.

But I'm aware of the problem and there are ideas to fix it. I don't think it's anything worth to discuss decisions from the past that fixed a real problem. We need to move forward, user-space software has changed since then, so the problem is fixable. But it is not as easy as simply putting the button back in place: The HID patching we currently do doesn't allow for that: We won't find a mapping that works and keeps everything compatible, I tried, believe me.

I will re-do the HID patching logic but it will need some planning for the future features, it's scheduled for v0.11 or v0.12.

it breaks working software right now since it is not how other drivers work (at least with none of controllers that I have).

The Xbox Elite 2 controller has such a sub device, newer kernels will show three devices when connecting it: gamepad, keyboard, and consumer. This is even more broken because it exposes the button on both devices at the same time, actually depending on the firmware version. I decided to go with the superset of all controller features in xpadneo and expose them in a unified way so user-space doesn't need to learn about fixing things up (and probably getting into trouble when using old and new software at the same time where some know these fixups and some don't).

So xpadneo will get back to this topic and fix it but not at the current stage of planned features. In my mind, this is a minor annoyance only because user-space software could be easily made aware of just listening to a keyboard event - it doesn't need to come from the gamepad device, it's how things already work.

If you don't care about compliance, there's probably actually nothing you'd need from xpadneo: The Bluetooth mode is fully supported just right now in the kernel. It's just missing support for the rumble motors and maybe the rumble firmware crash work-around.

So the question is: What do you use in xpadneo that you're missing from other drivers?

gudvinr commented 3 years ago

@kakra As I said, I understand reasons behind all of this. Technically you're right. It shouldn't be like that. Yes, it is bad decision that breaks some games.
The thing is, your fixing of this bad decision also breaks user experience in other way that isn't compatible with behaviour of other drivers. It is also real problem that didn't exist before.

But "We need to move forward, user-space software has changed since then, so the problem is fixable" basically means that someone else must support new way of doing things in their software for this particular driver that isn't even mainlined. What are chances that someone will even bother to look into this "minor annoyance"? And will it even be possible to mainline such driver that works differently if software doesn't support it?

It might be minor if you encounter it once in a while, but when you need to use keyboard every time you need to launch BPM or open overlay it becomes really annoying.

So the question is: What do you use in xpadneo that you're missing from other drivers?

I only use xpadneo because it's only viable option for wireless xbox controller right now. But what I miss in xpadneo is mode button that works with every other driver.

kakra commented 3 years ago

Well, the misery already started with people fixing kernel driver inconsistencies in user-space. Instead of all those funky SDL controller maps, the drivers should be fixed to properly expose the device. Xbox controllers report 15 buttons to user space, and at least 4 of them are dead, this completely messes up the jsdev interface. The work-around the SDL people took doesn't allow for later fixups in the kernel, and it leaves jsdev in its broken state. We are now where we are.

There are two ways of fixing it: Either someone breaks the chain, or user-space needs to carry their fixing forever - with everyone repeating that effort who do not use one of the existing libraries which do it for them. It's a pile of fixups that spreads everywhere like cancer: Chrome has its fixups in its code-base for that (and they do it the worst way), SDL2 has it, wine has to work around it, closed-source games will simply not work with some models because they won't receive fixes. So, if you want to argue about involving people for fixups who should not be responsible for maintaining that: That route has already been taken.

What's really needed is a way for xpadneo to expose all controller models in the same way, and in a way to bypass the various fixups at several user-space layers and expose all buttons in a conforming standard way. The development is currently on route for this, once that is done, we can bring the Xbox button back in the standard device keymap.

If you want to use the quirky way with the quirky fixups, you can do that by simply not using xpadneo: All Bluetooth models are fully supported by the standard kernel drivers: xpadneo even isn't a Bluetooth driver, it's just a HID driver, we only build upon what the kernel already supports. It's just a matter of whether you use hid-generic (the default HID driver which knows nothing about sparse keymaps of the Xbox controllers and sees 15 buttons), hid-microsoft (which also doesn't fix the keymaps but at least it adds rumble support), or hid-xpadneo (which fixes the keymaps, fixes rumble stability, and adds battery support) - none of these have anything special about Bluetooth: That's handled completely by the kernel for all three drivers.

I only use xpadneo because it's only viable option for wireless xbox controller right now.

Then hid-microsoft is your driver of choice currently, you do not need xpadneo for the wireless part because xpadneo doesn't handle the wireless part - that's done by default kernel drivers (the HID module in the Bluetooth stack actually).

xpadneo was born when the controller was still handled by hid-generic only, and the keymaps were completely messed up for user-space (because it expected the 10 button layout of the USB controller). Thus, xpadneo fixed the keymaps to be compatible with user-space, but then came SDL and fixed that in user-space, too. The double-fixing made it messed up again, that's why xpadneo has some countermeasures to be excluded from SDL2 fixups, and stay compatible with older games expectations. Later came hid-microsoft and added rumble support which we already had.

So actually, there's nothing left for xpadneo. I was already thinking about porting the rumble fixups over to kernel upstream, and then be done with the project because there's no point in developing this any further if it's fully supported in the kernel already. But then, why not continue this to add features that other drivers don't have?

There's a bigger goal in the background I'm working on, implementing a standard-compliant HID driver on top of different transports (USB, GIP, Bluetooth), then eventually upstreaming that (similar to Sony's efforts for their new controller which will hopefully add trigger rumble infrastructure to the kernel). There's really no point in adding back the quirky behavior to xpadneo that we are trying to prevent in the first place: That's just duplicated efforts over the standard kernel drivers which DO support this gamepad.

The standard kernel driver will do what you expect from it, so KEY_HOME is still just a minor annoyance that we can bother with later when the implementation details of custom mapping profiles are settled. Adding that back completely prevents me from implementing my vision for the driver, we would be stuck at the same functionality the kernel driver already provides.

I'm not sure what makes xpadneo the only viable option for Xbox wireless controllers for you. The controller is handled by hid-microsoft in the kernel just fine and SDL2 and Steam will do all the needed keymap fixups for it.

Side note: Developers working for Valve wanted to upstream a fix for the Xbox button for Xbox Elite 2 controllers because it is part of a secondary HID application in the descriptor (which is exposed as a secondary input device), and asked for how and why I handle the button the way I do. We discussed that and the idea was dropped because it would break Android user-space. This is an example of how broken the kernel drivers really are already. IMHO, the only viable option is to redo a driver from the ground up with proper mappings - and that's what I'm trying to do. But I don't expect user-space to do fixups for xpadneo already at this stage, otherwise I may have made a patch for SDL2 already to support the secondary HID device. But I don't want to add fixups for a moving target, and xpadneo is still a moving target. There are a few other quirky fixups in xpadneo already that I rather get rid of again, the v0.11 or v0.12 feature plan is a way to get to that point, I expect we can reach a final stable version shortly after, then bother with adjusting to user-space, or patching user-space itself. The planned protocol layering will allow for more easy upstreaming to the kernel.

TL;DR If you care only about Steam and SDL2 compliant handling of the Xbox button, use the default kernel drivers instead of xpadneo. If they don't work for you, let's discuss, why. Maybe that's an opportunity for upstreaming xpadneo fixes to hid-microsoft.

gudvinr commented 3 years ago

@kakra ok. So, xpadneo works as expected and I understand why you did what you did.
Let's assume that SDL2 will be eventually patched to support "consumer control" device and Steam will support mode button even if it's pressed on secondary input device.

At that point, if xpadneo will be mainlined, whether as new driver or as improved version of existing one, it will provide correct keymap for gamepad. And that's OK.

But there's some issues with that.

So far, it was discussion about Xbox controllers, and XBSC in particular. Implementing correct behaviour in the driver of that particular controller won't solve issues you're describing. In fact, it will only lead to additional workarounds because developers would need to implement old quirks for other devices in addition to "correct" implementation. So it won't solve anything if xpadneo will end up as additional driver in kernel tree or will replace existing one.

But how is this possible to solve that issue? The only viable solution that I see is that every other driver must also implement this "correct" behaviour. In that case it would definitely break this chain and developers can safely rely on this new implementation.
But if every driver will be modified, it will break software in many unpredictable ways. I don't think that anyone will allow that, honestly. It is possible to cover this using feature flags but older software will be broken forever by this.

I don't mean it's bad or you're doing the wrong thing. Quite opposite, honestly. But I don't see how it's possible to solve these issues for good without introducing new joystick/gamepad interfaces in kernel. Because in other cases either xpadneo will end up with workarounds or software developers will be forced to modify libraries/games to handle "correct" driver implementation in addition to their workarounds.

kakra commented 3 years ago

Let's assume that SDL2 will be eventually patched to support "consumer control" device and Steam will support mode button even if it's pressed on secondary input device.

This will be decided later. I'm not against your suggestion to support the button the way user-space expects it. But I just don't want to do it now at this stage because it reverts every effort we did previously.

it will provide correct keymap for gamepad. And that's OK.

If anything will be mainlined as improvements/fixes to existing drivers, nobody will touch the keymap. Otherwise we'll be breaking user-space. That's probably why it hasn't been fixed already: Because SDL2 already deployed its fixups widely, and it's pretty easy for them because it's not hard-coded. But that's both a blessing and a curse for several reasons too complex to elaborate here.

In fact, it will only lead to additional workarounds because developers would need to implement old quirks for other devices in addition to "correct" implementation.

No, that's not a plan. The plan is to have a driver which requires no user-space fixups at all. In turn, this means xpadneo will provide the devices in a way that existing user-space fixups would be bypassed and user-space doesn't need any further adjustments at all.

But how is this possible to solve that issue? The only viable solution that I see is that every other driver must also implement this "correct" behaviour.

This is unrealistic and breaks existing user-space. The only solution is a soft migration: Newer drivers will expose the devices in a way that user-space fixups superseded by this would be bypassed. That's the only way I see, as mentioned above. But I still have to make up my head about how exactly to do this.

By any means I want to prevent breaking older games (SDL1, jsdev games, the current kernel drivers already break that, so xpadneo is an alternative here). If possible, I want to prevent breaking modern games. I don't count the Xbox button issue against that, because (a) it's final fate hasn't settled yet in xpadneo and there are plans that will eventually fix it without involving breaking old games and (b) it's not really a gaming button, it just puts you out of your comfort zone (for which there are plans to make that much more enjoyable, xpadneo will add mouse emulation).

without introducing new joystick/gamepad interfaces in kernel

The jsdev API is broken in the kernel since ages: HID drivers are not able to tell the jsdev interface the order of buttons and axes, that's derived from the evdev ID numbers. So actually, the evdev ID numbers are broken because BTN_MODE comes before button 9 and 10 on the gamepad, also the trigger axes are broken that way because they don't form a common x/y axes pair as games would expect. There's a way around it: I could move the triggers to ABS_GAS and ABS_BRAKE but then again, games that expect brake to be left and gas to be right are broken, because someone ordered those two in reverse in the evdev mapping in the kernel. Actually, evdev doesn't have enough gamepad button and axis positions for a fully fledged 10-button 6-axis gamepad to expose both a correct jsdev input ordering AND correct evdev symbols for user-space. The current approach is a compromise and documents how to fix jsdev manually on demand (but that breaks the Chrome gamepad API).

Only way around that would be to make xpadneo a pure input driver and exclude HID: That way we could create an input device and a joystick device ourselves. But I kinda like the layered approach that HID provides: xpadneo sits on the HID bus, it doesn't need to care about the transport the device is attached to, it's agnostic about whether the device is Bluetooth, GIP, or USB. The only reason it's not working for USB is because the original xpad driver in the kernel doesn't expose its devices as HID devices, and a GIP bus is even not in the kernel (the xow author is working on it). xpadneo is really just a mapping layer (in both directions, receiving input, and sending rumble) with some quirk handling to work around rumble-induced firmware crashes, not a real driver. You can just unload it and the original hid-generic or hid-microsoft will take over.

The primary goal is to prevent user-space to do any adjustments at all when using xpadneo. If they did so, it would break some of the features I want to implement (like custom and switchable mapping profiles). So it has to work without any SDL2 fixups needed at all while coexisting with the ones shipped in the wild.

The Xbox Elite 2 controller has 4 additional buttons at the underside (while there's no place for them in the original evdev map for gamepads, there's a mapping extension at the end with "trigger happy" buttons we could use but I won't add them before the mapping feature is in because I fear someone would submit borked SDL mappings then and nullify our efforts). These buttons are mapped to A,B,X,Y with the default hardware mapping profile. When I added support for those hardware profiles (which requires some reverse engineering), I'll add a software emulation layer to support it for other controller models, too. When that happened, we'll deal with the Xbox button more easily because everyone can simply fix it for different games by switching the profile then. I'll then ship with a more sane default that makes BPM work again.

But first, I want to make our hidraw reports compatible with SDL2 again - which should make your Xbox button instantly available again as you expect. If Steam uses SDL2 to watch for this button, it will instantly work again, too. But Valve doesn't tell me how they read this button in their closed-source client. I'm in contact with the developer of the input layer within Proton, the topic has been fully covered and I know where to look and adjust things for games, but I don't want to carry a silly work-around for the Xbox button throughout the various feature additions just to drop it again later when I already have a work-around by just moving it to a separate device - given that people start to submit SDL2 mapping to their database hindering us from improving on that topic.

Maybe you'll see now why this is much more complex than you thought. And secretly, I'm hoping that time fixes it: Valve already asked about the Xbox button handling for Elite 2 controllers, so they may implement support for it into the Steam client if we just wait long enough: It's actually not an xpadneo issue here: With the kernel drivers, the XBE2 controller has such a secondary device with KEY_MODE in the keymap.

So be sure: This issue will eventually be closed, the one or the other way. ;-)

TL;DR People submit SDL2 controller mappings even for controllers that don't need it. xpadneo already shares this fate.

kakra commented 3 years ago

BTW: Thanks for an elaborate discussion, I'll keep your concerns in mind.

gudvinr commented 3 years ago

Thanks for the thorough explanation.

As for your question, without xpadneo rumble doesn't work with default driver and axis mappings are wrong (e.g. triggers act like right stick axis)

kakra commented 3 years ago

This may be an outdated firmware or a stray SDL controller mapping. The axis should work fine within Proton. It may be the game bypassing mappings or trying to be smarter than user-space.

See, this is why I think kernel drivers should be fixed and authorative about mappings. And the Xbox button is one example that's a button that doesn't belong in the position Linux maps it.