Convert net-shell to use new shell #9825

jukkar · 2018-09-06T13:17:20Z

jakub-uC · 2018-09-07T05:50:27Z

subsys/net/ip/net_shell.c

+
+SHELL_CREATE_STATIC_SUBCMD_SET(net_cmd_nbr)
+{
+	SHELL_CMD(rm, NULL, "Remove neighbor from cache.",


You are defining a help string for this command (and others):

SHELL_CMD(rm, NULL, "Remove neighbor from cache.", cmd_net_nbr_rm),

However in the command handler: cmd_net_nbr_rm you are not calling a function resposible for printing help.
In current command implementation when user will call command:
net nbr rm -h or net nbr rm --help
help string "Remove neighbor from cache." will not be dispayed by shell.

To be able to see help string you need to add following code at the beginning of command halder:

if (shell_help_requested(shell)) { shell_help_print(shell, opt, opt_len); return; }

In that case I would recommend using shell_cmd_precheck() function:

Prints help if request and prints error message on wrong argument count.

Ok, I was wondering how the subcommand help was suppose to be done but missed this one in the sample app.

Hint: Function shell_help_print prints help for existing command and subcommands 1 level below.

I prefere to add this function to each command handler, for either root command or subcommands.

nordic-krch · 2018-09-07T06:14:28Z

subsys/net/ip/net_shell.c

 {
 	enum ethernet_hw_caps caps = net_eth_get_hw_capabilities(iface);
 	int i;

 	for (i = 0; i < ARRAY_SIZE(eth_hw_caps); i++) {
 		if (caps & eth_hw_caps[i].capability) {
-			printk("\t%s\n", eth_hw_caps[i].description);
+			PR("\t%s\n", eth_hw_caps[i].description);


PR is too uninformative to me. Maybe something more decriptive (e.g. SHELL_PRINT)?

Tried to keep the macro name short, the lines are quite long already. I originally had the full shell_fprintf(shell, SHELL_NORMAL, ....); but that was just way too long.

nordic-krch · 2018-09-07T06:21:08Z

subsys/net/ip/net_shell.c

 			if (ret) {
-				printk("not supported\n");
+				PR("not supported\n");


There is dedicated coloring for errors (SHELL_ERROR) and warnings (SHELL_WARNING) in shell. You should use those in error scenarios.

Ok, makes sense.

nordic-krch · 2018-09-07T07:36:48Z

subsys/net/ip/net_shell.c

+	SHELL_CMD(rpl, NULL, "Show RPL mesh routing status.", cmd_net_rpl),
+	SHELL_CMD(stacks, NULL, "Show network stacks information.",
+		  cmd_net_stacks),
+	SHELL_CMD(stats, &net_cmd_stats, "Show network statistics.",


You could solve 'stats' command in more handy way by utilizing dynamic autocompletion. It could have syntax like:

net stats prints stats for all interfaces

net_stats - prints stats for all named interfaces
Very similar approach is taken in log disable command (https://github.com/zephyrproject-rtos/zephyr/blob/f2596952599df87d9dafaa8d692f7f737a0fb3f6/subsys/logging/log_cmds.c#L390)

That would be cool, there are also other commands that could benefit from this.

I have been experimenting with the dynamic commands. I added dynamic completion for net iface up <idx> command. I got it working but it is not very user friendly atm.
In this example I have two network interfaces. When completing the command I see this:

net:~$ net iface up <tab> 0 1

but it is not easy to understand what the values actually mean. For that one needs to add --help string in which case I get

net:~$ net iface up 1 --help up - 'net iface up <iface index>' takes network interface up. Options: -h, --help :Show command help. Subcommands: 0 :Ethernet (0x00421860) 1 :Ethernet (0x00421920)

For user this is quite cumbersome. Why not show the help during completion like this:

net:~$ net iface up <tab> 0 :Ethernet (0x00421860) 1 :Ethernet (0x00421920)

This would be much more user friendly and user could select the proper index immediately. If there is only one interface, then it is selected automatically currently and this is just fine.

So you would like to have help string for promted dynamic commands or all commands?
Or maybe I could prepare special type of dynamic commands printing this help?

I was just setting entry->help = set_iface_index_help(idx); for dynamic command, and was expecting that it's value would be used in completion. Will send new version shortly so you can see how I did this.

That is certainly expected but the output net iface up 0: Ethernet (0x12345678) after pressing the tab would be wrong for this command thus this approach cannot be used here.

Hmm, github does not seem to handle <tab> prints correctly in my comment, it does not print them without quotes.

To be honest I do not see easy solution for now to prompt command syntax + command help but to autocompleate only command name. :(

I will probably remove the dynamic completion support from net-shell as the feature uses lot of memory. Especially the IPv6 neighbor completion uses 240 bytes memory by default. The network interface index completion is using smaller amount of memory but is currently not very user friendly.

Could we use interface names, like eth0, at shell level?
The prefix will depend on interface type (eth = Ethernet) and number for interface index.

I also think, that keeping features like neighbor completion is a good thing. SoC with plenty of RAM will see direct benefit and the smaller SoC will be still OK if we make this option configurable.

Could we use interface names, like eth0, at shell level?

Currently we do not have a concept for interface name in IP stack, if we introduce such a name it would be only for shell which complicates things. This needs to be pondered more, atm there are more important things to do so this is left for later (at least from my part). Patches are welcome of course.

if we make this option configurable.

Yes, I was planning to make the completion configurable and enable it by default.

nordic-krch · 2018-09-07T07:37:46Z

subsys/net/ip/net_shell.c

+
+SHELL_CMD_REGISTER(net, &net_commands, "Networking commands", NULL);
+
+SHELL_UART_DEFINE(shell_transport_uart);


you should leave shell definition to the application. Here only commands should be specified.

Hmm, with net-shell the idea is that the app only needs to set CONFIG_NET_SHELL=y and get a shell automatically. This worked with legacy shell but has things changed with the new shell?

note, that in future there might be multiple transports (net too) and even multiple shell instances available so that approach will not fly for long. We might approach that the other way: add UART as default transport to the shell module.

@nordic-krch The SHELL_DEFINE() macro is a bit problematic as it requires the transport as a parameter. As a user I would just like to set CONFIG_NET_SHELL=y and expecting that net-shell is enabled without any configuration. Could the transport parameter left out or set to NULL? The shell subsystem would then just select the default backend in that case. Setting the backend here looks something that would be better done in some other part of the system.

nordic-krch · 2018-09-07T07:41:18Z

subsys/net/ip/net_shell.c

+
+SHELL_CREATE_STATIC_SUBCMD_SET(net_cmd_nbr)
+{
+	SHELL_CMD(rm, NULL, "Remove neighbor from cache.",


if it is possible you could try to provide dynamic subcommand to be able to use tab to help pick a neighbor.

nordic-krch · 2018-09-07T07:43:14Z

@jukkar, i'm very happy to see this. I've left few comments in the review.

jukkar · 2018-09-07T14:12:38Z

Added dynamic command support for "net iface", "net stats" and "net nbr" commands. Also added color output if there is error/warning/info message printed.

codecov-io · 2018-09-07T14:57:49Z

Codecov Report

Merging #9825 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9825      +/-   ##
==========================================
+ Coverage   53.18%   53.34%   +0.16%     
==========================================
  Files         210      211       +1     
  Lines       25825    25824       -1     
  Branches     5686     5684       -2     
==========================================
+ Hits        13735    13777      +42     
+ Misses       9781     9734      -47     
- Partials     2309     2313       +4

Impacted Files	Coverage Δ
subsys/net/ip/net_private.h	`83.33% <ø> (ø)`	⬆️
include/net/net_if.h	`58.97% <ø> (-1.03%)`	⬇️
subsys/net/ip/net_core.c	`73.48% <100%> (+0.2%)`	⬆️
subsys/net/ip/net_shell.h	`100% <100%> (ø)`
lib/posix/pthread_sched.c	`41.66% <0%> (-20.84%)`	⬇️
subsys/net/ip/net_if.c	`65.49% <0%> (+0.11%)`	⬆️
lib/posix/pthread_mutex.c	`77.77% <0%> (+1.38%)`	⬆️
lib/posix/pthread.c	`80.36% <0%> (+12.32%)`	⬆️
... and 3 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 b307be0...9c92690. Read the comment docs.

jukkar · 2018-09-14T12:58:31Z

Fixed merge conflict.

jukkar · 2018-09-17T08:07:32Z

Rebased against jarz-nordic:shell_rework

jakub-uC · 2018-09-17T08:09:52Z

@jukkar : Currently @nordic-krch is working on jarz-nordic:shell_rework branch to solve all conflicts. I hope it will be ready later today.

jukkar · 2018-10-08T12:53:01Z

Fixed sanity

jukkar · 2018-10-11T11:35:11Z

Updated because of changes in shell APIs.

jakub-uC · 2018-10-11T12:06:26Z

@jukkar : What about *.conf files under this location: tests/bluetooth/shell ?
Maybe not with this PR but I guess they need to be updated

jukkar · 2018-10-11T12:55:02Z

What about *.conf files under this location: tests/bluetooth/shell ?

I hope @Vudentz will look into these. Are you otherwise ok with the PR?

jakub-uC · 2018-10-11T13:00:31Z

@Vudentz : Please update *.conf files under: tests/bluetooth/shell to get rid off old shell dependencies.

Convert net-shell to use the new shell API. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

Use new shell instead of the legacy one. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

There were no shell commands implemented so remove the shell support from this sample application. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

Use new shell instead of the legacy one. This commit also fixes the configuration file mess and allows the program to be run on more hardware. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

ROM consumption is higher with new shell which prevents quark_se_c1000_devboard from building. Remove that board from sanitychecker runs. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

jukkar · 2018-10-12T09:55:10Z

Fixed merge conflict.

@nordic-krch does this look now ok for you?

subsys/net/ip/Kconfig

jukkar added In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on area: Networking area: Shell Shell subsystem DNM This PR should not be merged (Do Not Merge) labels Sep 6, 2018

jukkar requested review from nordic-krch and jakub-uC September 6, 2018 13:17

jukkar requested review from andrewboie, andyross, jhedberg, nashif, pfalcon, sjanc, tarunkum, tbursztyka and Vudentz as code owners September 6, 2018 13:17

nashif added this to the v1.14.0 milestone Sep 6, 2018

jakub-uC reviewed Sep 7, 2018

View reviewed changes

nordic-krch requested changes Sep 7, 2018

View reviewed changes

jukkar force-pushed the net-shell branch from f259695 to b0b4e0e Compare September 7, 2018 14:11

jukkar removed DNM This PR should not be merged (Do Not Merge) In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on labels Sep 10, 2018

nordic-krch mentioned this pull request Sep 12, 2018

New shell subsystem #8872

Closed

jukkar force-pushed the net-shell branch from b0b4e0e to 1af86d1 Compare September 14, 2018 12:58

jukkar force-pushed the net-shell branch from 1af86d1 to 1a2cfeb Compare September 17, 2018 08:05

jukkar force-pushed the net-shell branch from 88d54ed to 286e4d9 Compare October 8, 2018 12:52

jukkar mentioned this pull request Oct 10, 2018

net: Fix assert on net_if_api send for NET_OFFLOAD drivers #10441

Merged

jukkar force-pushed the net-shell branch 3 times, most recently from 9745ea6 to f292c9a Compare October 11, 2018 11:34

jakub-uC approved these changes Oct 11, 2018

View reviewed changes

jakub-uC mentioned this pull request Oct 11, 2018

config shell & bt shell same time cause fail #10166

Closed

jukkar added 9 commits October 12, 2018 12:50

net: shell: Migrate to new shell API

e2367e1

Convert net-shell to use the new shell API. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

net: wifi: Convert wifi shell to use new shell

1904f65

Use new shell instead of the legacy one. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

net: ieee802154: Convert IEEE 802.15.4 shell to use new shell

82d28d9

Use new shell instead of the legacy one. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

net: bt: Convert network Bluetooth shell to use new shell

223842a

Use new shell instead of the legacy one. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

net: openthread: Convert to use new shell

80307c9

Use new shell instead of the legacy one. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

samples: net: rpl_border_router: Convert to use new shell

9d0846d

Use new shell instead of the legacy one. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

samples: net: wpanusb: Removing useless shell

96e553e

There were no shell commands implemented so remove the shell support from this sample application. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

samples: net: zperf: Convert to use the new shell

1cdd478

Use new shell instead of the legacy one. This commit also fixes the configuration file mess and allows the program to be run on more hardware. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

samples: net: http_server: Remove Quark devboard from sanity

9c92690

ROM consumption is higher with new shell which prevents quark_se_c1000_devboard from building. Remove that board from sanitychecker runs. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

jukkar force-pushed the net-shell branch from f292c9a to 9c92690 Compare October 12, 2018 09:54

jakub-uC reviewed Oct 12, 2018

View reviewed changes

subsys/net/ip/Kconfig Show resolved Hide resolved

nordic-krch self-requested a review October 15, 2018 07:44

nordic-krch approved these changes Oct 15, 2018

View reviewed changes

jukkar merged commit a048a77 into zephyrproject-rtos:master Oct 15, 2018

jukkar deleted the net-shell branch October 15, 2018 08:14

rlubos mentioned this pull request Jan 22, 2019

Adapt net/lib/openthread subsystem to new shell subsystem #8879

Closed

dleach02 mentioned this pull request Jul 9, 2021

Adapt net/l2/ieee802154 subsystem to new shell subsystem #8876

Closed


		SHELL_CMD_REGISTER(net, &net_commands, "Networking commands", NULL);

		SHELL_UART_DEFINE(shell_transport_uart);

Convert net-shell to use new shell #9825

Convert net-shell to use new shell #9825

Conversation

jukkar commented Sep 6, 2018 • edited Loading

jakub-uC Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pizi-nordic Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch commented Sep 7, 2018

jukkar commented Sep 7, 2018 • edited Loading

codecov-io commented Sep 7, 2018 • edited Loading

Codecov Report

jukkar commented Sep 14, 2018

jukkar commented Sep 17, 2018

jakub-uC commented Sep 17, 2018 • edited Loading

jukkar commented Oct 8, 2018

jukkar commented Oct 11, 2018

jakub-uC commented Oct 11, 2018 • edited Loading

jukkar commented Oct 11, 2018

jakub-uC commented Oct 11, 2018

jukkar commented Oct 12, 2018

jukkar commented Sep 6, 2018 •

edited

Loading

jakub-uC Sep 7, 2018 •

edited

Loading

jukkar Sep 10, 2018 •

edited

Loading

pizi-nordic Sep 17, 2018 •

edited

Loading

jukkar Sep 7, 2018 •

edited

Loading

jukkar commented Sep 7, 2018 •

edited

Loading

codecov-io commented Sep 7, 2018 •

edited

Loading

jakub-uC commented Sep 17, 2018 •

edited

Loading

jakub-uC commented Oct 11, 2018 •

edited

Loading