feat: Native macOS backend #131

Tiwalun · 2023-07-23T14:16:30Z

This is a port of the hidapi code for macOS to pure Rust, with some small modifications to make it more thread safe.

I've tested it with probe-rs, and it seems to work fine so far.

The error handling could be improved, but that might be better to do in coordination with the other native backends.

Remaining tasks

Implement open options (exclusive / non-exclusive)
Handle blocking / non-blocking reads
Check thread-safety

ruabmbua · 2023-08-14T10:49:50Z

Hi, thank you for your work! I will hopefully have some time to review it in the coming week.

micolous

Having written something similar myself, this design is really strange. Rust's standard library has functionality which could make the implementation significantly simpler.

It looks like this attempts to decouple IOHIDDevices from the IOHIDManager which created it, in order to allow "opening by path" (which is nonsensical on macOS). It would be better to attach that to the HidApiBackend.

While this may be more of a comment on the state of hidapi's interface, the device matching rules available are severely lacking (only single VID+PID), which is a problem in modern versions of macOS, which put free-for-all access behind an "input monitoring" permission. There's also no way to monitor device insertion/removal events, so you're forced to continually prod HidApiBackend::enumerate().

micolous · 2023-09-13T03:55:56Z

src/macos_native.rs

+//
+// It is recommended that changing the configuration a run loop should be done from the thread that owns the run loop whenever possible.
+// In the use here, we only use the wrapper to wake up the run loop, which is safe.
+unsafe impl Send for WrappedCFRunLoop {}


This should no longer be needed as of servo/core-foundation-rs#610

Correction: still needed, the PR made it into v0.8.5, but it was yanked due to servo/core-foundation-rs#619.

micolous · 2023-09-13T03:56:45Z

src/macos_native.rs

