control_transfer not implemented

[r3n4ud]

control_transfer is not yet implemented in InterfaceHandle althought the binding is present in libusb-sys.

Any plan on that? Do you accept PR on this?

[Slightly OT but related]: xxx_transfer should have been implemented on DeviceHandle, no? I agree it makes sense on InterfaceHandle but

[dcuddeback]

@nibua-r I do plan to support control transfers. The reason it's not in the library yet is that libusb_control_transfer seems too low-level to directly translate to Rust. I was planning to implement each type of control transfer (http://libusb.sourceforge.net/api-1.0/group__misc.html#ga5b5912057c2d7e23b3ed5aa0c20236df) as a separate method. Some of them I think should not be exposed in safe code. For example, the documentation for libusb_set_configuration warns:

You should always use this function rather than formulating your own SET_CONFIGURATION control request.

http://libusb.sourceforge.net/api-1.0/group__dev.html#ga186593ecae576dad6cd9679f45a2aa43

I'd be happy to receive a PR that implements some of the control transfers. Is there a particular control transfer that you're hoping to see implemented?

It looks like your comment got cut off. I'm curious why you think bulk_transfer and interrupt_transfer should be implemented on DeviceHandle. I implemented them on InterfaceHandle because the documentation says that you must claim an interface before performing I/O on its endpoints, but I'm not super happy with the ergonomics of the current design. I'd love to hear your feedback on that.

[r3n4ud]

@dcuddeback I'm new to Rust so I've no enlightened design idea yet :wink: . In fact, I'm playing with Rust by porting a Ruby gem of mine: ligo. This gem provides an implementation of the Android Open Accessory Protocol to enable I/O communication through USB. As a matter of fact, I do use a control_transfer instead of a set_configuration (nothing particular here, I could probably switch to set configuration) but the more important point here is that the Android Accessory Protocol relies on LIBUSB::REQUEST_TYPE_VENDOR control transfers:

• https://github.com/nibua-r/ligo/blob/5e1feb086d34a3adaa617118a25f833fde817652/lib/ligo/device.rb#L266
• https://github.com/nibua-r/ligo/blob/5e1feb086d34a3adaa617118a25f833fde817652/lib/ligo/device.rb#L289
• https://github.com/nibua-r/ligo/blob/5e1feb086d34a3adaa617118a25f833fde817652/lib/ligo/device.rb#L305

The libusb ruby binding implement those transfer methods on the handle, that's the reason why I made this remark. Some usages on how the interface is claimed or used with the handle are here, for discussion.

Cheers! (and thanks for the already efficient enough libusb-rs!!!)

[dcuddeback]

Okay. The vendor requests are a good reason for having a generic control transfer function. For now, I think an almost literal translation of the libusb_control_transfer function will work. I think bmRequestType should be represented as a struct that hides the bitwise operations from the user.

[r3n4ud]

OK. I'll try to get something working for review.

[dcuddeback]

@nibua-r That would be great. Thank you. I will publish a new version of libusb-sys this morning with the definitions from libusb_request_type and libusb_request_recipient, which you'll need to implement the control transfer. I will be out of town for the rest of the week starting tonight, but I will look at anything you send me.

[dcuddeback]

@nibua-r libusb-sys 0.1.1 is now on crates.io. It should have everything you need to implement control transfers. Btw, when it comes to control transfers, I think you're right that it should be implemented on the device handle, because they don't require claiming an interface first.

[r3n4ud]

@dcuddeback :+1:

[r3n4ud]

I got something working but I need a review of my code since I'm not proficient enough yet with rust to trigger a merge request as is. I have work on a separate branch of my personal fork. I also need to document and figure out some unit tests (not depending on Android device).

[dcuddeback]

@nibua-r For not being proficient at Rust, that looks pretty good! I think it's ready for a PR, but I'll leave some feedback here since I've already looked at your branch:

• I think that device_handle.rs is not the most intuitive place for people to find the new types (RequestRecipient, BmRequestType, etc). I'd suggest putting them in a separate file request.rs and import them where they're needed.
• The RequestDirection enum is identical to the existing Direction enum in endpoint.rs (https://github.com/dcuddeback/libusb-rs/blob/0da8a2c6e47e8737bd432bedc6836f895e25e119/src/endpoint.rs#L3). I realize that the documentation for the Direction enum talks specifically about endpoints, but that's because it was the only use case for the enum at the time. I think it would be okay to reuse Direction for control transfers. I can update the documentation to make it less specific about endpoints. Maybe it makes sense to move the Direction enum to request.rs? I'm honestly not sure where it makes the most sense.
• The name BmRequestType will make the most sense to people who are familiar with the field names of USB packets. I've tried to use names that will be accessible to people who are new to USB. I'm not sure what a good name would be. Maybe ControlRequest?
• I'm kinda in favor of renaming RequestRecipient to be just Recipient. While RequestRecipient may be more specific (and thus avoid future name clashes), I think the need to add another recipient type in the future is pretty unlikely. This also matches the naming of the constants from libusb, e.g., LIBUSB_RECIPIENT_DEVICE.
• Last but not least, I'd like to keep the fields of BmRequestType private to allow for the possibility of optimizations or extending it with new functionality. We can instead set the fields via a constructor. Combining that with my previous suggestions, we'd have something like this: ControlRequest::new(Direction::In, RequestType::Standard, Recipient::Device).

Thanks for spear-heading this feature. I'd be happy to help out in any way I can. Do you want help writing the documentation or tests?

[r3n4ud]

@dcuddeback Thank you for this constructive and comprehensive review!

• I fully agree that device_handle.rs is not the right place to put the new types.
• There is no reason to not reuse Direction. At first, I would like to provide a relatively restricted modification. It make more sense to me to reuse and move Direction to a new file.
• ControlRequest may not be the best name, unless if it is extended to include the data buffer, etc.
• Agree to revert from RequestRecipient to Recipient
• Okay to use a constructor and private fields

I'll check this out and update my branch as soon as I can use some times on this.

[r3n4ud]

Tests are needed. Maybe some get_configuration using bare control_transfer should do?

[BenoitZugmeyer]

Btw, when it comes to control transfers, I think you're right that it should be implemented on the device handle, because they don't require claiming an interface first.

Are you sure about that? Disclaimer: I'm a beginner in both libusb and rust. I'm trying to adapt this script in rust, as an exercise (and because I want to see my mouse blinking)

In the script, they're are indeed claiming the interface before doing a control_transfer. I tried, without success, to do the same with libusb-rs, but whenever I run control_transfer function on the DeviceHandle, it fails, and the kernel logs:

usb 4-1.1: usbfs: process 23633 (test) did not claim interface 2 before use

I tried to move the control_transfer function to the InterfaceHandle, and it works like a charm.

EDIT: maybe you could add an example using control_transfer on on DeviceHandle?