sys/shell/cmds: fix shell_cmd_netif LoRaWAN integration by maribu · Pull Request #18649 · RIOT-OS/RIOT

maribu · 2022-09-26T10:32:10Z

Contribution description

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 #18355

Testing procedure

Run and compile examples/gnrc_lorawan for BOARD=b-l072z-lrwan1 as described in #18355 (comment)

Issues/PRs references

#18355 (comment)

Alternative to #18648

miri64

I like this one better then mine, as it seems to align more with what you did in #18355. However, I am somewhat confused where gnrc_netif_cmd_lora is defined. It should be deprecated and removed later.

maribu · 2022-09-26T10:54:40Z

This is the only occurrence of that module in the latest release branch:

RIOT/sys/Makefile.dep

Lines 336 to 338 in 72b14ac

    
           ifneq (,$(filter gnrc_lorawan,$(USEMODULE))) 
        
             USEMODULE += gnrc_netif_cmd_lora 
        
           endif

To me it looks like an intermediate symbol in the dependency resolution step and not like a "first class" module that users would actually select. But I guess, better be safe than sorry.

miri64 · 2022-09-26T11:08:21Z

After some deep digging in the history of those lines, it seem to have been introduced in c31e373 as a catch-all pseudo-module... This duck-hunt shows yet another reason, why catch-all pseudo-modules should be abolished.

miri64 · 2022-09-26T11:08:42Z

makefiles/deprecated_modules.inc.mk

 # Keep this list ALPHABETICALLY SORTED!!!!111elven
 DEPRECATED_MODULES += event_thread_lowest
 DEPRECATED_MODULES += gnrc_netdev_default
+DEPRECATED_MODULES += gnrc_netif_cmd_lora # use shell_cmd_gnrc_netif_lorawan instead


c31e373 introduced a catch-all (gnrc_netif_cmd_%). I think that catch-all should be used here for deprecation as well.

c31e373 introduced a catch-all (gnrc_netif_cmd_%). I think that catch-all should be used here for deprecation as well.

Ping?

But after all this PR deprecates gnrc_netif_cmd_lora by renaming it. If there are no other modules matching gnrc_netif_cmd_%, maybe we should instead change pseudomodules.mk to no longer match with wildcards.

There is a check for typos in module names. But that check is not as effective as it could be due to all the wildcards. IMO we should get rid of all wildcards in pseudomodules.mk.

There is a check for typos in module names. But that check is not as effective as it could be due to all the wildcards. IMO we should get rid of all wildcards in pseudomodules.mk.

You won't get resistance on that from me :-). Then for this PR, should we just replace the gnrc_netif_cmd_% wildcard with gnrc_netif_cmd_lora to get this along? The actual removal of wildcards should be done in a separate PR IMHO.

makefiles/pseudomodules.inc.mk

miri64 · 2022-10-01T13:54:17Z

@maribu ping? Also needs a rebase.

riot-ci · 2022-10-04T19:07:33Z

Murdock results

✔️ PASSED

fd80c53 sys/shell/cmds: fix shell_cmd_netif LoRaWAN integration

Success	Failures	Total	Runtime
1	0	1	01m:20s

Artifacts

Documentation preview

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

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

miri64 · 2022-10-05T09:37:41Z

doc/doxygen/src/deprecated-modules.md

+Deprecated Modules    {#modules-deprecated}
+==================


I would prefer, if this somehow could show up in the @deprecated list so it is not easily forgotten by release managers. Can you try adding the @deprecated command here?

The @deprecated command will link to some entity Doxygen is aware of such as function or type (or class). Since Doxygen is unaware of modules, I have no idea how to tell Doxygen to integrate it into the deprecation list.

Modules are represented in Doxygen as groups, so you could link to the group.

The more I think about this: Couldn't we just add the deprecated modules Makefiles add to doxygen and document the deprecation there in comments instead of yet another page that is required to be kept in sync? We do a similar thing with some pseudomodules already.

See #18698.

This would be my proposal to be in line with #18698

diff --git a/makefiles/pseudomodules.inc.mk b/makefiles/pseudomodules.inc.mk index 4059c1cab6..4f88138ca8 100644 --- a/makefiles/pseudomodules.inc.mk +++ b/makefiles/pseudomodules.inc.mk @@ -103,3 +103,10 @@ PSEUDOMODULES += gnrc_netif_mac PSEUDOMODULES += gnrc_netif_single -PSEUDOMODULES += gnrc_netif_cmd_% +## +## @defgroup net_gnrc_netif_cmd_lora gnrc_netif_cmd_lora +## @ingroup shell_commands +## @ingroup sys_gnrc_netif_lorawan +## @{ +## @deprecated Use `shell_cmd_gnrc_netif_lorawan` instead; will be removed after 2023.07 release. +PSEUDOMODULES += gnrc_netif_cmd_lora +## @} PSEUDOMODULES += gnrc_netif_dedup

OK. I dropped the second commit, so that #18698 could add the deprecation doc

Ok, then let's just merge this and add the deprecation note either in #18698 or in a follow-up. However, nothing speaks against adding it here. But let's not bike-shed this any further 🚴‍♀️.

See #18706.

miri64 · 2022-10-05T09:38:23Z

makefiles/deprecated_modules.inc.mk

 # Keep this list ALPHABETICALLY SORTED!!!!111elven
 DEPRECATED_MODULES += event_thread_lowest
 DEPRECATED_MODULES += gnrc_netdev_default
+DEPRECATED_MODULES += gnrc_netif_cmd_lora # use shell_cmd_gnrc_netif_lorawan instead


c31e373 introduced a catch-all (gnrc_netif_cmd_%). I think that catch-all should be used here for deprecation as well.

Ping?

doc/doxygen/src/deprecated-modules.md

miri64

Confirmed testing procedures:

> ifconfig
ifconfig
Iface  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

maribu · 2022-10-07T09:09:44Z

The RPL test failure on native is an unrelated glitch. The same issue happened to another unrelated PR.

I restart Murdock, but without spending days of CPU time this round.

aabadie · 2023-04-13T09:14:50Z

I think this PR introduced a regression with lora netif. #19466 is trying to fix it using by adding some lora parameters when the lora module is used.

maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Sep 26, 2022

maribu requested a review from miri64 September 26, 2022 10:32

maribu requested review from MrKevinWeiss, jia200x and leandrolanzieri as code owners September 26, 2022 10:32

github-actions bot added Area: build system Area: Build system Area: Kconfig Area: Kconfig integration Area: sys Area: System labels Sep 26, 2022

maribu mentioned this pull request Sep 26, 2022

shell: fix gnrc_netif_lorawan dependency #18648

Closed

miri64 reviewed Sep 26, 2022

View reviewed changes

maribu requested review from PeterKietzmann, cgundogan and haukepetersen as code owners September 26, 2022 10:54

github-actions bot added the Area: network Area: Networking label Sep 26, 2022

miri64 reviewed Sep 26, 2022

View reviewed changes

makefiles/pseudomodules.inc.mk Outdated Show resolved Hide resolved

maribu requested a review from aabadie as a code owner October 4, 2022 18:51

github-actions bot added the Area: doc Area: Documentation label Oct 4, 2022

maribu force-pushed the sys/shell/cmds branch from 95ef866 to 4528232 Compare October 4, 2022 19:00

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 4, 2022

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 force-pushed the sys/shell/cmds branch from 4528232 to 2963040 Compare October 4, 2022 21:06

miri64 requested changes Oct 5, 2022

View reviewed changes

miri64 reviewed Oct 6, 2022

View reviewed changes

doc/doxygen/src/deprecated-modules.md Outdated Show resolved Hide resolved

miri64 mentioned this pull request Oct 6, 2022

doc: add deprecation notice on deprecated pseudo-modules #18698

Merged

maribu force-pushed the sys/shell/cmds branch from 2963040 to fd80c53 Compare October 6, 2022 07:47

github-actions bot removed the Area: doc Area: Documentation label Oct 6, 2022

miri64 approved these changes Oct 6, 2022

View reviewed changes

maribu enabled auto-merge October 6, 2022 14:30

miri64 mentioned this pull request Oct 7, 2022

pseudomodules: document gnrc_netif_cmd_lora as deprecated #18706

Merged

maribu merged commit 213c35b into RIOT-OS:master Oct 7, 2022

miri64 mentioned this pull request Oct 12, 2022

gnrc: make pseudo-modules explicit + documentation improvements #18727

Merged

maribu added this to the Release 2022.10 milestone Oct 14, 2022

aabadie mentioned this pull request Apr 13, 2023

sys/shell: fix missing generic lora settings in ifconfig #19466

Merged

maribu deleted the sys/shell/cmds branch April 13, 2023 09:28

		Deprecated Modules {#modules-deprecated}
		==================

Conversation

maribu commented Sep 26, 2022

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Sep 26, 2022

Uh oh!

miri64 commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miri64 commented Oct 1, 2022

Uh oh!

riot-ci commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Oct 7, 2022

Uh oh!

aabadie commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

riot-ci commented Oct 4, 2022 •

edited

Loading

miri64 Oct 5, 2022 •

edited

Loading