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 low speed #678

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Usb low speed #678

wants to merge 4 commits into from

Conversation

aero530
Copy link

@aero530 aero530 commented Mar 9, 2023

Summary

Add cargo cfg feature to allow user to use USB in low speed mode instead of the default full speed mode.

Reasoning

My specific use case is using the microcontroller to emulate a keyboard / mouse for use in BIOS configuration. Some motherboards do not support full speed USB interfaces in BIOS so I need the ability to set the USB subsystem to low speed mode.

Notes

I'm not sure if the way I implemented the cfg feature is idiomatic so I'm open to feedback on that but it does work on the metro_m0 board I tested on. I did not add the feature to all the other board types as I have no way to actually test them (although I see no reason it would not work especially for any boards based on SAMD21).

Checklist

  • CHANGELOG.md for the BSP or HAL updated
  • All new or modified code is well documented, especially public items
  • No new warnings or clippy suggestions have been introduced (see CI or check locally)

If Adding a new cargo feature to the HAL

  • Feature is added to the test matrix for applicable boards / PACs in crates.json

@jbeaurivage
Copy link
Contributor

I agree with giving the user the option of using low speed, however I don't really agree with using a Cargo feature for that. A builder pattern method on the Usb struct would be much more user-friendly in my opinion.

@aero530
Copy link
Author

aero530 commented Mar 9, 2023

I wasn't super happy with using a Cargo feature either. A build pattern on USB would be a user-friendly way of setting the speed. I just couldn't figure out how to get that to work without also requiring updates to the usb-device crate so I opted for a solution that would as least be self contained.

@aero530
Copy link
Author

aero530 commented Mar 9, 2023

As best I can tell the USB speed has to be defined while initializing the USB subsystem (in the enable function from hal/src/thumbv6m/usb/bus.rs). The enable function is part of the UsbBus trait defined in the crate usb_device and does not have any inputs (other than self) so we would need to store the USB speed setting in some new field in UsbBus or Inner.

My (albeit limited) understanding would say that the USB speed could still be defined solely in the atsamd-rs crate but I don't see how a builder pattern method could be used. As far as I can tell, we are initalizing the USB subsystem when calling UsbBusAllocator::new() from the usb_allocator function in each board bsp. So wouldn't we need to send a parameter for speed through at that time?

I do see that we could add a speed parameter to the usb_allocator function for a board then pass that through to UsbBus::new() which is defined in hal/src/thumbv6m/usb/bus.rs. Then the speed parameter could be passed to (and stored in) Inner. That way, when running the enable() function (which is defined for Inner) we could access self.usb_speed (or whatever we want to call the new field in the Inner struct) and use that to set the USB speed.

This wouldn't satify the builder pattern idea but would at least prevent using a Cargo feature to set the USB speed.

Anyone have any other thoughts / advice?

@ianrrees
Copy link
Contributor

To me, this seems like a problem that should be worked on in usb-device first, then we'll need to make supporting adjustments here. Implementing it in just our HAL is problematic, because the USB class implementations don't have any way to determine what sort of device they're part of.

There has been a little bit of interest in different speed options over at usb-device, however mainly that's been about high speed support and has gone quiet lately.

We've got a breaking change to usb-device in the works here, I'll have a think about whether there's some way to make progress on this issue in the near term.

@sajattack
Copy link
Member

I'm going to convert this to a draft until the underlying support in usb-device is ready.

@sajattack sajattack marked this pull request as draft November 7, 2024 07:12
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.

4 participants