On Fri, Jan 15, 2021 at 06:32:04AM -0700, Thomas Frohwein wrote:
> On Sat, Jan 09, 2021 at 10:50:35AM -0700, Thomas Frohwein wrote:
> > Hi,
> >
> > This diff adds support for the XBox One gamecontroller in a similar way
> > to what we have for the (older) XBox 360 controller [1][2]. This diff
> > is based on the pertinent code in NetBSD's uhidev.c.
> >
> > Similarities include that the device doesn't provide a report
> > descriptor, so this diff adds one to uhid_rdesc.h.
> >
> > The biggest difference (that held this up for a while) is that the
> > controller needs an init byte sequence to "wake up."
> >
> > The original report descriptor from NetBSD just maps the buttons and
> > axes in the order that they appear in the report. I modified the report
> > descriptor to assign the buttons/axes in the most logical order for
> > compatibility, which results in the same layout that we already have
> > for the XBox 360 controller.
> >
> > The addition of the member sc_flags to struct uhidev_softc follows the
> > NetBSD example.
> >
> > I have tested this with usbhidctl(1), sdl2-jstest (from sdl-jstest
> > package), and a couple of SDL2 games (Owlboy, Cryptark).
> >
> > If you have an XBox One controller, the easiest way to test is with:
> >
> > $ usbhidctl -f /dev/uhidX -l
> >
> > Make sure to pick the right uhid device and that you have read
> > permissions for it.
> >
> > Of course, this works only with the controller connected via USB, and
> > doesn't magically add wireless support.
> >
> > If you test actual SDL2 applications, the button layout will likely
> > default to an odd configuration. I am simultaneously preparing a diff
> > for sdl2-2.0.14p0 that improves recognition and automatic mapping and
> > solves any issues with XBox One default button layout.
>
> I have an update to this diff. 2 testers used a more recent XBox One
> controller model (model number 1708 in both cases) and ran into
> problems with the original diff. Below is a new diff that uses a longer
> 5-byte init sequence taken from Linux' xpad.c [1], compared to the
> 2-byte sequence from NetBSD's uhidev.c that I offered in the initial
> diff. This fixed the issue for the 2 testers.
>
> My own XBox One controller is model number 1537 and still works with
> this diff.
>
> [1] https://github.com/paroj/xpad/blob/master/xpad.c#L479
solene@ has tested this and is ok with it, but I was hoping to get an
opinion from someone with more mileage with the USB internals.
The items of note to direct anyone wanting to give a quick opinion
inlined below.
>
> Index: uhid_rdesc.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uhid_rdesc.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 uhid_rdesc.h
> --- uhid_rdesc.h 25 Oct 2013 03:09:59 -0000 1.1
> +++ uhid_rdesc.h 9 Jan 2021 15:11:30 -0000
> @@ -272,4 +272,94 @@ static const uByte uhid_xb360gp_report_d
> 0x95, 0x01, /* REPORT COUNT (1) */
> 0x81, 0x01, /* INPUT (Constant) */
> 0xc0, /* END COLLECTION */
> +};
> +
> +static const uByte uhid_xbonegp_report_descr[] = {
> + 0x05, 0x01, /* USAGE PAGE (Generic Desktop)
> */
> + 0x09, 0x05, /* USAGE (Gamepad)
> */
> + 0xa1, 0x01, /* COLLECTION (Application)
> */
> + /* Button packet */
> + 0xa1, 0x00, /* COLLECTION (Physical)
> */
> + 0x85, 0x20, /* REPORT ID (0x20)
> */
> + /* Skip unknown field and counter */
> + 0x09, 0x00, /* USAGE (Undefined)
> */
> + 0x75, 0x08, /* REPORT SIZE (8)
> */
> + 0x95, 0x02, /* REPORT COUNT (2)
> */
> + 0x81, 0x03, /* INPUT (Constant, Variable,
> Absolute) */
> + /* Payload size */
> + 0x09, 0x3b, /* USAGE (Byte Count)
> */
> + 0x95, 0x01, /* REPORT COUNT (1)
> */
> + 0x81, 0x02, /* INPUT (Data, Variable, Absolute)
> */
> + /* 16 Buttons: 1-2=Unknown, 3=Start, 4=Back, 5-8=ABXY,
> + * 9-12=D-Pad(Up,Dn,Lt,Rt), 13=LB, 14=RB, 15=LS, 16=RS
> + */
> + /* Skip the first 2 as they are not used */
> + 0x75, 0x01, /* REPORT SIZE (1)
> */
> + 0x95, 0x02, /* REPORT COUNT (2)
> */
> + 0x81, 0x01, /* INPUT (Constant)
> */
> + /* Assign buttons Start(7), Back(8), ABXY(1-4) */
> + 0x15, 0x00, /* LOGICAL MINIMUM (0)
> */
> + 0x25, 0x01, /* LOGICAL MAXIMUM (1)
> */
> + 0x95, 0x06, /* REPORT COUNT (6)
> */
> + 0x05, 0x09, /* USAGE PAGE (Button)
> */
> + 0x09, 0x08, /* USAGE (Button 8)
> */
> + 0x09, 0x07, /* USAGE (Button 7)
> */
> + 0x09, 0x01, /* USAGE (Button 1)
> */
> + 0x09, 0x02, /* USAGE (Button 2)
> */
> + 0x09, 0x03, /* USAGE (Button 3)
> */
> + 0x09, 0x04, /* USAGE (Button 4)
> */
> + 0x81, 0x02, /* INPUT (Data, Variable, Absolute)
> */
> + /* D-Pad */
> + 0x05, 0x01, /* USAGE PAGE (Generic Desktop)
> */
> + 0x09, 0x01, /* USAGE (Pointer)
> */
> + 0xa1, 0x00, /* COLLECTION (Physical)
> */
> + 0x75, 0x01, /* REPORT SIZE (1)
> */
> + 0x15, 0x00, /* LOGICAL MINIMUM (0)
> */
> + 0x25, 0x01, /* LOGICAL MAXIMUM (1)
> */
> + 0x95, 0x04, /* REPORT COUNT (4)
> */
> + 0x05, 0x01, /* USAGE PAGE (Generic Desktop)
> */
> + 0x09, 0x90, /* USAGE (D-Pad Up)
> */
> + 0x09, 0x91, /* USAGE (D-Pad Down)
> */
> + 0x09, 0x93, /* USAGE (D-Pad Left)
> */
> + 0x09, 0x92, /* USAGE (D-Pad Right)
> */
> + 0x81, 0x02, /* INPUT (Data, Variable, Absolute)
> */
> + 0xc0, /* END COLLECTION
> */
> + /* Buttons 5-6 (Shoulder Buttons) and 9-10 (Stick Buttons) */
> + 0x15, 0x00, /* LOGICAL MINIMUM (0)
> */
> + 0x25, 0x01, /* LOGICAL MAXIMUM (1)
> */
> + 0x95, 0x04, /* REPORT COUNT (4)
> */
> + 0x05, 0x09, /* USAGE PAGE (Button)
> */
> + 0x09, 0x05, /* USAGE (Button 5)
> */
> + 0x09, 0x06, /* USAGE (Button 6)
> */
> + 0x09, 0x09, /* USAGE (Button 9)
> */
> + 0x09, 0x0a, /* USAGE (Button 10)
> */
> + 0x81, 0x02, /* INPUT (Data, Variable, Absolute
> */
> + /* Triggers */
> + 0x15, 0x00, /* LOGICAL MINIMUM (0)
> */
> + 0x26, 0xff, 0x03, /* LOGICAL MAXIMUM (1023)
> */
> + 0x75, 0x10, /* REPORT SIZE (16)
> */
> + 0x95, 0x02, /* REPORT COUNT (2)
> */
> + 0x05, 0x01, /* USAGE PAGE (Generic Desktop)
> */
> + 0x09, 0x32, /* USAGE (Z)
> */
> + 0x09, 0x35, /* USAGE (Rz)
> */
> + 0x81, 0x02, /* INPUT (Data, Variable, Absolute)
> */
> + /* Sticks */
> + 0x16, 0x00, 0x80, /* LOGICAL MINIMUM (-32768)
> */
> + 0x26, 0xff, 0x7f, /* LOGICAL MAXIMUM (32767)
> */
> + 0x09, 0x01, /* USAGE (Pointer)
> */
> + 0xa1, 0x00, /* COLLECTION (Physical)
> */
> + 0x95, 0x02, /* REPORT COUNT (2)
> */
> + 0x05, 0x01, /* USAGE PAGE (Generic Desktop)
> */
> + 0x09, 0x30, /* USAGE (X)
> */
> + 0x09, 0x31, /* USAGE (Y)
> */
> + 0x81, 0x02, /* INPUT (Data, Variable, Absolute)
> */
> + 0xc0, /* END COLLECTION
> */
> + 0x09, 0x01, /* USAGE (Pointer)
> */
> + 0xa1, 0x00, /* COLLECTION (Physical)
> */
> + 0x95, 0x02, /* REPORT COUNT (2)
> */
> + 0x09, 0x33, /* USAGE (Rx)
> */
> + 0x09, 0x34, /* USAGE (Ry)
> */
> + 0x81, 0x02, /* INPUT (Data, Variable, Absolute)
> */
> + 0xc0, /* END COLLECTION
> */
> + 0xc0, /* END COLLECTION
> */
> };
> Index: uhidev.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
> retrieving revision 1.83
> diff -u -p -r1.83 uhidev.c
> --- uhidev.c 31 Aug 2020 12:26:49 -0000 1.83
> +++ uhidev.c 9 Jan 2021 15:11:30 -0000
> @@ -63,8 +63,10 @@
> #include <dev/usb/uhid_rdesc.h>
> int uhidev_use_rdesc(struct uhidev_softc *, usb_interface_descriptor_t *,
> int, int, void **, int *);
> -#define UISUBCLASS_XBOX360_CONTROLLER 0x5d
> -#define UIPROTO_XBOX360_GAMEPAD 0x01
> +#define UISUBCLASS_XBOX360_CONTROLLER 0x5d
> +#define UIPROTO_XBOX360_GAMEPAD 0x01
> +#define UISUBCLASS_XBOXONE_CONTROLLER 0x47
> +#define UIPROTO_XBOXONE_GAMEPAD 0xd0
> #endif /* !SMALL_KERNEL */
>
> #define DEVNAME(sc) ((sc)->sc_dev.dv_xname)
> @@ -124,6 +126,11 @@ uhidev_match(struct device *parent, void
> id->bInterfaceSubClass == UISUBCLASS_XBOX360_CONTROLLER &&
> id->bInterfaceProtocol == UIPROTO_XBOX360_GAMEPAD)
> return (UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO);
> + if (id->bInterfaceClass == UICLASS_VENDOR &&
> + id->bInterfaceSubClass == UISUBCLASS_XBOXONE_CONTROLLER &&
> + id->bInterfaceProtocol == UIPROTO_XBOXONE_GAMEPAD) {
> + return (UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO);
> + }
> #endif /* !SMALL_KERNEL */
> if (id->bInterfaceClass != UICLASS_HID)
> return (UMATCH_NONE);
> @@ -306,6 +313,13 @@ uhidev_use_rdesc(struct uhidev_softc *sc
> /* The Xbox 360 gamepad has no report descriptor. */
> size = sizeof(uhid_xb360gp_report_descr);
> descptr = uhid_xb360gp_report_descr;
> + } else if ((id->bInterfaceClass == UICLASS_VENDOR &&
> + id->bInterfaceSubClass == UISUBCLASS_XBOXONE_CONTROLLER &&
> + id->bInterfaceProtocol == UIPROTO_XBOXONE_GAMEPAD)) {
> + sc->sc_flags |= UHIDEV_F_XB1;
This is taken from NetBSD's approach; UHIDEV_F_XB1 flag is used to let
uhidev_open know to run the special init sequence for this device.
> + /* The Xbox One gamepad has no report descriptor. */
> + size = sizeof(uhid_xbonegp_report_descr);
> + descptr = uhid_xbonegp_report_descr;
> }
>
> if (descptr) {
> @@ -557,6 +571,23 @@ uhidev_open(struct uhidev *scd)
> DPRINTF(("uhidev_open: couldn't allocate owxfer\n"));
> error = ENOMEM;
> goto out3;
> + }
> +
> + /* XBox One controller initialization */
> + if (sc->sc_flags & UHIDEV_F_XB1) {
> + uint8_t init_data[] = { 0x05, 0x20, 0x00, 0x01, 0x00 };
The following is the main usbd transfer part that would deserve a close
look. It works, but I would appreciate another set of eyes.
> + int init_data_len = sizeof(init_data);
> + usbd_setup_xfer(sc->sc_oxfer, sc->sc_opipe, 0,
> + init_data, init_data_len,
> + USBD_SYNCHRONOUS | USBD_CATCH, USBD_NO_TIMEOUT,
> + NULL);
> + err = usbd_transfer(sc->sc_oxfer);
> + if (err != USBD_NORMAL_COMPLETION) {
> + DPRINTF(("uhidev_open: xb1 init failed, "
> + "error=%d\n", err));
> + error = EIO;
> + goto out3;
> + }
> }
> }
>
> Index: uhidev.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uhidev.h,v
> retrieving revision 1.25
> diff -u -p -r1.25 uhidev.h
> --- uhidev.h 25 Aug 2018 18:32:05 -0000 1.25
> +++ uhidev.h 9 Jan 2021 15:11:30 -0000
> @@ -61,6 +61,9 @@ struct uhidev_softc {
> struct uhidev **sc_subdevs;
>
> int sc_refcnt;
> +
> + u_int sc_flags;
> +#define UHIDEV_F_XB1 0x0001 /* Xbox One controller */
> };
The location of the UHIDEV_F_XB1 within the struct follows what NetBSD
does.
>
> struct uhidev {
>
