RIOT-OS
sys/shell: make cmds submodules and add KConfig modeling
Contribution description
Previously shell_commands was a "catch-all" module that included
shell commands for each and every used module that has a shell
companion. Instead, the new shell_cmds module is now used to provide
shell commands as individually selectable submodules, e.g.
cmd_gnrc_icmpv6_echo now provides the ICMPv6 echo command (a.k.a.
ping). shell_commands is still provided as a pseudo-module that
pulls in "recommended" shell companions for the used set of modules.
The intention is, that shell_commands provides the same shell
commands as before.
For a handful of shell commands individual selection was already
possible. Those modules now depend on the corresponding cmd_% module
and they have been deprecated.
Testing procedure
Except for when explicitly using cmd_% modules, the machine code should not change. (I do expect changes to the debug symbols due to different path and file names of the sources.)
Issues/PRs references
None
For a handful of shell commands individual selection was already possible. Those modules now depend on the corresponding
cmd_%module and they have been deprecated.
As all of them are submodules of shell_cmd, I would prefer the name shell_cmd_% for these modules.
Still WIP?
I bet this contains still some issues. But now may be a good point in time to push for it, as there is still plenty of time until the next release to shake out any bugs it introduces.
I have to also model everything in KConfig now. Maybe not a bad idea to get some progress for the migration so that we eventually do not have to maintain two dependency systems anymore.
I may abuse this branch to sync state between to machines I am coding at. If I push any commits names wip it is just to get stuff I am working on to complete the work in progress on a second machine. Those will contains stuff that should better not be reviewed yet.
I can give a closer look at the kconfig side tomorrow as I don't have my fancy setup today.
After talking with @leandrolanzieri he recommended the following
config MODULE_SHELL_COMMANDS
bool "Default shell commands for enabled modules"
depends on MODULE_SHELL
select MODULE_SHELL_CMDS
help
Shell commands can be enabled and disabled individually as dedicated
modules. This module instead will select the shell commands of modules
already used, if this is considered as "good default choice".
menuconfig MODULE_SHELL_CMDS
bool "Support for shell commands"
depends on MODULE_SHELL
if MODULE_SHELL_CMDS
config MODULE_SHELL_CMD_APP_METADATA
bool
prompt "Command to print app meta data as JSON" if !MODULE_SHELL_COMMANDS
default y if MODULE_SHELL_COMMANDS
depends on MODULE_APP_METADATA
config MODULE_SHELL_CMD_AT30TSE75X
bool
prompt "Command to test AT30TSE75x temperature sensors" if !MODULE_SHELL_COMMANDS
default y if MODULE_SHELL_COMMANDS
depends on MODULE_AT30TSE75X
config MODULE_SHELL_CMD_BENCHMARK_UDP
bool
prompt "Command to perform a UDP benchmark" if !MODULE_SHELL_COMMANDS
depends on MODULE_BENCHMARK_UDP
....
endif # MODULE_SHELL_CMDS
Now this does not fulfill the MODULE_SHELL_CMD_* pulls in MODULE_*`, however, this is more a bit silly design. Should I really bring in a module if I want a shell command?
If you still want that functionality than maybe keep the config FORCE_MODULE_SHELL_CMD_APP_* that is just a helper function so you don't need to manually bring in the dependencies and the shell command.
I guess it is fine to also enable <MODULE_FOO> if you want to use <MODULE_SHELL_CMD_FOO>. It would result in an equivalent KConfig config being a bit more verbose than the legacy config, though. But maybe it makes it also more obvious what modules are actually used.
Another way that is closer to saul would be like this, the downside is that you have to manually disable the defaults (I renamed cmd to command_defaults):
menuconfig MODULE_SHELL_COMMANDS
bool "Support for shell commands"
depends on MODULE_SHELL
if MODULE_SHELL_COMMANDS
config MODULE_SHELL_COMMANDS_DEFAULT
bool "Default shell commands for enabled modules"
depends on MODULE_SHELL
default y
help
Shell commands can be enabled and disabled individually as dedicated
modules. This module instead will select the shell commands of modules
already used, if this is considered as "good default choice".
config MODULE_SHELL_CMD_APP_METADATA
bool
prompt "Command to print app meta data as JSON" if !MODULE_SHELL_COMMANDS_DEFAULT
default y if MODULE_SHELL_COMMANDS_DEFAULT
depends on MODULE_APP_METADATA
config MODULE_SHELL_CMD_AT30TSE75X
bool
prompt "Command to test AT30TSE75x temperature sensors" if !MODULE_SHELL_COMMANDS_DEFAULT
default y if MODULE_SHELL_COMMANDS_DEFAULT
depends on MODULE_AT30TSE75X
with tests/driver_at30tse75x/app.config.test looking like:
CONFIG_MODULE_AT30TSE75X=y
CONFIG_MODULE_SHELL=y
CONFIG_MODULE_SHELL_COMMANDS=y
CONFIG_MODULE_SHELL_COMMANDS_DEFAULT=n
CONFIG_MODULE_APP_METADATA=y # Only for testing purposes to show it is not selected
CONFIG_MODULE_SHELL_CMD_AT30TSE75X=y
I implemented the approach suggested in https://github.com/RIOT-OS/RIOT/pull/18355#issuecomment-1209063856
Note that I named the module CONFIG_MODULE_SHELL_CMDS rather than CONFIG_MODULE_SHELL_COMMANDS, as that is preexisting and has the semantics as CONFIG_MODULE_SHELL_CMDS_DEFAULT has. I deprecated that now (in both KConfig and non-KConfig modelling), but shell_commands does pull in shell_cmds_default now for backward compatibility.
Nice cleanup - let's see if we find some missing commands wink
I started https://github.com/RIOT-OS/RIOT/actions/runs/3088682900, so we can at least see early, which release tests might fail due to this (might be of interest to @leandrolanzieri and @maribu ;-))
Ah... the testbed is currently down due :-( So this will have to wait for a definitive answer.
Mh, there is a regression at least between the test run on the weekend and the run I linked above (on the merge commit for this PR). For some reason, the ifconfig command for lwIP now does not show the link-local address for an interface. The output can be seen here https://github.com/RIOT-OS/RIOT/actions/runs/3088682900/jobs/5000734122#step:14:169 (tests/lwip was used for that)
this seems to have broken tests/heap_cmd on samr21-xpro and esp32-wroom-32 (the nightly boards):
heap
heap
heap: 30232 (used 2500, free 27732) [bytes]
malloc 100
malloc 100
allocated 0x200013b0
heap
heap
heap: 30232 (used 2608, free 27624) [bytes]
free 0x200013b0
free 0x200013b0 freeing 0x200013b0 Timeout in expect script at "child.expect('freed 0x' + addr)" (tests/heap_cmd/tests/01-run.py:27)
make: *** [/tmp/dwq.0.2965737936981785/e884ddf95438f5dda65991941a2a27a7/makefiles/tests/tests.inc.mk:22: test] Error 1 make: Leaving directory '/tmp/dwq.0.2965737936981785/e884ddf95438f5dda65991941a2a27a7/tests/heap_cmd' make: *** [/home/kaspar/src/riot/makefiles/murdock.inc.mk:17: test-murdock] Error 1 make: Leaving directory '/home/kaspar/src/riot/tests/heap_cmd' f03f5384c1268a22020fe3af7fcb34efe38d43f5 is the first bad commit commit f03f5384c1268a22020fe3af7fcb34efe38d43f5 Merge: 4b87a300c0 c95e8553ef Author: benpicco [email protected] Date: Mon Sep 19 21:07:23 2022 +0200
Merge pull request #18355 from maribu/sys/shell/cmds
sys/shell: make cmds submodules and add KConfig modeling
Not sure, if this is caused by this PR or the general flakyness of the lorawan tests, but some spec11 tests failed since there was some output missing from ifconfig: https://github.com/RIOT-OS/RIOT/actions/runs/3116915173/jobs/5055168659#step:14:107 (lorawan_netif() expects a sf key in the the parsed output).
Also (from the output of the same test) there might be a fix required soon to the Release-Specs repo: Deprecated modules are in use: gnrc_pktbuf_cmd
Not sure, if this is caused by this PR or the general flakyness of the lorawan tests, but some spec11 tests failed since there was some output missing from
ifconfig: https://github.com/RIOT-OS/RIOT/actions/runs/3116915173/jobs/5055168659#step:14:107 (lorawan_netif()expects asfkey in the the parsed output).
With git bisect using examples/gnrc_lorawan on st-lrwan1-10.saclay.iot-lab.info (BOARD=b-l072z-lrwan1 IOTLAB_NODE=auto WERROR=0 make -C examples/gnrc_lorawan/ -j flash term) I found, that before c06335b71b3c2ffd2465f55a0460242e5e546e5e it was:
> ifconfig
ifconfig
fIface 3 HWaddr: 00:00:00:00 Frequency: 868299987Hz RSSI: -157 BW: 125kHz SF: 7 CR: 4/5 Link: down
TX-Power: 14dBm State: SLEEP Demod margin.: 0 Num gateways.: 0
OTAA
L2-PDU:255
but after:
> ifconfig
ifconfig
Iface 3 HWaddr: 00:00:00:00 Frequency: 868299987Hz RSSI: -157 Link: down
TX-Power: 14dBm State: SLEEP
OTAA
L2-PDU:255
I'd say, the problem is this #ifdef: https://github.com/RIOT-OS/RIOT/blob/c06335b71b3c2ffd2465f55a0460242e5e546e5e/sys/shell/cmds/gnrc_netif.c#L216 ~~The module gnrc_netif_cmd_lora does not exist~~ There is no reference to gnrc_netif_cmd_lora in tree due to this change anymore.