ieee802154/at86rf2xx: port to Radio HAL #19015
Conversation
jia200x
requested review from
smlng,
leandrolanzieri,
aabadie,
MichelRottleuthner,
fjmolinas,
miri64,
haukepetersen,
PeterKietzmann,
cgundogan and
daniel-k
as code owners
github-actions
bot
added
Area: doc
|
Some input would be appreciated, specially from @miri64, @maribu, @benpicco, @fabian18, @PeterKietzmann and @MichelRottleuthner |
jia200x
added
the
State: waiting for other PR
|
After some online discussions, note that most integrated transceivers have a dedicated SPI bus, which means |
jia200x
force-pushed
the
pr/at86rf2xx_radio_hal_v3
branch
from
ee36b67
to
fe2ff7c
Compare
benpicco
removed
the
State: waiting for other PR
jia200x
force-pushed
the
pr/at86rf2xx_radio_hal_v3
branch
from
fe2ff7c
to
61737f1
Compare
jia200x
force-pushed
the
pr/at86rf2xx_radio_hal_v3
branch
from
61737f1
to
95977a8
Compare
|
there was a rebase error I just fixed. I hope it passes all static tests now |
|
I squashed and rebased. I merged many of the adaptation commits (GNRC, openwsn) to satisfy |
benpicco
added
CI: ready for build
jia200x
force-pushed
the
pr/at86rf2xx_radio_hal_v3
branch
from
54ee7f4
to
0cf19ff
Compare
|
Everything seems to be fine now. |
|
I did some tests and managed to produce a case where the device gets stuck: If I ping the device from another node, it gets 'unstuck' again: Now this might just be another instance of #17924 |
|
Oops, never mind - this is actually the other device ( |
| ifneq (,$(filter netdev,$(USEMODULE))) | ||
| USEMODULE += netdev_ieee802154_submac | ||
| DEFAULT_MODULE += netdev_ieee802154_oqpsk | ||
| endif | ||
|
|
||
| # only needed for SPI based variants | ||
| ifeq (,$(filter at86rfa1 at86rfr2,$(USEMODULE))) |
There was a problem hiding this comment.
| ifeq (,$(filter at86rfa1 at86rfr2,$(USEMODULE))) | |
| ifeq (,$(filter at86rfa1 at86rfr2,$(USEMODULE))) | |
| ifneq (,$(filter ieee802154_security,$(USEMODULE))) | |
| USEMODULE += at86rf2xx_aes_spi | |
| endif |
There was a problem hiding this comment.
I would keep it optional whether to use hardware offloading. IIRC pings were actually faster without.
There was a problem hiding this comment.
That's probably due to all the spi_aquire() - we should move those to the entry points of rf_ops instead of doing it for every register read, but that's for a follow-up.
There was a problem hiding this comment.
and at86rf230 does not have this feature
There was a problem hiding this comment.
should I then just leave it as it is? or catch the at86rf230 case?
There was a problem hiding this comment.
Ok, we would not need cipher_modes if we know there is only at86rf2xx in use and for example not an additional at86rf215 or other 802.15.4 transceiver (I know its probably an uncommon case). To gain more from it we should test if we need cipher_ops or at86rf2xx_aes_spi.
USEMODULE += ieee802154_security
text data bss dec hex filename
106496 272 22328 129096 1f848 /home/fabian/forks/RIOT/examples/gnrc_networking/bin/miot-nucleo-f767zi/gnrc_networking.elf
ping -s 120 -c 50 fe80::4c49:6f54:4f76:102%7
2023-02-24 19:41:44,944 # --- fe80::4c49:6f54:4f76:102%7 PING statistics ---
2023-02-24 19:41:44,950 # 50 packets transmitted, 50 packets received, 0% packet loss
2023-02-24 19:41:44,953 # round-trip min/avg/max = 22.881/24.924/28.171 ms
USEMODULE += "ieee802154_security at86rf2xx_aes_spi"
text data bss dec hex filename
107328 272 22328 129928 1fb88 /home/fabian/forks/RIOT/examples/gnrc_networking/bin/miot-nucleo-f767zi/gnrc_networking.elf
ping -s 120 -c 50 fe80::4c49:6f54:4f76:102%7
2023-02-24 19:45:36,758 # --- fe80::4c49:6f54:4f76:102%7 PING statistics ---
2023-02-24 19:45:36,764 # 50 packets transmitted, 50 packets received, 0% packet loss
2023-02-24 19:45:36,768 # round-trip min/avg/max = 26.763/29.079/31.253 ms
There was a problem hiding this comment.
We should probably introduce a netdev_ieee802154_submac_soft_cipher (netdev_ieee802154_submac_sw_cipher?) (analogous to the proposed netdev_ieee802154_submac_soft_retrans) that drivers without HW crypto can then select.
There was a problem hiding this comment.
Sounds good. If we have that the right modules are selected dynamically base on the transceivers.
But changing the dependency of at86rf2xx_aes_spi in this PR is not really advisable.
Or would you still do it here?
There was a problem hiding this comment.
I have no strong feelings about this, I was just surprised that it was not build in.
That's probably also why @jia200x didn't notice that the current version crashes when at86rf2xx_aes_spi is enabled, because when you want to test ieee802154_security it just works - because the HW acceleration is not used.
There was a problem hiding this comment.
then should I just leave it as it is for now? We can address that in a follow up.
| const ieee802154_radio_cipher_ops_t *cipher_ops; | ||
| if ((cipher_ops = ieee802154_radio_get_cipher_ops(&submac->dev))) { | ||
| netdev_ieee802154->sec_ctx.dev.cipher_ops = cipher_ops; | ||
| netdev_ieee802154->sec_ctx.dev.ctx = &submac->dev; |
There was a problem hiding this comment.
Since it was basically submac->dev.priv before, benpiccos recent suggestions would be necessary.
It's actually not too hard to enable the CCA Done interrupt. I can give it a try. |
| @@ -195,6 +195,9 @@ static ieee802154_dev_t *_reg_callback(ieee802154_dev_type_t type, void *opaque) | |||
| case IEEE802154_DEV_TYPE_MRF24J40: | |||
| printf("mrf24j40"); | |||
| break; | |||
| case IEEE802154_DEV_TYPE_AT86RF2XX: | |||
There was a problem hiding this comment.
I tried to run the test with CONTINUE_ON_EXPECTED_ERRORS=1 USEMODULE+=at86rf233 BOARD=nucleo-f767zi.
I get linking errors:
/usr/lib/gcc/arm-none-eabi/12.2.0/../../../../arm-none-eabi/bin/ld: /home/fabian/forks/RIOT/tests/ieee802154_submac/bin/nucleo-f767zi/netdev_ieee802154_submac/netdev_ieee802154_submac.o: in function `ieee802154_submac_bh_request':
/home/fabian/forks/RIOT/drivers/netdev_ieee802154_submac/netdev_ieee802154_submac.c:133: multiple definition of `ieee802154_submac_bh_request'; /home/fabian/forks/RIOT/tests/ieee802154_submac/bin/nucleo-f767zi/application_tests_ieee802154_submac/main.o:/home/fabian/forks/RIOT/tests/ieee802154_submac/main.c:174: first defined here
/usr/lib/gcc/arm-none-eabi/12.2.0/../../../../arm-none-eabi/bin/ld: /home/fabian/forks/RIOT/tests/ieee802154_submac/bin/nucleo-f767zi/netdev_ieee802154_submac/netdev_ieee802154_submac.o: in function `ieee802154_submac_ack_timer_set':
/home/fabian/forks/RIOT/drivers/netdev_ieee802154_submac/netdev_ieee802154_submac.c:144: multiple definition of `ieee802154_submac_ack_timer_set'; /home/fabian/forks/RIOT/tests/ieee802154_submac/bin/nucleo-f767zi/application_tests_ieee802154_submac/main.o:/home/fabian/forks/RIOT/tests/ieee802154_submac/main.c:134: first defined here
/usr/lib/gcc/arm-none-eabi/12.2.0/../../../../arm-none-eabi/bin/ld: /home/fabian/forks/RIOT/tests/ieee802154_submac/bin/nucleo-f767zi/netdev_ieee802154_submac/netdev_ieee802154_submac.o: in function `ieee802154_submac_ack_timer_cancel':
/home/fabian/forks/RIOT/drivers/netdev_ieee802154_submac/netdev_ieee802154_submac.c:153: multiple definition of `ieee802154_submac_ack_timer_cancel'; /home/fabian/forks/RIOT/tests/ieee802154_submac/bin/nucleo-f767zi/application_tests_ieee802154_submac/main.o:/home/fabian/forks/RIOT/tests/ieee802154_submac/main.c:138: first defined here
collect2: error: ld returned 1 exit status
make: *** [/home/fabian/forks/RIOT/tests/ieee802154_submac/../../Makefile.include:740: /home/fabian/forks/RIOT/tests/ieee802154_submac/bin/nucleo-f767zi/tests_ieee802154_submac.elf] Fehler 1
There was a problem hiding this comment.
that board pulls an netdev_eth, which selects netdev. As a result, the build system pulls netdev_ieee802154_submac, which shouldn't be there...
The proper fix for this is #19052. I will try to come up with a workaround
There was a problem hiding this comment.
This is hard to fix as it is now :/. How should we proceed? Should we try to get #19052?
There was a problem hiding this comment.
Is this a blocker for this PR?
It only happens when manually adding the module to the test, right?
There was a problem hiding this comment.
yes, just because we cannot provide netdev and the radio HAL at the same time for IEEE 802.15.4 devices
There was a problem hiding this comment.
How is the border router going to work then?
There was a problem hiding this comment.
hmmm wait, it's definitely possible to run the nrf52840 with a border router... I think it hast to do with the way this driver is modeled.
It's still not possible to run the HAL and netdev_ieee802154 variants at the same time, but it shouldn't affect the BR. I will give it a look.
There was a problem hiding this comment.
ok, the reason is because the submac callbacks are just C functions that should be defined somewhere. This would only happen with this test application (as netdev_ieee802154_submac already implements these callbacks and therefore there won't be duplicated symbols).
So yes, this only occurs in tests/ieee802154_submac
There was a problem hiding this comment.
Not sure if it is a real solution but at least it links fine with DISABLE_MODULE += netdev_ieee802154_submac in tests/ieee802154_submac/Makefile
jia200x
force-pushed
the
pr/at86rf2xx_radio_hal_v3
branch
from
0cf19ff
to
a8990e5
Compare
|
This needs a rebase - please squash while you’re at it |
|
@jia200x ping ;) |
Could we maybe do this in a follow up? The existing driver also didn't implement the CCA_DONE interrupt and just did polling. |
| @@ -52,22 +56,24 @@ void auto_init_at86rf2xx(void) | |||
| for (unsigned i = 0; i < AT86RF2XX_NUM; i++) { | |||
| LOG_DEBUG("[auto_init_netif] initializing at86rf2xx #%u\n", i); | |||
|
|
|||
| at86rf2xx_setup(&at86rf2xx_devs[i], &at86rf2xx_params[i], i); | |||
| at86rf2xx_init_event(&at86rf2xx_bhp[i], &at86rf2xx_params[i], &at86rf2xx_netdev[i].submac.dev, EVENT_PRIO_HIGHEST); | |||
There was a problem hiding this comment.
We should check for the return value here, to avoid a crash when the driver init fails, in case of a hardware fault or misconfiguration. For example:
int init;
if ((init = at86rf2xx_init_event(&at86rf2xx_bhp[i], &at86rf2xx_params[i],
&at86rf2xx_netdev[i].submac.dev, EVENT_PRIO_HIGHEST))) {
LOG_ERROR("at86rf2xx #%u init failed: %i\n", i, init);
continue;
}Similar for lwip, openthread, openwsn, I think.
Contribution description
This PR is the third (and hopefully last) attempt to port
at86rf2xxseries to the Radio HAL.This one should work with Basic Mode and all the memory-mapped variants (although I don't have one to test... @maribu?)
Since discussions during the breakout sessions in the RIOT summit lead to the deprecation of the generic Bottom Half Processor modules in favour of Event Thread like BHP, I just implemented it like that in this PR. I provide two init functions now:
at86rf2xx_init) that receives a callback, which indicates IRQ offload through aat86rf2xx_irq_handlerfunction.eventbased init (at86rf2xx_init_event) that offloads IRQs to a given event thread queue.So far we have always assumed the IRQ offloading runs in the same thread context as the driver, which leads to undesired effects. Unless the driver is explicitly designed to run in "polling mode", IMO this pattern only causes headaches and shows little benefits compared to just processing IRQ as they are supposed to be. In fact, this facilitates upper layer code with snippets like:
Which are of course not possible if the IRQ offload runs in the same context. And note that many radios are memory mapped or have a dedicated SPI for the radio, which means this does not necessarily translates to an extra thread. Even if we need an extra thread, it can be tiny, since it only executes a couple of functions to retrieve the IRQ.
This is also beneficial for dual-radios (e.g
at86rf215) which currently uses 2 threads for offloading. With this approach, it would need only one.Note that given the proposed intiialization architecture, it's still possible to run everything on a single thread, at the cost of complex logic (e.g re-schedule packet transmissions if there are pending IRQs).
Testing procedure
The following test/suites should work:
tests/driver_at86rf2xx*I tested GNRC, LWIP,
driver_at86rf2xx*. The radio seems to be running stable:Issues/PRs references
Depends on #18988