shell/doc: Point users who run into shell buffer issues to the stdin buffer #20541
Conversation
chrysn
added
the
CI: ready for build
|
I'll wait with pushing the merge button until I've seen the rendered artifact. |
| * and overflows silently. As a consequence, commands through such terminals | ||
| * appear to be truncated at @ref STDIO_RX_BUFSIZE bytes (defaulting to 64) | ||
| * unless the command is sent in parts (on many terminals, by presing Ctrl-D | ||
| * half way through the command). | ||
| */ | ||
| #define SHELL_DEFAULT_BUFSIZE (128) |
There was a problem hiding this comment.
Why is this not the same as STDIO_RX_BUFSIZE?
There was a problem hiding this comment.
I can't comment on the authors' intentions, but generally I do still think that these are different -- SHELL_DEFAULT_BUFSIZE is about the longest command, STDIO_RX_BUFSIZE is about how many bytes are tolerated to be sent w/o blocking on the application to process them into the buffer (and 64 is surprisingly large for that purpose). Their relation only matters in the case of a line-buffered terminal, which is a choice outside of the control of the embedded application.
If we want to change code and not just docs, I'd prefer we do it right (by allowing backpressure or doing zero-copy) rather than creating a cross-layer configuration dependency when we're already struggling to get our configs transparent.
benpicco
added
the
Impact: minor
sys/include/shell.h
Outdated
| @@ -75,6 +75,15 @@ extern "C" { | |||
|
|
|||
| /** | |||
| * @brief Default shell buffer size (maximum line length shell can handle) | |||
| * | |||
| * @warning On terminals that buffer input and send the full command line in one go, | |||
There was a problem hiding this comment.
| * @warning On terminals that buffer input and send the full command line in one go, | |
| * @warning On terminals that buffer input and send the full command line in one go (like `pyterm`), |
let's not be vague, this affects our default terminal after all
There was a problem hiding this comment.
To be frank I don't have the overview of which terminal program we default to, we support so many and I often just socat in to the UART.
If it is really the default for all, we could even say
| * @warning On terminals that buffer input and send the full command line in one go, | |
| * @warning On terminals that buffer input and send the full command line in one go (like our default `pyterm`), |
but then I'd also look into sharpening the criterion, as it does apparently not affect all stdio. (I guess that UART based could be unaffected because UARTs' units of transfer are smaller than the buffer size, and slow enough that the shell thread can catch up).
There was a problem hiding this comment.
New text now says "When terminals that buffer input and send the full command line in one go are used on stdin implementations with fast bursts of data", and
+ * For example, this affects systems with direct USB stdio (@ref
+ * usbus_cdc_acm_stdio) with the default terminal `pyterm`.
(Sorry, should have been a fixup rather than an amend/force)
chrysn
force-pushed
the
doc-shell-constraint
branch
from
51a2409 to
b0bec02
Compare
Contribution description
Trouble with stdin and the shell can be hard to debug, and may send people off to a wild goose chase:
Is that an issue with CoAP? No. The shell? No. Let's ask on matrix... 🪿🪿🪿
This PR cuts the chase short when looking at the shell documentation and its buffer length.
Testing procedure
Please don't, it's annoying to forget everything written above and go through the debug steps once more with the updated documentation. Instead, just look at the documentation and ponder whether it'd have cut the chase short.
Further processing
The warning can go away once the ISR queue mechanism for stdin is replaced with something that has backpressure, so that (in the concrete case where it was discovered) the USB CDC-ACM can start sending NACKs until the stdin buffer has been read into the command line buffer (or we switch completely to something where stdin either has a configured receive buffer that is switched around fast enough, and the interrupts write into that directly).