Skip to content
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

Holding an InterfaceHandle in a struct #3

Closed
awelkie opened this issue Oct 7, 2015 · 17 comments
Closed

Holding an InterfaceHandle in a struct #3

awelkie opened this issue Oct 7, 2015 · 17 comments

Comments

@awelkie
Copy link

awelkie commented Oct 7, 2015

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:

  • Use Rc and RefCell as is suggested by reem in this post
  • Carry around the DeviceHandle, and then use claim_interface to get the InterfaceHandle every time I need it.
  • Use the libusb-sys crate directly, and just carry around the pointers.

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
Copy link
Owner

@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
Copy link
Author

awelkie commented Oct 7, 2015

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

@awelkie
Copy link
Author

awelkie commented Oct 8, 2015

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
Copy link
Owner

@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
Copy link
Owner

@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
Copy link
Author

awelkie commented Oct 13, 2015

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
#3 (comment).

@awelkie
Copy link
Author

awelkie commented Oct 19, 2015

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
Copy link
Owner

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
Copy link
Author

awelkie commented Oct 21, 2015

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
Copy link

meh commented Mar 24, 2016

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
Copy link
Contributor

See #5, where I fixed that.

yuvadm added a commit to yuvadm/rustl-sdr that referenced this issue Jul 26, 2018
This is due to a limitation where we can't hold a struct that has both
the context and a device handle (that internally references back to it).
The libusb context has to outlive the device handle.

Github issue:

dcuddeback/libusb-rs#3

Example solution via IRC:

https://play.rust-lang.org/?gist=c68bda27266249c781e6335c4127f301&version=stable&mode=debug&edition=2015
@yuvadm
Copy link

yuvadm commented Jul 26, 2018

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
Copy link

oberien commented Jul 26, 2018

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
Copy link

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
Copy link

oberien commented May 20, 2019

@stevenroose
Copy link

@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.

dmitry-zakablukov referenced this issue in passware/libusb-rs Dec 7, 2020
…to master

* commit 'ab83741c9604a523a233c7722b1bae7c1bdb7511':
  [PKM-308] Using libusb_set_option() function instead of deprecated libusb_set_debug() function
  [PKM-308] Do not alter device reference count inside Device struct
@yuvadm
Copy link

yuvadm commented Apr 5, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants