joystick: Add nxdk joystick driver #5

dracc · 2019-05-07T21:46:13Z

Still untested apart from build time smoke test.

Will require XboxDev/nxdk#106.

src/joystick/nxdk/SDL_nxdkjoystick.c

include/SDL_config_xbox.h

dracc · 2019-05-08T09:53:58Z

Apart from requiring the SDL_Hint SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_INPUTS, this is working beautifullly.

Edit: seems the hint is only necessary when running more than one thread.

src/joystick/SDL_gamecontrollerdb.h

src/joystick/nxdk/SDL_nxdkjoystick.c

src/joystick/SDL_joystick.c

src/joystick/nxdk/SDL_nxdkjoystick.c

src/joystick/SDL_gamecontrollerdb.h

JayFoxRox · 2019-05-09T19:46:47Z

src/joystick/xbox/SDL_xboxjoystick.c

  case 3:
-    return "Pad 4";
+    return "Xbox Pad 4";


We should handle it like XInput, https://github.com/XboxDev/nxdk-sdl/blob/nxdk/src/joystick/windows/SDL_xinputjoystick.c#L73

Note that XInput was developed on the original Xbox - so it's the API we try to replicate.

This is pretty much what we already do it, but we should probably follow its style more closely with Original Xbox %s #%u.

We'll probably also need another joystick driver in the future, for non-Xbox devices plugged via USB; which should probably use the USB device name: https://github.com/XboxDev/nxdk-sdl/blob/nxdk/src/joystick/windows/SDL_dinputjoystick.c#L501

Looking at this again.. wouldn't SDL_snprintf(name, sizeof(name), "XInput Controller #%u", 1 + userid); be exactly what we want? We are essentially emulating xinput with xpad (and I intend to bring it even closer in the future).

I think SDL / Windows XInput refers to the 360 variant with LB/RB and digital buttons, but otherwise we are 1:1 XInput. It even has the Xbox-style input device stuff.

Eitherway, we can fix it later, because we probably won't have users in the first weeks (or they'll have to update their code).

This is probably related to #5 (comment), will have to wait until xpad changes have been implemented.

src/joystick/xbox/SDL_xboxjoystick.c

src/joystick/SDL_gamecontroller.c

JayFoxRox · 2019-06-12T09:43:16Z

src/joystick/xbox/SDL_xboxjoystick.c

+
+static void SDL_XBOX_JoystickDetect() {
+  // Implemented in XInputGetEvents
+  return;


Why doesn't this call said function then?

What is that comment trying to express?

This might need a comment that this is temporary:

If XInput is initialized without polling it should auto-detect gamepads.

It just doesn't do that right now, because of nxdk bugs (which makes USBGetEvents necessary).

Additionally, it should be checked that it's fine to auto-update the detected joysticks even if this wasn't called.
I consider this a minor issue, but it might need special attention in the future.
So if an app loops over joysticks twice, is it allowed for the return values to change, even if SDL wasn't explicitly asked to update the list?

src/joystick/xbox/SDL_xboxjoystick.c

JayFoxRox · 2019-06-12T09:45:18Z

src/joystick/xbox/SDL_xboxjoystick.c

+static SDL_JoystickGUID SDL_XBOX_JoystickGetDeviceGUID(int index) {
+  /* FIXME: This should be implemented properly */
+  SDL_JoystickGUID ret;
+  SDL_memset(&ret.data, 0, sizeof(ret.data));


Is there other data in SDL_JoystickGUID that needs to be set to zero (now or in the future)? What do other backends do?

SDL_JoystickGUID is defined as typedef struct { Uint8 data[16]; } SDL_JoystickGUID;. I guess it could make sense to null the full struct for future proofing.

JayFoxRox · 2019-06-12T09:48:53Z

Bikeshed complete. Handing over to someone else for final review.

I did test this a while ago with the sample PR in nxdk, but not after recent changes.

Important note: Only this PR is fine. The samples in nxdk still need work.

Ernegien

Mostly nits...

Ernegien · 2019-06-25T20:12:57Z

src/joystick/xbox/SDL_xboxjoystick.c

+#include <assert.h>
+#include <stdbool.h>
+
+#define NUMAXES 6  /* LStickY, LStickX, LTrigg, RStickY, RStickX, RTrigg */


nit: underscore after NUM in these defines for readability unless these are SDL standards.

Ernegien · 2019-06-25T20:13:39Z

src/joystick/xbox/SDL_xboxjoystick.c

+#define NUMAXES 6  /* LStickY, LStickX, LTrigg, RStickY, RStickX, RTrigg */
+#define NUMBTNS 10 /* A, B, X, Y, black, white, back, start, LThumb, RThumb */
+#define NUMHATS 1  /* D-pad */
+#define NUMBLLS 0  /* No balls here */


nit: might as well fully expand BLLS into BALLS (unless these are SDL standards), what's an extra character when you're already using four for AXES?

Ernegien · 2019-06-25T20:14:25Z

src/joystick/xbox/SDL_xboxjoystick.c

+#define NUMHATS 1  /* D-pad */
+#define NUMBLLS 0  /* No balls here */
+
+#define BUTTONDEADZONE 0x20


nit: underscore separation for readability unless these are SDL standards.

I think this is fine

Ernegien · 2019-06-25T20:17:43Z

src/joystick/SDL_gamecontroller.c

-
+#ifdef SDL_JOYSTICK_XBOX
+    if (!mapping) {
+      SDL_bool existing;


Any need to assert what existing should be after the call or does it not matter?

Not sure, only know that the Linux code above doesn't do any extra checks.

Ernegien · 2019-06-25T20:25:14Z

src/joystick/xbox/SDL_xboxjoystick.c

+}
+
+static int SDL_XBOX_JoystickOpen(SDL_Joystick *joystick, int index) {
+  if (g_Pads[index].hPresent == 0) {


Should probably assert the index argument isn't out-of-bounds in the g_Pads array.

Ernegien · 2019-06-25T20:28:13Z

src/joystick/xbox/SDL_xboxjoystick.c

+  return 0;
+}
+
+static bool analogButtonPressed(SDL_Joystick *joystick, int button) {


nit: possibly rename button to button_index here and elsewhere.

Ernegien · 2019-06-25T20:28:52Z

src/joystick/xbox/SDL_xboxjoystick.c

+  return;
+}
+
+static const char* SDL_XBOX_JoystickNameForDeviceIndex(int i) {


nit: rename i to index as referred to in the rest of the code.

This might be SDL standard stuff. Otherwise a good suggestion

Ernegien · 2019-06-25T20:29:30Z

src/joystick/xbox/SDL_xboxjoystick.c

+}
+
+static bool digitalButtonPressed(SDL_Joystick *joystick, int button) {
+  return (joystick->hwdata->padData->CurrentButtons.usDigitalButtons & button) != 0;


nit: Parentheses can be removed here.

GCC gives this; warning: & has lower precedence than !=; != will be evaluated first [-Wparentheses]

Ernegien · 2019-06-25T20:29:54Z

src/joystick/xbox/SDL_xboxjoystick.c

+}
+
+static void analogButtonUpdate(SDL_Joystick *joystick, int button, int index) {
+  if (analogButtonPressed(joystick, button) != joystick->buttons[index]) {


Should probably assert index is within bounds here and elsewhere.

Ernegien · 2019-06-25T20:31:29Z

src/joystick/xbox/SDL_xboxjoystick.c

+  axisUpdate(joystick,  xpi->sLThumbX, 0);
+  axisUpdate(joystick,  xpi->sLThumbY, 1);
+  ltrigg = xpi->CurrentButtons.ucAnalogButtons[XPAD_LEFT_TRIGGER];
+  ltrigg = ((ltrigg << 8) | ltrigg) - (1 << 15);


Should convert this into a MACRO or utility function for reuse and readability.

Only used twice. This is fine as-is (in my opinion).

thrimbor

Tested the current state of the branch in XQEMU and on my 1.0 Xbox. Everything seemed to work and nothing catches my eye code-wise, so apart from @Ernegien's remarks, I'd say this is fine to get merged.

Make sure the thread is actually paused, and context backep-up, before SurfaceView is destroyed (eg surfaceDestroyed() actually returns). Add a timeout when surfaceDestroyed() is called, and check 'backup_done' variable. It prevents crashes like: #00 pc 000000000000c0d0 /system/lib64/libutils.so (android::RefBase::incStrong(void const*) const+8) #1 pc 000000000000c7f4 /vendor/lib64/egl/eglSubDriverAndroid.so (EglAndroidWindowSurface::UpdateBufferList(ANativeWindowBuffer*)+284) #2 pc 000000000000c390 /vendor/lib64/egl/eglSubDriverAndroid.so (EglAndroidWindowSurface::DequeueBuffer()+240) #3 pc 000000000000bb10 /vendor/lib64/egl/eglSubDriverAndroid.so (EglAndroidWindowSurface::GetBuffer(EglSubResource*, EglMemoryDesc*)+64) #4 pc 000000000032732c /vendor/lib64/egl/libGLESv2_adreno.so (EglWindowSurface::UpdateResource(EsxContext*)+116) XboxDev#5 pc 0000000000326dd0 /vendor/lib64/egl/libGLESv2_adreno.so (EglWindowSurface::GetResource(EsxContext*, EsxResource**, EsxResource**, int)+56) XboxDev#6 pc 00000000002ae484 /vendor/lib64/egl/libGLESv2_adreno.so (EsxContext::AcquireBackBuffer(int)+364) XboxDev#7 pc 0000000000249680 /vendor/lib64/egl/libGLESv2_adreno.so (EsxContext::Clear(unsigned int, unsigned int, unsigned int, EsxClearValues*)+1800) XboxDev#8 pc 00000000002cb52c /vendor/lib64/egl/libGLESv2_adreno.so (EsxGlApiParamValidate::GlClear(EsxDispatch*, unsigned int)+132)

dracc mentioned this pull request May 7, 2019

Add SDL input samples XboxDev/nxdk#106

Closed

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed May 7, 2019

View reviewed changes

include/SDL_config_xbox.h Show resolved Hide resolved

dracc marked this pull request as ready for review May 8, 2019 05:29

dracc force-pushed the nxdk_joystick branch from 5d0eadd to 680bf37 Compare May 8, 2019 09:36

dracc force-pushed the nxdk_joystick branch from 680bf37 to d76ef40 Compare May 8, 2019 13:52

dracc commented May 8, 2019

View reviewed changes

src/joystick/SDL_gamecontrollerdb.h Outdated Show resolved Hide resolved

JayFoxRox reviewed May 8, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

mborgerson reviewed May 9, 2019

View reviewed changes

src/joystick/SDL_joystick.c Outdated Show resolved Hide resolved

mborgerson reviewed May 9, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

mborgerson reviewed May 9, 2019

View reviewed changes

src/joystick/nxdk/SDL_nxdkjoystick.c Outdated Show resolved Hide resolved

dracc force-pushed the nxdk_joystick branch 2 times, most recently from 57a802f to 2913adf Compare May 9, 2019 11:33

JayFoxRox reviewed May 9, 2019

View reviewed changes

src/joystick/SDL_gamecontrollerdb.h Outdated Show resolved Hide resolved

JayFoxRox reviewed May 9, 2019

View reviewed changes

dracc force-pushed the nxdk_joystick branch 4 times, most recently from c461868 to 0109e5f Compare May 10, 2019 08:43

dracc mentioned this pull request May 11, 2019

Sdl input dracc/NevolutionX#14

Merged

dracc force-pushed the nxdk_joystick branch from 0109e5f to 9e5fccb Compare May 14, 2019 17:02

JayFoxRox reviewed Jun 8, 2019

View reviewed changes

src/joystick/xbox/SDL_xboxjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed Jun 8, 2019

View reviewed changes

src/joystick/xbox/SDL_xboxjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed Jun 8, 2019

View reviewed changes

src/joystick/SDL_gamecontroller.c Outdated Show resolved Hide resolved

dracc force-pushed the nxdk_joystick branch 4 times, most recently from 83a1279 to a6c6ffb Compare June 12, 2019 06:37

JayFoxRox reviewed Jun 12, 2019

View reviewed changes

src/joystick/xbox/SDL_xboxjoystick.c Outdated Show resolved Hide resolved

JayFoxRox reviewed Jun 12, 2019

View reviewed changes

JayFoxRox requested review from Ernegien, LukeUsher and thrimbor June 12, 2019 09:49

dracc force-pushed the nxdk_joystick branch from a6c6ffb to 4e26b26 Compare June 12, 2019 10:22

Ernegien reviewed Jun 25, 2019

View reviewed changes

thrimbor reviewed Jun 27, 2019

View reviewed changes

dracc force-pushed the nxdk_joystick branch 6 times, most recently from 0761259 to cac21e0 Compare July 4, 2019 22:34

dracc added 3 commits July 5, 2019 20:03

joystick: Add nxdk joystick driver

a676f8c

build: Update Makefile.xbox for Joystick

b6b10fd

joystick: Add nxdk GameController support

94d4739

dracc force-pushed the nxdk_joystick branch from cac21e0 to 94d4739 Compare July 5, 2019 18:03

thrimbor merged commit 96d6213 into XboxDev:nxdk Jul 5, 2019

thrimbor mentioned this pull request Jul 19, 2019

sdl: Update submodule XboxDev/nxdk#148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

joystick: Add nxdk joystick driver #5

joystick: Add nxdk joystick driver #5

dracc commented May 7, 2019 •

edited

Loading

dracc commented May 8, 2019 •

edited

Loading

JayFoxRox May 9, 2019 •

edited

Loading

JayFoxRox Jun 8, 2019

dracc Jun 12, 2019

JayFoxRox Jun 12, 2019

JayFoxRox Jun 12, 2019

JayFoxRox Jun 12, 2019

dracc Jun 12, 2019

JayFoxRox commented Jun 12, 2019

Ernegien left a comment

Ernegien Jun 25, 2019

Ernegien Jun 25, 2019

Ernegien Jun 25, 2019

JayFoxRox Jun 25, 2019

Ernegien Jun 25, 2019

dracc Jul 4, 2019

Ernegien Jun 25, 2019

Ernegien Jun 25, 2019

Ernegien Jun 25, 2019

JayFoxRox Jun 25, 2019

Ernegien Jun 25, 2019

dracc Jul 4, 2019

Ernegien Jun 25, 2019

Ernegien Jun 25, 2019

JayFoxRox Jun 25, 2019 •

edited

Loading

thrimbor left a comment

joystick: Add nxdk joystick driver #5

joystick: Add nxdk joystick driver #5

Conversation

dracc commented May 7, 2019 • edited Loading

dracc commented May 8, 2019 • edited Loading

JayFoxRox May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayFoxRox commented Jun 12, 2019

Ernegien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayFoxRox Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

thrimbor left a comment

Choose a reason for hiding this comment

dracc commented May 7, 2019 •

edited

Loading

dracc commented May 8, 2019 •

edited

Loading

JayFoxRox May 9, 2019 •

edited

Loading

JayFoxRox Jun 25, 2019 •

edited

Loading