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

USB Composite Device Support #58

Merged
merged 7 commits into from
Nov 27, 2024
Merged

Conversation

Dicklessgreat
Copy link
Contributor

This pull request aims to expand USB functionality by adding support for a composite USB device.

Motivation

The motivation behind creating a composite USB device is to enable versatile control of my audio device currently under development. In my case:

  • USB MIDI Interface: This interface will allow my device to be controlled by DAW software on a host PC, providing integration with standard audio workflows.
  • Vendor-Specific Interface (with postcard-rpc support): This secondary interface enables more detailed control of the device via CLI, even if a user does not have a DAW. This addition offers two primary benefits:
    • User Accessibility: Users can control the device using a simple CLI utility, making it accessible even outside of DAW environments.
    • Development & Debugging: From my end, this interface is invaluable for running simple tests from the PC, simplifying debugging during development.

Details

To implement this, I introduced a closure argument to new_raw_nusb, providing flexibility in selecting the interface number from InterfaceInfo (commit ba2ac52). However, I recognize alternative methods might exist, and I'm open to feedback on optimal design approaches.

Background

As a developer with limited experience in USB standards, I'm aware this approach may not fully align with best practices. If supporting multiple USB classes in a single device proves incompatible with the design goals of this crate, I'm open to revising or abandoning this feature.

@jamesmunns
Copy link
Owner

Hey there! I think I'm open to this, would you mind making this a different constructor, rather than modifying the existing one? This would allow us to avoid a breaking change.

@Dicklessgreat
Copy link
Contributor Author

Dicklessgreat commented Nov 21, 2024

Got it! I've made the requested changes. If I understand correctly, I noticed the HostClient::new_raw_nusb() constructor is intended specifically only for use with a "Vendor Specific" interface? If that's the case, the current adjustments should align with the expectations. Please let me know if I've misunderstood anything or if further modifications are needed.
Thanks.

Copy link
Owner

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm okay with this, with the caveat that if anyone complains that this breaks their setup, I may revert + yank it.

I am also open to more granular/detailed constructors that allow you to more specifically pick the interface (one closure for picking devices, and a second for picking an interface from the matching device?). I just haven't needed it yet. Feedback is definitely welcome, and I'm not a USB expert either!

Thanks!

@Dicklessgreat
Copy link
Contributor Author

WireStorage ctor

To avoid introducing a breaking change, I’ve added a new function called init_without_build(). This new function is 90% the same as the original init() but skips calling builder.build() at the end.

The existing init() now calls init_without_build() internally, followed by builder.build(). This approach ensures that there’s no duplication of logic while keeping the original functionality intact.

HostClient init

I’ve added a new function called try_new_raw_nusb_with_interface(). It’s essentially a renamed version of what I previously committed, added as a separate function. I thought it would be useful to include this as a precaution(for me) in case e6a0120 needs to be reverted or yanked later.

This function duplicates the logic of try_new_raw_nusb() intentionally, as it makes reverting or yanking easier.

While this duplication is fine for now, an alternative approach could be to modify try_raw_nusb() like this (details below). I’m happy to adjust things however is most convenient for you as the maintainer—just let me know!

pub fn try_new_raw_nusb<F: FnMut(&DeviceInfo) -> bool>(
    func: F,
    err_uri_path: &str,
    outgoing_depth: usize,
    seq_no_kind: VarSeqKind,
) -> Result<Self, String> {
    try_new_raw_nusb_with_interface(
        func, 
        |i| i.class() == 0xff,
        err_uri_path,
        outgoing_depth,
        seq_no_kind
    )
}

@jamesmunns jamesmunns merged commit 41fe2a5 into jamesmunns:main Nov 27, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

2 participants