cpu/nrf52: nrf802154: default to netdev_ieee802154_submac

[benpicco]

Contribution description

The basic nrf802154 driver lacks ACK handling and retransmissions, which degrades it's usefulness.

The 802.15.4 Sub-MAC fixes all those issues.

Enable it by default for this driver to make it better behaved.

Testing procedure

Benefits are immediately noticeable with ping

master

> 2020-10-01 17:32:20,009 #  ping fe80::d068:7df5:3123:3a22
2020-10-01 17:32:20,023 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=0 ttl=64 rssi=-45 dBm time=5.778 ms
2020-10-01 17:32:20,032 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=0 ttl=64 rssi=-45 dBm time=13.576 ms (DUP!)
2020-10-01 17:32:20,042 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=0 ttl=64 rssi=-45 dBm time=22.072 ms (DUP!)
2020-10-01 17:32:20,049 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=0 ttl=64 rssi=-45 dBm time=30.569 ms (DUP!)
2020-10-01 17:32:21,024 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=1 ttl=64 rssi=-44 dBm time=5.777 ms
2020-10-01 17:32:21,032 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=1 ttl=64 rssi=-44 dBm time=13.579 ms (DUP!)
2020-10-01 17:32:21,040 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=1 ttl=64 rssi=-44 dBm time=22.075 ms (DUP!)
2020-10-01 17:32:21,049 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=1 ttl=64 rssi=-44 dBm time=30.572 ms (DUP!)
2020-10-01 17:32:22,024 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=2 ttl=64 rssi=-45 dBm time=5.777 ms
2020-10-01 17:32:22,024 # 
2020-10-01 17:32:22,027 # --- fe80::d068:7df5:3123:3a22 PING statistics ---
2020-10-01 17:32:22,034 # 3 packets transmitted, 3 packets received, 6 duplicates, 0% packet loss
2020-10-01 17:32:22,038 # round-trip min/avg/max = 5.777/16.641/30.572 ms

sub-MAC

2020-10-01 17:31:32,322 #  ping fe80::d068:7df5:3123:3a22
2020-10-01 17:31:32,337 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=0 ttl=64 rssi=178 dBm time=7.791 ms
2020-10-01 17:31:33,338 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=1 ttl=64 rssi=178 dBm time=8.409 ms
2020-10-01 17:31:34,337 # 12 bytes from fe80::d068:7df5:3123:3a22%6: icmp_seq=2 ttl=64 rssi=178 dBm time=7.129 ms
2020-10-01 17:31:34,337 # 
2020-10-01 17:31:34,342 # --- fe80::d068:7df5:3123:3a22 PING statistics ---
2020-10-01 17:31:34,346 # 3 packets transmitted, 3 packets received, 0% packet loss
2020-10-01 17:31:34,350 # round-trip min/avg/max = 7.129/7.776/8.409 ms

This adds 2732 bytes of ROM and 256 bytes of RAM.

Issues/PRs references

Should probably wait until after the hard feature freeze.

[jia200x]

I'm fine with it! The ROM and RAM will be reduced when we remove the migration interfaces :), but the retransmission logic still adds some memory.
What do the others think about enabling this by default? @fjmolinas @bergzand

[benpicco]

Ah well, no lwip support yet.

[jia200x]

It really sucks that we don't have a common network stack independent bootstrap for all radios :(

[benpicco]

So all the other stacks work now - should we flip the switch?

[jia200x]

So all the other stacks work now - should we flip the switch?

Sure, I'm in :)

[benpicco]

Well it's up to you to merge this then 😉

[jia200x]

Let's go with it, but there's only a minor thing that I think it should be addressed: netdev_submac_ieee802154 doesn't implement all NETOPTs defined by this driver. There might be people that still need to use e.g NETOPT_CHANNEL_IS_CLR with this radio.

What I propose for the short-term is to add a netdev_legacy module that enables the netdev implementation. In the mid-run we can implement the missing NETOPTs.

The following patch does the trick in case you want to append it to this PR:

From d8b1b30ee8655c72b3f25860dc20855502c5fca5 Mon Sep 17 00:00:00 2001
From: Jose Alamos <jose.alamos@haw-hamburg.de>
Date: Mon, 23 Nov 2020 12:58:38 +0100
Subject: [PATCH 1/2] netdev: add legacy pseudomodule

---
 examples/gnrc_networking/Makefile | 3 +++
 makefiles/pseudomodules.inc.mk    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/examples/gnrc_networking/Makefile b/examples/gnrc_networking/Makefile
index 610d0bc501..e96762ea47 100644
--- a/examples/gnrc_networking/Makefile
+++ b/examples/gnrc_networking/Makefile
@@ -31,6 +31,9 @@ USEMODULE += netstats_l2
 USEMODULE += netstats_ipv6
 USEMODULE += netstats_rpl
 
+# Uncomment this to enable legacy support of netdev for IEEE 802.15.4 radios.
+# USEMODULE += netdev_ieee802154_legacy
+
 # Comment this out to disable code in RIOT that does safety checking
 # which is not needed in a production environment but helps in the
 # development process:
diff --git a/makefiles/pseudomodules.inc.mk b/makefiles/pseudomodules.inc.mk
index 41295be5f5..7ac0d4c7ef 100644
--- a/makefiles/pseudomodules.inc.mk
+++ b/makefiles/pseudomodules.inc.mk
@@ -133,6 +133,7 @@ PSEUDOMODULES += ztimer%
 # ztimer's main module is called "ztimer_core"
 NO_PSEUDOMODULES += ztimer_core
 NO_PSEUDOMODULES += netdev_ieee802154_submac
+NO_PSEUDOMODULES += netdev_ieee802154_legacy
 
 # print ascii representation in function od_hex_dump()
 PSEUDOMODULES += od_string
-- 
2.29.2


From 97edf7823829ccc61e0ed10eee9df856e0cf43cb Mon Sep 17 00:00:00 2001
From: Jose Alamos <jose.alamos@haw-hamburg.de>
Date: Mon, 23 Nov 2020 12:58:50 +0100
Subject: [PATCH 2/2] fixup! cpu/nrf52: nrf802154: default to
 netdev_ieee802154_submac

---
 cpu/nrf52/Makefile.dep | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cpu/nrf52/Makefile.dep b/cpu/nrf52/Makefile.dep
index 63a95395d5..e84d04c63e 100644
--- a/cpu/nrf52/Makefile.dep
+++ b/cpu/nrf52/Makefile.dep
@@ -5,7 +5,9 @@ ifneq (,$(filter nrf802154,$(USEMODULE)))
   FEATURES_REQUIRED += radio_nrf802154
   USEMODULE += luid
   USEMODULE += netdev_ieee802154
-  USEMODULE += netdev_ieee802154_submac
+  ifeq (,$(filter netdev_ieee802154_legacy,$(USEMODULE)))
+    USEMODULE += netdev_ieee802154_submac
+  endif
 endif
 
 # The nrf52832 requires gpio IRQ with SPI to work around errata 58
-- 
2.29.2

[jia200x]

Alternatively, we can open a follow up PR. As you wish 😉

[jia200x]

The following patch does the trick in case you want to append it to this PR:

Probably the module should be called netdev_ieee802154_legacy though...

EDIT: Already fixed it in #15132 (comment)

[benpicco]

added it

[jia200x]

ACK on my side.

This is in a gray zone regarding the number of ACKs (technically it's an API change if someone doesn't select netdev_ieee802154_legacy). So I would prefer an extra ACK (@miri64?)

[ghost]

I don't see any reason not to go forward with this. Sadly nrf802154 netdev will not have those features in the near future (despite #11150 being out there). This PR will definitely improve the current situation on those nrf boards.

[jia200x]

I don't see any reason not to go forward with this. Sadly nrf802154 netdev will not have those features in the near future (despite #11150 being out there). This PR will definitely improve the current situation on those nrf boards.

The idea would be to deprecate netdev_ieee802154 anyway, so I guess there won't be such a feature for this radio anyway. But as it is, setting the SubMAC for default will break applications that already run netdev_ieee802154 for this radio if and only if netdev_ieee802154_legacy is not selected. Probably it won't harm anyone, but I would prefer to have at least a second ACK because it's officially an API change (some NETOPTs won't be available out of the box with the SubMAC, although we might add them in a very short term)

[benpicco]

setting the SubMAC for default will break applications that already run netdev_ieee802154 for this radio

How is that? Unless the applications rely on ACKs/Retransmissions not being available, they shouldn't even notice a difference.

[ghost]

But as it is, setting the SubMAC for default will break applications that already run netdev_ieee802154

True, I disregarded this for there are probably no real users of this driver (who can't switch to the submac in an instance). In that case it might go into the next release, officially deprecating nrf802154.

EDIT:

How is that?

Well if someone have code that is connected to the driver, it will most likely not work anymore.

[miri64]

ACK on my side.

This is in a gray zone regarding the number of ACKs (technically it's an API change if someone doesn't select netdev_ieee802154_legacy). So I would prefer an extra ACK (@miri64?)

I don't see why this needs an extra ACK. Even if it is an API change (I disagree with that), I don't believe the nrf840154 is largely deployed yet, due to its lacking feature set. So let's go!