USB: Add CDC support, update capsule stack #1902
Conversation
This was split off from `Ok`, but not added to the return code handler for the SAM4L. Kinda feel like this one is on Rust, since it shouldn't compile without handling this case :(.
As far as I can tell, we only ever use the pointer address for these buffers, and not their length, so it should be fine if the buffers are too long (say 64 bytes rather than 8, as the capsule uses).
Buffer size increased from 8 to 64.
Per multiple resources, GET_DESCRIPTOR cannot be used to access interfaces or endpoints separately from configurations. https://www.beyondlogic.org/usbnutshell/usb6.shtml
Really, descriptors.rs should be handling all of this and the control endpoint handler should just be working with buffers of data. That wasn't totally accomplished here, but it was a start. Additionally, this means that descriptors created by the upper layers can just be made on the stack and passed in with no need for persistant storage.
I don't think I did it right
call enable/attach directly
no idea why this was written this way
capsules/src/usb/usbc_client_ctrl.rs
Outdated
| /// Buffer containing the byte-packed representation of the configuration | ||
| /// descriptor and all other descriptors for this device. | ||
| other_descriptor_buffer: DescriptorBuffer, |
There was a problem hiding this comment.
I don't think "other" is a meaningful name. Which descriptors are exactly in there? Maybe configuration_descriptors_buffer would make more sense?
There was a problem hiding this comment.
Well it isn't just configuration descriptors, it really is all other descriptors.
There was a problem hiding this comment.
Yes, but there is some structure in these descriptors, I assume they are not stored there in arbitrary order. Maybe "configuration descriptor tree" would match the concept more closely?
| _ => hil::usb::CtrlSetupResult::ErrInvalidConfigurationIndex, | ||
| } | ||
| } | ||
| DescriptorType::Interface => match descriptor_index { |
There was a problem hiding this comment.
I haven't tested it. But does anything break if a host sends us a query for an Interface descriptor anyway?
There was a problem hiding this comment.
Interface descriptor requests are not possible. Per multiple resources, GET_DESCRIPTOR cannot be used to access interfaces or endpoints separately from configurations. https://www.beyondlogic.org/usbnutshell/usb6.shtml So it should be fine to hit the default match case.
| /// Buffer for holding the configuration, interface(s), and endpoint(s) | ||
| /// descriptors. Also includes class-specific functional descriptors. | ||
| pub struct DescriptorBuffer { | ||
| pub buf: [Cell<u8>; 128], | ||
| pub len: usize, | ||
| } |
There was a problem hiding this comment.
From a memory usage point of view, I find it unfortunate to store some descriptors (e.g. HID) twice: one time already serialized here, and another time in the ClientCtrl object. Can't we serialize everything on-demand in the handle_standard_device_request function, as before?
There was a problem hiding this comment.
Would you be willing to add a commit? This code works, and gets us closer to a long standing item on our wishlist. The USB stack is by no means final, and I'm sure there are optimizations possible.
There was a problem hiding this comment.
Would you be willing to add a commit?
As far as I'm aware, I'm not able to push commits in this branch, as I'm not a "reviewer with write access".
There was a problem hiding this comment.
I think serializing everything immediately is actually the way to go. The ClientCtrl doesn't actually need to know much about the descriptors other than just hang on to several buffers of memory and provide them when desired. So it doesn't need a full struct sitting around. A way to stop the duplication would be to have separate buffers for endpoints, interfaces, and other stuff (like the HID) and then combine buffers when things are requested by the host. That would allow you to have only one copy of the HID. We didn't go that full route because the HID descriptor isn't very large so just having two copies lying around doesn't seem to matter much.
The other reason to go with a serialized approach is that it was getting difficult to make lifetimes work for the descriptors. The ClientCtrl can't have structs that it copies fields into because it doesn't know which descriptors are going to exist or how many. It's way easier for the ClientCtrl to have an array of bytes with a fixed size that it can just copy descriptors into than having pointers to arrays of structs that you have to reason about the lifetimes of. I'm sure there's some way to get things working that way, but I couldn't figure it out.
There was a problem hiding this comment.
If you think this is really important to change before this PR goes in and you have some ideas about how to do so, I think you could pull this branch into your own Tock repo, make changes, and then make a PR against this branch rather than against master.
There was a problem hiding this comment.
If you think this is really important to change before this PR goes in and you have some ideas about how to do so, I think you could pull this branch into your own Tock repo, make changes, and then make a PR against this branch rather than against master.
It's not really practical to make a PR against another PR to be honest...
Match other implementations, and not all settings actually matter in practice. Co-authored-by: Branden Ghena <brghena@berkeley.edu>
Co-authored-by: Branden Ghena <brghena@berkeley.edu>
Co-authored-by: Branden Ghena <brghena@berkeley.edu>
Co-authored-by: Branden Ghena <brghena@berkeley.edu>
| interface_descriptor: &mut [InterfaceDescriptor], | ||
| endpoint_descriptors: &[&[EndpointDescriptor]], |
There was a problem hiding this comment.
I find it weird to separate the arrays for interface and endpoint descriptors in this way. The assumption that isn't mentioned in this API is that interface_descriptor.len() == endpoint_descriptors.len().
Could it be possible to have some InterfaceHierarchy struct:
struct InterfaceHierarchy {
interface: InterfaceDescriptor,
endpoints: &[EndpointDescriptor],
}Then, this function would take an interface_hierarchies: &mut [InterfaceHierarchy] parameter which would logically tie the interfaces to their endpoints.
There was a problem hiding this comment.
Yes, of course, but USB is a WIP stack, so I don't think we should block this change (with a tracking issue dating back years) on the most preferred design, instead we should continue our policy of forward progress is good and iterate quickly.
There was a problem hiding this comment.
Fair point. My point was mostly that in my experience working with USB, a lot of things can be brittle and therefore overlooking design could fire back in terms of long hours of subtle debugging. But sure this can be improved in a separate pull request.
| // TODO should we be erroring here if len > 128? Otherwise we'll probably | ||
| // buffer overrun and panic. |
There was a problem hiding this comment.
This should definitely be handled explicitly, e.g. panic!("Configuration descriptor too large"). This shouldn't be a problem given that all of the configuration is defined at compile time, so even if this panic happens at runtime, it's reliably triggered depending on the compile-time descriptors.
This is also IMO a reason why these descriptors should rather be serialized on-demand following get-descriptor requests (which then doesn't impose a limit on the total buffer size), rather than eagerly serialized in a temporary buffer here.
There was a problem hiding this comment.
What is the best way to handle this? We don't want panic! in capsules generally, but if this overruns we do want a fast, obvious error.
Just leaving a note for now, is honestly probably sufficient for the foreseeable future (at least until a USB capsule cleanup and rewrite) since 128 bytes fits CDC, OpenSK stuff, and also would fit USB Mass Storage or additional HID stuff if we want to expand to that in the future.
There was a problem hiding this comment.
Unfortunately, I think even serializing on demand doesn't actually fix this simply. You would just end up with structs that, when serialized sometime at runtime, end up either overrunning the buffer or more likely getting truncated. You could handle this at runtime, but doing so would require you to account for partial progress in serializing some struct or another and picking up serializing part-way through that struct for the next packet. (You can't just stop at the last struct that fits because I believe you have to send the full 64 byte packet in order to fragment.) At least with the descriptors serialized at initialization time, the error occurs immediately.
There was a problem hiding this comment.
What is the best way to handle this? We don't want
panic!in capsules generally, but if this overruns we do want a fast, obvious error.
I don't see any unsafe in this file. So if the buffer overruns, the current behavior is that it will panic at one of the array indexing operations (e.g. within a write_to where the target buffer is smaller than the source). So the current code would yield a panic (which we want to avoid apparently), a fast error (in this function), but not so obvious (array indexing out of bounds somewhere).
What I suggest is an explicit panic with a clear message here, which improves over the current state by making the error obvious.
To avoid a panic altogether, the best would probably be to put all of this code in a const fn so that the contents of the descriptor buffer is pre-computed at compile time, and that any failure would prevent it from compiling. However, I'm not sure what the current status of const fn is in terms of supporting things like loops (although I remember there has been some improvements lately).
|
Are there any major blocking issues for this? |
|
@tock/core-wg |
There was a problem hiding this comment.
I think the only blockers I see are @gendx's comments:
- This one should be addressed by adding an explicit
panic!-- since it will panic either way, better to have something explicit than it happening from an array access. - This one should be added to a tracking issue so we don't lose it
- This one could either be addressed by a comment, or added to a tracking issue
+1 to having (a) tracking issue(s) for the comments I left, so that they are not forgotten in the long term once USB stabilizes, but without unnecessarily blocking progress of useful features in the short term. |
|
I created #1940. |
Pull Request Overview
This pull request is part of #1048, #1893.
This adds CDC support which implements
kernel::hil::Uart. This allows us to run console over USB without having to write our own host-side kernel drivers.On that path, this also updates the capsules USB stack to allow for the larger descriptors we use with the CDC device. This is also the first time we have needed multiple interface descriptors, so it adds support for that as well.
The bug fixes needed along the way are in separate PRs.
Testing Strategy
This pull request was tested by running apps that use the console on imix, and setting up the console to use UART over CDC rather than the UART driver.
TODO or Help Wanted
This does not work on the nRF52 (at least on my computer) because that driver does not support CTRL WRITE messages. FYI.
I had to add a platform-specific variable, since the nRF only works if the device descriptor says the packet size is 64, and the SAM4L driver only works if the device descriptor says the packet size is 8. This also makes the USB stack work on the SAM4L again.
Documentation Updated
/docs, or no updates are required.Formatting
make format.make clippy.