joystick: Rewrite driver for replacement USB stack #38

Ryzee119 · 2021-04-10T01:07:57Z

Rewrote the sdl_joystick driver to be compatible with the proposed replacement USB stack for nxdk (XboxDev/nxdk#438)

I added some features and fixed some issues in the legacy driver.

It now has:

USB hotplugging via SDL_Events
SDL rumble support
Better guid generator. So functions like SDL_GameControllerGetVendor/Product work.
Supports xbox one and xbox360 controllers
Supports more than 4 game controllers if needed. -DCONFIG_HID_MAX_DEV=x flag

dracc · 2021-04-10T05:46:46Z

Tested with NevolutionX, works great! 💪

JayFoxRox · 2021-04-10T07:43:47Z

src/joystick/xbox/SDL_xboxjoystick.c

-    joystick->hwdata->padData = &g_Pads[index];
-    joystick->guid = SDL_XBOX_JoystickGetDeviceGUID(index);
+static const char* SDL_XBOX_JoystickGetDeviceName(Sint32 device_index) {
+    HID_DEV_T *hdev = usbh_get_hdev_from_device_index(device_index);


With explicit hotplugging support, beware of race conditions like #30 / #31.

Imagine these steps in an app:

A user asks SDL for the number of connected joystick devices

User disconnects the USB device

User takes device count from step 1. to loop over SDL joystick devices to ask for their name

~~This will probably work fine if the SDL code is updating USB exclusively or if USB isn't handled by interrupts. You'd be using the USB driver for synchronization~~ (Edit: Just realized that usbh_get_hdev_from_device_index calls usbh_pooling_hubs, so this race condition probably exists).
However, if USB updates the device list in the background this will create this race condition.

You should check if / how other backends take care of this.

~~(If you already query devices in detect, that'd also be a potential location to set the LEDs for Xbox 360 controllers - but that's another discussion)~~ (Edit: Nevermind, the USB stack does it here: https://github.com/Ryzee119/ohci_usbhostlib/blame/607ffc8222a82f2f6dc0770c9bab4466dcab9452/src_hid/hid_driver.c#L158)

src/joystick/xbox/SDL_xboxjoystick.c

JayFoxRox · 2021-04-10T08:12:50Z

src/joystick/xbox/SDL_xboxjoystick.c

-    assert(button_bit <= 128);
-    return (joystick->hwdata->padData->CurrentButtons.usDigitalButtons & button_bit) != 0;
+    JOY_DBGMSG("SDL_XBOX_JoystickGetDevicePlayerIndex: %i\n", hdev->uid);
+    return hdev->uid;


What prevents you from using the same code as here: https://github.com/Ryzee119/ohci_usbhostlib/blame/607ffc8222a82f2f6dc0770c9bab4466dcab9452/src_hid/hid_driver.c#L160-L161 (other than code quality) ?

iface_t* iface = hdev->iface; if (iface == NULL) return -1; uint8_t port = iface->udev->port_num; port += (port <= 2) ? 2 : -2; //Xbox ports are numbered 3,4,1,2 return port;

It would just need more fine grained checks to query for more than 4 players.
But I'd rather have port number (even if multiple devices share the same port), than to have large and almost meaningless tick values as player index.

I wouldn't be surprised if some applications use the return value from this to access an array by index. So the tick solution is bad.

This might interact with #29, which might have to add this (and I'm not sure how SDL uses it internally):

static void SDL_XBOX_JoystickSetDevicePlayerIndex(int device_index, int player_index) {

Most drivers just return -1 for this but I think its a good function to have for xbox.
I wasnt too happy with just using the uid hence the comment.
Good point about the array index.
Just using port number is tricky. Devices on hubs or with multiple interfaces will return the same number. Also 1.0 with the daughter will need to be considered.
I can think about this though.

Ive updated this so playerindex just returns to device_index. This will just return 0-n, the lower number being the device that was plugged in first. Ideally this should reflect the physical port locations in a future update but it got quite complex managing downstream hubs too, so feel needs more thought.

src/joystick/xbox/SDL_xboxjoystick.c

JayFoxRox · 2021-04-10T08:22:51Z

src/joystick/xbox/SDL_xboxjoystick.c

+    switch (hdev->type)
+    {
+    case XBOXOG_CONTROLLER:
+        return "Original Xbox Controller";


With this driver (and the new USB HID driver) becoming the standard driver, please create issue after merge, about also querying what kind of subdevice this is ("Lightgun"/"Steering Wheel"/...).

This might also be necessary to properly handle the device input updates, so this driver has to take care of it.

Previously this could have been responsibility of the nxdk repository / xinput, but now the responsibility moves.

Noted. I looked into it a bit, but needs some investigaion on where the Xinput subtype comes from. If its just in the interface descriptor or some other control request. I dont have other xb360 peripheral types to investigate.

I looked into it a bit, but needs some investigaion on where the Xinput subtype comes from

If only there was some kind of documentation for XIDs...

I went on a tangent looking up xbox360 protocol for some reason. I already get bType in the usb stack to differentiate between steel battalion and xremote. Subtype shouldnt be too hard to query at enumeration stage so getting this should be easy at some stage.

Okay.

I'm keeping this unresolved so it's clear that an issue will have to be created.

The issue should also contain the content from this discussion (or reference it, but note #40, so links might be dead in the future if it's decided migrate repositories).

src/joystick/xbox/SDL_xboxjoystick.c

JayFoxRox · 2021-04-11T00:27:24Z

src/joystick/xbox/SDL_xboxjoystick.c

@@ -485,6 +493,8 @@ static void SDL_XBOX_JoystickClose(SDL_Joystick *joystick)
 static void SDL_XBOX_JoystickQuit(void) {
    JOY_DBGMSG("SDL_XBOX_JoystickQuit\n");
    usbh_install_hid_conn_callback(NULL, NULL);
+    //We dont call usbh_core_deinit() here incase the user is using
+    //the USB stack in other parts of their application other than game controllers.


Ideally the USB stack would refcount internally; did you check if it already does that?

What do you mean by this?

USB might be used from different spots (like network, which also has this problem).

For the user-facing USB (and network) API design this leaves few possibilities:

Should be managed globally using a singleton (avoid conflicting init/deinit at runtime from different locations, and do those automatically at startup / shutdown)

USB code must be able to handle multiple init / deinit without breaking other code (the typical solution here is reference-counting). Like this (imagine these calls coming from different drivers):
// === app loading phase starts here === // Boot initializes some `unsigned int refcount = 0;` in the USB_Stack // === app startup phase starts here === // Internally does realInit(), because we had a refcount of 0 on entry usbh_core_init(); // refcount++ -> refcount = 1; // Internally nop() because refcount was non-zero on entry (implies this was already initialized; so nothing to do) usbh_core_init(); // refcount++ -> refcount = 2 usbh_core_init(); // refcount++ -> refcount = 3 // === app shutdown phase starts here === // Internally nop() because refcount was non-zero on exit (implies this is still being used elsewhere; so nothing to do) usbh_core_deinit(); // refcount-- -> refcount = 2 usbh_core_deinit(); // refcount-- -> refcount = 1 // Internally does realDeinit(), because we had a refcount of 0 on exit usbh_core_deinit(); // refcount-- -> refcount = 0 // Internally does error(), because we had a refcount of 0 on entry (implies that the lib wasn't initialized anymore) usbh_core_deinit(); // refcount-- -> refcount = 0
(There might also be some extra-work required to verify that all of these calls came from the same thread, or initialized with compatible settings)
Some APIs will do this internally, so I recommend to check if the USB stack might already implement something like this.

Note that this expects that USB also manage access to devices so it's not possible for 2 drivers to access the same device (usually by blocking access to an already opened device).

Details of this should probably be discussed on the USB PR (+ some network API issue).
I just thought I'd mention it, because the USB stack isn't even merged, and it's already becoming a real-world problem.

The backend does not do this at all (apart from a basic initialisation guard to prevent issues by reinit the ohci hardware repeatedly). I dont think it will be too hard to add atleast to a basic level of this. I would prefer to leave this for now as I wont have time for a while.

Keeping this unresolved until issue has been created elsewhere (because this already contains a lot of documentation about why this is necessary, that should be part of an issue)

src/joystick/xbox/SDL_xboxjoystick.c

JayFoxRox · 2021-04-11T10:49:41Z

src/joystick/xbox/SDL_xboxjoystick.c

-    XInput_Init();
-    return 0;
-}
+static Sint32 xinput_parse_input(HID_DEV_T *hdev, PXINPUT_GAMEPAD controller, Uint8 *rdata);


nit: This should eventually move into a separate file / driver (probably outside of SDL).

Fine as-is, because we'll consider SDL the single source of truth for input related stuff after this PR, but we should have an issue that mentions that this is intended to be moved away.

src/joystick/xbox/SDL_xboxjoystick.c

JayFoxRox · 2021-04-12T17:35:29Z

src/joystick/xbox/SDL_xboxjoystick.c

+#ifdef SDL_JOYSTICK_XBOX_DEBUG
+#define JOY_DBGMSG debugPrint
+#else
+#define JOY_DBGMSG(...)


Where do these macro names come from? They don't feel very SDL2-esque.

I'm against these - shortened - custom prefixes.

I just made them up. It's just for debugging.
What do you propose? I dont want to have the back and forward commits just changing macro names

What do you propose?

No idea to be honest.
Feel free to ignore this request; although I'd be leaning towards renaming it LOGD (~LOGD...EBUG)

SDL code is way more inconsistent than I had assume.
I wish the backend code was more consistent - also something to fix upstream probably.

Because apparently SDL is the wild west of codebases (a bit of grep):

./video/directfb/SDL_DirectFB_video.h:#define SDL_DFB_DEBUG(x...) SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, x) ./video/directfb/SDL_DirectFB_video.h:#define SDL_DFB_DEBUG(x...) do {} while (0)

./video/directfb/SDL_DirectFB_video.h:#define SDL_DFB_LOG(x...) SDL_LogInfo(SDL_LOG_CATEGORY_VIDEO, x) ./video/directfb/SDL_DirectFB_video.h:#define SDL_DFB_LOG(x...) do {} while (0)

./audio/openslES/SDL_openslES.c:#define LOGI(...) __android_log_print(ANDROID_LOG_INFO,LOG_TAG,__VA_ARGS__) ./audio/openslES/SDL_openslES.c:#define LOGE(...) __android_log_print(ANDROID_LOG_ERROR,LOG_TAG,__VA_ARGS__)

./hidapi/libusb/hid.c:#define LOG(...) fprintf(stderr, __VA_ARGS__) ./hidapi/libusb/hid.c:#define LOG(...) do {} while (0)

./hidapi/android/hid.cpp:#define LOGE(...) __android_log_print(ANDROID_LOG_ERROR, TAG, __VA_ARGS__) ./hidapi/android/hid.cpp:#define LOGV(...) __android_log_print(ANDROID_LOG_VERBOSE, TAG, __VA_ARGS__) ./hidapi/android/hid.cpp:#define LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, TAG, __VA_ARGS__)

dracc · 2021-04-15T10:12:42Z

May I propose we merge this and open issues if there are unresolved comments here?
This PR is miles better than our current HEAD already, and XboxDev/nxdk#438 which fixes actually breaking stuff is dependent on this.

JayFoxRox · 2021-04-15T11:30:29Z

May I propose we merge this and open issues if there are unresolved comments here?

This PR has been here for less than a week.
So let's not stress too much about our review speed.

Also this has to wait until we are sure that XboxDev/nxdk#438 is complete (which is a lot of code), and that has gotten very little review (I personally didn't look much at it yet either), despite being there for more than 3 weeks.
I think it's very safe to assume that some form of it will be merged, so I had no problem reviewing this PR, knowing this will also eventually be accepted.

The related nxdk USB PR also removes the old USB code, so it will have to update the SDL submodule to the code from this PR in the same step, or SDL would break. So that ties reviews of both PRs + the USB stack together.

(Personally, I'd have liked to see the USB stack get integrated first, then the XID driver, and then the SDL driver coming last, then removal of the old USB stack - but whatever)

I could imagine that the underlying USB code on the nxdk PR will get some more discussion before it's merged (which might demand changes in this PR).
Aside from review, I also certainly hope for more testing on different Xbox revisions, with different gamepads etc., although it's not mandatory.

Changing the USB code is also a significant step, and we are likely one of only a handful of projects using the new USB stack for a purpose like ours.
The nxdk PR also not only includes this non-standard USB stack (with a non-standard interface), but also comes with an all-new custom Xbox input driver (part of which are in the USB stack, and parts of which are in this SDL PR.... why?).
There are also things to resolve like where we store the code or who maintains it (on which I have thoughts, and I assume other maintainers, too).

I think we all agree that slow review and nitpicking too much is annoying, but I think it's equally bad to deal with fallout from a broken or unmaintainable USB stack, or many follow up PRs which move stuff around.
It might feel better for the contributors (less complaints = happier contributor), but it's unmanageable for maintainers (of which nxdk has very little) and takes a lot of extra work.

@Ryzee119 has been here for very long and understands our quality requirements.
He's also review tolerant and likes learning new stuff.
So if review takes longer, I assume it to be less of an issue for him, than it would be for others.
Please correct me if I'm wrong / express your thoughts.

People can still try or use this PR by checking it out manually; it doesn't have to be on master right now (although it would be nice).
Overall, the input subsystem is also mostly a legal issue and less of a technical issue (although this change adds a couple of nice features, like force-feedback), so it's not high-priority either.

espes · 2021-05-26T11:24:53Z

Merging in line with nxdk#438. This still has some minor issues, but seems strictly better than the existing implementation.

Ryzee119 force-pushed the usbh branch from d9a1bb5 to c26296e Compare April 10, 2021 02:18

Ryzee119 mentioned this pull request Apr 10, 2021

USB: Replace OHCI USB host library XboxDev/nxdk#438

Merged

JayFoxRox reviewed Apr 10, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 10, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 10, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 10, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 10, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 10, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 10, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 11, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 11, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 11, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 11, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 11, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 11, 2021

View reviewed changes

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

This was referenced Apr 11, 2021

Reimplementing Hal code XboxDev/nxdk#449

Open

SDL Joystick information might change unexpectedly #31

Open

JayFoxRox reviewed Apr 12, 2021

View reviewed changes

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

JayFoxRox reviewed Apr 12, 2021

View reviewed changes

Ryzee119 force-pushed the usbh branch from 9962a36 to 119715e Compare April 13, 2021 09:13

Ryzee119 force-pushed the usbh branch from 119715e to 845c7e2 Compare April 15, 2021 10:35

Ryzee119 force-pushed the usbh branch from 845c7e2 to d881a08 Compare May 19, 2021 04:04

Ryzee119 force-pushed the usbh branch 2 times, most recently from b13c2a1 to 9cf1609 Compare May 25, 2021 08:59

joystick: Rewrite driver for replacement USB stack

60ff410

Ryzee119 force-pushed the usbh branch from 9cf1609 to 60ff410 Compare May 25, 2021 09:03

espes merged commit ed0dead into XboxDev:nxdk May 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

joystick: Rewrite driver for replacement USB stack #38

joystick: Rewrite driver for replacement USB stack #38

Ryzee119 commented Apr 10, 2021 •

edited

Loading

dracc commented Apr 10, 2021

JayFoxRox Apr 10, 2021 •

edited

Loading

JayFoxRox Apr 10, 2021 •

edited

Loading

JayFoxRox Apr 10, 2021

Ryzee119 Apr 10, 2021 •

edited

Loading

Ryzee119 Apr 11, 2021

JayFoxRox Apr 10, 2021

Ryzee119 Apr 11, 2021

JayFoxRox Apr 11, 2021

Ryzee119 Apr 11, 2021

JayFoxRox Apr 11, 2021

JayFoxRox Apr 11, 2021

Ryzee119 Apr 11, 2021

JayFoxRox Apr 11, 2021 •

edited

Loading

Ryzee119 Apr 12, 2021 •

edited

Loading

JayFoxRox Apr 12, 2021 •

edited

Loading

JayFoxRox Apr 11, 2021

JayFoxRox Apr 12, 2021

Ryzee119 Apr 13, 2021

JayFoxRox Apr 13, 2021

dracc commented Apr 15, 2021 •

edited

Loading

JayFoxRox commented Apr 15, 2021 •

edited

Loading

espes commented May 26, 2021

joystick: Rewrite driver for replacement USB stack #38

joystick: Rewrite driver for replacement USB stack #38

Conversation

Ryzee119 commented Apr 10, 2021 • edited Loading

dracc commented Apr 10, 2021

JayFoxRox Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

JayFoxRox Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryzee119 Apr 10, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayFoxRox Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

Ryzee119 Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

JayFoxRox Apr 12, 2021 • 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

dracc commented Apr 15, 2021 • edited Loading

JayFoxRox commented Apr 15, 2021 • edited Loading

espes commented May 26, 2021

Ryzee119 commented Apr 10, 2021 •

edited

Loading

JayFoxRox Apr 10, 2021 •

edited

Loading

JayFoxRox Apr 10, 2021 •

edited

Loading

Ryzee119 Apr 10, 2021 •

edited

Loading

JayFoxRox Apr 11, 2021 •

edited

Loading

Ryzee119 Apr 12, 2021 •

edited

Loading

JayFoxRox Apr 12, 2021 •

edited

Loading

dracc commented Apr 15, 2021 •

edited

Loading

JayFoxRox commented Apr 15, 2021 •

edited

Loading