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/cmds: fix shell_cmd_netif LoRaWAN integration #18649

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Sep 26, 2022

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

@maribu maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Sep 26, 2022
@maribu maribu requested a review from miri64 September 26, 2022 10:32
@github-actions github-actions bot added Area: build system Area: Build system Area: Kconfig Area: Kconfig integration Area: sys Area: System labels Sep 26, 2022
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maribu commented Sep 26, 2022

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

miri64 commented Sep 26, 2022

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.

@@ -2,6 +2,7 @@
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ping?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@miri64
Copy link
Member

miri64 commented Oct 1, 2022

@maribu ping? Also needs a rebase.

@maribu maribu requested a review from aabadie as a code owner October 4, 2022 18:51
@github-actions github-actions bot added the Area: doc Area: Documentation label Oct 4, 2022
@maribu 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
@riot-ci
Copy link

riot-ci commented Oct 4, 2022

Murdock results

✔️ PASSED

fd80c53 sys/shell/cmds: fix shell_cmd_netif LoRaWAN integration

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

Artifacts

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
Comment on lines 1 to 2
Deprecated Modules {#modules-deprecated}
==================
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@miri64 miri64 Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #18698.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🚴‍♀️.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #18706.

@@ -2,6 +2,7 @@
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ping?

@github-actions github-actions bot removed the Area: doc Area: Documentation label Oct 6, 2022
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 maribu enabled auto-merge October 6, 2022 14:30
@maribu
Copy link
Member Author

maribu commented Oct 7, 2022

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.

@maribu maribu added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 7, 2022
@maribu maribu merged commit 213c35b into RIOT-OS:master Oct 7, 2022
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
@aabadie
Copy link
Contributor

aabadie commented Apr 13, 2023

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 maribu deleted the sys/shell/cmds branch April 13, 2023 09:28
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: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants