dcuddeback / libusb-rs

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

Holding an InterfaceHandle in a struct #3

Closed awelkie closed 8 years ago

awelkie commented 8 years ago

I'd like to have a struct that contains an InterfaceHandle objects so that I can use the bulk_transfer method. I'm having trouble doing this because an InterfaceHandle object contains a reference to a DeviceHandle object, which contains a reference to a Context object. I'd be fine having the Context object be global, but I'd like to have the DeviceHandle and InterfaceHandle objects held in the same struct. But Rust has trouble with structs that contain references to objects within itself.

So what's the best approach for having a struct contain an InterfaceHandle? I can see a few options:

Here's an example of what I'm trying to do:

struct MyDevice<'a> {
    dev_handle: libusb::DeviceHandle<'a>,
    usb_interface: libusb::InterfaceHandle<'a>,
}

impl<'a> MyDevice<'a> {
    fn open(usb_ctx: &'a mut libusb::Context) -> Result<Self, libusb::UsbError> {
        let mut devices = try!(usb_ctx.devices());
        let mut dev_ref = devices.iter().next().unwrap();
        let mut dev_handle = try!(dev_ref.open());
        let interface = try!(dev_handle.claim_interface(0));
        Ok(MyDevice { dev_handle: dev_handle, usb_interface: interface })
    }
}

This fails to compile, saying that dev_handle does not live long enough.

dcuddeback commented 8 years ago

@awelkie Thanks for opening this issue. I wasn't aware that Rust's limitations would prevent storing a DeviceHandle and an InterfaceHandle in the same struct at the time that I wrote this library. That's a little disappointing. I thought it would be possible to work around it by having a "managing" struct that tracks claimed interfaces, like this:

struct UsbDevice<'a> {
  device:libusb::DeviceHandle<'a>,
  interfaces: HashMap<u8,libusb::InterfaceHandle<'a>>,
}

impl<'a> UsbDevice<'a> {
  fn interface(&mut self, iface: u8) -> ::libusb::UsbResult<&mut libusb::InterfaceHandle> {
    // hand out references from `self.interfaces`
  }
}

but I run into lifetime issues every time I try to get something working, even when working with Rc, RefCell, and friends.

I think this (among other issues) needs to be overhauled for v0.2. A library shouldn't be this cumbersome to use. The only reason for each struct to hold references to the other structs was to have the Rust compiler enforce order of releasing resources (libusb_release_interface() before libusb_close(), for example). I think that could be solved instead without lifetimes by using libusb's built-in reference counting (libusb_ref_device(), libusb_unref_device(), etc).

awelkie commented 8 years ago

Maybe the libusb_ref_device and libusb_unref_device functions could be used to implement ToOwned?

awelkie commented 8 years ago

Actually, I think a simpler solution might be to use a 'PhantomData' objects in the structs instead of unused references to the parent struct. For example, the _context field in the DeviceList struct could be a PhantomData objects with a lifetime tied to the parent Context.

This way, objects have the right lifetimes but don't have references that keep them from being in the same struct.

dcuddeback commented 8 years ago

@awelkie I had never seen PhantomData before. Thanks for pointing it out. That's exactly what I was looking for when I wrote this library originally. It would have been a much cleaner way to tell the compiler that the C struct I'm wrapping holds a reference to another struct internally.

Since the libusb types are (internally) equivalent to Arc<Mutex<T>>, I should be able to get rid of some of the lifetimes. I don't know how quick of a turn-around time I can promise, though. I'm going to take this as an opportunity to make all the breaking changes that I've been wanting to make. This was one of my first Rust projects, so there are a few of them. :) I'll start working on it this week.

dcuddeback commented 8 years ago

@awelkie When I dug into it, I found that libusb_device is the only struct that's internally reference counted, so I took a different approach. I moved the InterfaceHandle methods into DeviceHandle and made sure that the Drop impl for DeviceHandle releases all claimed interfaces. I think this makes sense, because the interrupt_transfer() and bulk_transfer() methods on InterfaceHandle don't actually use the interface's number in the libusb function.

You can try it out now on the v0.2-dev branch. Please let me know if you have any more issues. I'll be working the rest of the week on making further changes for v0.2 before I merge that branch to master and release to crates.io.

As an aside, I also replaced all of the references with PhantomData. Thanks for the suggestion.

awelkie commented 8 years ago

Awesome, thanks for making these changes. I'm traveling now, so I won't be able to check out the changes until next week. But I will give it a shot once I'm back. On Oct 13, 2015 4:34 PM, "David Cuddeback" notifications@github.com wrote:

@awelkie https://github.com/awelkie When I dug into it, I found that libusb_device is the only struct that's internally reference counted, so I took a different approach. I moved the InterfaceHandle methods into DeviceHandle and made sure that the Drop impl for DeviceHandle releases all claimed interfaces. I think this makes sense, because the interrupt_transfer() and bulk_transfer() methods on InterfaceHandle don't actually use the interface's number in the libusb function.

You can try it out now on the v0.2-dev branch. Please let me know if you have any more issues. I'll be working the rest of the week on making further changes for v0.2 before I merge that branch to master and release to crates.io.

As an aside, I also replaced all of the references with PhantomData. Thanks for the suggestion.

— Reply to this email directly or view it on GitHub https://github.com/dcuddeback/libusb-rs/issues/3#issuecomment-147752506.

awelkie commented 8 years ago

So this fixed the issue with holding an 'InterfaceHandle' and a 'DeviceHandle' in the same struct. But I'm still running into the same issue with holding a 'Context' and 'DeviceHandle' in the same struct.

What do you think about having the Context::devices' method consume theContextobject, and then all structs that currently hold aPhantomData<&Context>would hold anArcinstead? This way users don't need to worry about holding aContext` object. It comes with the cost of reference counting, but libusb already uses reference counting internally so I think that's fine.

dcuddeback commented 8 years ago

What's your use case for holding a Context and DeviceHandle in the same struct? I think most applications would open a Context at the top of main() and keep it open for the duration of the program, using references everywhere it's needed, but I could be overlooking some limitation.

awelkie commented 8 years ago

The issue I was having was that I needed to initialize the context and device within a C callback. So I was hoping to bundle the context and device together and return it from the callback.

But I solved the issue by using initializing the context in thread local storage. So I'm all good as far as this issue is concerned. Thanks for the changes!

meh commented 8 years ago

I'm having some trouble with this myself, I'm working on a library to deal with Steam Controller and I'd like to store the Context and DeviceHandle in the same struct, or at the very least have a Manager that owns the Context and is able to open multiple controllers at the same time.

This is a problem because Context::devices() takes the Context as &mut, so I can't have more than one controller open at the same time.

Does Context::devices() really need to take the context mutably? Or am I doing something horribly wrong?

kevinmehall commented 8 years ago

See #5, where I fixed that.

yuvadm commented 5 years ago

I'm attempting something similar to what @meh is describing. I'd like to create a library that uses libusb such that the user should have no notion of libusb or its Context. However, doing this naively with a MyLib struct that holds both the Context and a specific DeviceHandle is essentially impossible, since the context lifetime will never outlive the device.

The only real workaround is as @dcuddeback mentioned and that's to create the Context in the user application's main() but then the user is dealing with what I would like to be internally handled by my library.

Is there any better way to do this?

oberien commented 5 years ago

What I did in my program was to move the Context into a Box and leak that box, giving me a &'static Context. I put that into a newtype struct, which implements a Drop method rebuilding the Box and dropping it correctly.

stevenroose commented 5 years ago

What I did in my program was to move the Context into a Box and leak that box, giving me a &'static Context. I put that into a newtype struct, which implements a Drop method rebuilding the Box and dropping it correctly.

@oberien Could you share some code snippet of how you did that?

oberien commented 5 years ago

This is from a project I did some years ago:

https://github.com/oberien/logitech-g910-rs/blob/638f9635e81620bd66f4e04073b3f87eaf9657bc/src/utils.rs#L20-L79

stevenroose commented 5 years ago

@oberien That was very helpful, thanks! A bit sad that that's how we are supposed to use this crate, there should really be an easier way.

@dcuddeback, try build an example (in ./examples) where you are a library dev and you want to return a "client" struct to interact with a device. Meaning that the handle and context are not setup in main, but in the constructor method of the library, like SomeDeviceClient::new(). It will help you to understand the use case that @oberien had to fix with the unsafe blocks.

yuvadm commented 3 years ago

https://github.com/a1ien/rusb does a pretty good job of solving this issue