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

sys/shell: native: stop RIOT when the shell reads EOF #13675

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

benpicco
Copy link
Contributor

Contribution description

Currently when the shell receives EOF on native, sending EOF
(ctrl + D) closes the stdin file descriptor, leading to all
consecutive reads to also return EOF.

The result is that shell_run_forever() will re-start the shell
forever in a loop, filling up the terminal with

> > > > > > > > > > > > > > > > > > > > > > …

until the user hits ctrl + C.

This is annoying.

Instead, cleanly shutdown RIOT when receiving EOF on native, which
should match the expected behaviour of most users.

Testing procedure

Run any native example.
Press ctrl + D.

Issues/PRs references

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: native Platform: This PR/issue effects the native platform CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 21, 2020
sys/shell/shell.c Outdated Show resolved Hide resolved
@kaspar030
Copy link
Contributor

How about CONFIG_SHELL_SHUTDOWN_ON_EOF?

@benpicco
Copy link
Contributor Author

Sure can change it - I just wanted to be consistent with SHELL_NO_ECHO and SHELL_NO_PROMPT

@kaspar030
Copy link
Contributor

kaspar030 commented Mar 21, 2020

Sure can change it - I just wanted to be consistent with SHELL_NO_ECHO and SHELL_NO_PROMPT

ok, consistency is good. edit I mean, ok to leave as is.

@benpicco benpicco added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 21, 2020
@aabadie
Copy link
Contributor

aabadie commented Mar 22, 2020

There is #10788 which is also related to this (it seems, I haven't checked in details) but it provides a different approach.

sys/shell/shell.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor Author

@aabadie #10788 is for making the shell on the target exit on ctrl-d.
On native this is already turned into closing of stdin by the C library/the kernel, so the shell never sees the raw EOT.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 26, 2020
@benpicco benpicco force-pushed the native_eof_exit branch 2 times, most recently from b25d2a5 to d33a604 Compare April 2, 2020 17:50
@fjmolinas
Copy link
Contributor

fjmolinas commented Apr 22, 2020

BTW I don't get the same result, My shell doesn't exit but there is not loop either it just keeps printing the prompt the amount of time CNTRL+D is pressed:

RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2020.07-devel-108-gcac35-HEAD)
test_shell.
> > > > > 

> heelp
heelp
shell: command not found: heelp
> 

fjmolinas
fjmolinas previously approved these changes Jun 23, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

Tested the PR rebased on master. Please rebase!

  • master:
main(): This is RIOT! (Version: 2020.07-devel-1415-gae95e-HEAD)
test_shell.
> > > > > 

> 

> 

> > > > 

> 

  • pr
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2020.04-devel-1867-g2992d9-pr-13675)
test_shell.
> 
native: exiting
make: Leaving directory '/home/francisco/workspace/RIOT/tests/shell'

@fjmolinas fjmolinas self-assigned this Jun 23, 2020
@benpicco
Copy link
Contributor Author

On a second thought, how about we do

diff --git a/sys/include/shell.h b/sys/include/shell.h
index 234fe80b35..1045325dfb 100644
--- a/sys/include/shell.h
+++ b/sys/include/shell.h
@@ -21,6 +21,7 @@
 #define SHELL_H
 
 #include <stdint.h>
+#include "periph/pm.h"
 
 #include "kernel_defines.h"
 
@@ -97,7 +98,12 @@ static inline void shell_run_forever(const shell_command_t *commands,
 static inline void shell_run(const shell_command_t *commands,
                              char *line_buf, int len)
 {
+#ifdef CPU_NATIVE
+    shell_run_once(commands, line_buf, len);
+    pm_off();
+#else
     shell_run_forever(commands, line_buf, len);
+#endif
 }
 
 #ifdef __cplusplus

@benpicco benpicco dismissed fjmolinas’s stale review June 23, 2020 11:47

alternative solution proposed

sys/include/shell.h Outdated Show resolved Hide resolved
sys/include/shell.h Outdated Show resolved Hide resolved
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 23, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

I'm unable to reproduce the described behaviour locally but @aabadie gets the same behviour. Not sure where the difference comes from, but I'm OK with this temporary fix. The proper fix IMO would be to not have native shell running in cookmode.

@benpicco feel free to squash when you add the comment.

@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 Jun 30, 2020
@fjmolinas
Copy link
Contributor

@benpicco the commit you reverted is still present!

Currently when the shell receives EOF on `native`, sending EOF
(ctrl + D) closes the stdin file descriptor, leading to all
consecutive reads to also return EOF.

The result is that `shell_run_forever()` will re-start the shell
forever in a loop, filling up the terminal with

    > > > > > > > > > > > > > > > > > > > > > > …

until the user hits ctrl + C.

This is annoying.

Instead, cleanly shutdown RIOT when receiving EOF on native, which
should match the expected behaviour of most users.
@benpicco
Copy link
Contributor Author

Good catch, dropped it.

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK.

@fjmolinas fjmolinas merged commit 960cca7 into RIOT-OS:master Jun 30, 2020
@benpicco benpicco deleted the native_eof_exit branch June 30, 2020 13:30
@miri64 miri64 added this to the Release 2020.07 milestone Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants