On Thu, Jan 21, 2021 at 12:05:59PM -0700, Thomas Frohwein wrote:
> On Sat, Jan 09, 2021 at 10:46:17AM -0700, Thomas Frohwein wrote:
> > Hi,
> >
> > The next stepping stone in my project to improve out-of-the-box
> > gamecontroller support.
>
> Haven't gotten much feedback since I sent this out. Below is a slightly
> adjusted diff that generates the GUID in BSD_JoystickOpen rather than
> BSD_JoystickInit, avoiding a duplicate open(2) - close(2) dance.
>
> Some testing would be appreciated - I tested it on 4 controllers, but
> it would be good to see that this works with some more controllers.
>
> Here the Cliff notes of the diff:
> - Enables using GUID to get joystick mapping from the included (or
> game-specific) controller mapping.
> - This replaces the prior workaround that was much more primitive, with
> a fallback to standard XBox360 mapping and customization with a hand-
> crafter SDL_GAMECONTROLLERCONFIG environment variable.
> - The entire patch for SDL_gamecontroller.c can now be dropped.
> - This gets us much closer to what's done on other platforms (Linux,
> MacOS) to appropriately map a large number of gamecontrollers.
> - The solution is functional, though not yet 100% ideal in regards to
> the creation and storage of the GUID, as it's stored as the string
> which results in conversion to the numerical and then back to the
> string. We will figure out the solution to this eventually, but I
> don't think this should hold up the transition as the current
> solution does not adversely affect performance.
>
> More details in the original email.
>
> Any of {testing,feedback,oks} would be appreciated.
>
> >
> > Background: On other platforms (Linux, MacOSX), SDL2 uses a GUID for
> > gamecontrollers combined with a database [1] to provide the default
> > button/axis mappings. This hasn't worked on OpenBSD (or apparently any
> > *BSD) and the SDL2 code resorts to simply converting the first 16 chars
> > of the device name to the GUID, which doesn't help with making use of
> > the gamecontrollerdb [2]. Therefore, the way we have been handling this
> > so far is that the env var SDL_GAMECONTROLLERCONFIG was available to
> > hold a mapping string [3]. This poses a significant barrier, as users
> > have to learn the syntax of the mapping strings and find one fitting
> > their controller by trial and error mostly.
> >
> > This diff fixes the issue. To accomplish that, a couple of tasks are
> > added:
> >
> > - Calculate and store the GUID (Credit goes to brynet@ for finding the
> > GUID calculation and writing the initial proof-of-concept code).
> > - Remove the workaround code in SDL_gamecontroller.c that would either
> > read the mapping from SDL_GAMECONTROLLERCONFIG if present, or
> > otherwise default to the (commonly used) XBox 360 controller layout.
> > - Enable the mappings in SDL_gamecontrollerdb.h for OpenBSD.
> > - Assign buttons correctly - not in the order that they appear in the
> > HID report, but based on their numbering in the HID report descriptor.
> > - Invert Y/Ry axes for the newer XInput-type gamecontrollers. This has
> > been a particularly annoying problem, as USB standards recommend the
> > positive directions to be left to right and far to near, but these
> > controllers just use near-to-far as the positive direction. [4]
> >
> > The problem with the last bullet point is that the Y/Ry axis inversion
> > needs to be applied *only* to the XInput devices, and *not* to the
> > older DInput devices. I can't check for XBox360 and other interface
> > subclasses (as used by the kernel in uhidev.c), as
> > USB_GET_INTERFACE_DESC is not exposed in the uhid(4) driver (see
> > uhid_do_ioctl() in uhid.c).
> > The solution I came up with is admittedly a (minor) hack. It turns out
> > that the old DInput devices report their axis values as an unsigned
> > 8bit integer, while the newer ones use a signed 16bit integer. With my
> > collection of gamecontrollers, checking if hitem.logical_maximum is
> > greater than 255 allows to reliably pick the correct handling of the
> > Y/Ry axes.
> >
> > Other caveats:
> >
> > - With the newly enabled GUID detection, SDL2 checks for a matching
> > controller in gamecontrollerdb.txt in the application directory.
> > I found that with one game (Cryptark), this contained bogus mappings
> > for the XBox One controller (see the post on tech@), so some buttons
> > and the Dpad didn't work until I removed that file.
> > - For some reason, the GUID for DInput devices matches the MACOSX
> > entries in SDL_gamecontrollerdb.h, but the XInput devices match the
> > LINUX entries. It's gonna take me a bit of time to get to the bottom
> > of this; in the meantime just allowing both sets of GUID mappings
> > works for the devices here on my testing
> > - The BSD_JoystickGetDeviceGUID() is not ideal at this point, as it
> > takes the GUID stored as a string in char **joyguids and converts it
> > into the raw numerical. I haven't figured out how to do the GUID
> > calculation to avoid this additional transformation step yet. In the
> > meantime, I don't notice performance issues and I don't think it's
> > worth holding up the GUID mapping support over this.
> >
> > Testing:
> >
> > I tested this with sdl2-jstest (from sdl-jstest package) and few
> > games (Owlboy, Cryptark) with these controllers that I have:
> > XBox 360 controller (XInput), XBox One controller (XInput) (see separate
> > mail on tech@), Logitech F310 (both DInput and XInput), Logitech Dual
> > Action (DInput)). This maps everything correctly without additional
> > configuration (as long as faulty gamecontrollerdb.txt in the game
> > directory is removed).
> >
> > Testing with a variety of controllers would be useful to see if this
> > really works across most of them as intended. If you do that, please
> > let me know the mapping/functioning before and after the diff is
> > applied.
> >
> > [1]
> > https://hg.libsdl.org/SDL/file/2370522dac4c/src/joystick/SDL_gamecontrollerdb.h
> > [2]
> > https://hg.libsdl.org/SDL/file/2370522dac4c/src/joystick/bsd/SDL_bsdjoystick.c#l700
> > [3] https://marc.info/?l=openbsd-ports-cvs&m=152080802204959&w=2
> > [4] https://www.usb.org/sites/default/files/documents/hid1_11.pdf (page 20)
>
> Here the updated diff:
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/devel/sdl2/Makefile,v
> retrieving revision 1.32
> diff -u -p -r1.32 Makefile
> --- Makefile 6 Jan 2021 22:32:08 -0000 1.32
> +++ Makefile 21 Jan 2021 18:47:48 -0000
> @@ -5,6 +5,7 @@ COMMENT= cross-platform multimedia libra
> V= 2.0.14
> DISTNAME= SDL2-${V}
> PKGNAME= sdl2-${V}
> +REVISION= 0
> CATEGORIES= devel
> MASTER_SITES= https://www.libsdl.org/release/
>
> Index: patches/patch-src_joystick_SDL_gamecontroller_c
> ===================================================================
> RCS file: patches/patch-src_joystick_SDL_gamecontroller_c
> diff -N patches/patch-src_joystick_SDL_gamecontroller_c
> --- patches/patch-src_joystick_SDL_gamecontroller_c 6 Jan 2021 22:32:08
> -0000 1.7
> +++ /dev/null 1 Jan 1970 00:00:00 -0000
> @@ -1,50 +0,0 @@
> -$OpenBSD: patch-src_joystick_SDL_gamecontroller_c,v 1.7 2021/01/06 22:32:08
> thfr Exp $
> -
> -enable GameController API the Linux fallback way (by posing as Xbox360
> -controller)
> -also disable checking string "Xbox 360 Wireless Receiver", so for now
> -everything will be Xbox360 controller (works with generic joysticks)
> -map to SDL_GAMECONTROLLERCONFIG envvar if available
> -Use layout for XBox360 controller to maximize compatibility because
> -many controllers use this mapping
> -
> -Index: src/joystick/SDL_gamecontroller.c
> ---- src/joystick/SDL_gamecontroller.c.orig
> -+++ src/joystick/SDL_gamecontroller.c
> -@@ -968,7 +968,7 @@ static char *SDL_PrivateGetControllerGUIDFromMappingSt
> - SDL_memcpy(&pchGUID[8], &pchGUID[0], 4);
> - SDL_memcpy(&pchGUID[0], "03000000", 8);
> - }
> --#elif __MACOSX__
> -+#elif defined(__MACOSX__) || defined(__OpenBSD__)
> - if (SDL_strlen(pchGUID) == 32 &&
> - SDL_memcmp(&pchGUID[4], "000000000000", 12) == 0 &&
> - SDL_memcmp(&pchGUID[20], "000000000000", 12) == 0) {
> -@@ -1131,17 +1131,21 @@ static ControllerMapping_t
> *SDL_PrivateGetControllerMa
> - ControllerMapping_t *mapping;
> -
> - mapping = SDL_PrivateGetControllerMappingForGUID(guid, SDL_FALSE);
> --#ifdef __LINUX__
> -+#if defined(__LINUX__) || defined(__OpenBSD__)
> - if (!mapping && name) {
> -- if (SDL_strstr(name, "Xbox 360 Wireless Receiver")) {
> -- /* The Linux driver xpad.c maps the wireless dpad to buttons */
> -- SDL_bool existing;
> -+ /* The Linux driver xpad.c maps the wireless dpad to buttons */
> -+ SDL_bool existing;
> -+ char guid_str[1024];
> -+ SDL_JoystickGetGUIDString(guid, guid_str, sizeof(guid_str));
> -+ if (SDL_GetHint(SDL_HINT_GAMECONTROLLERCONFIG) == NULL) {
> - mapping = SDL_PrivateAddMappingForGUID(guid,
> --"none,X360 Wireless
> Controller,a:b0,b:b1,back:b6,dpdown:b14,dpleft:b11,dpright:b12,dpup:b13,guide:b8,leftshoulder:b4,leftstick:b9,lefttrigger:a2,leftx:a0,lefty:a1,rightshoulder:b5,rightstick:b10,righttrigger:a5,rightx:a3,righty:a4,start:b7,x:b2,y:b3",
> -+"none,XBox360
> Controller,a:b7,b:b8,back:b1,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b4,leftstick:b2,lefttrigger:a2,leftx:a0,lefty:a1~,rightshoulder:b5,rightstick:b3,righttrigger:a5,rightx:a3,righty:a4~,start:b0,x:b9,y:b10",
> - &existing,
> SDL_CONTROLLER_MAPPING_PRIORITY_DEFAULT);
> -+ } else {
> -+ mapping = SDL_PrivateAddMappingForGUID(guid,
> SDL_GetHint(SDL_HINT_GAMECONTROLLERCONFIG), &existing,
> SDL_CONTROLLER_MAPPING_PRIORITY_DEFAULT);
> - }
> - }
> --#endif /* __LINUX__ */
> -+#endif /* __LINUX__ || __OpenBSD__ */
> -
> - if (!mapping && name && !SDL_IsJoystickWGI(guid)) {
> - if (SDL_strstr(name, "Xbox") || SDL_strstr(name, "X-Box") ||
> SDL_strstr(name, "XBOX")) {
> Index: patches/patch-src_joystick_SDL_gamecontrollerdb_h
> ===================================================================
> RCS file: patches/patch-src_joystick_SDL_gamecontrollerdb_h
> diff -N patches/patch-src_joystick_SDL_gamecontrollerdb_h
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_joystick_SDL_gamecontrollerdb_h 21 Jan 2021 18:47:48
> -0000
> @@ -0,0 +1,26 @@
> +$OpenBSD$
> +
> +enable controller detection by GUID on OpenBSD
> +use both LINUX and MACOSX guids to match both XInput and DInput devices
> +
> +Index: src/joystick/SDL_gamecontrollerdb.h
> +--- src/joystick/SDL_gamecontrollerdb.h.orig
> ++++ src/joystick/SDL_gamecontrollerdb.h
> +@@ -338,7 +338,7 @@ static const char *s_ControllerMappings [] =
> +
> "030000004f04000003d0000000000000,run'n'drive,a:b1,b:b2,back:b8,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,guide:b7,leftshoulder:a3,leftstick:b10,lefttrigger:b4,leftx:a0,lefty:a1,rightshoulder:a4,rightstick:b11,righttrigger:b5,rightx:a2,righty:a5,start:b9,x:b0,y:b3,",
> + "03000000101c0000171c000000000000,uRage
> Gamepad,a:b2,b:b1,back:b8,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b4,leftstick:b10,lefttrigger:b6,leftx:a0,lefty:a1,rightshoulder:b5,rightstick:b11,righttrigger:b7,rightx:a2,righty:a3,start:b9,x:b3,y:b0,",
> + #endif
> +-#if defined(__MACOSX__)
> ++#if defined(__MACOSX__) || defined(__OpenBSD__)
> + "03000000c82d00000090000001000000,8BitDo FC30
> Pro,a:b0,b:b1,back:b10,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b6,leftstick:b13,lefttrigger:a4,leftx:a0,lefty:a1,rightshoulder:b7,rightstick:b14,righttrigger:a5,rightx:a2,righty:a3,start:b11,x:b3,y:b4,hint:SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,",
> + "03000000c82d00000090000001000000,8BitDo FC30
> Pro,a:b1,b:b0,back:b10,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b6,leftstick:b13,lefttrigger:a4,leftx:a0,lefty:a1,rightshoulder:b7,rightstick:b14,righttrigger:a5,rightx:a2,righty:a3,start:b11,x:b4,y:b3,hint:!SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,",
> + "03000000c82d00001038000000010000,8BitDo FC30
> Pro,a:b0,b:b1,back:b10,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b6,leftstick:b13,lefttrigger:a5,leftx:a0,lefty:a1,rightshoulder:b7,rightstick:b14,righttrigger:a4,rightx:a2,righty:a3,start:b11,x:b3,y:b4,hint:SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,",
> +@@ -479,7 +479,7 @@ static const char *s_ControllerMappings [] =
> + "03000000830500006020000000010000,iBuffalo SNES
> Controller,a:b1,b:b0,back:b6,dpdown:+a1,dpleft:-a0,dpright:+a0,dpup:-a1,leftshoulder:b4,rightshoulder:b5,start:b7,x:b3,y:b2,hint:!SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,",
> + "03000000830500006020000000000000,iBuffalo USB 2-axis 8-button
> Gamepad,a:b1,b:b0,back:b6,leftshoulder:b4,leftx:a0,lefty:a1,rightshoulder:b5,start:b7,x:b3,y:b2,",
> + #endif
> +-#if defined(__LINUX__)
> ++#if defined(__LINUX__) || defined(__OpenBSD__)
> + "03000000c82d00000090000011010000,8BitDo FC30
> Pro,a:b0,b:b1,back:b10,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b6,leftstick:b13,lefttrigger:a4,leftx:a0,lefty:a1,rightshoulder:b7,rightstick:b14,righttrigger:a5,rightx:a2,righty:a3,start:b11,x:b3,y:b4,hint:SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,",
> + "03000000c82d00000090000011010000,8BitDo FC30
> Pro,a:b1,b:b0,back:b10,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b6,leftstick:b13,lefttrigger:a4,leftx:a0,lefty:a1,rightshoulder:b7,rightstick:b14,righttrigger:a5,rightx:a2,righty:a3,start:b11,x:b4,y:b3,hint:!SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,",
> + "05000000c82d00001038000000010000,8BitDo FC30
> Pro,a:b0,b:b1,back:b10,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b6,leftstick:b13,lefttrigger:a5,leftx:a0,lefty:a1,rightshoulder:b7,rightstick:b14,righttrigger:a4,rightx:a2,righty:a3,start:b11,x:b3,y:b4,hint:SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,",
> Index: patches/patch-src_joystick_bsd_SDL_bsdjoystick_c
> ===================================================================
> RCS file: patches/patch-src_joystick_bsd_SDL_bsdjoystick_c
> diff -N patches/patch-src_joystick_bsd_SDL_bsdjoystick_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_joystick_bsd_SDL_bsdjoystick_c 21 Jan 2021 18:47:48
> -0000
> @@ -0,0 +1,125 @@
> +$OpenBSD$
> +
> +assign buttons correctly
> +get GUID using USB_GET_DEVICEINFO
> +detect newer (XInput-style) gamecontroller if hitem.logical_maximum is
> +> 255; if so invert y axes
> +
> +Index: src/joystick/bsd/SDL_bsdjoystick.c
> +--- src/joystick/bsd/SDL_bsdjoystick.c.orig
> ++++ src/joystick/bsd/SDL_bsdjoystick.c
> +@@ -83,6 +83,9 @@
> +
> + #ifdef __OpenBSD__
> +
> ++#define DEV_USB 3 /* needed to get GUID from
> USB_GET_DEVICEINFO */
> ++#define GUID_LEN 32 /* GUID string has length 32 */
> ++
> + #define HUG_DPAD_UP 0x90
> + #define HUG_DPAD_DOWN 0x91
> + #define HUG_DPAD_RIGHT 0x92
> +@@ -192,6 +195,9 @@ struct joystick_hwdata
> +
> + static char *joynames[MAX_JOYS];
> + static char *joydevnames[MAX_JOYS];
> ++#ifdef __OpenBSD__
> ++static char joyguids[MAX_JOYS][GUID_LEN];
> ++#endif
> +
> + static int report_alloc(struct report *, struct report_desc *, int);
> + static void report_free(struct report *);
> +@@ -222,6 +228,9 @@ BSD_JoystickInit(void)
> +
> + SDL_memset(joynames, 0, sizeof(joynames));
> + SDL_memset(joydevnames, 0, sizeof(joydevnames));
> ++#ifdef __OpenBSD__
> ++ SDL_memset(joyguids, 0, sizeof(char) * MAX_JOYS * GUID_LEN);
> ++#endif
> +
> + for (i = 0; i < MAX_UHID_JOYS; i++) {
> + SDL_Joystick nj;
> +@@ -356,6 +365,9 @@ BSD_JoystickOpen(SDL_Joystick *joy, int device_index)
> + #endif
> + int fd;
> + int i;
> ++#ifdef __OpenBSD__
> ++ struct usb_device_info di;
> ++#endif
> +
> + fd = open(path, O_RDONLY);
> + if (fd == -1) {
> +@@ -434,6 +446,17 @@ BSD_JoystickOpen(SDL_Joystick *joy, int device_index)
> + }
> + desc_failed:
> + #endif
> ++#if defined(__OpenBSD__)
> ++ if (ioctl(fd, USB_GET_DEVICEINFO, &di) != -1) {
> ++ SDL_snprintf(joyguids[numjoysticks],
> ++ SDL_arraysize(joyguids[device_index]),
> ++ "%02x%02x0000%02x%02x0000%02x%02x0000%02x%02x0000",
> ++ DEV_USB & 0xFF, DEV_USB >> 8,
> ++ di.udi_vendorNo & 0xFF, di.udi_vendorNo >> 8,
> ++ di.udi_productNo & 0xFF, di.udi_productNo >> 8,
> ++ di.udi_releaseNo & 0xFF, di.udi_releaseNo >> 8);
> ++ }
> ++#endif
> + if (report_alloc(rep, hw->repdesc, REPORT_INPUT) < 0) {
> + goto usberr;
> + }
> +@@ -544,6 +567,7 @@ BSD_JoystickUpdate(SDL_Joystick *joy)
> + Sint32 v;
> + #ifdef __OpenBSD__
> + Sint32 dpad[4] = {0, 0, 0, 0};
> ++ int actualbutton;
> + #endif
> +
> + #if defined(__FREEBSD__) || SDL_JOYSTICK_USBHID_MACHINE_JOYSTICK_H ||
> defined(__FreeBSD_kernel__) || defined(__DragonFly_)
> +@@ -618,6 +642,18 @@ BSD_JoystickUpdate(SDL_Joystick *joy)
> + naxe = joy->hwdata->axis_map[joyaxe];
> + /* scaleaxe */
> + v = (Sint32) hid_get_data(REP_BUF_DATA(rep),
> &hitem);
> ++#ifdef __OpenBSD__
> ++ /* XInput controllermapping relies on inverted
> Y axes.
> ++ * These devices have a 16bit signed space, as
> opposed
> ++ * to older DInput devices (8bit unsigned), so
> ++ * hitem.logical_maximum can be used to
> differentiate them.
> ++ */
> ++ if ((joyaxe == JOYAXE_Y || joyaxe == JOYAXE_RY)
> ++ && hitem.logical_maximum > 255) {
> ++ if (v != 0)
> ++ v = ~v;
> ++ }
> ++#endif
> + v -= (hitem.logical_maximum +
> + hitem.logical_minimum + 1) / 2;
> + v *= 32768 /
> +@@ -652,7 +688,12 @@ BSD_JoystickUpdate(SDL_Joystick *joy)
> + }
> + case HUP_BUTTON:
> + v = (Sint32) hid_get_data(REP_BUF_DATA(rep), &hitem);
> ++#ifdef __OpenBSD__
> ++ actualbutton = HID_USAGE(hitem.usage) - 1; /* sdl
> buttons are zero-based */
> ++ SDL_PrivateJoystickButton(joy, actualbutton, v);
> ++#else
> + SDL_PrivateJoystickButton(joy, nbutton, v);
> ++#endif
> + nbutton++;
> + break;
> + default:
> +@@ -697,11 +738,16 @@ static SDL_JoystickGUID
> + BSD_JoystickGetDeviceGUID( int device_index )
> + {
> + SDL_JoystickGUID guid;
> ++#ifdef __OpenBSD__
> ++ guid = SDL_JoystickGetGUIDFromString(joyguids[device_index]);
> ++ return guid;
> ++#else
> + /* the GUID is just the first 16 chars of the name for now */
> + const char *name = BSD_JoystickGetDeviceName( device_index );
> + SDL_zero( guid );
> + SDL_memcpy( &guid, name, SDL_min( sizeof(guid), SDL_strlen( name ) ) );
> + return guid;
> ++#endif
> + }
> +
> + static int
I tested this on my generic USB SNES controller (along with the ujoy
patch), and it works perfectly.
It even finds a better GUID than the one I had been using in my local
SDL_GAMECONTROLLERCONFIG override.
Found 1 joystick(s)
Joystick Name: 'Joystick (0)'
Joystick GUID: 030000001008000001e5000006010000
Joystick Number: 0
Number of Axes: 2
Number of Buttons: 10
Number of Hats: 0
Number of Balls: 0
GameController:
Name: 'NEXT SNES Controller'
Mapping: '030000001008000001e5000006010000,NEXT SNES
Controller,a:b2,b:b1,back:b8,dpdown:+a1,dpleft:-a0,dpright:+a0,dpup:-a1,leftshoulder:b4,rightshoulder:b6,start:b9,x:b3,y:b0,'
Tested with:
uhidev4 at uhub0 port 12 configuration 1 interface 0 "vendor 0x0810 usb
gamepad" rev 1.00/1.06 addr 2
uhidev4: iclass 3/0, 1 report id
ujoy0 at uhidev4 reportid 1: input=7, output=0, feature=0
ok brynet@ (along with the /dev/ujoy/%d patch).
-Bryan.
