Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/native: use async read for stdio_read() #19002

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Dec 1, 2022

Contribution description

The real_read() function will block the thread but won't preempt it.
That means all other threads on the same (or higher) priority level are blocked as RIOT still considers the thread that called stdio_read() as running.

Use async_read/isrpipe to properly block the thread when reading from stdin.

Testing procedure

Run a 2nd thread with the same priority as the main thread:

--- a/tests/shell/main.c
+++ b/tests/shell/main.c
@@ -132,10 +132,28 @@ static int _xfa_test2(int argc, char **argv)
 SHELL_COMMAND(xfa_test2, "xfa test command 2", _xfa_test2);
 SHELL_COMMAND(xfa_test1, "xfa test command 1", _xfa_test1);
 
+#include "thread.h"
+#include "ztimer.h"
+
+static char _stack[1024];
+
+static void *_func(void *arg)
+{
+    while (1) {
+        ztimer_sleep(ZTIMER_MSEC, 1000);
+        puts(arg);
+    }
+
+    return NULL;
+}
+
 int main(void)
 {
     printf("test_shell.\n");
 
+    thread_create(_stack, sizeof(_stack), THREAD_PRIORITY_MAIN, THREAD_CREATE_STACKTEST,
+                  _func, "Hello shell", "periodic");
+
     /* define own shell commands */
     shell_run_once(shell_commands, line_buf, sizeof(line_buf));

On master the periodic "Hello shell" message is never printed.

Issues/PRs references

fixes #16834

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: native Platform: This PR/issue effects the native platform labels Dec 1, 2022
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Dec 1, 2022
@benpicco benpicco requested a review from kfessel December 1, 2022 17:04
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 2, 2022
@riot-ci
Copy link

riot-ci commented Dec 2, 2022

Murdock results

FAILED

41ce809 sys/net/telnet: align API with stdio API

Build failures (10)
Application Target Toolchain Runtime (s) Worker
tests/pkg/libschc native64 llvm 290.08 comsys-minion-0
tests/pkg/libschc native llvm 290.10 mobi7
tests/pkg/libschc native gnu 290.01 mobi7
tests/pkg/libschc native64 gnu 290.11 sarajevo
tests/sys/congure_quic native64 gnu 290.01 breeze
tests/sys/congure_quic native llvm 290.01 mobi3
tests/sys/congure_quic native64 llvm 290.10 mobi6
tests/sys/congure_quic native gnu 290.08 comsys-minion-0
tests/sys/congure_test native gnu 290.10 mobi6
tests/sys/congure_test native64 llvm 290.10 mobi6
Test failures (21)
Application Target Toolchain Runtime (s) Worker
tests/periph/flashpage native64 gnu 0.00 mobi6
tests/periph/gpio native64 gnu 0.00 mobi6
tests/periph/gpio native llvm 0.00 mobi6
tests/pkg/flatbuffers native gnu 0.00 mobi6
tests/pkg/uzlib native64 gnu 0.00 mobi6
tests/pkg/uzlib native64 llvm 0.00 mobi3
tests/sys/atomic_utils native gnu 0.00 mobi6
tests/sys/atomic_utils native64 gnu 0.00 mobi6
tests/sys/atomic_utils native llvm 0.00 mobi6
tests/sys/cpp11_mutex native gnu 0.00 comsys-minion-0
tests/sys/rng native gnu 0.00 breeze
tests/sys/rng native llvm 0.00 comsys-minion-0
tests/sys/rng native64 gnu 0.00 mobi6
tests/sys/senml_cbor native64 llvm 0.00 sarajevo
tests/sys/senml_cbor native64 gnu 0.00 mobi3
tests/sys/senml_cbor native gnu 0.00 mobi6
tests/sys/senml_cbor native llvm 0.00 mobi6
tests/sys/senml_phydat native gnu 0.00 mobi6
tests/sys/senml_phydat native llvm 0.00 breeze
tests/sys/struct_tm_utility native64 gnu 0.00 mobi6

and 57 more test failures...

Artifacts

@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework labels Dec 2, 2022
@benpicco benpicco requested a review from maribu December 2, 2022 15:22
@benpicco
Copy link
Contributor Author

benpicco commented Dec 2, 2022

@leandrolanzieri do you have an idea how to solve the Kconfig issue?

@github-actions github-actions bot added Area: sys Area: System and removed Area: Kconfig Area: Kconfig integration labels Dec 2, 2022
@github-actions github-actions bot added the Area: examples Area: Example Applications label Dec 2, 2022
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works as expected.

@gschorcht
Copy link
Contributor

Please squash.

@benpicco
Copy link
Contributor Author

CI will still fail as some tests that use float will now preempt, which triggers #495

@github-actions github-actions bot added the Area: pkg Area: External package ports label Jun 12, 2023
@github-actions github-actions bot added Area: network Area: Networking Area: build system Area: Build system Area: drivers Area: Device drivers Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: USB Area: Universal Serial Bus labels Feb 8, 2024
benpicco and others added 7 commits February 9, 2024 19:07
The real_read() function will block the thread but won't preempt it.
That means all other thereads on the same (or higher) priority level
are blocked as RIOT still consideres the thread that called stdio_read()
as running.

Use async_read/isrpipe to properly block the thread when reading from
stdin.
It's no longer working on native, but native behaves more like a
real board now.
@github-actions github-actions bot removed Area: network Area: Networking Area: drivers Area: Device drivers Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: USB Area: Universal Serial Bus labels Feb 9, 2024
@github-actions github-actions bot added the Area: network Area: Networking label Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

native getchar is blocking RIOT
4 participants