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

nrf52840: implement Radio HAL #14802

Merged
merged 4 commits into from
Sep 10, 2020
Merged

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Aug 20, 2020

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

jia200x commented Sep 4, 2020

rebased to current master

@benpicco 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
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
@jia200x jia200x force-pushed the pr/nrf52840_radio_hal branch 2 times, most recently from 6ba21fd to b26a79e Compare September 7, 2020 15:29
@jia200x
Copy link
Member Author

jia200x commented Sep 7, 2020

I think I addressed all comments

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
@jia200x
Copy link
Member Author

jia200x commented Sep 9, 2020

everything was addressed in 8358bb6

@benpicco 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
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

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

cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154_radio.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

benpicco commented Sep 9, 2020

Murdock is not happy.

@jia200x
Copy link
Member Author

jia200x commented Sep 10, 2020

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

@jia200x
Copy link
Member Author

jia200x commented Sep 10, 2020

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

@benpicco
Copy link
Contributor

Murdock is still unhappy.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

image

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

We could just introduce another radio capability for this

return -EINVAL;
}

NRF_RADIO->FREQUENCY = (((uint8_t) conf->channel) - 10) * 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 5 channel spacing?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea 5 MHz channels make sense for O-QPSK.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Please squash!

@benpicco benpicco merged commit f75ca11 into RIOT-OS:master Sep 10, 2020
@benpicco
Copy link
Contributor

Looks like auto_init_nrf802154.c still needs an update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants