Working HID driver for Wii U [TESTING NEEDED] - #6580
Merged
Conversation
== DETAILS - detect() methods in device_* files now check for VID/PID instead of just returning false - add "name" field on hid device, mainly for logging purposes == TESTING Verified my WiiU GC adapter detected properly
== DETAILS I've created the concept of a hid_driver_instance_t which is basically a central place to store the hid pad driver, hid subsystem driver, the pad list, and the instance data for the above in a central location. The HID pad device drivers can use it to perform HID operations in a generic manner. This is more-or-less a pause point so I can catch up with upstream. == TESTING Haven't tested this yet. Compiles without warnings though!
== DETAILS We're at a point where we need to do more than just clean up a local data structure, so I've started implementing the "detach" part of the code so that everything gets cleaned up properly. Also, added error handling inside the polling thread. == TESTING Have not tested yet.
== DETAILS Turns out the cause of the crash was a bad cast, resulting in a function call to nowhere. Also, I think the DSI exception handler only works on the primary core; when this was happening in the background thread, I got a black screen error instead. Next up: finishing up the GCA driver.
== DETAILS (I think) - Uncomment the call in the read loop to start feeding packets to the driver - implement the GCA packet driver - implement the pad interface - fix indentations in GCA driver == TESTING Compiles. Haven't tested yet.
== DETAILS Turns out freeing memory that's already been freed is.. bad. Fix two double-free instances; one due to over-freeing and the other due to wrong order-of-operations causing a double free. Also updated logging a little. == TESTING The GC adapter still clobbers slot 0, but the "emergency exit" sequence works to quit RA cleanly.
== DETAILS Trying to do weird pad math just wasn't working so I bit the bullet and just let it allocate all 16 pads in the slot list, then just mark 0-4 as connected so that the slot allocator would start at 5. I can see it detect the pad, but no idea if it works. Out of time for today.
== DETAILS - Added a new method to the joypad_connection_t interface for getting a single button - wired everything into the hidpad driver - for testing purposes, hacking the top-level joypad driver so that kpad isn't used - add a new RARCH_LOG_BUFFER method to verbosity for logging the contents of a binary buffer (useful for writing/debugging pad drivers) - fix a few bugs in the wiiu GC pad driver The button mapping isn't quite right, and I'm not sure what's going wrong.
== DETAILS The WiiU GC adapter is working! Next up: DualShock 3 I have the skeleton of the driver started, need to work out the activation packet. == TESTING The DS3 driver is broke as hell right now.
== DETAILS Now that I have a working implementation, it's time to tidy up a bit: - there was no need for the HID subsystem's object data to have a reference to the global hid state (since it's global), so removed it. - refactored the users of that member to use the global state, defining reusable macros. - reorganized the information in *.h files - removing the hid state also made the constructor changes to the hid driver unneeded, so I reverted those changes. == TESTING Confirmed clean build. Haven't tested the build yet to make sure everything still works, though.
== DETAILS Whereas the last commit had a hack (that disabled the wiimote driver in the process), this has.. well, a *different* hack that allows pads to register in any order. Note that due to the initialization routines, the gamepad will still likely always get slot 0. Not sure if this can be overridden via config or not. == TESTING Tested locally with GC adapter
== DETAILS - update to not try starting the read loop until after the device is successfully initialized - add new HID wrapper macros needed by ds3 driver - add some debug logging to help with troubleshooting - add button map for DS3 == TESTING Tested with local build. DS3 init is not working.
== DETAILS - fix the bitshift math - read the right bytes out of the ds3 data packet - remove verbose logging in critical path - stop caring about errors in the hid read loop -- seems to just be benign "device not ready" -- or at least, that's what I'm assuming given that the read eventually succeeds. == TESTING Played Mario 3 with the DS3 with no issues.
Fortunately, the gcc port implements the builtins and, from basic testing, they seem to work. This is only really useful on Wii U--other platforms have more robust atomic operations, or aren't using gcc to build.
== DETAILS TIL that it's bad to call synchronization code from callbacks. To avoid that, I made the following changes: - Implemented an atomic swap (see previous commit) to avoid explicit locking when working with the event list - ensure locks are only acquired in either the main thread or the I/O polling thread - use an explicit polling loop; we still use async reads, but the read doesn't immediately re-invoke itself. - remove the sleep in the polling thread. - remove unnecessary locking in the thread cleanup call--verified that the list can't be modified while it is being executed. == TESTING I tested locally, and was able to disconnect/reconnect USB devices several times without the worker thread getting deadlocked.
retro_bits_t turned into input_bits_t and there were parts of my code that needed to update. == TESTING No idea if upstream changes broke anything, but it compiles cleanly now.
== DETAILS I think this will fix the problem with duplicate pads--pads weren't properly de-initializing and registering as disconnected. When a pad is disconnected, the slot should properly release now.
== DETAILS Hooray for conditional compile directives. Moving things around broke things in unexpected ways on non-WiiU builds. Well, not *completely* unexpected. But still. Changes: - Move some typedefs around to avoid circular include dependencies - Include the file where the HID driver definition got moved to == TESTING - verified build for Wii U still runs successfully - did a local build without any errors (some weird warnings, but since they happen in code I didn't change, I'm assuming they're pre-existing?)
Contributor
|
Haven't been able to test (no compatible controllers) but the code looks great; it's an OK from me. Will have to wait on some users who can give it a go. |
=== DETAILS So, the GCA has 2 USB connections; one is the data connection, and the second is used to drive rumble. Due to a driver bug, if the second cable wasn't attached, the pads wouldn't get detected. I fixed that bug.
This should fix the issue where R/L buttons didn't register when doing input detection. This also brings the GC pad in line with the rest of the gamepads in input_autodetect_builtin.c. Also fixed a really stupid bug that was part of why analog inputs aren't being read. Analog still isn't working, mind, but it's a lot closer to working now that it's actually getting down into the pad driver level!
aliaspider
reviewed
Apr 21, 2018
| if(min_size > 32 && min_size % 32 != 0) | ||
| cacheblocks++; | ||
|
|
||
| *actual_size = 32 * cacheblocks; |
Contributor
There was a problem hiding this comment.
*actual_size = (min_size + 0x1F) & ~0x1F; will align to 32bytes.
actual alignment requirement is 64 though, so use 0x3F instead.
== DETAILS The Wii U GC adapter doesn't seem to like doing async reads if it is connected via a USB hub. It seems to be device-specific, though, because my DS3 works just fine through the same hub. I tried creating a fallback to synchronous reads, but it resulted in a hard lock of the system. So, for the time being, it's going to be a known limitation. Might be solved by using a powered USB hub. Learned that the cache alignment is 64, not 32, so the alignment math has been updated. Thanks, @aliaspider for that info.
== DETAILS So, it turns out that there *is* a autoconfig disconnect handler. Took digging through tasks/task_autodetect.c to find it! So, I added a call to the handler when the pad gets disconnected. This seems to solve the problem of the pad not disappearing from the menu. (At the very least, the user's pad index reverts to "none" which is still an improvement) == TESTING Tested manually, made sure it didn't crash or leak slots.
== DETAILS After a little trial and error, I got analog input working for the Wii U GC adapter. DS3 might work, but it's untested.
== DETAILS - DS3 analog wasn't working mainly because I forgot to actually declare the axes in input/input_autoconfig.c when declaring the pad. Whoops. - I also moved the axis decoding logic to a more central place, because it clearly is not Wii U specific. - Removed some dead commented-out code == TESTING Can use analog inputs on both GCA and DS3. Tested in Mario 3 on Nestopia core. Haven't tested with any actual analog games, but I did confirm via logging that the correct ranges are produced.
Contributor
|
Works fine for me on OSX - any more additional testing needed? |
Contributor
Author
|
I need to get new builds in the hands of folks at GBATemp so I can be sure I've addressed the problems they reported in the first build. So, not ready to merge yet (but a lot closer!) |
== DETAILS This is the wiimote version of the same bug I previously fixed in the HID driver, where disconnected pads didn't actually invoke the unregister task. This has an extra wrinkle, in that we *also* need to invoke the unregister task when the wiimote device changes (e.g. user plugs in a nunchuk or classic controller). Now, there's still the problem of the "disconnect" detection being broken; so a consequence of this commit is OSD spam. However, the actual wiimote input is processed successfully and there's no noticeable issues in the pad handling. == TESTING Using Mario 3, I played a level in which I started as bare wiimote, then hot-plugged the nunchuk, and the input switched automatically. At the end of the level, I hot-unplugged the nunchuk and it automatically reverted to horizontal layout; and the pad remained 100% responsive the entire time.
== DETAILS So, the KPadRead function will sometimes return 0, but this doesn't mean the wiimote is actually disconnected. It's usually something transient, like the BT chip has nothing to send or whatever. I don't know. So, I added a buffer so that it won't disconnect the pad without 5 consecutive 0-reads. This is a temporary hack; a proper solution will use the Wii U's callback mechanisms to do wiimote detection. But that's a separate project. This at least prevents OSD spam. == TESTING Tested locally. Verified that connecting/disconnecting nunchuk during play still works properly.
Contributor
|
OK, @gblues told me this is fine for merging now so nightly users can get to grips with it, so here goes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TESTING NEEDED: I've tested the digital inputs, but I don't have any cores that use analog inputs handy to test those.
This is a pretty big change to how HID is handled in RetroArch.
Some background:
RA was designed around the assumption that gamepads and HID devices enjoyed a 1:1 relationship. However, this is not the case. Past HID solutions had implemented special-case handling for specific devices, but I wanted something more generic.
So I made it.
Now, in addition to the joypad driver, there's a HID device driver. The HID device driver can then manage its pad or pads.
These interfaces are implemented as structs of function-pointers, a pseudo-OO approach.
The HID device drivers are designed to be as platform-independent as possible. There's still a small amount of special-casing (like preambles in the activation packets needed for specific platforms), but it's pretty manageable.
The poster child use case for this approach is the GameCube adapter, which exposes 4 controllers through a single HID device. And that's why the Wii U is the lead platform for this new approach.
I was also able to provide some quality-of-life enhancements for WiiU, by making HID pads use the first available port instead of being hard-coded to 5+. Pads will now be activated in the order they connect. The GamePad will still be pad 0, although you can reassign via the menus.
Some of the code is going to look similar to what's already in the
input/connect/hierarchy. There's some duplication; but since the new drivers are only used on Wii U, it's premature to delete those drivers. Eventually we'll want to port them over to the new style, although it'd be helpful to actually have one of them to test with.Reviewers
@aliaspider @QuarkTheAwesome
[If possible @mention all the people that should review your pull request]