dcuddeback / libusb-rs

A safe Rust wrapper for libusb.
MIT License
199 stars 64 forks source link

Tweaks to make the API more ergonomic and flexible #5

Closed kevinmehall closed 7 years ago

goertzenator commented 8 years ago

This worked out really well for me. I was able to use find() on the device list iterator, open my device, and conduct some control point io. All with nice looking immutable variable bindings!

dcuddeback commented 8 years ago

@kevinmehall Thanks for doing this. Is this related to the async API that you're working on?

I just want to give you a head's up that it's going to take me a while to check the immutability changes. I want to spend some time reading the source code for the underlying C library to verify that the immutable references and Sync traits are safe. My time is a bit limited over the next few months, but I am looking at this PR in my free time.

I'm not entirely sold on the idea of adding as_raw methods to the wrapper structures. My personal preference is to address the library's shortcomings so that others don't have to resort to the C API. Implementing async will go a long way toward closing the gap in APIs. Are there other reasons for adding as_raw that I'm not thinking of?

kevinmehall commented 8 years ago

My async branch depends on this. If opening a device holds an &mut borrow of the Context, the Context is not available for the AsyncGroup, which needs to call libusb_wait_events_completed on it. This part was ready first, and is useful separately.

Libusb is documented to be thread safe, which corresponds to shared references with Sync bounds in Rust. That's not to say that auditing libusb for thread safety won't turn up any interesting bugs, of course.

I actually implemented as_raw first in order to use the libusb async API externally before wrapping that up in the abstractions on the async branch. Another case that comes to mind is libusb_get_pollfds and related functions needed to use async with mio. That may make sense as a mio cargo feature on this crate, but if it were a separate crate, it would need raw access to the libusb_context.

The main reason I see not to add as_raw is that it would make it hard to replace libusb with OS-specific Rust backends at a later date and maintain API compatibility, but that's likely a different library and different API anyway.

meh commented 8 years ago

as_raw could also be useful to interact with other C libraries that use libusb in a way or another.

dcuddeback commented 8 years ago

@kevinmehall I just wanted to let you know that this is still on my radar, but I haven't had time to look at this yet. I hope to have time this weekend. I know libusb is documented to be thread-safe, but I've seen other libraries claim that and not live up to it. Sometimes only a subset of types in a library are thread-safe. Libusb seems to be one of the better-written C libraries from what I've seen, so hopefully it's okay.

Regarding using libusb_get_pollfds to integrate with mio from a separate crate, I don't see how that would require direct access to a libusb_context*. I would expect that libusb_get_pollfds and other methods related to polling (http://libusb.sourceforge.net/api-1.0/group__poll.html) would be exposed by this crate, and the mio integration would use the wrapped methods to implement mio support. I've never integrated with mio before, so maybe I'm overlooking something. However, @meh brings up a good point about interoperating with other libraries that use libusb. I think as_ptr() is a more idiomatic name for this method (looking at CString for inspiration): https://doc.rust-lang.org/std/ffi/struct.CString.html#method.as_ptr. Another option is to implement into_raw() if the other library takes ownership: https://doc.rust-lang.org/std/ffi/struct.CString.html#method.into_raw.

jasonmartens commented 8 years ago

I would love to see this get merged. I ran into this problem: http://stackoverflow.com/questions/37620491/how-to-find-mutable-items-in-rust-iterator

I think removing the mut from many of these methods would make the problem go away for most usage.

jrozner commented 7 years ago

Are there any updates on this? It would be really nice to get these changes in

kevinmehall commented 7 years ago

I've removed the as_raw part for now, and I've split out the from_libusb changes into #10 after discovering that the APIs in question were being actively misused by the test suite. It would be great to get this merged, as the current API is unusable for many applications.

dcuddeback commented 7 years ago

After reading through the source code for the relevant libusb functions, I agree this looks thread-safe. I'm comfortable merging this now, and then I'll write some test programs to try it out before releases to crates.io. Thank you all for your patience, and thank you @kevinmehall for putting up this PR.