sys/stdio: add stdio_notify to emit callback on write#22429
Conversation
|
The failing test is likely a consequence of what #22424 fixes. |
mguetschow
left a comment
There was a problem hiding this comment.
Thanks, I think this is a very useful addition - I've run into similar problems before while using the RIOT shell module which also unconditionally blocks right now. May be a nice follow-up to enhance the shell module so that it does not necessarily block the main thread from doing anything else.
Regarding your three choices:
- I did not implement this as part of
isrpipe, which I could have done too. With #22426 providingisrpipe_available, this would be feasible. However, if you have the option to choose forisrpipeto begin with, then there are probably other methods to solve the same issue. With thestdiomodule, you cannot. It keepsisrpipelightweight.
Agree to your reasoning.
- I only add support for a single callback, because stdin is typically read by a single consumer.
True, and it wouldn't be very helpful for a second consumer to be woken up if stdin is drained by the time it is scheduled.
- I debated whether it should be called
stdio_notifyorstdin_notify, but given thatstdio_availableis not calledstdio_stdin_available, I went withstdio_notify.
Also +1
I haven't tested it yet, so no formal approval yet - and we should definitely hold this back until after hard feature freeze anyhow.
Route all received stdin data through new stdio_rx_write()/ stdio_rx_write_one() helpers and add stdio_set_notify(), which registers a callback fired from those helpers once data is pushed to stdin. This also deprecates the direct use of isrpipe_stdin, which improves on hiding implementation details as well. The callback must assume to run in ISR context and must be non-blocking. It is gated behind the stdio_notify pseudomodule and adds no overhead when the module is not used. Typical use-case for this feature is a consumer thread that also needs to wake up on other sources, for instance on thread flags. With this module, the consumer can register an appropriate callback, and handle it accordingly.
Replace direct access to stdin_isrpipe with new helpers, which also take care of the notification callback when enabled.
This application demonstrates the main/intended use of the stdio_notify module: one consumer that has multiple reasons to be unblocked while reading from stdio.
717edc4 to
7fb3488
Compare
I guess you meant to post this at #22424? |
It was too late yesterday, and too early this morning ;-P |
Contribution description
For #22350 I need to be able to execute scheduled work on the same thread that runs and executes the REPL. With the
stdiomodule proving anstdin_isrpipe, this is not possible, unless you run a second thread to read and rebuffer, then waking up the main thread with something like thread flags. This definitely costs more memory and flash than this proposed solution.After some experimentation, I came up with
stdio_notify. This provides a way to register a write callback, that can be used in an application for aforementioned reason.I made three deliberate choices (which are not set in stone though):
isrpipe, which I could have done too. With sys/isrpipe: add convenience helpers to improve sys/stdio #22426 providingisrpipe_available, this would be feasible. However, if you have the option to choose forisrpipeto begin with, then there are probably other methods to solve the same issue. With thestdiomodule, you cannot. It keepsisrpipelightweight.stdio_notifyorstdin_notify, but given thatstdio_availableis not calledstdio_stdin_available, I went withstdio_notify.stdiobackends now have to route incoming bytes viastdio_rx_write()/stdio_rx_write_one(). Whenstdio_notifyis disabled, it inlines directly. Public access tostdin_isrpipehas been marked as deprecated, and writing to it directly will not invoke the callback (which is still backwards compatible when disable). I set it to 2027.01 for now.Testing procedure
A test application has been provided in
tests/sys/stdio_notify, which exactly shows one of its main intended uses.Issues/PRs references
This PR builds for
stdio_uart, but not for others due to #22424. #22426 was split off as another improvement.Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: