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

Need more information in joystick GUID #6004

Closed
slouken opened this issue Aug 5, 2022 · 10 comments · Fixed by #6111
Closed

Need more information in joystick GUID #6004

slouken opened this issue Aug 5, 2022 · 10 comments · Fixed by #6111
Assignees
Labels
early in milestone This change should be made early in the milestone for additional testing
Milestone

Comments

@slouken
Copy link
Collaborator

slouken commented Aug 5, 2022

There are a number of controllers which share the same VID/PID, but have different manufacturer and product strings. This is especially common for the DragonRise controllers, but also happens for others as well.

'HORIPAD FPS for Nintendo Switch' VID 0x0f0d, PID 0x00dc would also benefit from this, as it shares the same VID/PID as a number of other HORI controllers, such as the HORIPAD Mini, but where the other controllers map the face buttons by label, the HORIPAD FPS maps the face buttons by position (so A is at the bottom instead of to the right as labeled.)

We need a way of differentiating these controllers so they can have different mappings.

The current GUID layout is:
16-bit BUS
16-bit 0000
16-bit VID
16-bit 0000
16-bit PID
16-bit 0000
16-bit VERSION
16-bit 0000

Many backends use the final 16-bits as 2 bytes, a driver identifier (e.g. 'x'), and a controller type.

Since we can't fit the entire name in the GUID, I propose using the second 16-bit value as a 16-bit CRC of the name of the controller. We might have some collisions, but it would be much better than no information at all.

We would have to do some tap dancing when loading controller mappings because older mappings will have a 0 CRC that should be used as a wildcard, but newly generated mappings would have more unique information that will help differentiate different controllers that have the same VID/PID.

Thoughts?

@slouken slouken added the early in milestone This change should be made early in the milestone for additional testing label Aug 5, 2022
@slouken slouken added this to the 2.26.0 milestone Aug 5, 2022
@flibitijibibo
Copy link
Collaborator

Locking down the last two bytes would definitely help figure out what to do with the rest - I've seen 'h' and 'x' but I dunno what the rest of the examples are. If it's used consistently those bytes should probably be left as-is and marked as in-use for future reference.

The CRC idea sounds reasonable too, guess it just depends on what is most prone to collisions. I imagine most of the names will either be pretty similar or completely different, so knowing what is likely to collide would help a bit.

@slouken
Copy link
Collaborator Author

slouken commented Aug 10, 2022

This will also help with the Nintendo Joy-Con Charging Grip using the hid_nintendo driver on Linux, where both sides have the same VID/PID, but different mappings.

@slouken
Copy link
Collaborator Author

slouken commented Aug 10, 2022

Good news, in the entire set of Linux controller mappings, there is only one CRC16 collision:
0x0585: MP-8866 Super Dual Box
0x0585: QanBa Arcade JoyStick

I think this is a reasonable approach going forward.

slouken added a commit that referenced this issue Aug 23, 2022
This will make it possible to have mappings for different controllers
that have the same VID/PID. This happens frequently with some generic
controller boards that have been reused in many products.

Fixes #6004
@icculus
Copy link
Collaborator

icculus commented Aug 23, 2022

Does this work solve this issue? #3197

@Swyter
Copy link

Swyter commented Aug 23, 2022

@icculus @slouken I don't think this will be enough in the DragonRise case because from the lsusb dumps available on this thread (mdqinc/SDL_GameControllerDB#202) they seem to share everything naming-wise. I imagine you guys at Valve have other DragonRise pads and can try to dump their EEPROM using the method I found to see if hashing that will be enough.

At the very least I think maybe hashing the HID descriptor may be a step up. But we do need more samples to know if this is worthwhile.

@Swyter
Copy link

Swyter commented Aug 23, 2022

@icculus @slouken For the DragonRise dumping method take a look at the comments below. But it's as easy as retrieving the string descriptor 3 with at least a length of 0x200, and seems safe enough:

mdqinc/SDL_GameControllerDB#202 (comment)
mdqinc/SDL_GameControllerDB#202 (comment)

That dump should at least contain the device descriptor, the configuration descriptor, all the actual strings and the HID descriptor. I imagine that it doesn't get better than that hashing-wise.

@slouken
Copy link
Collaborator Author

slouken commented Aug 23, 2022

This might be enough. The string we're hashing contains iProduct, so they should be different. Can you try out the latest code and see if that works?

@Swyter
Copy link

Swyter commented Aug 23, 2022

So yeah, if they turn out to be different enough it would just be a matter of expanding #6111 to have a exception for the 0079:0006 VID/PID pair that uses a special codepath for the hashed data.

I disassembled one, looked at the microcontroller which is of type chip-on-board and still fear that only the buttons are wired differently.

@Swyter
Copy link

Swyter commented Aug 23, 2022

@slouken Please take a look at the lsusb dumps; at least the NGS Phantom, Topway faux-dualshock, Defender Cobra R4 and Trust CXT24 seem to share string IDs and contents. And this is just a small developer-contributed sample.

@slouken
Copy link
Collaborator Author

slouken commented Aug 23, 2022

I'm moving this discussion over to #3197 so we can follow it on the bug report.

PJB3005 pushed a commit to PJB3005/SDL that referenced this issue Oct 5, 2022
This will make it possible to have mappings for different controllers
that have the same VID/PID. This happens frequently with some generic
controller boards that have been reused in many products.

Fixes libsdl-org#6004
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early in milestone This change should be made early in the milestone for additional testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants