Skip to content
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

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

Merged
merged 5 commits into from
Sep 19, 2022

Conversation

maribu
Copy link
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
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
Contributor

benpicco commented Aug 2, 2022

Still WIP?

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

@miri64
Copy link
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
Member

miri64 commented Sep 20, 2022

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

@miri64
Copy link
Member

miri64 commented Sep 20, 2022

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

@miri64
Copy link
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)

@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
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
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
Copy link
Member Author

maribu commented Sep 21, 2022

Thanks! Fix provided in #18616

@kaspar030
Copy link
Contributor

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

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 benjamin.valentin@ml-pa.com
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
Copy link
Member Author

maribu commented Sep 23, 2022

Thanks, fix provided in #18634

@miri64
Copy link
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
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

@miri64
Copy link
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
Member

miri64 commented Sep 26, 2022

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

#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
Copy link
Member

miri64 commented Sep 26, 2022

See #18648

maribu added a commit to maribu/RIOT that referenced this pull request Sep 26, 2022
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants