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
