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
2.02k stars 114 forks source link

SDL update adjusts Xbox One S Android mapping #27

Closed kakra closed 6 years ago

kakra commented 6 years ago

With changeset http://hg.libsdl.org/SDL/rev/6aade58208aa libSDL adjusted the mapping to correctly map the original Android mapping of the controller which renders xpadneo again unusable with games using libSDL.

The most simple solution currently is to export the xpadneo mapping into your environment:

export SDL_GAMECONTROLLERCONFIG="\
050000005e040000fd02000003090000,Xbox One Wireless Controller,\
a:b0,b:b1,back:b6,\
dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,guide:b8,\
leftshoulder:b4,leftstick:b9,lefttrigger:a2,leftx:a0,lefty:a1,\
rightshoulder:b5,rightstick:b10,righttrigger:a5,rightx:a3,righty:a4,\
start:b7,x:b2,y:b3,"

My current solution is to add this mapping to both my Steam installations (native and Wine) in $STEAM_INSTALL_DIR/config/config.vdf. It works in Wine only when Wine was compiled with SDL support but without you usually don't have support for the controller in xinput games anyways.

Could there be a better (more automatic) solution?

Is it possible that using xpadneo could result in a unique controller ID for SDL? I'm not sure what it is build from but it consists of four 32-bit words in LSB order:

unknown1(05000000)+vendor(5e040000)+product(fd020000)+unknown2(03090000)

xpadneo should probably not change the vendor+product part but maybe it could change what SDL sees for the both unknown values? E.g., SDL maps joydev/xinput devices to unknown1(03000000) it seems.

Reverting exposed button mapping in xpadneo seems no option as then /dev/input/js0 will be broken again.

As a start, we could add the needed env var in the README.

Update: It seems to be bustype+vendor+product+version, see http://hg.libsdl.org/SDL/file/f162f5d7798d/src/joystick/linux/SDL_sysjoystick.c#l226

Maybe version could be faked from the xpadneo driver?

atar-axis commented 6 years ago

Hey there, I am a bit busy at the moment but I will take a look at that within the next days (hopefully).

You are always welcome to change the manual, especially on those more higher-level topics :)

Regarding the string: looks like the same as the "kernel string" in UDEV, bus:vid:pid:instance. Where instance is nothing more than a running number, not sure if it is the same here - we will see..

Cheers!

kakra commented 6 years ago

Instance is not a running number. It looks like it's an interface identifier. A USB device can expose multiple interfaces, and this number identifies it. See it more like a revision number of an API. I'm closing this as I put a pull request instead.

atar-axis commented 6 years ago

It looks like it is possible to manipulate this version value by the id.version entry of the input_dev struct.

atar-axis commented 6 years ago

It's just a one-liner to change the version to whatever value you prefer, like so:

cat /proc/bus/input/devices 

I: Bus=0005 Vendor=045e Product=02fd Version=dead
N: Name="Xbox Wireless Controller"
P: Phys=34:02:86:71:55:0b
S: Sysfs=/devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:045E:02FD.0009/input/input35
U: Uniq=c8:3f:26:ab:64:c0
H: Handlers=event15 js0 
B: PROP=0
B: EV=20001b
B: KEY=7cdb000000000000 0 0 0 0
B: ABS=3003f
B: MSC=10
B: FF=107030000 0

If we do so, would this realy solve that problem? Does the correction depend on the Version string too? And furthermore: Why the heck does SDL fix something on it's layer which is cause by a lower one?

atar-axis commented 6 years ago

https://github.com/atar-axis/xpadneo/tree/input_version

Would really appreciate if you can give it a try - I don't know too much about SDL and the other high-level layers :)

kakra commented 6 years ago

Why the heck does SDL fix something on it's layer which is cause by a lower one?

It think it is the most obvious way at a first glance, and the one with best chances to propagate fast to all deployed systems out there. Kernel changes are much slower, and getting patches accepted in the kernel is also a slow task, especially when it would touch things like l2cap handling. The next problem with acceptance is that software already relies on this wrong mapping, i.e. Android. Fixing the mapping at xpadneo layer would revert what Microsoft did in the first place to adjust the mappings to Android expectancy, thus destroying compatibility with Android. I'm not sure why Android was designed to expect such uncommon mappings... Maybe some marketing requirement aka "we don't want users to be able to use their existing controllers, they should buy new ones". OTOH, Android isn't forced to include xpadneo in its kernel.

Now, with xpadneo having fixes in place at the correct layer, it becomes even more complicated. We would now re-fix things that were already fixed, thus introducing wrong behavior again. That is really bad UX.

Also, this behavior is hard to understand because different pieces of software either make the strange mapping visible or not, depending on how they look at the events, and depending even on which API they are using. This probably has been an issue with other input devices out there in the past when looking at the huge library of SDL gamepad bindings hard-coded into libSDL. So it was most obvious to just add this controller, too.

If we do so, would this realy solve that problem? Does the correction depend on the Version string too?

Yes, SDL compares the version number in the identifier. With a changed version, the gamepad would appear as unknown device to SDL and thus inherit a (correct) standard gamepad mapping. I'm not sure if this would also affect feature detection of some software but I don't think so.

I'll try your changes later but I guess this won't be before weekend. But I'm eagerly looking forward trying it. :-)

atar-axis commented 6 years ago

Well, yes - to be honest, even the driver is the wrong layer, the correct one would be the firmware^^ But we cannot change the firmware and - since there is nothing between the firmware and the driver - it is legit to fix it at the driver-layer. But it is definitely not on such a high level as where the SDL is.

That's something I really don't like about OpenSource software - way too much Entropy.

The question is, is is really a good idea to create another workaround? What we do is that we fix something at a low level which is the result of a high-level fix, which is itself the resut of a low-level misbehaviour.

The cleanest solution would be to somehow change SDL in a way, that it also takes care of the driver in use, something like bus:vid:pid:version:driverUID. Most probably that's too much effort, right?

A mapping which does not map anything (the one I called none at the other issue), would result in combination with SDL in the correct behaviour, wouldn't it? If I understood you correctly, the problem then is: others, who do not rely on SDL again have a wrong mapping then.

So yes, changing the version seems to be the only doable solution - it is a clever one without doubt, but it is ugly, isn't it? If we really decide to do so, then we definitely have to make that optional (as a parameter) and explain it thoroughly on the already existing SDL part at the README.

kakra commented 6 years ago

Well, SDL already has a platform tag in the mappings. Actually, the mapping should look like

export SDL_GAMECONTROLLERCONFIG="\
050000005e040000fd02000003090000,Xbox One Wireless Controller,\
platform:Linux,\      # <--- HERE
a:b0,b:b1,back:b6,\
dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,guide:b8,\
leftshoulder:b4,leftstick:b9,lefttrigger:a2,leftx:a0,lefty:a1,\
rightshoulder:b5,rightstick:b10,righttrigger:a5,rightx:a3,righty:a4,\
start:b7,x:b2,y:b3,"

So it could also add something like driver:xpadneo. But this won't work out well because you want to blacklist by driver not select by driver. Otherwise you would need to add all the drivers out there (xpad, xboxdrv, whatever) that need mapping correction. Maybe it could add blacklist:xpadneo.

But that probably adds another layer of detection to libSDL and makes it overly complex. Drivers in Linux already fix a lot of quirks by patching reports. I think it's safe to stick with that.

I suggest that the driver should have a modparam or sysfs var to set the version id instead of hard-coding it. The default is whatever the device provides. This way you can circumvent version id collisions later if firmware is updated. We then document when changing the mapping type it is recommended to also change the version identifier so SDL sees it as a new, unknown device. It would also allow to arbitrarily switch version ids with mappings and keep whatever SDL fixups you have in place and adjust automatically. I'm not sure but Steam may also use these mappings from the config to apply custom mappings to the controller, so it will see different controllers if you change the mapping. The driver probably needs to detach and attach the device node so it get's properly redetected and seen by udev. The jscal routines also support adjusting mappings and restore those upon udev events. But I think those only look at vendor/product id:

$ cat /var/lib/joystick/joystick.state
NAME="Logitech Gamepad F710"
VENDOR="046d"
PRODUCT="c21f"
jscal -u 8,0,1,2,3,4,5,16,17,11,304,305,307,308,310,311,314,315,316,317,318
jscal -s 8,1,16,-128,128,16513,16513,1,16,-128,128,16513,16513,1,0,127,127,4227330,4227330,1,16,-128,128,16513,16513,1,16,-128,128,16513,16513,1,0,127,127,4227330,4227330,1,0,0,0,536870912,536870912,1,0,0,0,536870912,536870912

NAME="Xbox Wireless Controller"
VENDOR="0a12"
PRODUCT="0001"
jscal -u 8,0,1,2,3,4,5,16,17,11,304,305,307,308,310,311,314,315,316,317,318
jscal -s 8,1,255,28672,36862,21844,21844,1,255,28672,36862,21844,21844,1,3,448,574,1394469,1394469,1,255,28672,36862,21844,21844,1,255,28672,36862,21844,21844,1,3,448,574,1394469,1394469,1,0,0,0,536870912,536870912,1,0,0,0,536870912,536870912

Thus, it won't integrate nicely for jsX interfaces.

About the firmware: Well, the controller already has a fixed firmware - unless you paired it in Linux. ;-) So the driver is actually not the worst place to undo this dumb behavior. I still wonder how it distinguishes if it was paired to the Windows bluetooth stack or the Linux bluetooth stack. A possible fix could be to instead grab the problem by its roots and make the gamepad think it's pairing with Windows. But I'm not sure if that could be done.

Maybe the problem would be gone if the device was paired to the dongle that came with it. It's then using wifi instead of bluetooth. Apparently, a working Linux driver for this dongle seems to be missing.

But even then it may not work because according to my research, Microsoft changed something with a firmware update that requires a driver update. Otherwise you'd see messed up controls there, too. So maybe even Microsoft fixes up the mappings in the driver to be compatible with games.

I never tried to force Windows mode of the gamepad in Linux (by copying the pairing key from Windows over to Linux), so every theory here is pure guessing. ;-)

kakra commented 6 years ago

The question is, is is really a good idea to create another workaround? What we do is that we fix something at a low level which is the result of a high-level fix, which is itself the resut of a low-level misbehaviour.

Woah, fix-ception! :-D

I think it's okay because it's a driver thing. You either load the non-correcting driver = SDL correction applied, or you load the correcting driver = SDL correction does not apply.

kakra commented 6 years ago

Another idea (mind blow): If the driver undoes this dumb remapping, isn't it actually identical to Windows mode then? Instead of patching the version id, we could patch the product id. SDL then sees it as a normal behaving Xbox One S controller.

atar-axis commented 6 years ago

Another idea (mind blow): If the driver undoes this dumb remapping, isn't it actually identical to Windows mode then? Instead of patching the version id, we could patch the product id. SDL then sees it as a normal behaving Xbox One S controller.

kai, that's a really really really good idea! let me think about that for a while - at first glance I love it, would possibly make all those workaround needless.

kakra commented 6 years ago

The only fear I have with this: How does it interact with the various processes in kernel and user space which handle those ids? Like udev, listing in dmesg etc? Also, it feels a bit wrong to emit a different product id than is actually used on the "wire". Plus, will Linux still communicate correctly to the device if you patch the product id?

atar-axis commented 6 years ago

To me it feels "more correct" than all those other workaround here and there - a lot of birds with one stone... We will see if we can come up with a clean solution, I definitely have to dig a bit more into the kernel now. I shouldn't be at all surprised if there is a hook somewhere to do exactly that.

atar-axis commented 6 years ago

Doing this in a proper way is harder than I thought... we can easily change the vendor id inside the HID probing hook, but that does of course only affect everything up from the HID layer since xpadneo is a hid-driver,... that's what you where talking about @kakra probably, right?

I think that that wouldn't be a problem since all layers above (those in the userspace at least) communicate with the driver exclusively, so nobody "would know" that the product_id is not the same on lower layers.

Unfortunately you can see the "original Id" e.g. on the sysfs path of the device:

I: Bus=0005 Vendor=dead Product=beef Version=0903
N: Name="Xbox Wireless Controller"
P: Phys=34:02:86:71:55:0b
S: Sysfs=/devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:045E:02FD.0006/input/input32

I fear that our driver is too high level to change that, the driver stack is somehow like that:

Hardware (Gamepad)

    ⬍ Low Level Communication, Plug `n Play Detection, ...

I/O Driver (Bluetooth L2CAP)

    ⬆ Register to get informed when new devices are added

Transport Driver (e.g. BT-HIDP)

    ⬇ Register new devices

HID core

    ⬍

Custom driver: xpadneo.

    ⬍

Input Subsystem

    ⬍

Userspace

The transport driver is responsible for allocating HID device objects and registering them with HID core (see here). It sets the VendorId, ProductId and so on. If we would write our own transport driver, then we would possibly be able to modify the productId before the device gets registered.

This is what is done by hid-logitech-dj. This driver is modifying e.g. the country tag.

I do not know if it is realy worth it to write an own transport driver just to change the productId and I am simply missing time to work in at the moment. I am not even sure that changing the productId in the transport driver would even result in a systemwide change - maybe there are other layers in between which I don't know.

Maybe I also overlooked something which would make it easier? At least I am relatively sure that it is too late to change the productId cleanly when xpadneo is loaded (since HIDP already registered the gamepad using the original productId when xpadneo is being loaded).


To sum it up:

I would go for changing the versionId to a value which isn't yet used (+ a parameter to change it by hand in case of n collision). I will keep digging here and there.

atar-axis commented 6 years ago

I manipulated HIDP in order to replace any vendor_id == 0x45E by 0xDEAD, this is the result after pairing the controller:

$ cat /proc/bus/input/devices 
I: Bus=0005 Vendor=dead Product=02fd Version=0903
N: Name="Xbox Wireless Controller"
P: Phys=34:02:86:71:55:0b
S: Sysfs=/devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:DEAD:02FD.0003/input/input26
$ dmesg
[   23.727692] hid-generic 0005:DEAD:02FD.0003: unknown main item tag 0x0
[   23.728364] input: Xbox Wireless Controller as /devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:DEAD:02FD.0003/input/input26
[   23.734909] hid-generic 0005:DEAD:02FD.0003: input,hidraw2: BLUETOOTH HID v9.03 Gamepad [Xbox Wireless Controller] on 34:02:86:71:55:0b
$ udevadm info -a -p  /devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:DEAD:02FD.0004

Udevadm info starts with the device specified by the devpath and then
walks up the chain of parent devices. It prints for every device
found, all possible attributes in the udev rules key format.
A rule to match, can be composed by the attributes of the device
and the attributes from one single parent device.

  looking at device '/devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:DEAD:02FD.0004':
    KERNEL=="0005:DEAD:02FD.0004"
    SUBSYSTEM=="hid"
    DRIVER=="hid-generic"
    ATTR{country}=="21"

That's something like a proof-of-concept that - developing our own transport driver would most probably enable us to manipulate the product_id on the lowest level possible. At least if I understood it correctly.


I still prefer the version workaround.

kakra commented 6 years ago

The version approach has the least impact. Especially after you prefer to not include the force feedback code into hid-microsoft but built a separate small driver for non-complexity reasons.

atar-axis commented 6 years ago

The version approach has the least impact.

I'm right there with you. I will work on it as soon as you confirm (no hurry!) that the version thingy works as expected.

I tested it on my machine using http://www.generalarcade.com/gamepadtool/ and there it works like a charm :) With the altered version number the Controller is detected as a Xinput device (not as the Xbox One S Controller) - hopefully without side effects.

kakra commented 6 years ago

I have not tried it yet but since it's a hack, it shouldn't be enabled by default. So we should make an option enable_versionid_hack which is either empty (using the default versionid) or it's set to a hex value (thus overwriting the versionid). I consider the word hack important here, so anybody understands it's a hack. What do you think?

atar-axis commented 6 years ago

I would prefer to enable it by default to be honest, not everybody who is using it wants to understand the internals of the kernel, SDL etc. It should be "as easy as possible for as many users as possible".

On the other hand we may come up with a better idea - or SDL does (like including a driver tag as you mentioned above). But if this is the case, we can still disable it by default.

Is there any intereference or side-effect you know of (which is caused by a custom version#)?

kakra commented 6 years ago

Like in "firmware upgrades may go horribly wrong or don't work at all"? OTOH, I doubt that MS will any time soon offer firmware upgrades of the controller through Linux...

atar-axis commented 6 years ago

hm... I dont think that this will ever happen :laughing: but let us discuss the default behaviour after we confirmed that this hack even works :) btw! did you know that DKMS will backup your current driver version (in case you want to remove the new version afterwards)

kakra commented 6 years ago

I'm not sure if this is how it works in Gentoo... But I've set Gentoo to backup packages including configuration before upgrades so I can restore the previous instance of the package easily.

Gentoo has it's own means of compiling and installing kernel modules if they come separate from the kernel. There's no DKMS in Gentoo. It's completely controlled by the package manager (including compiling). DKMS software is usually decomposed into just compiling, creating an install image and merging this into the system as a binary package. The only exception here is the kernel itself: The package script will just install the sources, you're expected to configure and compile the kernel yourself (but there's a helper to automate this process). The compiled and installed kernel is never controlled by the package manager, including the modules (even from DKMS packages, they are left with the old kernel after upgrading).

As such, I'm also not able to test this DKMS branch. In Gentoo, it is more convenient for me to drop kernel patches into the patches directory of the package manager, and thus having a patched, package manager controlled kernel source tree.

kakra commented 6 years ago

@atar-axis Please update me, what's awaiting acceptance here currently? The version thingy? I'm not yet up to trying that. The job is demanding currently.

atar-axis commented 6 years ago

@kakra yes, the version workaround - but no hurry, business before pleasure.

atar-axis commented 6 years ago

As with d179a2097bb513e19147fa7fd6eaa7939497fa1f the fake version is now reconfigurable - the default value is 0x1130 ("NEO").

atar-axis commented 6 years ago

I am closing this issue now (I merged the input_version branch).

Could we remove the SDL part from the README (and maybe outsource it to the wiki and the "Third party Bugs" section) @kakra? To keep it neat

kakra commented 6 years ago

I was already thinking about it. But I guess we should keep it in a visible place because the version hack may not be wanted by everyone. And in that case these explanations are important. Maybe create a doc directory and move the section there into its own file?

atar-axis commented 6 years ago

https://atar-axis.github.io/xpadneo/

kakra commented 6 years ago

Looks good as a starting point. But it jumps right into the topic, maybe we could add some introduction, download pointers, something more like this... Just brainstorming. ;-)

atar-axis commented 6 years ago

Yeah for sure, I'm just a bit busy today hence I did it "quick 'n dirty" :) But I like the idea of the GitPages /docs directory - never heard about it before.Thanks

atar-axis commented 6 years ago

Hm, don't you think that a GitHub Page is... a little bit... overkill? Looks like you do not really like the wiki, right? Is there a specific reason for that? We could link to it from the README - in case you are concerned only about visibility :)

kakra commented 6 years ago

I just don't like decoupling these from the source distribution. It is a problem of many projects here: The wiki is specific to the latest version of the software but especially when using projects as part of a dependency of another project, I'm sometimes stuck with older versions. And then such a wiki often does not match what really has to be done to solve specific problems, or to follow some installation guide. OTOH, this is not a devel dep of some project. But I also dislike the disconnection of the docs and the distribution. I'd prefer a package manager to install such docs along with the package because my first look would be at /usr/share/doc. So the GitHub Page approach is maybe the best way to go: You get some sort of versioning like in a Wiki but you also get the docs alongside with the package.

So one has to chose wisely what to put in the wiki and what to not put there. In the end, it's up to your decision. This is just my two cents.

BTW: The way you currently use the wiki is the most obvious way I would also think of using it: As a scratch book or notepad for the current state of development where documentation can be improved step by step, or to take note of changes in development (aka changelog), or document hardware behavior. But IMO documentation specific to a version of the package (like a manual) belongs into the source tree. It would also be okay to use a wiki, e.g., to describe a public (HTTP) API because everyone is forced to use the deployed version of such an API. I think you get the idea... :-)