sys/usb/cdc_acm: Change API to report errors #21894
Conversation
According to API doc, `stdio_write()` may write less data then requested or report an error. This updates the code to loop in the flushing until either all data is written or an error is returned.
- Change return type of `usbus_cdc_acm_submit` from `size_t` to `ssize_t`
to be able to report errors
- Change `stdio_cdc_acm` to make use of the new API
- Change `stdio_cdc_acm` to not loop until all data is flushed, but just
report what could be added to the buffer now and post the flush event
- The layers above should already loop until all data is written
- The layers above could opt to not loop when in IRQ context. This
would allow using `stdio_usb_cdc_acm` from IRQ context.
github-actions
bot
added
Area: USB
|
Warning This is completely untested |
crasbe
added
Type: enhancement
| while (pos < end) { | ||
| size_t left = (size_t)end - (size_t)pos; | ||
| ssize_t written = stdio_write(pos, left); | ||
| if (written < 0) { | ||
| /* failed to flush, drop data */ | ||
| break; | ||
| } | ||
| pos += written; |
There was a problem hiding this comment.
Could this become an endless loop with high CPU load if stdio_write returns 0 (and does not write anything)? I'd have to look up if stdio_write could behave like that.
There was a problem hiding this comment.
Returning 0 is very much valid. If the stdio would have flow control (e.g. UART with flow control or some network protocol), it may not be able to send out any data right now.
There is an expectation towards the transport that returning 0 is a temporary thing that will sort itself out within reasonable time. The layer above cannot reasonably judge when stuff won't work, so it just has to trust that at some point the transport will either make progress or give up.
| return (char *)buffer - start; | ||
| ssize_t retval = usbus_cdc_acm_submit(&cdcacm, buffer, len); | ||
| usbus_cdc_acm_flush(&cdcacm); | ||
| return retval; |
There was a problem hiding this comment.
Handling the partial transfer in the upper layer won't work when stdio_dispatch is used.
There was a problem hiding this comment.
We can do the looping in stdio_dispatch for now and have the option to later improve, e.g. be asking the transports what amount of data they can handle right away and dispatching only the minimum of those values.
If we force all transports to be smart and handle all the special cases and IRQ themselves, we easily run into duplicates code and transport specific behaviour.
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Thanks for doing this! Looks reasonable to me, but I'm not deep enough into the stdio stacks to feel confident enough to approve :) Not tested yet either.
Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
5 participants
Contribution description
usbus_cdc_acm_submitfromsize_ttossize_tto be able to report errors
stdio_cdc_acmto make use of the new APIstdio_cdc_acmto not loop until all data is flushed, but justreport what could be added to the buffer now and post the flush event
would allow using
stdio_usb_cdc_acmfrom IRQ context.This also sneaks in a fix for our
picolibintegration: We didn't loop overstdio_write()in the flush implementation, as we should have.Testing procedure
STDIO via USB CDC ACM should still work correctly with both
newlibandpicolibcas standard C lib.Issues/PRs references
Follow up to: #21890