Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Built-in SDL HIDAPI/XInput mappings are being overridden #478

Open
cgutman opened this issue Jun 29, 2021 · 13 comments · Fixed by #480
Open

Built-in SDL HIDAPI/XInput mappings are being overridden #478

cgutman opened this issue Jun 29, 2021 · 13 comments · Fixed by #480

Comments

@cgutman
Copy link
Contributor

cgutman commented Jun 29, 2021

This is a continuation of the discussion from 9219d19, moonlight-stream/moonlight-qt#620, and #477. I took a look at the SDL code to figure out what's going on.

Some of the mapping GUIDs have special suffixes that mean they are associated with a particular API:
https://github.com/libsdl-org/SDL/blob/c59d4dcd38c382a1e9b69b053756f1139a861574/src/joystick/windows/SDL_xinputjoystick.c#L279-L280
https://github.com/libsdl-org/SDL/blob/b63cb822bf02ffa53e7a25755ff4606af0417389/src/joystick/hidapi/SDL_hidapijoystick.c#L918-L920

The trouble is that these special APIs almost always have an existing mapping built into SDL like this one for HIDAPI. Allowing mappings for these special APIs here creates the possibility for error like we've seen here.

For HIDAPI joysticks, this suffix is 6800 (0x68 = 'h') which is exactly what the suffix of the broken mapping is from 9219d19. The mistake there seems to be that they assumed it would be equivalent to the non-HIDAPI mapping for a PS4 Controller. This is not the case, since HIDAPI does not necessarily expose all axes and buttons in the same order that DirectInput does.

There is a similar case for XInput joysticks, where 78XX(0x78 = 'x', XX is the XInput subtype) is the magic suffix. Due to the unnecessary mapping in here, an ordinary Xbox 360 controller shows up as a ShanWan PS3/PC Wired GamePad when using the XInput API. We should definitely remove this mapping, because it's unnecessary and causes the much more common official Microsoft Xbox 360 controller to be mistaken for some knock-off PS3 controller (though at least the mapping is correct).

Due to these issues, I propose we remove all existing mappings where the last 2 bytes are 6800 or 78XX and closely scrutinize any attempts to add more. Mappings for these special controllers should only be necessary in the extremely rare case of a third-party Xbox/PlayStation/Nintendo gamepad that doesn't behave like the standard ones or very specialized hardware like a dance pad, guitar, or something that isn't used like a "normal" controller.

@offalynne
Copy link
Collaborator

offalynne commented Jun 29, 2021

sounds good to me.

the extremely rare case of a third-party Xbox/PlayStation/Nintendo gamepad that doesn't behave like the standard

significantly less rare than you'd think, unfortunately

specialized hardware like a dance pad, guitar, or something that isn't used like a "normal" controller

there are a few exceptions but we generally intend to omit these anyway

@cgutman
Copy link
Contributor Author

cgutman commented Jun 29, 2021

As luck would have it, I pulled my PS4 controller out of my drawer and it happens to be one of the affected ones.

When I have the broken mapping in place, Moonlight prints:

00:00:27 - SDL Info (0): Gamepad 0 (player 0) is: PS4 Controller (haptic capabilities: 0x10000) (mapping: 030000004c050000cc09000000016800 -> 030000004c050000cc09000000016800,PS4 Controller,a:b1,b:b2,back:b8,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,guide:b12,leftshoulder:b4,leftstick:b10,lefttrigger:a3,leftx:a0,lefty:a1,rightshoulder:b5,rightstick:b11,righttrigger:a4,rightx:a2,righty:a5,start:b9,x:b0,y:b3,platform:Windows,)

and with it removed from gamecontrollerdb.txt, the built-in SDL mapping prevails:

00:00:09 - SDL Info (0): Gamepad 0 (player 0) is: PS4 Controller (haptic capabilities: 0x10000) (mapping: 030000004c050000cc09000000016800 -> 030000004c050000cc09000000016800,*,a:b0,b:b1,back:b4,dpdown:b12,dpleft:b13,dpright:b14,dpup:b11,guide:b5,leftshoulder:b9,leftstick:b7,lefttrigger:a4,leftx:a0,lefty:a1,rightshoulder:b10,rightstick:b8,righttrigger:a5,rightx:a2,righty:a3,start:b6,x:b2,y:b3,touchpad:b15,)

The * for the name field gives away that it's a generic mapping. The controller works correctly with the erroneous mapping removed.

@flibitijibibo
Copy link
Contributor

Should we do something about this in controllermap as well? We should probably detect XInput/HIDAPI devices and point out that they're already mapped, but at the same time why are we having to make mappings at all if the joystick drivers are supposed to do that in the first place... presumably someone hit a bug and these busted mappings fixed the issue.

@offalynne
Copy link
Collaborator

offalynne commented Jun 29, 2021

Should we do something about this in controllermap

I think a warning at least would be a good idea !

