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.87k stars 110 forks source link

SDL support of Share button and Paddles? #428

Closed slouken closed 1 year ago

slouken commented 1 year ago

Version of xpadneo

hid-xpadneo-v0.9-137-g13dd267

Controller Model

Connection mode

Describe your feature reqeust

Is your feature request related to a problem? Please describe. SDL just added tentative support for automatically detecting the paddles for the Xbox Elite 2 controller, and could do the same for the Share button, but currently has no way of detecting whether the connected controller has these features. https://github.com/libsdl-org/SDL/commit/b0677f476fa43f4a113b04a959228fd38f95d740

I also noticed that the Share button gets sent as a keyboard key and doesn't generate the KEY_RECORD that I would have expected from the evdev description.

Describe the solution you'd like It would be ideal if the real VID/PID of the controller were available, as well as EVIOCGBIT() reporting the actual buttons available.

It would also be nice if there were a way to report the Share button, but I understand that you're converting it to a keystroke for compatibility with other software. FWIW, Steam binds the Share button to screenshot by default, so you wouldn't be missing any functionality there and would make it possible for users to bind the button to other in-game functionality if you reported it as a controller button instead of a keyboard key.

If changes to xpadneo would benefit from code changes in SDL, feel free to create an issue or PR at the SDL repo and I'll be happy to discuss them.

smcv commented 1 year ago

If this driver's mapping of evdev capabilities is not the same as in-kernel drivers, then it would be very useful to have evemu-describe output for each supported device as a reference, similar to what's on https://github.com/libsdl-org/SDL/issues/7814 - that will let SDL's unit tests for gamepad detection/classification look at simulated xpadneo controllers without needing to have actual physical hardware, making it less likely that using SDL and xpadneo together will regress.

Similarly, if xpadneo presents its supported devices as HID controllers, it would be very useful to have a hex-dump of the HID descriptor (usually od -t x1 /sys/class/input/eventXX/device/device/report_descriptor) as a reference. SDL doesn't yet use HID descriptors as input to its classification of devices, but it could in future.

If these details are likely to change with future versions of xpadneo, then please mention the version number of xpadneo when generating them.

smcv commented 1 year ago

https://github.com/libsdl-org/SDL/issues/7801#issuecomment-1589114910 explains how to get that information and why SDL would benefit from having it.

kakra commented 1 year ago

@slouken Thanks for reaching out to me. Yes, the aspects you mention are there for historic reasons (mostly, to make jsdev software happy), and during v0.10 to v0.11 I want to eliminate these problems. The user-space stack has evolved enough that there is no need for work-arounds. jsdev compatibility will be ignored, jscal can be used to remap things (which breaks Chrome, btw).

This includes:

I also had a chat with Proton devs who told me there's native HID parsing support in Proton now, so everything should settle around proper HID support. We could in theory craft our own HID descriptor.

Without the extra features, xpadneo no longer provides any benefits over the in-kernel drivers (except we work around the rumble crash properly which ff-memless cannot do natively). So one step forward would be to submit patches to the kernel to request fixed time intervals from ff-memless (I'm currently working on that).

@smcv You mention that we expose the paddles as BTN_TRIGGERHAPPY. This is because the evdev button range of BTN_GAMEPAD is too small to fit all the inputs of the controller, and evdev devices are not allowed to bleed into the other ranges. There are a few hard-coded exceptions in the kernel which either leave it to user-space, or work around it in a similar way by shifting key codes to a different range. The reasoning behind this is: If a devices has a BTN_GAMEPAD, it's a gamepad. If it has BTN_JOYSTICK, it's a joystick. This is how it's meant for user-space to detect the type of device as far as I understand it. So if we bleed out of our range, we would bleed into BTN_DIGI which could make user-space to detect our device as a digitizer/pen. Technically, we could move two paddles to BTN_C and BTN_Z, there are controllers out there which have only two paddles (which are dead in Xbox mode but available in Switch mode). But that would completely mess up how SDL or Steam Input would handle the buttons. [1]

Thus, implementations using evdev should be prepared to handle sparse keymaps.

MS decided to not use sparse keymaps with the HID-mode of the controller, instead it ships with blind buttons in the keymap (BTN_C and friends). This was probably done to properly map buttons to evdev symbols on Android which wouldn't get updated drivers for all the Android devices out there. xpadneo went another route and actually made the bitmap sparse, but this confused some software while it fixed other software (except Chrome which uses evdev to detect mappings but then uses jsdev to read the device and counts positions), until SDL added support which also didn't respect sparse bitmaps by not looking at the actual evdev symbols, it suddenly broke a lot of games. We mostly fixed this by patching the HID descriptor and disable hidraw access but still had a few games that would rotate the last three buttons (because BTN_MODE is technically in the wrong position in the Linux bitmaps). That's why I also excluded that Guide button, later the Share button, too, and mapped those differently.

Mapping to F12 is just a stop-gap solution until we have native PID/VID and custom mappings in place.

So yes, xpadneo is working towards fixing that. But anyone who is not depending on special xpadneo features should really use the in-kernel drivers. That's what Steam Input expects, that's what SDL expects.

Until then, I would appreciate if our GUID 050003f05e0400008e02000030110000 would not be included into controllerdb because it just does not work properly with the report parser of SDL (the parser ignores evdev symbols and HID descriptors, and thus cannot handle sparse keymaps).

I'd be happy to support efforts in SDL to use evdev natively for most devices instead of using controllerdb (or at least have a controllerdb line which would say "use evdev"). With proper evdev symbol matching in place, most of the controllerdb entries should become obsolete. I don't think that HID parsing is the way forward (the kernel already does it and creates the evdev symbol mapping). While hidraw is nice to support devices without native kernel driver, evdev should probably be preferred if a native kernel driver is available - tho, I'm not sure how to detect that. Maybe by looking if hid-generic is used, and only then use hidraw if ACLs allow?

Looking forward to a solution which makes SDL, evdev, jsdev (optional), some emulators, and Steam Input happy. :-)

[1]: This is why the BTN_TRIGGER_HAPPY range has been introduced: To support game input devices which have more buttons than the joystick/gamepad ranges support. It's anecdotal: Because there was no space for additional joystick bits, and "joy" means "happy", a newly named bitmap range was placed at the end.

kakra commented 1 year ago

as well as EVIOCGBIT() reporting the actual buttons available.

@slouken Didn't know of that. I'll look into it, maybe we can fast-forward to better support that way.

Do you have an example of how to properly use it from user-space?

slouken commented 1 year ago

SDL's auto-mapping based on evdev capability bits has improved quite a bit, and at least for Xbox controllers I think will be able to replace the mapping database.

Here is how SDL gets the bits and figures out the mapping based on capabilities: https://github.com/libsdl-org/SDL/blob/2c3717881f3500ae73ece85bffb70dd868535dbf/src/joystick/linux/SDL_sysjoystick.c#L943 https://github.com/libsdl-org/SDL/blob/2c3717881f3500ae73ece85bffb70dd868535dbf/src/joystick/linux/SDL_sysjoystick.c#L1726 https://github.com/libsdl-org/SDL/blob/2c3717881f3500ae73ece85bffb70dd868535dbf/src/joystick/linux/SDL_sysjoystick.c#L2083

kakra commented 1 year ago

So from this, I'm guessing that it should return a bitmap of physically existing buttons (or rather, buttons that are able to generate events through the driver): https://github.com/libsdl-org/SDL/blob/2c3717881f3500ae73ece85bffb70dd868535dbf/src/joystick/linux/SDL_sysjoystick.c#LL956C1-L959C71

Feel free to find me on our Discord channel.

kakra commented 1 year ago

@slouken Regarding: https://github.com/libsdl-org/SDL/blob/2c3717881f3500ae73ece85bffb70dd868535dbf/src/joystick/linux/SDL_sysjoystick.c#LL2090C8-L2090C92

You can still see the original PID/VID when looking at the hidraw device from which the evdev device derives:

# realpath /sys/class/input/event24/device
/sys/devices/virtual/misc/uhid/0005:045E:0B22.001A/input/input32

# grep '^' /sys/devices/virtual/misc/uhid/0005:045E:0B22.001A/input/input32/id/*
id/bustype:0005
id/product:028e
id/vendor:045e
id/version:1130

# realpath /sys/class/hidraw/hidraw17
/sys/devices/virtual/misc/uhid/0005:045E:0B22.001A/hidraw/hidraw17

# "0B22" in the sysfs path vs "028e" in the id path
kakra commented 1 year ago

@slouken https://github.com/libsdl-org/SDL/blob/2c3717881f3500ae73ece85bffb70dd868535dbf/src/joystick/linux/SDL_sysjoystick.c#L2083

Yes, because these additional buttons would bleed out of the BTN_GAMEPAD range. xpadneo tries to put buttons at fixed locations on the bitmap [1], so the paddles will stay at their event code, and other buttons map to their actual symbol name.

If it helps, you can detect the driver from the sysfs path:

# realpath /sys/devices/virtual/misc/uhid/0005:045E:0B22.001C/driver
/sys/bus/hid/drivers/xpadneo

[1]: This code is problematic with that idea: https://github.com/libsdl-org/SDL/blob/2c3717881f3500ae73ece85bffb70dd868535dbf/src/joystick/linux/SDL_sysjoystick.c#L2102

Future xpadneo will move BTN_SHARE and other non-standard buttons to the start of the BTN_TRIGGER_HAPPY range.

Following the standard of Linux input, you can expect the controller to have paddles, if BTN_TRIGGER_HAPPY37 is set (in a later version of xpadneo at least).

kakra commented 1 year ago

I think it may be time for an official kernel patch to add alias names to trigger happy, something like #define BTN_PADDLE_TOPRIGHT BTN_TRIGGER_HAPPY1 or something similar. We're clearly seeing a trend towards more or less de-facto standards for additional gamepad buttons.

kakra commented 1 year ago

@slouken That above commit should fix the bitmap for you. Only controllers having paddles will report the bits set now. I'll backport that to v0.9 if needed.

kakra commented 1 year ago

[1]: This code is problematic with that idea

@slouken I think I'll also expose the position of the profile button and trigger ranges. So this may end up in trigger happy, too. Your code should really not count the bits all across the trigger happy range but instead assume fixed positions. xpadneo may support gamepads in the future with only two paddles (I have such a model here for testing).

slouken commented 1 year ago

The 6.x kernel puts the paddles at BTN_TRIGGER_HAPPY5 - BTN_TRIGGER_HAPPY8, maybe that should be used instead?

kakra commented 1 year ago

The 6.x kernel puts the paddles at BTN_TRIGGER_HAPPY5 - BTN_TRIGGER_HAPPY8, maybe that should be used instead?

Oh then yes, will do. Do you know what happy 1 to 4 is used for then?

slouken commented 1 year ago

I have no idea. :) I just verified that the 6.1 kernel uses KEY_RECORD for the Share button, and that works well. I'm going to go ahead and update the SDL code to look for those symbols, assuming that future xpadneo versions will use those as well?

slouken commented 1 year ago

I went ahead and made that change in https://github.com/libsdl-org/SDL/commit/db1d4d3d76f5e21b2547463710b513fe0ebd7fad.

slouken commented 1 year ago

BTN_TRIGGER_HAPPY1 - BTN_TRIGGER_HAPPY4 are the D-PAD buttons when they're reported as buttons instead of a hat: https://github.com/torvalds/linux/blob/15adb51c04ccfcf58ac3eec7464a2768fe175fcc/drivers/input/joystick/xpad.c#L760

kakra commented 1 year ago

I have no idea. :)

But I have: This actually followed a discussion I had with the xpad author, and I suggested to move that to the happy bits instead of bleeding out in the gamepad range. But I suggested to leave some space for additional future buttons (e.g., Share). Now it follows the d-pad directly.

kakra commented 1 year ago

I went ahead and made that change in libsdl-org/SDL@db1d4d3.

See my comments on that commit, the devil is in the details. ;-)

kakra commented 1 year ago

I just verified that the 6.1 kernel uses KEY_RECORD for the Share button, and that works well. I'm going to go ahead and update the SDL code to look for those symbols, assuming that future xpadneo versions will use those as well?

I had KEY_RECORD but it never worked because user-space isn't prepared to see the consumer device range on regular input devices (rather on remote controls). And then, this bit is not allowed on a gamepad device.

The original controller firmware for XBE2 had that on a sub device (consumer control), currently it may show up on the gamepad device but it doesn't work there except user-space explicitly looks for it. So yes, SDL2 could support this but it's really not the way it should be. I find no code that explicitly uses that code so it probably comes from a broken HID device descriptor if it appears in the gamepad application of the descriptor.

slouken commented 1 year ago

It looks like KEY_RECORD is being explicitly used for this purpose: https://github.com/torvalds/linux/blob/15adb51c04ccfcf58ac3eec7464a2768fe175fcc/drivers/input/joystick/xpad.c#L1891

kakra commented 1 year ago

Yes, but that code should probably not have gone into the kernel then. I found no way to bind KEY_RECORD in the desktop, neither in KDE shortcuts/hotkeys nor in Steam. It is just ignored because it doesn't belong there. It's a consumer control key as far as I could figure out. Documentation about the purpose of each event code is quite sparse.

Maybe xpad should be fixed to report it as BTN_TRIGGER_HAPPY<nextfreenumber>, and then SDL2 and xpadneo can follow?

slouken commented 1 year ago

Possibly. It does function in SDL, and by virtue of that will work in Steam.

We'll need to continue supporting it because of the long line of 6.x kernels that report that key, but we can certainly add a new one if it gets changed in the future.

kakra commented 1 year ago

Okay, I'll do some tests and update my README then. Which version of SDL2 will have the new paddle detection so I can reference that in the README?

slouken commented 1 year ago

2.28 is about to be released with the new paddle support.

kakra commented 1 year ago

@slouken Okay I found you mentioned 2.28 in the other linked issue report. I'll use that and add a tag line to close this report then, unless it isn't fixed for you. Is that okay for you?

I'd like to defer resolving the KEY_RECORD issue until after xpadneo v0.10 because that branch currently still uses sub devices.

slouken commented 1 year ago

Sure. On my end SDL has been upgraded so auto-mapping works with the 6.x xpad driver, and it sounds like it will work with xpadneo in a future release, so that sounds great.

Thanks!

slouken commented 1 year ago

FYI, because current versions of xpadneo report KEY_RECORD in the caps bits, SDL 2.28 will say that all controllers have the share button.

kakra commented 1 year ago

FYI, because current versions of xpadneo report KEY_RECORD in the caps bits, SDL 2.28 will say that all controllers have the share button.

Yep, currently released v0.9 stable still has this, but (see below)

next v0.9 will remap to happy 1, see:

(it was reported as not working with KEY_RECORD by multiple people, primarily emulator users)

and v0.10 will remap to F12 because it introduces sub-devices. I'll not revert that at this stage. xpadneo will actually create a keyboard device additionally to the gamepad device. This will be needed for keyboard support (the xbox controllers can actually be keyboards, XBE2 has a full 127 keymap in the descriptor by default). But I'll see what we can do about the Share button before releasing v0.11.

That change mentioned above for moving to happy 1 is still queued - I could still change it. I just now realized it collides with xpad's dpad mapping. I should maybe move it to a different bit then but OTOH it must come before the paddles, otherwise it will still be broken in said emulators (thus why I suggested xpad to move the paddles further to the end of the happy bits).

Or could your code handle xpadneo having Share on happy 1, even when xpad uses it for dpad?

Update:

Actually, no, current v0.9 already uses happy 1, why do you see KEY_RECORD? And v0.10-dev already uses F12.

kakra commented 1 year ago

But you're right:

