| Message ID | 20211113131734.46fbc9bf@rechenknecht2k11 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: xpad - add more Xbox one controller IDs | expand |
On 11/13/21 06:17, Benjamin Valentin wrote: > These are collected from issue reports on [0]. > > I wonder a bit why we need to maintain this list to begin with, when > the consoles (and Windows) can apparently identify any new third party > controller just fine. There are 2 related problems here: 1) Detection of compatible gamepads 2) User-visible identification of gamepads and quirks For 1, the way Windows does it is via the Microsoft OS descriptor [0]. AFAIK, the specific OS descriptor strings are "XUSB20" for Xbox 360 and "XGIP10" for Xbox One. That functionality is handled by xpad_table[] and the bInterfaceProtocol detection logic in xpad_probe(). The xpad_device[] entry isn't required for detection or functionality of devices, unless those devices need special treatment like MAP_TRIGGERS_TO_BUTTONS or something. Since the protocols are vendor-defined, we end up having to maintain the vendor list. If we could detect that Microsoft OS descriptor, that would avoid the need to maintain the xpad_table[] vendor list. 2 is the main reason xpad_device[] exists (other than special devices). Most devices have pretty useless string descriptors which doesn't really allow userspace to provide a human-friendly descriptor for the user. OTOH, Windows totally abdicates this responsibility and just refers to XInput gamepads by a player index, so maybe it's not something we have to solve. [0]: https://docs.microsoft.com/en-us/windows-hardware/drivers/usbcon/microsoft-os-1-0-descriptors-specification > I don't have *that* many controllers to do extensive tests, but I wonder > if there could be a better way. >> [0] https://github.com/paroj/xpad/issues > > Signed-off-by: Benjamin Valentin <benpicco@googlemail.com> Regards, Cameron
On Sat, 13 Nov 2021 19:34:54 -0600 Cameron Gutman <aicommander@gmail.com> wrote: > For 1, the way Windows does it is via the Microsoft OS descriptor [0]. > AFAIK, the specific OS descriptor strings are "XUSB20" for Xbox 360 > and "XGIP10" for Xbox One. > > That functionality is handled by xpad_table[] and the > bInterfaceProtocol detection logic in xpad_probe(). The xpad_device[] > entry isn't required for detection or functionality of devices, > unless those devices need special treatment like > MAP_TRIGGERS_TO_BUTTONS or something. The problem is that the gamepad then gets assigned the type XTYPE_UNKNOWN which excludes it from all run-time code path switches. E.g. if I comment out // { 0x045e, 0x028e, "Microsoft X-Box 360 pad", 0, XTYPE_XBOX360 }, my X-Box 360 pad will not only lose any LED or rumble support, it will also not report any input events with jstest because xpad360_process_packet() is no longer called. I'll try to check on how to read out the OS descriptor string, maybe that allows us to detect the type on init and provide a better default experience. Best, Benjamin
On Sun, 14 Nov 2021 20:51:22 +0100 Benjamin Valentin <benpicco@googlemail.com> wrote: > The problem is that the gamepad then gets assigned the type > XTYPE_UNKNOWN which excludes it from all run-time code path switches. Ah sorry for the noise. xpad_probe() already takes care of detecting the gamepad type for the XTYPE_UNKNOWN case. And this works for my Xbox 360 pad - not sure what's wrong with jstest, but that is unrelated. To avoid that confusion, how about diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c index 4c914f75a902..155ee644295d 100644 --- a/drivers/input/joystick/xpad.c +++ b/drivers/input/joystick/xpad.c @@ -1783,14 +1785,19 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id if (xpad->xtype == XTYPE_UNKNOWN) { if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) { - if (intf->cur_altsetting->desc.bInterfaceProtocol == 129) + if (intf->cur_altsetting->desc.bInterfaceProtocol == 129) { xpad->xtype = XTYPE_XBOX360W; - else if (intf->cur_altsetting->desc.bInterfaceProtocol == 208) + xpad->name = "Generic Xbox 360 wireless pad"; + } else if (intf->cur_altsetting->desc.bInterfaceProtocol == 208) { xpad->xtype = XTYPE_XBOXONE; - else + xpad->name = "Generic Xbox One pad"; + } else { xpad->xtype = XTYPE_XBOX360; + xpad->name = "Generic Xbox 360 pad"; + } } else { xpad->xtype = XTYPE_XBOX; + xpad->name = "Generic Xbox classic pad"; } if (dpad_to_buttons)
On 11/14/21 13:51, Benjamin Valentin wrote: > On Sat, 13 Nov 2021 19:34:54 -0600 > Cameron Gutman <aicommander@gmail.com> wrote: > >> For 1, the way Windows does it is via the Microsoft OS descriptor [0]. >> AFAIK, the specific OS descriptor strings are "XUSB20" for Xbox 360 >> and "XGIP10" for Xbox One. >> >> That functionality is handled by xpad_table[] and the >> bInterfaceProtocol detection logic in xpad_probe(). The xpad_device[] >> entry isn't required for detection or functionality of devices, >> unless those devices need special treatment like >> MAP_TRIGGERS_TO_BUTTONS or something. > > The problem is that the gamepad then gets assigned the type > XTYPE_UNKNOWN which excludes it from all run-time code path switches. > > E.g. if I comment out > > // { 0x045e, 0x028e, "Microsoft X-Box 360 pad", 0, XTYPE_XBOX360 }, > > my X-Box 360 pad will not only lose any LED or rumble support, it will > also not report any input events with jstest because > xpad360_process_packet() is no longer called. > XTYPE_UNKNOWN is a special value that should always be overridden by the check in xpad_probe() to be the proper XTYPE value at runtime: if (xpad->xtype == XTYPE_UNKNOWN) { if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) { if (intf->cur_altsetting->desc.bInterfaceProtocol == 129) xpad->xtype = XTYPE_XBOX360W; else if (intf->cur_altsetting->desc.bInterfaceProtocol == 208) xpad->xtype = XTYPE_XBOXONE; else xpad->xtype = XTYPE_XBOX360; } else { xpad->xtype = XTYPE_XBOX; } ... Those values should definitely be correct, because they're what we use in the xpad_table[] itself. If they don't match, xpad_probe() won't get called at all. Can you see what's going on in xpad_probe() when you comment out that line for your gamepad? > I'll try to check on how to read out the OS descriptor string, maybe > that allows us to detect the type on init and provide a better default > experience. That type derivation code above already handles determining the type for unknown gamepads (assuming it actually works). The problem is having to maintain the xpad_table[] list with every vendor that produces an Xbox gamepad. That's where an extension to usb_device_id and something like a USB_MS_OS_DESCRIPTOR("XUSB20") macro would come in, if the USB core had first-class support for Microsoft OS descriptors. The problem is that adding that Microsoft OS descriptor query may cause compatibility issues for other devices. Windows actually keeps track of whether the OS descriptor query fails to avoid querying it again [0]. [0]: https://docs.microsoft.com/en-us/windows-hardware/drivers/usbcon/microsoft-defined-usb-descriptors > > Best, > Benjamin > Regards, Cameron
On 11/14/21 14:37, Benjamin Valentin wrote: > On Sun, 14 Nov 2021 20:51:22 +0100 > Benjamin Valentin <benpicco@googlemail.com> wrote: > >> The problem is that the gamepad then gets assigned the type >> XTYPE_UNKNOWN which excludes it from all run-time code path switches. > > Ah sorry for the noise. xpad_probe() already takes care of detecting > the gamepad type for the XTYPE_UNKNOWN case. > And this works for my Xbox 360 pad - not sure what's wrong with jstest, > but that is unrelated. Ah okay good, that's what I was expecting. > > To avoid that confusion, how about > > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c > index 4c914f75a902..155ee644295d 100644 > --- a/drivers/input/joystick/xpad.c > +++ b/drivers/input/joystick/xpad.c > @@ -1783,14 +1785,19 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id > > if (xpad->xtype == XTYPE_UNKNOWN) { > if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) { > - if (intf->cur_altsetting->desc.bInterfaceProtocol == 129) > + if (intf->cur_altsetting->desc.bInterfaceProtocol == 129) { > xpad->xtype = XTYPE_XBOX360W; > - else if (intf->cur_altsetting->desc.bInterfaceProtocol == 208) > + xpad->name = "Generic Xbox 360 wireless pad"; > + } else if (intf->cur_altsetting->desc.bInterfaceProtocol == 208) { > xpad->xtype = XTYPE_XBOXONE; > - else > + xpad->name = "Generic Xbox One pad"; > + } else { > xpad->xtype = XTYPE_XBOX360; > + xpad->name = "Generic Xbox 360 pad"; > + } > } else { > xpad->xtype = XTYPE_XBOX; > + xpad->name = "Generic Xbox classic pad"; > } > > if (dpad_to_buttons) > > I like this idea. There's a small risk of causing userspace breakages for games matching "Generic X-Box pad" by name, but we already run that risk each time we update xpad_devices[] anyway. You might even consider dropping "Generic" from the name and just call them "Xbox One gamepad" or similar. Can you also replace "Generic X-Box pad" with NULL in xpad_devices[]? I don't think it would ever be used anymore after this change. Regards, Cameron
On Sun, 14 Nov 2021 15:14:38 -0600 Cameron Gutman <aicommander@gmail.com> wrote: > I like this idea. There's a small risk of causing userspace breakages > for games matching "Generic X-Box pad" by name, but we already run > that risk each time we update xpad_devices[] anyway. Since the button mappings already differ between the original Xbox controller and the Xbox 360 controller, matching that string would have always been broken. > Can you also replace "Generic X-Box pad" with NULL in xpad_devices[]? > I don't think it would ever be used anymore after this change. Will do! Best, Benjamin
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c index e51f3a2bf06b..d99dd9aed137 100644 --- a/drivers/input/joystick/xpad.c +++ b/drivers/input/joystick/xpad.c @@ -150,6 +150,7 @@ static const struct xpad_device { { 0x06a3, 0x0200, "Saitek Racing Wheel", 0, XTYPE_XBOX }, { 0x06a3, 0x0201, "Saitek Adrenalin", 0, XTYPE_XBOX }, { 0x06a3, 0xf51a, "Saitek P3600", 0, XTYPE_XBOX360 }, + { 0x0738, 0x4503, "Mad Catz Racing Wheel", 0, XTYPE_XBOXONE }, { 0x0738, 0x4506, "Mad Catz 4506 Wireless Controller", 0, XTYPE_XBOX }, { 0x0738, 0x4516, "Mad Catz Control Pad", 0, XTYPE_XBOX }, { 0x0738, 0x4520, "Mad Catz Control Pad Pro", 0, XTYPE_XBOX }, @@ -250,6 +251,7 @@ static const struct xpad_device { { 0x102c, 0xff0c, "Joytech Wireless Advanced Controller", 0, XTYPE_XBOX }, { 0x1038, 0x1430, "SteelSeries Stratus Duo", 0, XTYPE_XBOX360 }, { 0x1038, 0x1431, "SteelSeries Stratus Duo", 0, XTYPE_XBOX360 }, + { 0x10f5, 0x7005, "Turtle Beach Recon Controller", 0, XTYPE_XBOXONE }, { 0x11c9, 0x55f0, "Nacon GC-100XF", 0, XTYPE_XBOX360 }, { 0x1209, 0x2882, "Ardwiino Controller", 0, XTYPE_XBOX360 }, { 0x12ab, 0x0004, "Honey Bee Xbox360 dancepad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360 }, @@ -263,6 +265,7 @@ static const struct xpad_device { { 0x1532, 0x0037, "Razer Sabertooth", 0, XTYPE_XBOX360 }, { 0x1532, 0x0a00, "Razer Atrox Arcade Stick", MAP_TRIGGERS_TO_BUTTONS, XTYPE_XBOXONE }, { 0x1532, 0x0a03, "Razer Wildcat", 0, XTYPE_XBOXONE }, + { 0x1532, 0x0a29, "Razer Wolverine v2", 0, XTYPE_XBOXONE }, { 0x15e4, 0x3f00, "Power A Mini Pro Elite", 0, XTYPE_XBOX360 }, { 0x15e4, 0x3f0a, "Xbox Airflo wired controller", 0, XTYPE_XBOX360 }, { 0x15e4, 0x3f10, "Batarang Xbox 360 controller", 0, XTYPE_XBOX360 }, @@ -329,6 +332,7 @@ static const struct xpad_device { { 0x24c6, 0x550e, "Hori Real Arcade Pro V Kai 360", MAP_TRIGGERS_TO_BUTTONS, XTYPE_XBOX360 }, { 0x24c6, 0x551a, "PowerA FUSION Pro Controller", 0, XTYPE_XBOXONE }, { 0x24c6, 0x561a, "PowerA FUSION Controller", 0, XTYPE_XBOXONE }, + { 0x24c6, 0x581a, "ThrustMaster XB1 Classic Controller", 0, XTYPE_XBOXONE }, { 0x24c6, 0x5b00, "ThrustMaster Ferrari 458 Racing Wheel", 0, XTYPE_XBOX360 }, { 0x24c6, 0x5b02, "Thrustmaster, Inc. GPX Controller", 0, XTYPE_XBOX360 }, { 0x24c6, 0x5b03, "Thrustmaster Ferrari 458 Racing Wheel", 0, XTYPE_XBOX360 }, @@ -336,6 +340,7 @@ static const struct xpad_device { { 0x24c6, 0xfafe, "Rock Candy Gamepad for Xbox 360", 0, XTYPE_XBOX360 }, { 0x2563, 0x058d, "OneXPlayer Gamepad", 0, XTYPE_XBOX360 }, { 0x3285, 0x0607, "Nacon GC-100", 0, XTYPE_XBOX360 }, + { 0x3285, 0x0614, "Nacon Pro Compact", 0, XTYPE_XBOXONE }, { 0x3767, 0x0101, "Fanatec Speedster 3 Forceshock Wheel", 0, XTYPE_XBOX }, { 0xffff, 0xffff, "Chinese-made Xbox Controller", 0, XTYPE_XBOX }, { 0x0000, 0x0000, "Generic X-Box pad", 0, XTYPE_UNKNOWN } @@ -435,6 +440,7 @@ static const struct usb_device_id xpad_table[] = { XPAD_XBOX360_VENDOR(0x0f0d), /* Hori Controllers */ XPAD_XBOXONE_VENDOR(0x0f0d), /* Hori Controllers */ XPAD_XBOX360_VENDOR(0x1038), /* SteelSeries Controllers */ + XPAD_XBOXONE_VENDOR(0x10f5), /* Turtle Beach */ XPAD_XBOX360_VENDOR(0x11c9), /* Nacon GC100XF */ XPAD_XBOX360_VENDOR(0x1209), /* Ardwiino Controllers */ XPAD_XBOX360_VENDOR(0x12ab), /* X-Box 360 dance pads */ @@ -455,6 +461,7 @@ static const struct usb_device_id xpad_table[] = { XPAD_XBOXONE_VENDOR(0x2e24), /* Hyperkin Duke X-Box One pad */ XPAD_XBOX360_VENDOR(0x2f24), /* GameSir Controllers */ XPAD_XBOX360_VENDOR(0x3285), /* Nacon GC-100 */ + XPAD_XBOXONE_VENDOR(0x3285), /* Nacon XBOX Series S/X Controllers */ { } };
These are collected from issue reports on [0]. I wonder a bit why we need to maintain this list to begin with, when the consoles (and Windows) can apparently identify any new third party controller just fine. I don't have *that* many controllers to do extensive tests, but I wonder if there could be a better way. [0] https://github.com/paroj/xpad/issues Signed-off-by: Benjamin Valentin <benpicco@googlemail.com> --- drivers/input/joystick/xpad.c | 7 +++++++ 1 file changed, 7 insertions(+)