+    fn enumerate(vendor_id: Option<u16>, product_id: Option<u16>) -> HidResult<Vec<DeviceInfo>> {
+        let manager = IOHIDManager::create();
+
+        if vendor_id.is_some() || product_id.is_some() {


Allow matching by Usage/Usage Page, and multiple VIDs/PIDs.

Without any matching rules, macOS demands "Input Monitoring" permission, which is quite bad.

This would require changes to the other backends as well, or additional Mac-specific functions.

micolous · 2023-09-13T04:01:12Z

src/macos_native.rs

+
+    // Condition variable linked to input_reports
+    condition: std::sync::Condvar,
+    input_reports: Mutex<VecDeque<Vec<u8>>>,


Use an mpsc::channel rather than a VecDeque. That way you get queue size limits for free, you don't need to wrap everything in a Mutex.

You also get helpers like recv_timeout...

Right now, the oldest received input report is dropped when the limit is reached. I don't think this is possible with the mpsc::sync_channel, the size limit there could only be used stop sending new input reports, but it doesn't seem possible to remove elements from the channel.

Using a channel would be nice, but it would change the behaviour compared to the C hidapi library.

micolous · 2023-09-13T04:04:44Z

src/macos_native.rs

+    {
+        let device = shared_state.device.lock().unwrap();
+
+        // unregister the input report callback


This isn't necessary - IOHIDDevice automatically unregisters these on close: https://github.com/apple-oss-distributions/IOKitUser/blob/b0b3f822b7507c265aa8a1e37c3100c03ca82039/hid.subproj/IOHIDDevice.c#L483-L486

micolous · 2023-09-13T04:24:59Z

src/macos_native.rs

+        && (!shared_state.disconnected.load(Ordering::Relaxed))
+    {
+        // TODO: Verify timeout value
+        let code = unsafe { CFRunLoopRunInMode(run_loop_mode.as_concrete_TypeRef(), 1000.0, 0) };


I'm struggling to understand the design here, but why not just CFRunLoopRun()? It does basically the same thing as your loop: https://github.com/apple-oss-distributions/CF/blob/dc54c6bb1c1e5e0b9486c1d26dd5bef110b20bf3/CFRunLoop.c#L2676-L2682

On disconnect/Drop, you can issue a CFRunLoopStop() from the main thread – HidDevice::drop already has a reference to the device's CFRunLoop.

I don't think it matters in this loop whether the device was closed by the application (terminating the run loop) or there was an explicit disconnect (terminating the run loop); a disconnected device is a disconnected device. :)

mpsc::channel also saves you here too, because Droping the Sender will unblock any pending Receiver::recv calls on another thread, and make future calls fail.

If you've only associated a single IOHIDDevice with the CFRunLoop, the run loop will stop on disconnect anyway. hid_removal_callback appears to only set a flag that it was disconnected, but this could also be detected by setting your own (not-mutex-guarded) flag on close().

micolous · 2023-09-13T05:07:34Z

src/macos_native/ffi.rs

+
+    pub fn copy_devices(&self) -> Vec<IOHIDDevice> {
+        let set: CFSet<IOHIDDeviceRef> = unsafe {
+            let set = IOHIDManagerCopyDevices(self.as_concrete_TypeRef());


This isn't documented, but IOHIDManagerCopyDevices can return nullptr if there are no matching devices.

micolous · 2023-09-13T05:16:40Z

src/macos_native.rs

+            .get_i32_property("MaxInputReportSize")
+            .unwrap_or_default();
+
+        let run_loop_mode = format!("HIDAPI_{:p}", device.as_concrete_TypeRef());


This isn't right, but the IOHIDDeviceUnscheduleFromRunLoop runLoopMode parameter is unused anyway.

Could you explain what you mean by not right? Using custom run modes seems to be possible according to the documentation at https://developer.apple.com/documentation/corefoundation/cfrunloop?language=objc.

Or do you mean that a custom run mode shouldn't be used?

Tiwalun · 2023-09-13T19:55:48Z

Thanks for looking at this! It's good to get some feedback from somebody who knows these APIs.

Having written something similar myself, this design is really strange. Rust's standard library has functionality which could make the implementation significantly simpler.

As mentioned in the description, this is more or less a direct port of the hidapi C library, with only minor changes to work around the problem mentioned in #127.

I'm not sure what the requirements regarding compatibilty with the existing implementation are. If this is only an optional backend, and the hidapi library is kept as an alternative, then it should probably be compatible, which limits the amount of changes that can be done. On the other hand, if compatibility is not a concern, then some of the improvements like using a mpsc::channel could be done more easily.

@ruabmbua What are your thoughts regarding compatibility with the C library?

ruabmbua · 2023-09-15T11:32:34Z

Thanks for the review and the work done! I already started with it myself, but could not motivate myself to start learning macos API`s ^^.

Regarding compatibility to the current public API of the library: I would keep it as is for now (with non breaking changes allowed). This means I agree that the new optional pure rust macos backend should try to mimic the old one as close as it can.

I might revisit the public API in the future (v3.0.0), and do mostly some fixes that make it more idomatic rust.
I am not sure about async as of yet, I struggle to see the large benefit of making the library true async. Its not likely,
that an application will need to handle a huge amount of hid devices in comparison to e.g. network connections.
You can always spawn dedicated worker threads for devices, and integrate them with e.g. async channels + an API wrapper into e.g. tokio. Maybe I am missing a use case, where a true async implementation for hidapi would help.

Tiwalun added 9 commits July 15, 2023 17:25

WIP: Working list

a169a9a

WIP: Working implementation

dc1045c

WIP: Cleanup

c53f9e0

More refactoring

083eeb4

Cleanup

6a73a3c

Add filtering for enumeration

377c560

Merge branch 'master' into feat/mac-native

92835c2

Add build for native macos backend

4106f70

Use cell for blocking property

c75432c

micolous reviewed Sep 13, 2023

View reviewed changes

micolous mentioned this pull request Sep 13, 2023

Create pure rust macos backend (using IOHidManager) #30

Open

5 tasks

micolous reviewed Sep 13, 2023

View reviewed changes

Tiwalun added 2 commits September 13, 2023 20:21

Merge branch 'master' into feat/mac-native

b2371e1

Handle null ptr when copying devices

fc27528

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Native macOS backend #131

feat: Native macOS backend #131

Tiwalun commented Jul 23, 2023

ruabmbua commented Aug 14, 2023

micolous left a comment

micolous Sep 13, 2023

micolous Sep 13, 2023

micolous Sep 13, 2023

Tiwalun Sep 13, 2023

micolous Sep 13, 2023

Tiwalun Sep 13, 2023

micolous Sep 13, 2023

micolous Sep 13, 2023

micolous Sep 13, 2023

micolous Sep 13, 2023

Tiwalun Sep 13, 2023

Tiwalun commented Sep 13, 2023

ruabmbua commented Sep 15, 2023

feat: Native macOS backend #131

Are you sure you want to change the base?

feat: Native macOS backend #131

Conversation

Tiwalun commented Jul 23, 2023

Remaining tasks

ruabmbua commented Aug 14, 2023

micolous left a 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

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

Tiwalun commented Sep 13, 2023

ruabmbua commented Sep 15, 2023