Add USBHost #3307
Add USBHost #3307
Conversation
…isochronuous channels
|
Hey @ijager, I'm working on extending this PR to support 'universal' drivers (see wip @ https://github.com/i404788/embassy/tree/usb-asynch). I was wondering if there is a reason to have Channels be static (i.e. non-reconfigurable, and only one of In/Out). On the platform I'm looking to target (USBOTG), the current strategy will quickly consume all channels. In the Hub handler example I'm working on, it's significantly more efficient to allocate 2 pipes: 1 for interrupt, and one for control IN/OUT, but that requires re-configuring the pipe. Does the stm32 not support changing attributes? If it does, I think a shared |
|
I'm also working in this direction. I completely redesigned channel and host API to make it usable from multiple concurrent tasks. On RP2040 you need to reconfigure hardware for each control/bulk/isochronous transfer and only interrupt channels are special. So in my variant channels other from interrupts are basically free, because the only resource they use is stack space. Interrupt channels are limited, but automatically freed on channel drop. Other type of channels should reconfigure hardware and block other tasks that want to issue transfers. https://github.com/nikvoid/embassy/tree/rp2040-usb-host https://github.com/nikvoid/embassy/blob/rp2040-usb-host/embassy-usb-driver/src/host.rs#L282 Updated host example: https://github.com/nikvoid/embassy/blob/rp2040-usb-host/examples/rp/src/bin/usb_host_keyboard.rs |
|
@nikvoid Thanks for the notif. That does look a lot closer to what I can work with. There is one difference that might cause an issue for USBOTG, there channels are limited (even non-interrupt) but each are independent (async-safe) and reconfigurable. Which is why I was looking at a reconfigure API for all channels (can still be type-state by consuming the channels); it seems like your RP2040 would support this (just changing values on the stack), and it would allow for the USBOTG to run without mutexes. I will rebase to your branch, and if you're up for it we can collaborate on the final traits/interfaces. |
|
Good that there's more people working on this. I am happy to change things up. If we have three targets and use cases together that's great for testing. I am still on vacation for a week without computer so cannot really checks things right now. |
|
OK. For now I'm going to write a hub driver and test it on few keyboards. By the way, what are good devices to test bulk and isochronous transfers on? I think mainly of mass storage and web camera, but I don't have one. |
|
If you have a second RP2040, you could try the CDC-ACM, or MIDI drivers from embassy-usb in device mode (or just put it in usb flash mode). Isochronous isn't used much, mainly audio & video equipment, if you have an android device you might be able to configure it as a webcam (or potentially in audio accessory mode: https://source.android.com/docs/core/audio/usb#accessoryAudio). |
|
Now I have USB hub working for full speed devices, but not yet for low speed. |
|
Could be that you are missing the LS preamble on the interrupt channel, at least in USBOTG you need to set it per channel. |
|
Yeah, indeed. I overlooked a register for that. Now it works, except for timeouts. There may be another register to configure it, I'll research on that later. |
|
Timeouts appear to be a problem with specfic hub. Another hub is working fine. |
|
@nikvoid I've converted most of your hub driver to a usb driver trait: https://github.com/i404788/embassy/blob/usb-asynch/embassy-usb/src/handlers/hub.rs). If you could review the general structure, especially |
|
Well, I don't see a reason to create device driver trait. I have updated rp/usb_host_hid_keyboard example with hub usage, as you can see, user can just try all drivers with some priorities, then create task for device. I think this is the very flexible way. About handler trait: what purpose it serves? I see that it uses raw https://github.com/i404788/embassy/blob/fb70372f52f3615e92863ef01afcb5d176e5d4c5/embassy-usb/src/handlers/hub.rs#L115 https://github.com/i404788/embassy/blob/fb70372f52f3615e92863ef01afcb5d176e5d4c5/embassy-usb/src/handlers/hub.rs#L119 https://github.com/i404788/embassy/blob/fb70372f52f3615e92863ef01afcb5d176e5d4c5/embassy-usb/src/handlers/hub.rs#L53 |
|
Thank you for the review.
The main purpose is to create a common interface for drivers such that a user can easily re-use them & create a common initialization sequence. This allows (for example) for automatic enumeration & initialization of drivers from a registry without mapping out what each driver needs to get started; this is effectively similar to how larger OSes do it. As for why UsbHostDriver instead of UsbHost, it's because UsbHost doesn't seem to provide anything you'd need in a driver while increasing the restrictions on usage. I'm aiming to avoid having to pigeonhole the drivers into embassy-usb for future development. I didn't know you had integrated the HW interrupt into the Ah yes I see what you mean, I misinterpreted your impl and tried to optimize; I'll fix that. Ditto, my mistake. |
I think the main problem with this is that we are on embedded device without
Usb is host-centric, and even interrupts are not interrupts, but rather poll-based things, so I though it would be appropriate name. We could just state about this behaviour in docs and/or rename method, e.g. to |
|
While I agree we should assume no-alloc for any infra code, I do regularly use alloc in project development, with the traits you now have the options. Functionally, I don't think your struct pattern is much worse; just that it seems to me most drivers don't need to assume it will run on embassy, rather any async rust would have the equivalent code; so a trait would allow for more portability and for 3rd party drivers that neatly integrate (similar to A few things I was planning to build based on the trait was a faster enumeration filter using the static spec, also with the new structure of the hub you don't need a mutexed device registry (or no registry at all just unique address assignment). Btw the only reason I'm calling them
If you configure HW interrupt polling you may still manually poll, at least on USBOTG (maybe that's a unnecessary use-case to consider). If we only allow automatic polling I think just documenting the behavior around this would be good enough. (I do agree that the HostDriver should ensure software polling works the same as hw polling with polyfills) |
|
So, if I finally understand correctly, handler trait will be something like a middleware between device driver and usb host driver (or just device driver that stands directly over bus driver). And I have more questions: https://github.com/i404788/embassy/blob/fb70372f52f3615e92863ef01afcb5d176e5d4c5/embassy-usb/src/handlers/mod.rs#L69 https://github.com/i404788/embassy/blob/fb70372f52f3615e92863ef01afcb5d176e5d4c5/embassy-usb/src/handlers/mod.rs#L87 |
Hmm I was hoping this could be done in the Channel itself to avoid complicating the trait & implementation; e.g. give a Ref<AtomicU32>, and a it's own index, which it can use to xor to mark it is cleared & clear the registers needed if it's an hw interrupt pipe.
True I suppose it could be
Correct, so at a higher layer it can be decided to suspend a device in order to start/resume a different device. |
ijager
force-pushed
the
stm32-usb-host
branch
from
c7cc82e to
5565ae4
Compare
|
@i404788, I have included all latest changes of your branch and have now a working example for STM32G0 based on the new traits. Regarding the If a device is already connected and then I call Perhaps as an optional, so // either
async fn wait_for_device_event(&self, expected_event: host::DeviceEvent) ->host::DeviceEvent
// or
async fn wait_for_device_event(&self, expected_event: Option<host::DeviceEvent>) ->host::DeviceEventIf passing in |
|
Hmm also not ideal as we have to pass in the Device Speed, which is dummy: debug!("Detecting device");
// Wait for root-port to detect device
let speed = loop {
match usbhost.wait_for_device_event(Connected(USBSpeed::Full)).await {
Connected(speed) => break speed,
_ => {}
}
}; |
In case of bad device that ignores these requests
Hmm that does sound strange, it could also be a bad request I recently got this with USBOTG as well. For example if the CRC is incorrect the device will just not respond.
Sounds good, I'll see if I can get a decent interface around it for the configuration.
I can see your reasoning here, but |
|
You are right, the driver can be a state machine. Then we could make sure we always get For unexpected disconnects, those are usually noticed because of failed channel operations/transfers. Is the idea to call Currently my USB task looks like this: loop {
// Wait for device connected
usbhost.wait_for_device_event().await;
// do enumeration etc ...
let Ok(mut kbd) = KbdHandler::try_register(&usbhost, enum_info).await else {
...
break;
};
// Read device endpoints until disconnect
loop {
match kbd.wait_for_event().await {
Ok(HandlerEvent::HandlerEvent(KbdEvent::KeyStatusUpdate(ev))) => {
...
}
Err(HostError::ChannelError(ChannelError::Disconnected)) | Ok(HandlerEvent::HandlerDisconnected) => {
info!("Device disconnected");
break;
}
other => warn!("Unhandled event: {:?}", other),
}
}
// kbd is dropped here
}So I don't really need to |
I tried to take a closer look into this issue. EDIT: RX timeout seems to be no serious error, but rather an indication that hardware is about to repeat request. So +1 to adding a failsafe timeout. |
| let mut desc_len = slice[offset] as usize; | ||
| let mut desc_type = slice[offset + 1]; | ||
|
|
||
| while desc_type != T::DESC_TYPE || desc_len != T::SIZE { |
There was a problem hiding this comment.
Only accepting descriptors that are the exact size expected as is done in this line would make the descriptor fail to parse if there were existance of any extentions to the descriptors such as those that exist in USB 2.0+.
For example MIDI 2.0 DeviceEndpoints can potentially have two extra bytes in positions 7 and 8 and so the DeviceEndpointDescriptor bLength may be 9 rather than 7.
| Offset | Field | Size | Value | Description |
|---|---|---|---|---|
| 0-6 | (same as in USB 1.1) | |||
| 7 | bRefresh |
1 | 0x00 | Unused |
| 8 | bSynchAddress |
1 | 0x00 | Unused |
What this means is in practice if you assume exact sizes, newer devices will be ignored by this driver if they use any extensions even though they should be backwards compatible.
The assertion that bytes beyond the expected size must be ignored is mentioned briefly in the USB spec in Chapter 9.5 "Descriptors":
If a descriptor returns with a value in its length field that is less than defined by this specification, the
descriptor is invalid and should be rejected by the host. If the descriptor returns with a value in its length field that is greater than defined by this specification, the extra bytes are ignored by the host, but the next
descriptor is located using the length returned rather than the length expected.
This would also be relevant to parse_endpoints (and other descriptor parsing logic above) which assumes the record length when advancing the working_buffer slice working_buffer = &working_buffer[EndpointDescriptor::SIZE..]; as that could break if some bytes in the extended area happened to collide with a valid start to a descriptor.
I think it would be best to refactor the code to always advance the slice being considered by the amount specified by bLength of the current slice, and never assume that it's safe to advance by a static amount.
There was a problem hiding this comment.
A sent an attempted fix as another PR targeting this PR branch. Please take a look :)
|
This branch currently has quite a few commits mixed in that are duplicates of things that have already been merged into main that I think make it a bit unwieldy to review. I tried my hand at rebasing it in my fork's usb-host that did require a couple conflict resolutions but hopefully I got them right. |
|
Would this work support (virtual) serial ports easily? |
|
@JYouren Yes, one of the drivers I made for myself is very similar to USB Serial (technically mine is a printer port but works largely the same), and it's quite easy to implement. |
|
Awesome, you wouldn't be able to sign post me to that would you so I can have a look? |
|
It isn't public but here are the important parts:
Code:pub struct Handler<H: UsbHostDriver> {
bulk_in_channel: H::Channel<channel::Bulk, channel::In>,
bulk_out_channel: H::Channel<channel::Bulk, channel::Out>,
control_channel: H::Channel<channel::Control, channel::InOut>,
}
impl<H: UsbHostDriver> Handler<H> {
pub async fn try_read<'a>(&mut self, buf: &'a mut [u8]) -> Result<&'a [u8], ChannelError> {
let actual_read = self.bulk_in_channel.request_in(buf).await?;
Ok(&buf[..actual_read])
}
pub async fn try_write(&mut self, buf: &[u8]) -> Result<(), ChannelError> {
match select::select(
Timer::after_millis(200),
self.bulk_out_channel.request_out(buf),
)
.await
{
select::Either::First(_) => Err(ChannelError::Timeout),
select::Either::Second(res) => {
res?;
Ok(())
}
}
}
}
impl<H: UsbHostDriver> UsbHostHandler for Handler<H> {
type PollEvent = ();
type Driver = H;
fn static_spec() -> StaticHandlerSpec {
StaticHandlerSpec::new(None)
}
async fn try_register(bus: &H, enum_info: EnumerationInfo) -> Result<Self, RegisterError> {
println!("Trying to register");
for iface in enum_info.cfg_desc.iter_interface() {
debug!("iface: {}", iface);
}
let iface = enum_info
.cfg_desc
.iter_interface()
.find(|v| {
matches!(
v,
InterfaceDescriptor {
interface_class: 0x07,
interface_subclass: 0x1,
interface_protocol: 0x2,
..
}
)
})
.ok_or(RegisterError::NoSupportedInterface)?;
debug!("Found matching iface!");
let interrupt_ep = iface
.iter_endpoints()
.find(|v| v.ep_type() == EndpointType::Interrupt && v.ep_dir() == Direction::In)
.ok_or(RegisterError::NoSupportedInterface)?;
debug!("Found matching intr ep");
let bulk_in_ep = iface
.iter_endpoints()
.find(|v| v.ep_type() == EndpointType::Bulk && v.ep_dir() == Direction::In)
.ok_or(RegisterError::NoSupportedInterface)?;
debug!("Found bulk in ep");
let bulk_out_ep = iface
.iter_endpoints()
.find(|v| v.ep_type() == EndpointType::Bulk && v.ep_dir() == Direction::Out)
.ok_or(RegisterError::NoSupportedInterface)?;
debug!("Found all endpoints");
let bulk_in_channel = bus.alloc_channel(
enum_info.device_address,
&bulk_in_ep.into(),
enum_info.ls_over_fs,
)?;
let bulk_out_channel = bus.alloc_channel(
enum_info.device_address,
&bulk_out_ep.into(),
enum_info.ls_over_fs,
)?;
let control_channel = bus.alloc_channel(
enum_info.device_address,
&EndpointInfo::new(
0.into(),
EndpointType::Control,
(enum_info.device_desc.max_packet_size0 as u16)
.min(enum_info.speed.max_packet_size()),
),
enum_info.ls_over_fs,
)?;
Ok(Handler {
control_channel,
bulk_in_channel,
bulk_out_channel,
})
}
}The main difference for the serial version will be the InterfaceDescriptor matching, the rest can likely stay the same. Then you can use it similarly to how I've used it here for kbd (note it does not handle disconnects or use any custom functions): https://github.com/i404788/esp-hal/blob/719d96101174fcfbd718b6bdb0177fe65d46a86f/examples/src/bin/embassy_usb_host.rs#L75-L97 |
|
It this still under development? I'd really love USB host support |
Actually we are also waiting for USB host support! If we know latest state, it would be great. |
|
@Dummyc0m and I are trying to create an implementation here: https://github.com/vapor-keeb/async-usb-host/ very early stage, still fighting with a lot of design challenge with how to support hot plug and unplug multiple (kinds) of devices. We got enumeration / hubs working so far. Trying to support a real device (Likely HID) next |
|
@mikeemoo @deluxetiky The reason it's pretty quiet is because it's mostly complete, it already works on rp2040, stm32 and esp-hal. Although they may need some adjustments for upstream. The last item open is the optional 'Transaction Timeout' which admittedly I've been procrastinating on since it's not needed for my use case. I'll try to rebase to main & add it this week, and open a final PR (or @ijager can rebase if he's available). @Codetector1374 Neat, hot-plug is the hardest part indeed. Seems like it's duplicating a lot of the work of this PR, if you're just interested in a different implementation don't let me stop you; but if it's just for the functionality it's already finished. |
|
I am also using this branch in a completed project since end of last year. I will probably get back to this as the client has asked for HUB support as well, but not in the next few months. So totally fine to open a new PR and then I can close this one. |
|
I've also spun off a branch to fix some issues present in this one (but otherwise, I'm very appreciative of the work that was put in here!) While there are a number of commits in the branch, there are really only three issues to speak of. First, Second, assuming I haven't misunderstood something about how USB works (which could well be the case) I don't think it's possible for Third, with only 256 bytes provided, it's not hard to exceed the configuration buffer size, causing a panic. This commit bumps the size to 512 bytes, which happens to be large enough for the device I'm testing with, but some configurability would probably be good. I've also added a guard, so that we get an error rather than a panic. |
|
@AlexCharlton the first issue seems to have to do with the alignment of the endpoints (if the enpoints are aligned you won't encounter the issue, which likely was in our cases). You're right that this general area could be expanded upon to support more complex device configurations. For point 2, I believe it depends on the hardware IP used, iirc USBOTG will do the speed detection for you. I could be wrong here as well, but detection worked correctly as far as I've tested with esp-hal, it's possible a redundant For the configuration buffer, it seems like they can be up to 65535 bytes ( I appreciate your testing, I'll collate & add your improvements into the final PR. |
|
I've opened a new PR at #4032. @AlexCharlton I've refactored the Descriptor parsing (also integrating @jakerr's test cases) & SetConfiguration procedure, so now the application or handler can choose the configuration through |
Makes sense to me! |
|
Closed in favor of #4032 |
This PR adds USBHost in several layers:
usb_v4devices inembassy-stm32crateembassy-usb-drivercrateembassy-usbcratestm32g0This is the implementation for issue #3295.