macOS controller duplication bugfix #12745
Conversation
It seems the intention of uniqueId added in 9008684 was to reconnect controllers to their previous port based on the "uniqueId" of the device reported by the operating system. However, as noted in the comments, the uniqueId changes every time a device is reconnected and so this code can never accomplish its goal.
In this commit, I have disabled the pad-matching feature at ~L:218 in joypad_connection.c You can re-enable the functionality from there to experience the current issues. When I plug in a PS4 controller, the iohidmanager_hid_adapter is being overwritten by what I believe is a pad_controller struct within hidpad_ps4_init() The *data being passed to hidpad_ps4_init() is a struct iohidmanager_hid_adapter, but it's being de-referenced as a pad_connection I do not know what this function is intended to be doing - but it's certainly not this. This is likely an issue across most/all of the other connect_xxx.c files. I suspect this code was broken for a very long time, but went unnoticed because the thing it broke was already broken.
What seems to have been happening is that the iohidmanager_hid_adaptor was getting cast as a pad_connection and then overwritten, resulting in corrupt data and bad behavior. I solved this by breaking these two structures apart. I added a `void* connection` to joypad_connection for the pad_connection to get stored in, and then changed the instances of joyconn->data to joyconn->connection so that the iohidmanager_hid_adaptor no longer gets overwritten. within struct joypad_connection: void* connection = pad_connect void* data = iohidmanager_hid_adapter These two structures of data should no longer collide.
Turns out uniqueId DID do something. It prevented a controller from being added a second time - once on launch, and again when the controller-connection-callback is started Can we accomplish the same thing with the kIOHIDLocationIDKey? I'd like to avoid using kIOHIDUniqueIDKey because it's incompatible with powerPC.
I don't know C. I'm changing some of this stuff back to how it was before, because my changes resulted in warnings, and I'm not experienced enough to fix them.
Might need this for memory reasons - I'm not sure
|
It was noted to me that there's quite a few whitespace inconsistencies. That wasn't something I could see while working on this in xcode, but will need to get fixed. There's also still a handful of debug |
ComradeEcho
changed the title
|
This pull request introduces 2 alerts when merging 6b89aee into a2fb084 - view on LGTM.com new alerts:
|
input/drivers_hid/iohidmanager_hid.c
Outdated
| { | ||
| struct iohidmanager_hid_adapter *adapter = NULL; | ||
| //loop though the controller ports and find the device with a matching IOHINDeviceRef | ||
| for (int i=0; i<MAX_USERS; i++) |
There was a problem hiding this comment.
Can you change this? Don't use internal declaration for loops. Instead, define 'int i' outside of the for loop, at the start of a code function or block. This makes the code C89-compliant.
input/drivers_hid/iohidmanager_hid.c
Outdated
| static void iohidmanager_hid_device_remove(IOHIDDeviceRef device, iohidmanager_hid_t* hid) | ||
| { | ||
| struct iohidmanager_hid_adapter *adapter = NULL; | ||
| //loop though the controller ports and find the device with a matching IOHINDeviceRef |
There was a problem hiding this comment.
Can you change these and other comments to C-based comments?
input/drivers_hid/iohidmanager_hid.c
Outdated
| } | ||
| if (!adapter) | ||
| { | ||
| printf("Error removing device %p from HID %p\n",device, hid); |
There was a problem hiding this comment.
Are all these printfs necessary? We could replace them with RARCH_LOG/RARCH_ERR/RARCH_WARN if they are truly essential. Do note that the logging code is not thread-safe so if this function happens on a thread, perhaps not logging at all would be the safest choice. But I'll leave this determination up to you to decide.
There was a problem hiding this comment.
The printfs aren't necessary, but I was reluctant to remove them yet as they may still be useful as things get tested. The list_controllers() function can also be axed before merge - that's just something I added to aid in testing.
However, I think some additional logs would be nice. As of right now the only log that's output is when a controller is connected. I think we at least have logs when a controller is disconnected as well as a log detailing whether the device was matched by the autoconfigurator.
I'm not sure where any of this falls in regards to threading. My untrained eye sees that this all seems to be going on within thread 1 - but I have no idea tbh. I'd love to learn about that :)
There was a problem hiding this comment.
I removed the printfs, and added a couple logs, both info and debug
declare i outside of the for loop, and use C style comments. Style changes
…g' into bugfix/controller-duplication-bug
|
This pull request introduces 2 alerts when merging cb147f7 into 64244cd - view on LGTM.com new alerts:
|
|
I am currently getting a
This is the point where I have no idea what the actual issue is, or how to solve it.
|
…ger_hid_device_add_device
for various CoreFoundation functions with 'Copy' or 'Create' in the title
Description
Hey y'all.
Just to preface, I have ~zero experience with github, and this is literally my first time ever touching C or anything close to this low-level. Please scrutinize the heck out of this, because I don't know what I'm doing. Especially pointer/memory/free()/deinit stuff.
This PR aims to take care of a couple of things having to do with the macOS HID Driver.
The
IOHIDDeviceRegisterRemovalCallbacksimply was not working as I believe it should. I don't know if it was implimented improperly, or if there's a bug in macOS related to it. It was being set, but it wouldn't trigger reliably. I replacedIOHIDDeviceRegisterRemovalCallbackwithIOHIDManagerRegisterDeviceMatchingCallbackwhich seemed to have perfect detection of device removal on my system.Upon fixing the first issue, I discovered that devices matched by the autoconfigurator still were not disconnecting properly. This was caused an
struct iohidmanager_hid_adaptorbeing improperly cast as astruct pad_connection, and having its data overwritten/corrupted.I solved this issue by adding a
void* connectiontojoypad_connectionand assigning the offendingstruct pad_connectionthere instead of it going invoid* data, thus removing the data collision.I removed all references to
uniqueIdbecause I assumed it accomplished nothing. I later realized what it was accomplishing, but decided to try to implement the same goal withkIOHIDLocationIDKeyinstead, becausekIOHIDUniqueIDKeyis unsupported by powerPCWhat it was accomplishing was: controllers are detected both when the application is launched, and by the controller connection callback. uniqueId was being used to make sure the controllers are not duplicated by getting added by both of those.
I made a two (unrelated) changes to how debug logs are shows in the logs. I don't know how to exclude those from my PR, but it fixes what I assume is a bug. (debug-level logs would output as info-level logs) :(Notes
I have done testing solely on M1 macOS 11.
I have done testing with these controllers:
I have access to a few more untested controllers including:
I have absolutely no idea if this affects windows/linux/android. It seems like a very Apple-heavy driver, and I don't know if the HID input driver is even functional on other platforms.
Related Issues
For sure:
#12561
#11294
#10546
Possibly:
#3414
#2808
#4215
#3415
#6731
#4793
Reviewers
@gblues @jdgleaver @hunterk @markwkidd
@twinaphex With the changes made surrounding uniqueId - I think this needs a good test on powerPC