jiegec / usbip

A Rust library to run a USB/IP server
MIT License
255 stars 25 forks source link

Add basic implementation of FtdiDeviceHandler #8

Open jamesadevine opened 1 year ago

jamesadevine commented 1 year ago

Adds a non-functional implementation of UsbDeviceHandler for FTDI devices. An FTDI device now enumerates with FtdiDeviceHandler using the following code:

use usbip::ftdi::FtdiDeviceHandler;
...
let device_handler = Arc::new(Mutex::new(Box::new(FtdiDeviceHandler::new())
    as Box<dyn usbip::UsbDeviceHandler + Send>));

...

usbip::UsbDevice::new(0).with_interface(...).with_device_handler(device_handler);

Closes #7

jiegec commented 1 year ago

The ftdi code should go to examples.

jamesadevine commented 1 year ago

@jiegec - sure! I also just want to say that having no filter on new_from_host completely messed up USB on my machine. I would suggest it be removed and only allow new_from_host with a filter (user provided). There could be other wrapper functions that implement default filters: e.g. new_from_host_with_vid_pid

jamesadevine commented 1 year ago

@jiegec I'm not sure what you meant with your comment. I've added an example that spins up a fake 4 port FTDI device and echoes "hello" once a second. If you would like me to move the entirety of the FTDI code into examples, then I'd suggest that would be part of a bigger refactor. I mirrored the structure of the cdc/hid keyboard examples.

jiegec commented 1 year ago

@jiegec I'm not sure what you meant with your comment. I've added an example that spins up a fake 4 port FTDI device and echoes "hello" once a second.

Good job!

If you would like me to move the entirety of the FTDI code into examples, then I'd suggest that would be part of a bigger refactor. I mirrored the structure of the cdc/hid keyboard examples.

I put cdc/acm in src because it is a standard usb interface. But ftdi is not.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4105479928


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/host.rs 0 1 0.0%
src/device.rs 17 32 53.13%
src/ftdi.rs 0 42 0.0%
src/lib.rs 0 89 0.0%
<!-- Total: 17 164 10.37% -->
Files with Coverage Reduction New Missed Lines %
../../../.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.25.0/src/io/util/write_int.rs 1 72.22%
src/host.rs 1 0%
src/lib.rs 1 52.7%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 4061119800: -1.7%
Covered Lines: 542
Relevant Lines: 1354

💛 - Coveralls
jamesadevine commented 1 year ago

I put cdc/acm in src because it is a standard usb interface. But ftdi is not.

I think that's a rather arbitrary distinction. A driver is a driver. Putting ftdi code in examples means it has less reuse for others. It does lessen the maintenance burden which might be your concern. If you provide me a concrete suggestion for a folder structure you'd like me to observe I will implement it.

I'm also going to break this PR up into smaller PRs since it has taken a while to get this one merged in and there are now other improvements and bugfixes contained in the commits.

jiegec commented 1 year ago

I put cdc/acm in src because it is a standard usb interface. But ftdi is not.

I think that's a rather arbitrary distinction. A driver is a driver. Putting ftdi code in examples means it has less reuse for others. It does lessen the maintenance burden which might be your concern. If you provide me a concrete suggestion for a folder structure you'd like me to observe I will implement it.

You are right, maybe we can take driver code to a separate crate e.g. usbip-xxx?

I'm also going to break this PR up into smaller PRs since it has taken a while to get this one merged in and there are now other improvements and bugfixes contained in the commits.

Happy to see that.