-
Notifications
You must be signed in to change notification settings - Fork 391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MacOS Hotplug: connection-callback implementation for MacOS #653
MacOS Hotplug: connection-callback implementation for MacOS #653
Conversation
github action builds failed for macOS (automake and cmake) |
I managed to set up a build environment two days ago and it showed me parts that I had not considered, currently trying to understand their API. Welp, this was just a mock-up, after all :) |
convertin into draft for now |
Found an example implementation that should aid in finishing this code |
libusb macOS supports hotplug. Maybe that can be a reference as well. |
It took me a while to understand that the approach I took was in a completely wrong direction (thanks, bad documentation!), and the example I linked - as well as the example in libusb - are not suitable for hidapi. The reason is that hidapi relies on IOHIDDeviceRef's to get info on usage pages - and we can't get said info from io_object_t that we get from the callbacks in all of those examples. The right approach would be to:
I will, of course, try to make a fresh mock-up within the next few days. I do have a functional build environment now, so I will be able to check if it builds - but I will not be able to check if it reacts to the events as expected. UPD: it would have been nice if we at least had any kind of documentation regarding the state of structures provided by macos in the disconnect callback :( |
It took a while, but the implementation is finished and ready for a review. It might require some adjustment regarding the RunLoop for the hotplug event processing (I couldn't find any reliable information to ensure it would work as expected with the current state of the implementation). |
Checked the failed build, there were warnings treated as errors. Fixed them. |
Ran some simple tests in a VM, all the initialization seems to work, but the events are not fired. Currently investigating how to fix runloops, should be done with that tomorrow. Converting to draft until I'm done with it. |
@Youw
|
Great. I see the same thing under my Mac Mini M1 2020 (latest macOS 14.3.1) -- no events fired. So it is good your VM behaves the same. |
21e1101
to
9f0e41d
Compare
I just rebased this branch onto the fresh I added some code for the background event thread, going to test that before doing anything else with the PR. |
@mcuee Ran tests on the new fixed implementation. IORegistryEntryGetRegistryEntryID returns 268435459, which isn't even a valid return code for a kern_return_t. So far, it's all good news. |
It worked. I'm gonna need confirmation that this is an allowed method to store io_service_t (I made the hidtest output AFTER enumeration
|
@mcuee may I ask to re-test on a real Mac with the latest changes? |
It seems to work fine under my Mac Mini M1 2020 (macOS 14.3.1).
|
Testing with Microsoft Bluetooth Mouse is also good.
|
Amazing! Thank you! Now it just needs to pass the codestyle review, I guess |
I am more on the testing side. @Youw is the current active developer and he will be reviewing the PRs. |
@Youw I apologize for the PR being a bit messy. The target branch needs a rebase onto the fresh master. I had to do it for this branch to avoid other issues, so the diff includes all the commits that didn't yet make their way into |
That I can do (if no conflicts) |
I'm afraid there are a few, but they aren't that bad |
it looked trivial, I believe I didn't break anything but need to check |
d324ab7
to
4d851df
Compare
All done on my side as well 👍 |
No issue with latest commit.
|
hidtest/test.c
Outdated
// return 1; | ||
|
||
return 0; | ||
if (event & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
maybe a separate PR with the fix into master?
if will definitely get there faster as a separate change, than the feature branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need - it was done the same way in the Windows Mutex PR (was asked to fix indentation and also had troubles with no fflush()
), so this change no longer even pops up in the diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I only meant replacing spaces with tabs inside of this function
everything else, obviously, is feature/branch-specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal as with #645 - getting it merged into a feature branch now.
This is a
MOCK-UP of afinalized but untested hotplug implementation for Mac OS. It has the entire structure already set up (callback list management, device cache, mutex - everything copied straight from Linux version), but may still have some issues with RunLoop and event processing, which I will not be able to resolve myself because of a total lack of any meaningful documentation.