We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5048 - [PATCH] Only enumerate HID devices that have gamepad HID usages
Summary: [PATCH] Only enumerate HID devices that have gamepad HID usages
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: joystick (show other bugs)
Version: HG 2.0
Hardware: x86 Windows 10
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-20 21:11 UTC by Cameron Gutman
Modified: 2020-03-21 04:05 UTC (History)
0 users

See Also:


Attachments
Only enumerate HID devices on Windows that have gamepad HID usages (3.26 KB, patch)
2020-03-20 21:11 UTC, Cameron Gutman
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Gutman 2020-03-20 21:11:04 UTC
There are a number of poorly behaved HID devices that time out on attempts to 
read various strings on Windows. Rather than end up on an endless treadmill of blacklisting broken devices, reduce our risk by only querying devices that are gamepads.

Examples of bugs this would have likely fixed:
https://bugzilla.libsdl.org/show_bug.cgi?id=5010
https://bugzilla.libsdl.org/show_bug.cgi?id=4389
https://bugzilla.libsdl.org/show_bug.cgi?id=4485
https://bugzilla.libsdl.org/show_bug.cgi?id=4585
Comment 1 Cameron Gutman 2020-03-20 21:11:45 UTC
Created attachment 4273 [details]
Only enumerate HID devices on Windows that have gamepad HID usages
Comment 2 Sam Lantinga 2020-03-20 22:52:09 UTC
There are a number of devices that hang inside HidD_GetPreparsedData(). In addition, checking gamepad HID usages won't catch Xbox controllers or Steam controllers.

However, I'm not opposed to the intent of this patch. If those issues can be fixed and it can be applied generally across all the backends, I'd be fine with that.
Comment 3 Cameron Gutman 2020-03-20 23:38:00 UTC
(In reply to Sam Lantinga from comment #2)
> There are a number of devices that hang inside HidD_GetPreparsedData(). In
> addition, checking gamepad HID usages won't catch Xbox controllers or Steam
> controllers.

All of the bugs I've seen (the ones mentioned above) are hanging in either HidD_GetManufacturerString(), HidD_GetSerialNumberString(), or HidD_GetProductString(). Those would be fixed by this patch. Hangs in HidD_GetPreparsedData() would still need a blacklist entry, but that should be a much smaller subset of devices. If you have a specific bug in mind, I could take a look there.

Checking HID usages does work for Xbox controllers on Windows, because the Windows HID stack creates fake HID descriptors for them even though XInput isn't really HID.

You're right that Steam Controller don't seem to have gamepad HID usages, but I don't think this patch changes the existing behavior because SDL_hidapijoystick.c already performs the same check here: https://hg.libsdl.org/SDL/file/82e1e144c03b/src/joystick/hidapi/SDL_hidapijoystick.c#l433 since https://hg.libsdl.org/SDL/rev/a5459597367f

I tested this patch successfully against Xbox One (USB and BT), Xbox 360 (USB and wireless adapter), and PS4 (USB and BT) controllers. They all matched the HID usage check as expected. The Steam Controller did not (it has the vendor-defined usage page), but that doesn't seem to be new behavior as it was already filtered out in SDL_hidapijoystick.c.

> 
> However, I'm not opposed to the intent of this patch. If those issues can be
> fixed and it can be applied generally across all the backends, I'd be fine
> with that.

There is already a generic check in SDL_hidapijoystick.c (see above), so I don't think this is needed for all backends individually. I targeted Windows here specifically because that's the platform affected by most (all?) of these hang issues (similar to how we only have hid_blacklist() on Windows).
Comment 4 Sam Lantinga 2020-03-21 01:40:43 UTC
Fair enough. I'll review this, thanks!
Comment 5 Sam Lantinga 2020-03-21 03:54:31 UTC
Patch added, thanks!
https://hg.libsdl.org/SDL/rev/25927a14e406
https://hg.libsdl.org/SDL/rev/8a949642b6eb
Comment 6 Sam Lantinga 2020-03-21 03:54:42 UTC
Fixed!
Comment 7 Cameron Gutman 2020-03-21 04:00:38 UTC
Thanks!

FYI, in order for https://hg.libsdl.org/SDL/rev/8a949642b6eb to effectively enable Steam Controllers, you'll also need to add that VID exception for Valve devices to the HID usage check in HIDAPI_GetDeviceDriver() too.
Comment 8 Sam Lantinga 2020-03-21 04:05:35 UTC
Fixed!
https://hg.libsdl.org/SDL/rev/d32d92e782ab