nrf52840: implement Radio HAL by jia200x · Pull Request #14802 · RIOT-OS/RIOT

jia200x · 2020-08-20T09:30:04Z

Contribution description

Following #14791 , this PR implements the Radio HAL API for nrf52840. As documented in #14792, this PR also expands the ieee802154_hal test to make it compatible with this radio.

Testing procedure

Please follow the test procedure described in #14791.

Issues/PRs references

#14792
#13943

jia200x · 2020-09-04T10:04:32Z

rebased to current master

cpu/nrf52/radio/nrf802154/nrf802154_radio.c

jia200x · 2020-09-07T15:29:41Z

I think I addressed all comments

benpicco

Looking good.
Travis spotted a white space issue and the case should not be indented as per Linux coding style mandated by coding conventions (and IMHO less deep indentations are more readable anyway)

If @bergzand doesn't have any objections, let's move forward with this as well!

cpu/nrf52/radio/nrf802154/nrf802154_radio.c

jia200x · 2020-09-09T11:39:23Z

everything was addressed in 8358bb6

benpicco

Looking good!
Only some style nits, including those from Travis.

cpu/nrf52/radio/nrf802154/nrf802154_radio.c

benpicco · 2020-09-09T19:06:25Z

Murdock is not happy.

jia200x · 2020-09-10T12:37:23Z

since #14997 is imminent, I implemented the off function here :)

jia200x · 2020-09-10T12:53:26Z

I think I addressed all comments, plus some stuff found by Vera++.
I added the IFS definitions to common ieee802154.h header

benpicco · 2020-09-10T13:21:39Z

Murdock is still unhappy.

benpicco · 2020-09-10T13:51:30Z

cpu/nrf52/radio/nrf802154/nrf802154_radio.c

+        dev->cb(dev, IEEE802154_RADIO_INDICATION_RX_DONE);
+    }
+    else if (chan == MAC_TIMER_CHAN_IFS) {
+        cfg.ifs = false;


Should this generate a notification to the submac that the next frame can now be transmitted?

Hmmm this is the only radio that doesn't handle IFS (I think it's quite rare...).
All the other radios might report "BUSY" when trying to send within the IFS. This is what we have here.

So maybe I would keep it as it is, otherwise we would need to implement a generic IFS handler for only one radio

Do I get it right that the IFS period starts after the TX done event?
Or do you say other radios will only report TX done after the IFS period is over?

(I'm almost certain at86rf215 won't do any internal calculations w.r.t. timings as I already had do the calculations for all the other timings manually. They'll probably differ between modulations too.)

No delay between the end of RF signal and TXFE on at86rf215

Do I get it right that the IFS period starts after the TX done event?
Or do you say other radios will only report TX done after the IFS period is over?

I'm not sure when this event is generated, but I'm sure most radios already handle the IFS (maybe @PeterKietzmann can confirm this). The actually, the nrf802154 supports hardware IFS but it's disabled in order to use the fast mode (this means, switching between TRX_OFF to TX_ON in 40 us). I think the trade off is totally worth it for this radio.

They'll probably differ between modulations too.

Yes, it depends on the modulation (it's expressed in symbol times).

We could just introduce another radio capability for this

benpicco · 2020-09-10T13:52:13Z

cpu/nrf52/radio/nrf802154/nrf802154_radio.c

+        return -EINVAL;
+    }
+
+    NRF_RADIO->FREQUENCY = (((uint8_t) conf->channel) - 10) * 5;


Is 5 channel spacing?

I don't know, I simply copied this line from the original netdev driver.

From what I read from the datasheet, yes. It seems to be the channel spacing.

Basically, the center frequency is 2400 + FREQUENCY (in MHz). I will maybe take the opportunity to add a comment here

Yea 5 MHz channels make sense for O-QPSK.

If it turns out that most radios will allow to set the frequency directly, there would be even more reason to move the channel calculations to the submac 😉
That would also ease supporting multiple regions in the sub-GHz band (868 MHz vs 902 MHz, etc)

yes, we can do that. Actually, most O-QPSK radios already have a frequency register (even the at86rf2xx)

I checked again. On at86rf2xx and at86rf215 you set the base frequency (and channel spacing), then you can write the channel register and the radio will calculate frequency + chan * spacing internally.

benpicco · 2020-09-10T14:57:09Z

Please squash!

tests/ieee802154_hal/Makefile

benpicco · 2020-09-11T17:30:45Z

Looks like auto_init_nrf802154.c still needs an update.

jia200x requested review from a user, benpicco and bergzand August 20, 2020 09:30

jia200x requested review from MichelRottleuthner, aabadie, fjmolinas, haukepetersen, leandrolanzieri and smlng as code owners August 20, 2020 09:30

jia200x mentioned this pull request Aug 20, 2020

[tracker] ieee802154_hal: implementation status #14792

Open

8 tasks

jia200x force-pushed the pr/nrf52840_radio_hal branch 2 times, most recently from 8b23aba to 1002b64 Compare September 4, 2020 10:04

benpicco added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 4, 2020

benpicco reviewed Sep 4, 2020

View reviewed changes

aabadie mentioned this pull request Sep 4, 2020

cpu/cc2538/radio: only build required files #14951

Merged

benpicco reviewed Sep 4, 2020

View reviewed changes

cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved

jia200x force-pushed the pr/nrf52840_radio_hal branch 2 times, most recently from 6ba21fd to b26a79e Compare September 7, 2020 15:29

benpicco reviewed Sep 8, 2020

View reviewed changes

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 9, 2020

benpicco reviewed Sep 9, 2020

View reviewed changes

This was referenced Sep 10, 2020

RDM: The 802.15.4 Radio HAL #13943

Merged

ieee802154_hal: add cap for register retention during off #14997

Merged

benpicco reviewed Sep 10, 2020

View reviewed changes

jia200x force-pushed the pr/nrf52840_radio_hal branch from 21d655d to b3dc8cd Compare September 10, 2020 14:55

jia200x added 3 commits September 10, 2020 17:01

net/ieee802154.h: add IFS constants

c74ee68

nrf52840: add radio hal implementation

377560f

tests/ieee802154_hal: add nrf802154

d523ae3

jia200x force-pushed the pr/nrf52840_radio_hal branch from b3dc8cd to 83257a7 Compare September 10, 2020 15:01

benpicco reviewed Sep 10, 2020

View reviewed changes

tests/ieee802154_hal/Makefile Show resolved Hide resolved

tests/ieee802154_hal: use netdev_default

5ec4a7f

jia200x force-pushed the pr/nrf52840_radio_hal branch from 83257a7 to 5ec4a7f Compare September 10, 2020 15:24

benpicco approved these changes Sep 10, 2020

View reviewed changes

benpicco merged commit f75ca11 into RIOT-OS:master Sep 10, 2020

benpicco mentioned this pull request Oct 8, 2020

nrf802154: Add ACK handling capabilities #11150

Closed

11 tasks

This was referenced Nov 16, 2020

drivers/ata8510: Microchip ATA8510 radio module driver, with test #14852

Open

drivers/rail: efr32 radio driver #9212

Closed

Conversation

jia200x commented Aug 20, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

jia200x commented Sep 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jia200x commented Sep 7, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jia200x commented Sep 9, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benpicco commented Sep 9, 2020

Uh oh!

jia200x commented Sep 10, 2020

Uh oh!

jia200x commented Sep 10, 2020

Uh oh!

benpicco commented Sep 10, 2020

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!

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!

benpicco commented Sep 10, 2020

Uh oh!

Uh oh!

benpicco commented Sep 11, 2020