Input device name: "Xbox Wireless Controller"
Supported events:
  Event type 0 (EV_SYN)
  Event type 1 (EV_KEY)
    Event code 167 (KEY_RECORD)
    Event code 304 (BTN_SOUTH)
    Event code 305 (BTN_EAST)
    Event code 307 (BTN_NORTH)
    Event code 308 (BTN_WEST)
    Event code 310 (BTN_TL)
    Event code 311 (BTN_TR)
    Event code 314 (BTN_SELECT)
    Event code 315 (BTN_START)
    Event code 316 (BTN_MODE)
    Event code 317 (BTN_THUMBL)
    Event code 318 (BTN_THUMBR)

I'll investigate that after some sleep to think about it. ;-)

smcv commented 1 year ago

So from this, I'm guessing that it should return a bitmap of physically existing buttons (or rather, buttons that are able to generate events through the driver)

Yes please! The capabilities bitmaps are the key thing for recognising what a gamepad or other evdev input device is, and what it can do. Wherever possible, evdev input consumers like SDL need to be able to assume that a button appears in the bitmap if and only if it physically exists on the device and can generate events.

BTN_TRIGGER_HAPPY1 - BTN_TRIGGER_HAPPY4 are the D-PAD buttons when they're reported as buttons instead of a hat: https://github.com/torvalds/linux/blob/15adb51c04ccfcf58ac3eec7464a2768fe175fcc/drivers/input/joystick/xpad.c#L760

That seems like a kernel (xpad) bug - surely that's what BTN_DPAD_UP to BTN_DPAD_RIGHT are meant to be for? According to our test data in SDL, the in-kernel drivers for Dualshock 3 (PS3), Steam Controller and Wii U Pro Controller report those.

Being able to make statements like this without having the physical hardware on my desk, or the driver loaded, is one of the reasons why I'd like to have evemu-describe output and the raw HID descriptor for xpadneo devices available as a reference.

You mention that we expose the paddles as BTN_TRIGGERHAPPY. This is because the evdev button range of BTN_GAMEPAD is too small to fit all the inputs of the controller, and evdev devices are not allowed to bleed into the other ranges

That's fine, gamepad controller buttons should certainly be in one of the ranges that the kernel reserves for gamepads. If there's no better place to put "extension" event codes, then it will have to be BTN_TRIGGER_HAPPYn. I was mainly confused about why the in-kernel xpad driver was using 5 to 8 for the paddles, rather than the more obvious 1 to 4 - but if it's (mis?)using 1 to 4 for the digital d-pad on some devices, then that would explain that interpretation.

So if we bleed out of our range, we would bleed into BTN_DIGI which could make user-space to detect our device as a digitizer/pen.

Yes, definitely don't do that.

I think it may be time for an official kernel patch to add alias names to trigger happy, something like #define BTN_PADDLE_TOPRIGHT BTN_TRIGGER_HAPPY1 or something similar. We're clearly seeing a trend towards more or less de-facto standards for additional gamepad buttons.

I thought the intention of the BTN_TRIGGER_HAPPY range was that they're explicitly for non-standardized, controller-specific buttons, so it might be better to define new BTN_GAMEPAD_BACK_LEFT1, ..., BTN_GAMEPAD_BACK_RIGHT1, ... in a different numeric range, documenting them as numbered from the top down (so the Steam Controller would ideally have BACK_LEFT1 and BACK_RIGHT1, and Xbox Elite controllers with 4 back buttons would reuse those for the top pair and additionally have BACK_LEFT2 and BACK_RIGHT2 for the bottom pair). Or they could be numbered from the bottom up, or whatever - it doesn't matter as long as there's a documented convention in https://www.kernel.org/doc/html/latest/input/gamepad.html.

I suspect that it might only be a matter of time before we see controllers with 3 or 4 back buttons per side (if you hold the controller with index fingers on the triggers and 3 fingers underneath, then 3 back buttons per side would be reasonably comfortable to use), so reserving space for more and counting them from the top or bottom (perhaps in the 0x22x range just above BTN_DPAD_?) is probably safer than assuming we can always describe them as top/bottom left/right.

The Steam Controller currently represents its back buttons as GEAR_DOWN and GEAR_UP, of all things...

Following the standard of Linux input, you can expect the controller to have paddles, if BTN_TRIGGER_HAPPY37 is set (in a later version of xpadneo at least).

Where is that standard documented / what standard are you referring to here? I don't see anything in https://www.kernel.org/doc/html/latest/input/gamepad.html assigning any specific interpretations to the TRIGGER_HAPPY range.

I'd be happy to support efforts in SDL to use evdev natively for most devices instead of using controllerdb (or at least have a controllerdb line which would say "use evdev"). With proper evdev symbol matching in place, most of the controllerdb entries should become obsolete. I don't think that HID parsing is the way forward (the kernel already does it and creates the evdev symbol mapping).

Unfortunately, evdev doesn't give us enough information to distinguish between devices that we really should be able to distinguish - a prominent example is that a 3-axis accelerometer like the one in the Wii Remote is indistinguishable from a set of 3 driving-simulator pedals - so that is not going to be the full answer. "Most" gamepads have sensible evdev mappings, but many don't. We're collecting test data in SDL's test/testevdev.c, currently only used for testing the "is this a joystick?" heuristic, but in future I'd like to use that same test data to assert that we default to the desired mappings as well.

To be able to do that, it would be really, really useful to have full evemu-describe output for the devices that xpadneo supports.

smcv commented 1 year ago

use evdev natively for most devices instead of using controllerdb (or at least have a controllerdb line which would say "use evdev"). With proper evdev symbol matching in place, most of the controllerdb entries should become obsolete

This also only works if the kernel-side drivers reliably map physical buttons to evdev in a way that makes sense, but our test data in SDL indicates that there is a disappointing amount of nonsense, whether that's a result of the kernel repeating nonsense found in the HID descriptor or making up its own.

kakra commented 1 year ago

Where is that standard documented / what standard are you referring to here? I don't see anything in https://www.kernel.org/doc/html/latest/input/gamepad.html assigning any specific interpretations to the TRIGGER_HAPPY range.

Okay, I did not properly express what I was meaning by that. I'm referring the standard of interpreting a single set bit as assuming we see some kind of device, like in "if it has BTN_GAMEPAD, it is a gamepad". In that sense I meant "following the standards", I want SDL to assume "if this bit is set, it has paddles". But we now settled with happy 5-8, the same as xpad, and SDL2 no longer scans the whole range of bits but looks for specific positions. I think that makes more sense.

To be able to do that, it would be really, really useful to have full evemu-describe output for the devices that xpadneo supports.

I have the following devices on my desk and create such data:

Is there an open issue where I can report that data?

kakra commented 1 year ago

Yes please! The capabilities bitmaps are the key thing for recognising what a gamepad or other evdev input device is, and what it can do. Wherever possible, evdev input consumers like SDL need to be able to assume that a button appears in the bitmap if and only if it physically exists on the device and can generate events.

This was partially a bug which is fixed now: The capability bitmap is filled properly now, except I need to look into KEY_RECORD.

smcv commented 1 year ago

I'm referring the standard of interpreting a single set bit as assuming we see some kind of device, like in "if it has BTN_GAMEPAD, it is a gamepad".

I believe that convention is only meant to apply to rather broad device categories, namely joysticks and gamepads (which are implicitly assumed to have at least one button). The rest of the bitfield is capability discovery - now that we know we're looking at a gamepad, does it have a TL button or a clickable left thumbstick or whatever? - which only really works if the button has a well-known conventional purpose, and device drivers follow it.

To be able to do that, it would be really, really useful to have full evemu-describe output for the devices that xpadneo supports.

Is there an open issue where I can report that data?

Not at the moment, please could you open one? I hope @slouken won't mind us using https://github.com/libsdl-org/SDL/issues for this. One issue for the whole batch (which we can close when the test data has been added), with one comment per physical device, is probably a good way to organise them.

slouken commented 1 year ago

I hope @slouken won't mind us using https://github.com/libsdl-org/SDL/issues for this. One issue for the whole batch (which we can close when the test data has been added), with one comment per physical device, is probably a good way to organise them.

Sure, that would be fine. Thanks!