Skip to content

sys/shell: Add function to call the shell with a list of strings.#9733

Closed
jcarrano wants to merge 5 commits intoRIOT-OS:masterfrom
jcarrano:shell-call
Closed

sys/shell: Add function to call the shell with a list of strings.#9733
jcarrano wants to merge 5 commits intoRIOT-OS:masterfrom
jcarrano:shell-call

Conversation

@jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Aug 7, 2018

Contribution description

The new function makes it easier to call command line tools from within code.

I'm using it to provide a sort of system()/execl() call from within Lua.

Related PRs

Depends on #10788 to integrate the tests into the main shell tests.

@miri64 miri64 requested a review from kaspar030 August 7, 2018 18:59
@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Oct 17, 2018
@tcschmidt tcschmidt requested a review from danpetry December 23, 2018 16:03
Copy link
Contributor

@danpetry danpetry left a comment

Choose a reason for hiding this comment

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

Re the high level aspects of the PR:

  1. Could you please provide a test?
  2. Could you please explain briefly why you want to provide a Lua binding to the shell rather than binding to the modules individually, and accessing their commands directly? (Please correct me if my understanding here is inaccurate)

@jcarrano
Copy link
Contributor Author

@danpetry I added the test. The rebase was unintended (our awesome static-test script did it.)

@jcarrano
Copy link
Contributor Author

Could you please explain briefly why you want to provide a Lua binding to the shell rather than binding to the modules individually, and accessing their commands directly? (Please correct me if my understanding here is inaccurate)

Because I won't write individual bindings to modules (other than as an example). This will allow me to provide a system()-like (lua has os.execute but that takes a single string.)

The reason for providing such a function from lua is convenience: having an easy to use example application without having to add a lot of code.

@kaspar030
Copy link
Contributor

LGTM.

How about merging the test with tests/shell?


include $(RIOTBASE)/Makefile.include

test:
Copy link
Contributor

Choose a reason for hiding this comment

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

This target is not needed anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

anyhow, please merge the test into tests/shell. They're testing the same module, and tests are kinda expensive for the CI...

Right now the only way to exit the shell is if stdin is closed. This
works on native, but on an embedded platform stdin is the uart and thus
is never closed.

This patch causes the shell loop to exit on:

* ETX (ASCII 0x03 / ctrl-C) "End-of-Text".
* EOT (ASCII 0x04 / ctrl-D) "End-of-Transmission".
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.
pyterm messes up control characters, so the test was changed to use
miniterm.py instead.
The new function makes it easier to call command line tools from within
code.
@jcarrano jcarrano added the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 16, 2019
@jcarrano
Copy link
Contributor Author

I rebased on top of #10788 so that tests can be more easily integrated.

@jcarrano
Copy link
Contributor Author

I'm not rebasing this (yet) again. Closing.

@jcarrano jcarrano closed this Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: waiting for other PR State: The PR requires another PR to be merged first 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.

4 participants