Skip to content

sys/shell: cancel current line on CTRL-C.#11004

Merged
kaspar030 merged 4 commits intoRIOT-OS:masterfrom
jcarrano:shell_cancel_line_ctrl-c
Aug 16, 2019
Merged

sys/shell: cancel current line on CTRL-C.#11004
kaspar030 merged 4 commits intoRIOT-OS:masterfrom
jcarrano:shell_cancel_line_ctrl-c

Conversation

@jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Feb 12, 2019

Contribution description

Just like in a regular UNIX terminal, control-C now cancels the current line. This is a suggestion of @benemorius.

This is useful if one is using a dumb terminal to communicate with a node, as it saves having to repeatedly type backspace to discard the current line. It also helps when connecting to an already running node,
as one does not know what is on the line buffer, the safest thing to do is to begin by sending a ctrl-C.

Other changes

To be able to test this I had to do a couple of modifications to tests/shell.

  • Fix the use of python dictionaries: Python dictionaries are not guaranteed to be ordered until version
    3.7. In 3.6 they are ordered too, but that is an implementation detail. riotdocker seems to be using 3.5. The commands in this test were stored in a dict.

  • Use miniterm instead of pyterm: pyterm messes up control characters, so the test was changed to use
    miniterm.py instead.

Testing procedure

I have included a test. It should work both on a board or in native:

$ cd tests/shell
$ BOARD=samr21-xpro make all
$ BOARD=samr21-xpro make flash
$ BOARD=samr21-xpro make test

If testing by hand:

  • On a board, be sure to use a terminal that forwards ctrl-C, otherwise you will not be testing the thing. This means no pyterm.
  • On native, use Ctrl-V+ctrl-C to insert a literal ETX, otherwise you will be sending a SIGINT.

Issues/PRs references

Depends on #11003 for testing.
Part of #10994 .
Split from #10788 .

@jcarrano jcarrano added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 12, 2019
@jcarrano jcarrano force-pushed the shell_cancel_line_ctrl-c branch from e8ac6a8 to 968c720 Compare February 12, 2019 16:10
@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 12, 2019
@miri64
Copy link
Member

miri64 commented Feb 12, 2019

Doesn't work on native ;-)

@jcarrano
Copy link
Contributor Author

Doesn't work on native ;-)

It works if you do Ctrl-V Ctrl-C, which is what the test script does. Otherwise the native port catches it as a SIGINT and the process exits. That can be changed if it makes sense.

@miri64
Copy link
Member

miri64 commented Feb 12, 2019

Nope, when I try it in examples/gnrc_networking with ping6 -c 10000 fe80::... (where the address is the address of another node or the TAP bridge), I'm not able to cancel it.

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Apart from conditionally changing "RIOT_TERMINAL", please split out both the miniterm and dict-to-tuple commits. They should be very fast to review.

@benemorius
Copy link
Member

Nope, when I try it in examples/gnrc_networking with ping6 -c 10000 fe80::... (where the address is the address of another node or the TAP bridge), I'm not able to cancel it.

I didn't test this yet but it sounds like you're thinking this should terminate a running process with ^C (that's what #10715 is about) which would be lovely too, but this PR is instead about clearing the command you're typing to get a fresh/empty prompt to replace one that has some partial or wrong command typed in. It's a little thing but it's a very nice thing because the shells we're all accustomed to using on *nix behave this way so it improves our shell UX considerably.

In practice it looks like this:

> pin6 26^C
> ping6 2600::
...

@jcarrano
Copy link
Contributor Author

jcarrano commented Mar 4, 2019

I'm realising now that this is not the best way to implement this feature (it is now wrong either.)

The right thing to do here would be to implement a sort of "line discipline" in stdio_uart. It would be the correct place to implement:

Of course, stdio_uart would get bigger and more complex but it would probably get offset by the decrease in complexity of the shell code that handles this at the moment. In addition to that, the functionality would be available to any shell command (for free) and if it is considered too heavy, an alternative implementation of stdio_uart without the magic can be provided.

By the way, the line discipline is the place to put stuff like ethos/slip (this is what's done in unix) though ideally in a separate module and not patched with #ifdefs.

@miri64
Copy link
Member

miri64 commented Mar 7, 2019

but it sounds like you're thinking this should terminate a running process with ^C (that's what #10715 is about) which would be lovely too, but this PR is instead about clearing the command you're typing to get a fresh/empty prompt to replace one that has some partial or wrong command typed in.

Yepp, I misunderstood the purpose of this PR. Sorry for the noise!

For test scripts, a terminal that does not modify the input and output
streams, configured without local echo, is preferred as it ensures the
test setup is introducing as little noise as possible.
CTRL-C cancels the current line, similar to how getty works.

This is useful if one is using a dumb terminal to communicate with
a node, as it saves having to repeatedly type backspace to discard the
current line. It also helps when connecting to an already running node,
as one does not know what is on the line buffer, the safest thing to do
is to begin by sending a ctrl-C.

This is a suggestion of @benemorius.
Python dictionaries are not guaranteed to be ordered until version
3.7. In 3.6 they are ordered too, but that is an implementation
detail. riotdocker seems to be using 3.5.

As it stands now, it would not be a problem if the test commands
are run in a random order, except that:

- It would result in non-reproduceable tests.
- It hinders testing other functionality, such as exiting the shell.
@jcarrano jcarrano force-pushed the shell_cancel_line_ctrl-c branch from 968c720 to 21a4cec Compare August 15, 2019 15:15
@jcarrano jcarrano added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 15, 2019
@jcarrano
Copy link
Contributor Author

Now that socat is available as a RIOT_TERMINAL it rebased and used it in the test.

@jcarrano jcarrano force-pushed the shell_cancel_line_ctrl-c branch from 21a4cec to 2946f4b Compare August 15, 2019 15:35
Send garbage, cancel it and issue a valid command. No errors should
ocurr.
@kaspar030 kaspar030 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 15, 2019
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Tested on a nucleo-072rb, works like a charm.
native is more troublesome, but that's expected.

ACK.

@kaspar030 kaspar030 merged commit 913614e into RIOT-OS:master Aug 16, 2019
@cladmi
Copy link
Contributor

cladmi commented Aug 26, 2019

This breaks on boards that cannot run when configured with RIOT_TERMINAL != pyterm

git grep TERMFLAGS boards/common/msba2/ boards/lobaro-lorabox/
boards/common/msba2/Makefile.include:TERMFLAGS += -tg -p "$(PORT)"
boards/lobaro-lorabox/Makefile.include:TERMFLAGS +=  --set-rts 0

@cladmi
Copy link
Contributor

cladmi commented Aug 26, 2019

If it expects a clean terminal output, it will, I think, also fail on all boards using stdio_rtt.

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
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 CI: run tests If set, CI server will run tests on hardware for the labeled PR 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.

6 participants