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

embassy-usb modifications #1509

Closed
wants to merge 2 commits into from
Closed

embassy-usb modifications #1509

wants to merge 2 commits into from

Conversation

pcbzzz
Copy link

@pcbzzz pcbzzz commented May 30, 2023

BosWriter doesn't write valid attributes, seems like it was just added as an example. BOS needs more data than is currently being passed to the builder, so I don't think it's even really necessary as a builder param until it's more flushed out. Though I've left is as optional for now.

@alexmoon
Copy link
Contributor

I'm not sure making the BOS descriptor optional is the best approach. While it's not required for USB 2.0 devices it is required in later editions of the spec and it is being used for various extensions. For example, the MSOS descriptors feature needs to write a BOS capability. If the BOS descriptor is optional that could lead to a runtime panic in the case that someone using the msos-descriptor feature tries to write a MSOS descriptor while passing None for the BOS descriptor buffer.

/// Handler for HID-related control requests.
pub trait RequestHandler {
/// Gets the current HID protocol
fn get_protocol(&self) -> Option<Protocol> {
Copy link
Member

Choose a reason for hiding this comment

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

None here means "return 1", it has no special meaning. I think it'd make more sense to make this method not return an Option, and make the default impl return 1.

@Dirbaio Dirbaio marked this pull request as draft June 26, 2023 14:13
@Dirbaio
Copy link
Member

Dirbaio commented Dec 3, 2024

closing due to staleness.

  • The HID protocol seems a useful addition, feel free to repoen a PR with just that.
  • I'm not sure about the BOS thing.

@Dirbaio Dirbaio closed this Dec 3, 2024
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.

None yet

3 participants