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.

Sign up for GitHub

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

Jump to bottom

Add basic implementation of FtdiDeviceHandler #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add basic implementation of FtdiDeviceHandler #8

wants to merge 7 commits into from

Conversation

jamesadevine
Copy link
Collaborator

@jamesadevine jamesadevine commented Feb 1, 2023 •
edited
Loading

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

jiegec commented Feb 4, 2023

The ftdi code should go to examples.

@jamesadevine
Copy link
Collaborator Author

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

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

jiegec commented Feb 6, 2023

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

coveralls commented Feb 6, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 4105479928

  • 17 of 164 (10.37%) changed or added relevant lines in 4 files are covered.
  • 3 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-1.7%) to 40.03%
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%
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%
Totals Coverage Status
Change from base Build 4061119800: -1.7%
Covered Lines: 542
Relevant Lines: 1354
💛 - Coveralls

@jamesadevine
Copy link
Collaborator Author

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

jiegec commented Feb 7, 2023

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.

@coveralls
Copy link

coveralls commented Jul 14, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 4103377666

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 22 of 157 (14.01%) changed or added relevant lines in 4 files are covered.
  • 75 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-1.0%) to 40.694%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/host.rs 0 1 0.0%
src/device.rs 22 29 75.86%
src/ftdi.rs 0 42 0.0%
src/lib.rs 0 85 0.0%
Files with Coverage Reduction New Missed Lines %
src/host.rs 1 0.0%
src/lib.rs 11 53.36%
src/device.rs 63 50.41%
Totals Coverage Status
Change from base Build 4061119800: -1.0%
Covered Lines: 516
Relevant Lines: 1268
💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

not implemented: control in
3 participants
@jamesadevine @jiegec @coveralls