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

subsys: shell rework #9362

Merged
merged 10 commits into from Sep 19, 2018

Conversation

@jarz-nordic
Collaborator

jarz-nordic commented Aug 9, 2018

Me and @nordic-krch have reworked Zephyr shell Task 8872.
New one has a lot of useful features that were missing in the old one.

We have changed commands concept.

  1. Shell commands are now organized in a tree structure and grouped into the following types:
  • Root command (level 0): Kept in the section variable area. Root command is always static.
  • Static subcommand (level > 0): Number and syntax must be known during compile time. Kept in the software module.
  • Dynamic subcommand (level > 0): Number and syntax does not need to be known during compile time. Created in the software module.

On one command level all commands must be the same type: static or dynamic.
An example usage of dynamic commands concept is described here:
https://devzone.nordicsemi.com/b/blog/posts/new-command-line-interface-part-1

  1. Commands execution
    Each command or subcommand on each level can either have or not have a handler. Shell executes the handler that is found deepest in the command tree and further subcommands (without a handler) are passed as arguments. Characters within parentheses are treated as one argument. Each command or subcommand without a handler can be treated as an argument, or it can still have a subcommand with a handler and there is no problem for the user to execute it.
    It is recommended to use subcommands over options. Options apply mainly in the case when an argument with '-' or "--" is requested.

  2. Commands help
    Every user-defined command, subcommand, or option can have its own help description. The help for commands and subcommands can be created with respective macros: SHELL_CMD_REGISTER, SHELL_CMD. In addition, you can define options for commands or subcommands using the macro SHELL_OPT. By default, each and every command or subcommand has these two options implemented: "-h" and "--help".
    In order to add help functionality to a command or subcommand, you must call the help function shell_help_print inside of a command handler.

New features:

  • Multi-instance - you can have independent shells on different transmission mediums

  • Multiline commands - Shell will correctly print and allow to edit commands longer than 1 line.

  • Integration with Log module - you can read logs on the shell screen, filter them dynamically (activate logs only for needed modules), suspend or resume. At the same time you can type, edit or execute a command.

  • Smart completion with the Tab key. One can prompt and partially/fully complete commands or its subcommands.

  • Commands history - Executed commands can be listed by history command or recalled with up / down arrows.

  • Built-in commands - Shell related commands already implemented and available for the user:

  1. clear - clears the screen
  2. history - prints recently called commands
  3. resize - command shall be called each time terminal width has changed. It will ensure correct text formatting (currently it is only working with UART flow control on)
  4. shell colors - switch on / off colored syntax
  5. shell echo - switch on / off shell echo
  6. shell backspace_mode - allows to adjust backspace escape code during runtime.
  7. shell stats - gives an information about lost (not printed) logs
  • Easy command edition with buttons: Tab/Backspace/Delete/Arrows/Home/End

  • Meta Keys:

  1. ctrl + a - Moves the cursor to the beginning of the line.
  2. ctrl + c - Preserves the last command on the screen and starts a new command in a new line.
  3. ctrl + e - Moves the cursor to the end of the line.
  4. ctrl + l - Clears the screen and leaves the currently typed command at the top of the screen.
  5. ctrl + u - Clears the currently typed command.
  6. ctrl + w - Removes the word or part of the word to the left of the cursor. Words separated by period instead of space are treated as one word.
  • Wildcards support for: * and ?

  • Kconfig configuration to optimize resources usage.

@jarz-nordic

This comment has been minimized.

Show comment
Hide comment
@jarz-nordic
Collaborator

jarz-nordic commented Aug 9, 2018

@nordic-krch

This comment has been minimized.

Show comment
Hide comment
@nordic-krch

nordic-krch Aug 9, 2018

Collaborator

@jukkar @jhedberg please take a look. Shell is ready thus bluetooth and network commands adaptation can start. Checkout samples/subsys/shell to see how to port.

Collaborator

nordic-krch commented Aug 9, 2018

@jukkar @jhedberg please take a look. Shell is ready thus bluetooth and network commands adaptation can start. Checkout samples/subsys/shell to see how to port.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 9, 2018

Codecov Report

Merging #9362 into master will decrease coverage by 0.76%.
The diff coverage is 38.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9362      +/-   ##
==========================================
- Coverage   52.94%   52.18%   -0.77%     
==========================================
  Files         214      212       -2     
  Lines       26163    25924     -239     
  Branches     5639     5570      -69     
==========================================
- Hits        13853    13528     -325     
- Misses      10037    10133      +96     
+ Partials     2273     2263      -10
Impacted Files Coverage Δ
subsys/shell/shell_service.c 100% <ø> (ø) ⬆️
include/misc/dlist.h 92.59% <ø> (ø) ⬆️
subsys/logging/log_core.c 67.66% <0%> (-2.57%) ⬇️
subsys/logging/log_output.c 2.64% <0%> (-62.96%) ⬇️
subsys/shell/legacy_shell.c 47.16% <47.16%> (ø)
subsys/random/rand32_timer.c 0% <0%> (-100%) ⬇️
lib/cmsis_rtos_v1/cmsis_kernel.c 0% <0%> (-80%) ⬇️
lib/cmsis_rtos_v1/cmsis_msgq.c 35.89% <0%> (-39.72%) ⬇️
lib/cmsis_rtos_v1/cmsis_mailq.c 50% <0%> (-30.77%) ⬇️
subsys/net/ip/ipv6.c 37.09% <0%> (-21.85%) ⬇️
... and 91 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 732b45c...9b8682c. Read the comment docs.

codecov-io commented Aug 9, 2018

Codecov Report

Merging #9362 into master will decrease coverage by 0.76%.
The diff coverage is 38.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9362      +/-   ##
==========================================
- Coverage   52.94%   52.18%   -0.77%     
==========================================
  Files         214      212       -2     
  Lines       26163    25924     -239     
  Branches     5639     5570      -69     
==========================================
- Hits        13853    13528     -325     
- Misses      10037    10133      +96     
+ Partials     2273     2263      -10
Impacted Files Coverage Δ
subsys/shell/shell_service.c 100% <ø> (ø) ⬆️
include/misc/dlist.h 92.59% <ø> (ø) ⬆️
subsys/logging/log_core.c 67.66% <0%> (-2.57%) ⬇️
subsys/logging/log_output.c 2.64% <0%> (-62.96%) ⬇️
subsys/shell/legacy_shell.c 47.16% <47.16%> (ø)
subsys/random/rand32_timer.c 0% <0%> (-100%) ⬇️
lib/cmsis_rtos_v1/cmsis_kernel.c 0% <0%> (-80%) ⬇️
lib/cmsis_rtos_v1/cmsis_msgq.c 35.89% <0%> (-39.72%) ⬇️
lib/cmsis_rtos_v1/cmsis_mailq.c 50% <0%> (-30.77%) ⬇️
subsys/net/ip/ipv6.c 37.09% <0%> (-21.85%) ⬇️
... and 91 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 732b45c...9b8682c. Read the comment docs.

@Qbicz

Great work. I like the dynamic command sample. Some minor comments from me.

u32_t dynamic_lvl;
u32_t compiled_lvl;
u32_t i;

This comment has been minimized.

@Qbicz

Qbicz Aug 9, 2018

Contributor

Excess empty line, please also check between functions.

@Qbicz

Qbicz Aug 9, 2018

Contributor

Excess empty line, please also check between functions.

Show outdated Hide outdated subsys/logging/log_cmds.c
static void cmd_log_backends_list(const struct shell *shell,
size_t argc, char **argv)
{
int backend_count;

This comment has been minimized.

@Qbicz

Qbicz Aug 9, 2018

Contributor

Unsigned or size_t

@Qbicz

Qbicz Aug 9, 2018

Contributor

Unsigned or size_t

@jarz-nordic jarz-nordic changed the title from Shell rework to subsys: shell rework Aug 10, 2018

@nashif nashif added the area: Shell label Aug 10, 2018

@nashif

This comment has been minimized.

Show comment
Hide comment
@nashif

nashif Aug 11, 2018

Member

few quick observations:

  • backspace does not work
  • works fine with qemu_cortex_m3, not with qemu_x86
  • fails to build with native_posix

Otherwise, it looks great :)

Member

nashif commented Aug 11, 2018

few quick observations:

  • backspace does not work
  • works fine with qemu_cortex_m3, not with qemu_x86
  • fails to build with native_posix

Otherwise, it looks great :)

@jarz-nordic

This comment has been minimized.

Show comment
Hide comment
@jarz-nordic

jarz-nordic Aug 11, 2018

Collaborator

@nashif thank you for checking it :)

In order to make backspace work (in PuTTY) you need to change terminal settings:
PuTTY -> Terminal->Keybord -> change"Control-?" to " Control-H"
I will fix it to be more universal.

Now we will work to make tests green :)

Collaborator

jarz-nordic commented Aug 11, 2018

@nashif thank you for checking it :)

In order to make backspace work (in PuTTY) you need to change terminal settings:
PuTTY -> Terminal->Keybord -> change"Control-?" to " Control-H"
I will fix it to be more universal.

Now we will work to make tests green :)

@nashif

This comment has been minimized.

Show comment
Hide comment
@nashif

nashif Aug 12, 2018

Member

@jarz-nordic using screen here, not putty. it works with current shell implementation without having to change anything.

Member

nashif commented Aug 12, 2018

@jarz-nordic using screen here, not putty. it works with current shell implementation without having to change anything.

@jarz-nordic

This comment has been minimized.

Show comment
Hide comment
@jarz-nordic

jarz-nordic Aug 12, 2018

Collaborator

@nashif I guess that currently for your terminal del and backspace are behaving in the same way.
These settings will make not possible to have del and backspace at the same time

Collaborator

jarz-nordic commented Aug 12, 2018

@nashif I guess that currently for your terminal del and backspace are behaving in the same way.
These settings will make not possible to have del and backspace at the same time

@jarz-nordic

This comment has been minimized.

Show comment
Hide comment
@jarz-nordic

jarz-nordic Aug 13, 2018

Collaborator

@nashif I have reproduced the issue with "screen" application.
This is limitation of "screen", it is sending the same VT100 code for delete and backspace button.
I see three solutions for this problem:

  1. Implement new shell in the same way as old one. This will exclude "delete" button - well it will behave in the same way as backspace button.
  2. Implement a command in the shell to work in mode where delete and backspace are treated in the same way.
  3. You can fix the problem on terminal side:
    a) Create a file: $HOME/.screenrc
    b) add following line inside this file: bindkey -d -k kb stuff "\010"
    This was proposed here: https://bugs.launchpad.net/ubuntu/+source/vte/+bug/29787/comments/13
    I tested 3. and it is working fine for me.
Collaborator

jarz-nordic commented Aug 13, 2018

@nashif I have reproduced the issue with "screen" application.
This is limitation of "screen", it is sending the same VT100 code for delete and backspace button.
I see three solutions for this problem:

  1. Implement new shell in the same way as old one. This will exclude "delete" button - well it will behave in the same way as backspace button.
  2. Implement a command in the shell to work in mode where delete and backspace are treated in the same way.
  3. You can fix the problem on terminal side:
    a) Create a file: $HOME/.screenrc
    b) add following line inside this file: bindkey -d -k kb stuff "\010"
    This was proposed here: https://bugs.launchpad.net/ubuntu/+source/vte/+bug/29787/comments/13
    I tested 3. and it is working fine for me.
@pfalcon

This comment has been minimized.

Show comment
Hide comment
@pfalcon

pfalcon Aug 13, 2018

Member

Implement new shell in the same way as old one.

Please implement terminal line editing capabilities in a way that they just work on a typical Linux system (like thousands of other tools there work). Thanks.

Member

pfalcon commented Aug 13, 2018

Implement new shell in the same way as old one.

Please implement terminal line editing capabilities in a way that they just work on a typical Linux system (like thousands of other tools there work). Thanks.

@jarz-nordic

This comment has been minimized.

Show comment
Hide comment
@jarz-nordic

jarz-nordic Aug 13, 2018

Collaborator

@pfalcon this is what I did. In standard Linux shell you have possibility to use Backspace and Delete buttons and they realize different functions :).

Old shell implementation assumes that Backspace and Delete are sending the same escape code so you have no possibility to realize different functions for them.

There are many terminals and each is behaving in different way. For instance "minicom" is working correctly out of the box: Backspace and Delete are sending different (and correct) escape codes.

Screen out of the box is sending the same escape code for Backspace and Delete button (incorrect behaviour). To fix the Screen application you need to do what I've proposed.

In my opinion the best solution is to combine point 2 and 3. I can implement a command to configure shell in the "old mode" where Backspace and Delete are not distinguished.

Collaborator

jarz-nordic commented Aug 13, 2018

@pfalcon this is what I did. In standard Linux shell you have possibility to use Backspace and Delete buttons and they realize different functions :).

Old shell implementation assumes that Backspace and Delete are sending the same escape code so you have no possibility to realize different functions for them.

There are many terminals and each is behaving in different way. For instance "minicom" is working correctly out of the box: Backspace and Delete are sending different (and correct) escape codes.

Screen out of the box is sending the same escape code for Backspace and Delete button (incorrect behaviour). To fix the Screen application you need to do what I've proposed.

In my opinion the best solution is to combine point 2 and 3. I can implement a command to configure shell in the "old mode" where Backspace and Delete are not distinguished.

@jarz-nordic jarz-nordic referenced this pull request Aug 13, 2018

Closed

Dlist fix code #9399

@pfalcon

This comment has been minimized.

Show comment
Hide comment
@pfalcon

pfalcon Aug 13, 2018

Member

@jarz-nordic: Ok, thanks for the detailed reply. I hope to give it a try with "picocom" I use (and that's part of the issue - everyone uses their own favorite terminal app, so the shell should "work with any of them", which entails working "as any other app working over serial".) Btw, while an individual terminal app may of course do adhoc processing, I'd assume that overall the key codes they send would be governed by system-wide termcap/terminfo database.

Member

pfalcon commented Aug 13, 2018

@jarz-nordic: Ok, thanks for the detailed reply. I hope to give it a try with "picocom" I use (and that's part of the issue - everyone uses their own favorite terminal app, so the shell should "work with any of them", which entails working "as any other app working over serial".) Btw, while an individual terminal app may of course do adhoc processing, I'd assume that overall the key codes they send would be governed by system-wide termcap/terminfo database.

@nashif

This comment has been minimized.

Show comment
Hide comment
@nashif

nashif Sep 17, 2018

Member

@nordic-krch is this PR still DNM? I would like to merge it ASAP and have module owners work on top of master rather than this PR. What is blocking us from merging it now?

Member

nashif commented Sep 17, 2018

@nordic-krch is this PR still DNM? I would like to merge it ASAP and have module owners work on top of master rather than this PR. What is blocking us from merging it now?

@nordic-krch

This comment has been minimized.

Show comment
Hide comment
@nordic-krch

nordic-krch Sep 17, 2018

Collaborator

@nashif let me clean after last upmerge and we can remove DNM label.

Collaborator

nordic-krch commented Sep 17, 2018

@nashif let me clean after last upmerge and we can remove DNM label.

@nordic-krch

This comment has been minimized.

Show comment
Hide comment
@nordic-krch

nordic-krch Sep 17, 2018

Collaborator

recheck

Collaborator

nordic-krch commented Sep 17, 2018

recheck

@nashif

This comment has been minimized.

Show comment
Hide comment
@nashif

nashif Sep 17, 2018

Member

please add shell_root_cmds_sections to scripts/sanitycheck to have it as whitelisted section.

Member

nashif commented Sep 17, 2018

please add shell_root_cmds_sections to scripts/sanitycheck to have it as whitelisted section.

@jarz-nordic jarz-nordic requested a review from ramakrishnapallala as a code owner Sep 18, 2018

@jarz-nordic

This comment has been minimized.

Show comment
Hide comment
@jarz-nordic

jarz-nordic Sep 18, 2018

Collaborator

@nashif done, shell_root_cmds_sections added.

Collaborator

jarz-nordic commented Sep 18, 2018

@nashif done, shell_root_cmds_sections added.

@carlescufi carlescufi removed the DNM label Sep 18, 2018

@jukkar

jukkar approved these changes Sep 19, 2018

LGTM

nordic-krch added some commits Aug 9, 2018

shell: Rename shell to legacy_shell
New shell implementation is on the way. For now old one and all
references are kept to be gradually replaced by new shell.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
ext: lib: Add fnmatch library
Library will be used by new shell implementation.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Signed-off-by: Jakub Rzeszutko <jakub.rzeszutko@nordicsemi.no>
shell: Shell subsystem reimplementation
New shell support features like:
- multi-instance
- command tree
- static and dynamic commands
- multiline
- help print function
- smart tab (autocompletion)
- meta-keys
- history, wildcards etc.
- generic transport (initially, uart present)

Signed-off-by: Jakub Rzeszutko <jakub.rzeszutko@nordicsemi.no>
Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Signed-off-by: Piotr Zięcik <piotr.ziecik@nordicsemi.no>
samples: subsys: shell: Port sample to new shell subsystem
Ported shell sample to use new shell.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
sys: dlist: Add sys_dlist_peek_prev_no_check and sys_dlist_peek_prev
Added function for peeking into previous item in the list.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
shell: Add shell history feature
Extending shell with terminal-like  history feature.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
shell: Add built-in shell commands
Added optional shell commands:
- clear - for clearing terminal
- history - commands history
- resize - terminal resize
- shell - controling echo and colors

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Signed-off-by: Jakub Rzeszutko <jakub.rzeszutko@nordicsemi.no>
shell: Extend shell as a log backend
Initial logger backend support added to shell.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
logging: Add shell commands
Added commands for getting current status and controlling which log
messages are forwared to available backends.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
shell: Add wildcard support
Extended shell to support wildcard characters: * and ? and expand
commands accordingly.
Increased default stack size.

Signed-off-by: Jakub Rzeszutko <jakub.rzeszutko@nordicsemi.no>
Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@nashif

nashif approved these changes Sep 19, 2018

@nashif nashif merged commit 68249ce into zephyrproject-rtos:master Sep 19, 2018

1 check passed

Shippable Run 22677 status is SUCCESS.
Details

aescolar added a commit to aescolar/zephyr that referenced this pull request Sep 24, 2018

subsys: logger: fix merge error
Fix merge error introduced in:
ba01a39
(as part of #9362)
which deleted the native_posix backend for the logger.

Signed-off-by: Alberto Escolar Piedras <alpi@oticon.com>

carlescufi added a commit that referenced this pull request Sep 24, 2018

subsys: logger: fix merge error
Fix merge error introduced in:
ba01a39
(as part of #9362)
which deleted the native_posix backend for the logger.

Signed-off-by: Alberto Escolar Piedras <alpi@oticon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment