gnrc: deprecate gnrc_netdev_default, use netdev_default instead by benpicco · Pull Request #16744 · RIOT-OS/RIOT

benpicco · 2021-08-15T22:07:30Z

Contribution description

A generic application might select netdev_default instead of gnrc_netdev_default to pull in the default network interface.

So pull in gnrc_netif if netdev_default is selected with GNRC.

Testing procedure

For a basic application using TCP it should be enough to select

USEMODULE += netdev_default
USEMODULE += sock_tcp
USEMODULE += gnrc_ipv6

For BOARD=nrf52840dongle this however fails as gnrc_netif is not pulled in by some other (driver) dependency.

Issues/PRs references

#16739

jia200x · 2021-08-16T14:39:05Z

Actually, what's the purpose of gnrc_netdev_default? Is there are use case where gnrc_netdev_default would be selected but not netdev_default? Can't netdev_default be modeled to be stack independent?

benpicco · 2021-08-16T14:40:48Z

Actually, what's the purpose of gnrc_netdev_default?

I was wondering the same, but since pretty much all apps are using that instead of netdev_default we should keep it for at least a deprecation period.

It looks like nrfmin and nimble_netif are only selected with gnrc_netdev_default, there might be a hard dependency there.

jia200x · 2021-08-16T14:56:55Z

at least git grep GNRC_NETDEV_DEFAULT returns blank and as it is now with this PR, gnrc_netdev_default and netdev_default are almost interchangeable.

It looks like nrfmin and nimble_netif are only selected with gnrc_netdev_default, there might be a hard dependency there.

It seems that those lines just select nimble_netif if neither nrfmin nor nrf802154 are present. It seems they could be replaced with:

diff --git a/boards/common/nrf52/Makefile.nimble.dep b/boards/common/nrf52/Makefile.nimble.dep
index a682d92a03..61bee67136 100644
--- a/boards/common/nrf52/Makefile.nimble.dep
+++ b/boards/common/nrf52/Makefile.nimble.dep
@@ -1,4 +1,4 @@
-ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
+ifneq (,$(filter gnrc_netif,$(USEMODULE)))
   ifeq (,$(filter nrfmin nrf802154,$(USEMODULE)))
     USEMODULE += nimble_netif
   endif

We could then simply deprecate gnrc_netdev_default and just use netdev_default in a network stack independent manner. As of this PR, we could simply keep gnrc_netdev_default as it is but just add the deprecation note.

kaspar030 · 2021-08-16T19:45:58Z

Isn't "netdev_default" the network stack independent version, and used as such by tests?

edit yeah that's mentioned above already.

benpicco · 2021-08-17T16:10:35Z

Do we even have a mechanism for marking modules as deprecated?

maribu · 2021-08-31T08:50:12Z

sys/net/gnrc/Makefile.dep

 ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
  # enable default network devices on the platform
  USEMODULE += netdev_default
+endif
+
+ifneq (,$(filter netdev_default,$(USEMODULE)))
  USEMODULE += netdev
  USEMODULE += gnrc_netif
 endif


At this point, what is the difference between gnrc_netdev_default and netdev_default?

Mhhh netdev_default (at least in theory) should also be used to pull in default network devices for stacks other than GNRC. So this is the difference. Since this here is the GNRC dependency file, this is still observed.

I think the goal of this PR is to make gnrc_netdev_default unnecessary so it can be deprecated ;-).

miri64 · 2021-08-31T08:56:09Z

Actually, what's the purpose of gnrc_netdev_default?

It used to be the pseudo-module to pull in gnrc_netdev + netdev_default. Since gnrc_netdev was eaten by gnrc_netif (and the whole setup simplified), it does not make sense to exist anymore.

makefiles/deprecated_modules.inc.mk

benpicco · 2021-08-31T09:01:00Z

Oh no there are some radio drivers (esp-now, nrfmin) that depend on gnrc_netif_set_from_netdev

miri64 · 2021-08-31T09:02:59Z

Oh no there are some radio drivers (esp-now, nrfmin) that depend on gnrc_netif_set_from_netdev

No they don't. Those lines are within the gnrc_netif adapter code, so they only should be compiled in, when gnrc_netif is compiled in. If that is not the case, that is an error previously obfuscated by the inclusion of gnrc_netdev_default :-)

miri64 · 2021-09-03T14:34:37Z

Apparently there are also some modules in the test for the kw2xrf driver missing...

tests/driver_kw2xrf/Makefile

tests/driver_nrfmin/Makefile

tests/nimble_autoconn_gnrc/Makefile

tests/nimble_statconn_gnrc/Makefile

miri64

Can't compile the changed tests partially now :-/. Maybe add USEMODULE += nimble_netif to them instead?

miri64 · 2021-09-06T13:29:40Z

tests/nimble_autoconn_gnrc/Makefile

 USEMODULE += ps

 # Include GNRC and RPL
-USEMODULE += gnrc_netdev_default


I get

/usr/lib/gcc/arm-none-eabi/11.2.0/../../../../arm-none-eabi/bin/ld:cortexm_base.ld:217: warning: memory region `bkup' not declared /usr/lib/gcc/arm-none-eabi/11.2.0/../../../../arm-none-eabi/bin/ld:cortexm_base.ld:217: syntax error

now here when compiling on Arch

Operating System Environment ---------------------------- Operating System: "Arch Linux" Kernel: Linux 5.13.13-arch1-1 x86_64 unknown System shell: GNU bash, version 5.1.8(1)-release (x86_64-pc-linux-gnu) make's shell: GNU bash, version 5.1.8(1)-release (x86_64-pc-linux-gnu) Installed compiler toolchains ----------------------------- native gcc: gcc (GCC) 11.1.0 arm-none-eabi-gcc: arm-none-eabi-gcc (Arch Repository) 11.2.0 avr-gcc: avr-gcc (GCC) 11.2.0 mips-mti-elf-gcc: missing msp430-elf-gcc: missing riscv-none-elf-gcc: missing riscv64-unknown-elf-gcc: missing riscv-none-embed-gcc: missing xtensa-esp32-elf-gcc: missing xtensa-esp8266-elf-gcc: missing clang: clang version 12.0.1 Installed compiler libs ----------------------- arm-none-eabi-newlib: "4.1.0" mips-mti-elf-newlib: missing msp430-elf-newlib: missing riscv-none-elf-newlib: missing riscv64-unknown-elf-newlib: missing riscv-none-embed-newlib: missing xtensa-esp32-elf-newlib: missing xtensa-esp8266-elf-newlib: missing avr-libc: "2.0.0" ("20150208") Installed development tools --------------------------- ccache: ccache version 4.4 cmake: cmake version 3.21.2 cppcheck: Cppcheck 2.5 doxygen: 1.9.2 git: git version 2.33.0 make: GNU Make 4.3 openocd: Open On-Chip Debugger 0.10.0+dev-01089-g3bfe49266 (2020-02-26-14:18) python: Python 3.9.6 python2: Python 2.7.18 python3: Python 3.9.6 flake8: 3.9.2 (mccabe: 0.6.1, pycodestyle: 2.7.0, pyflakes: 2.3.1) CPython 3.9.6 on coccinelle: spatch version 1.1.0 compiled with OCaml version 4.11.1

should be fixed by #16753

Should make sense to add nimble_netif anyways.

The Department of Redundancy Department approves 😉

I don't see the redundancy. Currently the nimble_netif is pulled in implicitly, but since the tests are testing nimble-functionality it should be pulled in explicitly.

miri64 · 2021-09-06T13:30:21Z

tests/nimble_statconn_gnrc/Makefile

 USEMODULE += ps

 # Include GNRC and RPL
-USEMODULE += gnrc_netdev_default


For me the binary does not change when I add nimble_netif

miri64 · 2021-09-06T13:34:21Z

Maybe add USEMODULE += nimble_netif to them instead?

Yepp that fixes the tests for me again ;-).

diff --git a/tests/nimble_autoconn_gnrc/Makefile b/tests/nimble_autoconn_gnrc/Makefile
index 4e189a0544..dba57ab696 100644
--- a/tests/nimble_autoconn_gnrc/Makefile
+++ b/tests/nimble_autoconn_gnrc/Makefile
@@ -15,6 +15,7 @@ USEMODULE += auto_init_gnrc_rpl
 USEMODULE += gnrc_rpl
 
 # Setup Nimble
+USEMODULE += nimble_netif
 USEMODULE += nimble_autoconn_ipsp
 
 TEST_ON_CI_WHITELIST += nrf52dk

(with nimble_statconn_gnrc as well)

A generic application might select netdev_default instead of gnrc_netdev_default to pull in the default network interface. So pull in gnrc_netif if netdev_default is selected with GNRC.

benpicco requested review from PeterKietzmann, cgundogan, haukepetersen and miri64 as code owners August 15, 2021 22:07

benpicco added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Aug 15, 2021

github-actions bot added Area: network Area: Networking Area: sys Area: System and removed Area: build system Area: Build system labels Aug 15, 2021

benpicco requested a review from jia200x August 16, 2021 14:33

benpicco requested a review from fjmolinas August 18, 2021 13:23

benpicco requested a review from maribu August 31, 2021 08:21

maribu reviewed Aug 31, 2021

View reviewed changes

benpicco force-pushed the gnrc_netdev_default branch from 3fc031f to 643882f Compare August 31, 2021 08:55

benpicco requested review from MichelRottleuthner, aabadie, leandrolanzieri and smlng as code owners August 31, 2021 08:55

github-actions bot added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: examples Area: Example Applications Area: tests Area: tests and testing framework Platform: native Platform: This PR/issue effects the native platform labels Aug 31, 2021

benpicco added Process: deprecation Integration Process: The PR is deprecating a feature or API and removed Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Aug 31, 2021

miri64 reviewed Aug 31, 2021

View reviewed changes

makefiles/deprecated_modules.inc.mk Show resolved Hide resolved

benpicco changed the title ~~gnrc: select gnrc_netif if netdev_default is selected~~ gnrc: deprecate gnrc_netdev_default, use netdev_default instead Aug 31, 2021

benpicco added 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 Sep 3, 2021

miri64 reviewed Sep 3, 2021

View reviewed changes

tests/driver_kw2xrf/Makefile Show resolved Hide resolved

miri64 requested changes Sep 6, 2021

View reviewed changes

tests/driver_nrfmin/Makefile Outdated Show resolved Hide resolved

tests/nimble_autoconn_gnrc/Makefile Outdated Show resolved Hide resolved

tests/nimble_statconn_gnrc/Makefile Outdated Show resolved Hide resolved

miri64 requested changes Sep 6, 2021

View reviewed changes

benpicco added 4 commits September 6, 2021 15:43

gnrc: select gnrc_netif if netdev_default is selected

459f398

A generic application might select netdev_default instead of gnrc_netdev_default to pull in the default network interface. So pull in gnrc_netif if netdev_default is selected with GNRC.

treewide: replace gnrc_netdev_default with netdev_default

9a6fc85

tests/driver_kw2xrf: use gnrc for test

4a32402

tests: remove unneeded netdev_default

5b25973

benpicco force-pushed the gnrc_netdev_default branch from 7ecf993 to 5b25973 Compare September 6, 2021 13:48

miri64 approved these changes Sep 6, 2021

View reviewed changes

benpicco merged commit f79207e into RIOT-OS:master Sep 6, 2021

benpicco deleted the gnrc_netdev_default branch September 6, 2021 19:03

benpicco mentioned this pull request Sep 6, 2021

examples, tests: replace gnrc_netdev_default with netdev_default #16817

Merged

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

Conversation

benpicco commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

jia200x commented Aug 16, 2021

Uh oh!

benpicco commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jia200x commented Aug 16, 2021

Uh oh!

kaspar030 commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Aug 17, 2021

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 commented Aug 31, 2021

Uh oh!

Uh oh!

benpicco commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 31, 2021

Uh oh!

miri64 commented Sep 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benpicco commented Aug 15, 2021 •

edited

Loading

benpicco commented Aug 16, 2021 •

edited

Loading

kaspar030 commented Aug 16, 2021 •

edited

Loading

benpicco commented Aug 31, 2021 •

edited

Loading