Skip to content

sys/shell: make cmds submodules and add KConfig modeling#18355

Merged
benpicco merged 5 commits intoRIOT-OS:masterfrom
maribu:sys/shell/cmds
Sep 19, 2022
Merged

sys/shell: make cmds submodules and add KConfig modeling#18355
benpicco merged 5 commits intoRIOT-OS:masterfrom
maribu:sys/shell/cmds

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Jul 21, 2022

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

@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: sys Area: System labels Jul 21, 2022
@github-actions github-actions bot added Area: build system Area: Build system Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: tests Area: tests and testing framework labels Jul 21, 2022
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jul 21, 2022

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.

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 2, 2022

Still WIP?

benpicco
benpicco reviewed Aug 2, 2022
View reviewed changes
@maribu maribu removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 4, 2022
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Aug 4, 2022

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.

benpicco
benpicco reviewed Aug 4, 2022
View reviewed changes
benpicco
benpicco reviewed Aug 4, 2022
View reviewed changes
@benpicco benpicco merged commit f03f538 into RIOT-OS:master Sep 19, 2022
@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 20, 2022

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 ;-))

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 20, 2022

Ah... the testbed is currently down due :-( So this will have to wait for a definitive answer.

@miri64 miri64 mentioned this pull request Sep 20, 2022
Merged
@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 20, 2022

Testbed is up again. Let's see :-)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 20, 2022

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)

@miri64 miri64 mentioned this pull request Sep 21, 2022
Closed
@maribu maribu deleted the sys/shell/cmds branch September 21, 2022 07:13
maribu added a commit to maribu/RIOT that referenced this pull request Sep 21, 2022
@maribu
treewide: fix conditionals on deprecated module
f29ba73 
As a leftovers from RIOT-OS#18355 are still
present that check for `MODULE_SHELL_COMMANDS` rather than
`MODULE_SHELL_CMDS`. This updates the conditionals as needed.
maribu added a commit to maribu/RIOT that referenced this pull request Sep 21, 2022
@maribu
treewide: fix conditionals on deprecated module
3b2df89 
As a leftovers from RIOT-OS#18355 are still
present that check for `MODULE_SHELL_COMMANDS` rather than
`MODULE_SHELL_CMDS`. This updates the conditionals as needed.
@maribu maribu mentioned this pull request Sep 21, 2022
Merged
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Sep 21, 2022

Thanks! Fix provided in #18616

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Sep 23, 2022

this seems to have broken tests/heap_cmd on samr21-xpro and esp32-wroom-32 (the nightly boards):

Details socat - open:/dev/ttyACM0,b115200,echo=0,raw,cs8,parenb=0,cstopb=0

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'
f03f538 is the first bad commit
commit f03f538
Merge: 4b87a30 c95e855
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

@maribu maribu mentioned this pull request Sep 23, 2022
Merged
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Sep 23, 2022

Thanks, fix provided in #18634

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 24, 2022

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).

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 24, 2022

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

This was referenced Sep 24, 2022
Merged
Merged
Merged
@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 26, 2022

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).

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 c06335b 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

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 26, 2022 •
edited
Loading

I'd say, the problem is this #ifdef:

RIOT/sys/shell/cmds/gnrc_netif.c

Line 216 in c06335b

#ifdef MODULE_GNRC_NETIF_CMD_LORA
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.

@miri64 miri64 mentioned this pull request Sep 26, 2022
Closed
@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 26, 2022

See #18648

maribu added a commit to maribu/RIOT that referenced this pull request Sep 26, 2022
@maribu
sys/shell/cmds: fix shell_cmd_netif LoRaWAN integration
39a1745 
Model the LoRaWAN integration to GNRC's netif command (ifconfig) as
submodule of it, namely `shell_cmd_gnrc_netif_lorawan`.

This should fix a regression introduced by
RIOT-OS#18355
@maribu maribu mentioned this pull request Sep 26, 2022
Merged
maribu added a commit to maribu/RIOT that referenced this pull request Oct 4, 2022
@maribu
sys/shell/cmds: fix shell_cmd_netif LoRaWAN integration
782a62c 
Model the LoRaWAN integration to GNRC's netif command (ifconfig) as
submodule of it, namely `shell_cmd_gnrc_netif_lorawan`.

This should fix a regression introduced by
RIOT-OS#18355
maribu added a commit to maribu/RIOT that referenced this pull request Oct 4, 2022
@maribu
sys/shell/cmds: fix shell_cmd_netif LoRaWAN integration
fd80c53 
Model the LoRaWAN integration to GNRC's netif command (ifconfig) as
submodule of it, namely `shell_cmd_gnrc_netif_lorawan`.

This should fix a regression introduced by
RIOT-OS#18355
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
@krzysztof-cabaj krzysztof-cabaj mentioned this pull request Oct 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@smlng smlng Awaiting requested review from smlng

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

@jeandudey jeandudey Awaiting requested review from jeandudey

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

6 participants

@maribu @miri64 @benpicco @MrKevinWeiss @kaspar030 @leandrolanzieri