Fix macOS Reset() causing crash on input thread when GCAdapter is unplugged

[gpipes]

Steps to repro:

1. On macOS 11.0 (probably others macOS too) start Slippi Dolphin with an adapter plugged in
2. Unplug the adapter

Expected: Slippi Dolphin remains open
Actual: Slippi Dolphin closes and MacOS reports Slippi Dolphin closed unexpectedly (crash)

Change notes + thoughts:

• On macOS 11.0 I noticed that the libusb hotplug event was getting
  called on the read thread. This seemed to be different behavior than
  my windows platform where the hotplug event was happening on it's
  own thread

• when a hotplug detach event happens the reset thread waits for the
  in and out threads to error out with "join"

• however, on macOS 11.0 (when started with an adapter plugged in),
  the detach event would attempt to join itself causing a crash.

• To get around this before trying to join threads reset will try to
  see if it is one of those threads and start a new reset thread if
  so.

• If we wait attempt to join the reset thread from the caller they
  will both be trying to join each other which will also result in a crash

[gpipes]

I've also tested this change against this branch - #257

And with this, mac supported adapter hot-swapping for unplug. The controller menu showed the polling rate go from a number to adapter not detected. It seems there are still issues with a new GCAdapter showing up but unplugging is half the battle!

[ryanmcgrath]

The adapter not showing up (a "new" one) is possibly due to IOKit/kext oddities. Big Sur can run the DriverKit variant instead, which would support dynamically loading/unloading (and thus re-blocking) Apple's handlers.

https://secretkeys.io/gcadapterdriver/

[gpipes]

This got me on the right track! It definitely is mac/obj-c oddities. it's again related to the weirdness of libusb though. Mac can temporarily transfer execution on a thread and that is how the hotplug works.

This means in the scan thread here

Ishiiruka/Source/Core/InputCommon/GCAdapter.cpp

Line 478 in cef2aa7

s_hotplug_event.Wait();

we register the hotplug and wait for it to fire, however it can't ever fire because the thread it's on is blocked on a wait condition. If you remove this and just sleep on the loop the usb detection works exactly as expected and the hotplug event is handled after the sleep completes.

Still trying to work out a clean solution in my head, if there should be a hotplug+reset thread or something like that. Maybe even don't use the wait condition on mac due to hotplug oddities. #ifdef the wait condition seems like the lowest impact, especially since it wasn't working anyway.

any thoughts?

[ryanmcgrath]

You might want to take a gander at how mainline Dolphin does this. I'm not at my laptop but I recall it still uses Wait(), so there should be a way.

[ryanmcgrath]

Also probably worth noting that the libusb used in this project is older and lacks some macOS-specific updates, and I'm not sure offhand what revision mainline Dolphin uses. Updating libusb in this repo is probably not an option given a past effort that went awry with regards to other platforms.

[gpipes]

Looking at the mainline dolphin was the key.

Still trying to work out a clean solution in my head, if there should be a hotplug+reset thread or something like that.

Was what they did except much cleaner. They wrapped the libusb context in an object that spawns an event thread that lives for the lifetime of the context. This way as long as the context is around there is at least 1 dedicated thread willing to handle events others are waiting on. I was able to port over just the Wrapper they used and apply the minimal changes to Ishiiruka's GCAdapter like the mainline. This fixes the macOS crash and when run with #257 seemingly has full hot swap support, at least on macOS now I wasn't able to break it like I used to be able to.

[gpipes]

I'm thinking the linux support for #257 is probably a similar issue/same issue resolved by an event thread. I don't have an ubuntu box to test it on though

[NikhilNarayana]

I'm thinking the linux support for #257 is probably a similar issue/same issue resolved by an event thread. I don't have an ubuntu box to test it on though

I will be testing this PR on windows and Ubuntu just to be sure everything is working fine. Thanks for investigating and implementing a fix!

[gpipes]

Very happy to help, it's my first attempt at a public contribution and I felt like I could jump in with something I really use. It was really exciting my skill set lined up with something I felt was tackle-able relating to Slippi 😄

[gpipes]

oooo hmmm looks like the new files aren't as easy to add to multiplatform stuff as a cmakelists, failing on linker stuff. I'll try to fix this soon

[ryanmcgrath]

On Windows, you'll need to add (I believe) a line to Source/Core/Core/Core.vcxproj, like:

<ClCompile Include="LibusbUtils.cpp" />

Offhand not sure what the Ubuntu issue is, considering it's CMake like macOS and all.

[gpipes]

The windows issue was a vcxproj build and the ubuntu issue was aggressive symbol stripping since usbutils was being built into core/core but not used by core/core. We won't need them in core/core for what mainline dolphin used it for so I put them in inputcommon instead of changing linker options.

I described it a bit more here 7ec1692

I expect it to build across the board now

[NikhilNarayana]

These changes + the PR 257 did add Linux adapter hot swapping.