why are we having to make mappings at all if the joystick drivers are supposed to do that in the first place

Good question, overzealousness possibly? Once one understands maps are for normalizing pads it may be motivation to throw in whatever device is at hand even if the default mapping works fine.

The only potential problem besides a bad name (eg. Shanwan clone) are those that fail to match a subset of the default mapping (a:b0,b:b1,back:b6,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,guide:b10,leftshoulder:b4,leftstick:b8,lefttrigger:a2,leftx:a0,lefty:a1,rightshoulder:b5,rightstick:b9,righttrigger:a5,rightx:a3,righty:a4,start:b7,x:b2,y:b3).

Most of the above-mentioned maps in the DB already do match, and for those that don't we need a better binary to recommend with a more recent SDL to test properly as this was presumably the problem that led to the PS4 map that started this discussion.

@cgutman
Copy link
Contributor Author

cgutman commented Jun 29, 2021

Every one of the 7801 mappings exactly match the generic xinput mapping string, so these are almost certainly safe to remove.

For 7803 mapping, it looks like the only change there is that some axes are removed since they are not physically present on the device. It appears from the MSDN docs that some controls are optional for for some XInput subtypes, so we should probably leave mappings for the special XInput subtypes where controls may vary by device (basically any 78XX value except 7801). SDL should probably handle more of this than it currently does (only including controls that are at least possibly present on particular subtypes in the default XInput mapping).

Several of the 6800 mappings are present upstream, so we'll defer to upstream for these. Of the remaining 6800 mappings:

cgutman added a commit to cgutman/SDL_GameControllerDB that referenced this issue Jun 29, 2021
- Mappings for XInput devices with a non-gamepad subtype
- Mappings present in upstream SDL

Fixes mdqinc#478
@offalynne
Copy link
Collaborator

Feel free to remove the DualSense mappings in #480 as well, those maps are mine and the newer driver in SDL has them covered.

@cgutman
Copy link
Contributor Author

cgutman commented Jun 29, 2021

Unless you're talking about something else, I think I removed those in #480 (line 848 and 850)

@offalynne
Copy link
Collaborator

Gonna reopen this for posterity until we have a better up-to-date mapping binary to distribute as per #476.

The 'issue' is tentatively solved but we need to keep an eye for incoming PRs of similar format.

@arrowgent
Copy link

arrowgent commented Dec 16, 2021

can we pinpoint when this started in SDL2? do we have an issue to report over on their github?
https://github.com/libsdl-org/SDL/issues

same controller different systems:

ubuntu 18.04 x86_64
libsdl2-2.0-0/bionic,now 2.0.18+dfsg-1~18.04.sav1 amd64 [installed]

/usr/lib/x86_64-linux-gnu/installed-tests/SDL2 $ ./controllermap

030000000d0f0000c100000072056800,Nintendo Switch Pro Controller,platform:Linux,a:b1,b:b0,x:b3,y:b2,back:b4,guide:b5,start:b6,leftstick:b7,rightstick:b8,leftshoulder:b9,rightshoulder:b10,dpup:b11,dpdown:b12,dpleft:b13,dpright:b14,leftx:a0,lefty:a1,rightx:a2,righty:a3,lefttrigger:a4,righttrigger:a5,

raspberry pi debian 11
libsdl2-2.0-0/stable,now 2.0.14+dfsg2-3 armhf [installed,automatic]

/usr/lib/arm-linux-gnueabihf/installed-tests/SDL2 $ ./controllermap

030000000d0f0000c100000011010000,CO.LTD. HORIPAD S,platform:Linux,b:b2,x:b0,y:b3,back:b8,guide:b12,start:b9,leftstick:b10,rightstick:b11,leftshoulder:b4,rightshoulder:b5,dpup:h0.1,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,misc1:b13,leftx:a0,lefty:a1,rightx:a2,righty:a3,lefttrigger:b6,righttrigger:b7,

@offalynne
Copy link
Collaborator

offalynne commented Dec 22, 2021

same controller different systems

this is working as intended — the first mapping is presumably benefitting from the hid-nintendo kernel driver and so is being handled in SDL with HIDAPI, while the pi running debian doesn’t have the driver and so will benefit from a db mapping.

offalynne added a commit to CdrJamison/SDL_GameControllerDB that referenced this issue Jan 27, 2022
@offalynne
Copy link
Collaborator

offalynne commented Sep 3, 2023

why are we having to make mappings at all if the joystick drivers are supposed to do that in the first place

interesting answer to this in #702: if a user wants to configure their own device-specific remapping of something covered by a joystick driver through this interface (not the intended use case, but may work nonetheless)

additionally i've noticed a number of projects consuming this data directly for their own dinput/udev/etc handling, even if they do not implement SDL itself. depending on how GUID parsing and matching is implemented by these, mappings with the driver hint suffix would normally not apply, but using an SDL-based mapping tool in an (untested) attempt to "fix" this will produce the SDL GUID

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants