ieee802154/radio_hal: introduce Radio HAL for IEEE802.15.4 compatible radios by jia200x · Pull Request #14371 · RIOT-OS/RIOT

jia200x · 2020-06-26T11:33:31Z

Contribution description

This PR introduces the IEEE802.15.4 Radio HAL (described in #13943 ).
As discussed, the idea is to provide a more stable implementation before finishing the RDM.

~~The API definition comes along with the implementation for CC2538. A test application will be provided soon~~ Moved to #14655

Testing procedure

See #14655

Issues/PRs references

#13943

miri64 · 2020-06-28T19:45:39Z

I did not follow all the discussion in #13943 that closely but how come it is now that the CC2538 is the PoC? I thought nrf802154 and at86rf2xx where your "ends of two extremes" candidates.

miri64 · 2020-06-28T19:46:25Z

(I like that for an API overhaul it is "only" 1500 additions :-))

jia200x · 2020-06-29T08:16:51Z

I did not follow all the discussion in #13943 that closely but how come it is now that the CC2538 is the PoC? I thought nrf802154 and at86rf2xx where your "ends of two extremes" candidates.

I will add them 2. This was only a third radio :)

jia200x · 2020-06-29T08:18:13Z

(I like that for an API overhaul it is "only" 1500 additions :-))

For the API itself it's mostly documentation 🙈

miri64 · 2020-06-29T08:57:14Z

(I like that for an API overhaul it is "only" 1500 additions :-))

For the API itself it's mostly documentation 🙈

Even better! :-)

miri64

Minor style nit-pick for the start

sys/include/net/ieee802154/radio.h

PeterKietzmann

@jia200x thanks for the PR. I would really like to get my hands at it, thus, a test application (as well as the other drivers we agreed on) would be a very valuable next step.

cpu/cc2538/radio/cc2538_rf_radio_ops.c

sys/include/net/ieee802154/radio.h

cpu/cc2538/radio/cc2538_rf.c

sys/include/net/ieee802154/radio.h

cpu/cc2538/radio/cc2538_rf_radio_ops.c

sys/include/net/ieee802154/radio.h

jia200x · 2020-07-29T11:18:51Z

Implemented what I mentioned in #13943 (comment)

PeterKietzmann · 2020-07-30T09:00:07Z

With #14655 in place, you could remove the CC2538 changes from this PR here, right?

jia200x · 2020-07-30T09:02:11Z

With #14655 in place, you could remove the CC2538 changes from this PR here, right?

Yes, it's the idea. I just wanted people to validate the latest fixups before squashing and moving all CC2538 commits there

jia200x · 2020-07-30T09:03:46Z

all comments were addressed. May I squash and move out the CC2538 commits?

PeterKietzmann · 2020-07-30T09:08:55Z

Yes.

jia200x · 2020-07-30T09:13:58Z

done!

PeterKietzmann

Several doxygen warnings:

RIOT/sys/include/net/ieee802154/radio.h:292: warning: unable to resolve reference to `ieee802154_radio_ops::transmit' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:292: warning: unable to resolve reference to `ieee802154_radio_ops::transmit' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:461: warning: unable to resolve reference to `IEEE802154_RADIO_CONFIRM_CCA' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:464: warning: unable to resolve reference to `call' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:391: warning: unable to resolve reference to `IEEE802154_RADIO_CONFIRM_AWAKE_END' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:394: warning: unable to resolve reference to `call' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:396: warning: unable to resolve reference to `IEEE802154_TRX_STATE_TRX_OFF' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:426: warning: unable to resolve reference to `IEEE802154_RADIO_CONFIRM_TX_READY' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:427: warning: unable to resolve reference to `IEEE802154_RADIO_CONFIRM_RX_READY' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:428: warning: unable to resolve reference to `IEEE802154_RADIO_CONFIRM_TRX_OFF_READY' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:431: warning: unable to resolve reference to `call' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:292: warning: unable to resolve reference to `ieee802154_radio_ops::transmit' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:296: warning: unable to resolve reference to `IEEE802154_RADIO_CONFIRM_TX_DONE' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:323: warning: unable to resolve reference to `IEEE802154_RADIO_RX_DONE' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:363: warning: unable to resolve reference to `IEEE802154_TRX_STATE_TRX_OFF' for \ref command
RIOT/sys/include/net/ieee802154/radio.h:278: warning: unable to resolve reference to `IEEE802154_TRX_STATE_TX_ON' for \ref command

sys/include/net/ieee802154/radio.h

PeterKietzmann · 2020-07-30T10:15:27Z

sys/include/net/ieee802154/radio.h

+ */
+typedef enum {
+    /**
+     * @brief transmission uses Auto CCA or frame retransmissions with CSMA-CA


Mention that compiling firmware with both features enabled would not make sense and explain how to figure out if it is actually CSMA or CCA

this would change anyway with the proposal in #14371 (comment)

PeterKietzmann · 2020-07-30T10:20:50Z

sys/include/net/ieee802154/radio.h

+     * @return 0 on success
+     * @return negative errno on error
+     */
+    int (*config_phy)(ieee802154_dev_t *dev, ieee802154_phy_conf_t *conf);


Would misconfiguration of conf->pow lead to an error? If set to 1000dBm, should the device set the maximum transmission power or return an error? For channel and page it is quite clear that errors should be reported.

I would say an error should be reported.

See e941563

Should we also be able to configure channel spacing, center frequency, chip rate etc here?
A solution could be to extend ieee802154_phy_conf_t like so

typedef struct { ieee802154_phy_conf_t base; uint8_t rate; uint8_t chips; … } ieee802154_mr_oqpsk_phy_conf_t;

without having to change the API.

exactly! That's why I added in the beginning an ieee802154_phy_config_t struct, to add members without breaking the API. But never thought about extending it, so that would be really nice!

What's a MIB?

Th MAC Information Base, the list of variables that keep track of the MAC state.

Does this yet exist?

hmmm we have traces of it in the netdev_ieee802154_t struct (channel, page, etc).
But the idea is that this should be handled by the upper layer anyway.

what do you think?

Yea we'll need a place to manage channel spacing, center frequency, allowed channels, rates, etc. based on region anyway.

The drivers shouldn't need to worry which part of the world they are used in but just receive configuration parameters to comply with the local regulations from an upper layer that keeps all those gory tables contained in one place (and ideally configurable at compile-time, so we don't need to store the tables for the US, China, etc. in every device - would be nice if that was an option though)

PeterKietzmann · 2020-07-30T11:30:12Z

sys/include/net/ieee802154/radio.h

+     * @brief Confirmation function for @ref ieee802154_radio_ops::request_cca
+     *
+     * This function must be called to finish the CCA procedure.  This
+     * function should be called on @ref IEEE802154_RADIO_CONFIRM_CCA,


Isn't it more that this function waits for IEEE802154_RADIO_CONFIRM_CCA to be called which unlocks the mutex that was locked after the CCA request?

Wait, I think I misread here. Adding a mutex is a use case specific thing. The IEEE802154_RADIO_CONFIRM_CCA event doesn't necessarily unlock a mutex.

PeterKietzmann · 2020-07-30T11:46:20Z

sys/include/net/ieee802154/radio.h

+     *            NULL if this information is not needed.
+     *
+     * @return number of bytes written in @p buffer (0 if @p buf == NULL)
+     * @return -EINVAL if the CRC is not valid. Frame is dropped immediately.


I think this need update after the latest agreements on how to handle CRC.

PeterKietzmann · 2020-07-30T12:23:41Z

All in all I'm fine with the API (besides minor change requests). I would like to focus on testing the PoC in #14655 to prove the API is acting as expexted. As indicated there, a rudimentary "test specification" would be nice.

jia200x · 2020-08-13T13:54:41Z

Uncrustify did some very weird stuff this time... I re-aranged some parts of the documentation and now looks much better.

jia200x · 2020-08-13T13:55:10Z

Since there's nothing to test here, should we skip Murdock?

jia200x · 2020-08-13T14:04:33Z

@maribu @PeterKietzmann do you have more comments?

miri64 · 2020-08-13T14:25:28Z

Since there's nothing to test here, should we skip Murdock?

We can't, but we can skip compilation :-).

benpicco · 2020-08-13T14:31:42Z

It's a slow day on Murdock and there are no jobs scheduled, giving it a spin won't hurt to make sure no compiler complains.

miri64 · 2020-08-13T14:35:21Z

Ok, let's take it for a spin ;-)

jia200x · 2020-08-13T15:13:41Z

Murdock is super happy :)

miri64 · 2020-08-13T15:18:37Z

Should this be merged now or do we wait for the three ACKs in #13943 first? Feels weird to merge the API definition before the design document for it is merged.

miri64 · 2020-08-13T15:20:03Z

But this all is not set in stone.

I guess since RDMs are somewhat set in stone, the plan is to merge this first and adapt the RDM from experience?

jia200x · 2020-08-13T15:22:32Z

I guess since RDMs are somewhat set in stone, the plan is to merge this first and adapt the RDM from experience?

Yes, that would be ideal. I expect the API to change a bit while it's experimental. When this is more stable, I would adopt the RDM as needed.

jia200x · 2020-08-14T09:59:42Z

sys/include/net/ieee802154/radio.h

+     * function should return -EINVAL
+     *
+     * @pre the device is on
+     * @post the upper layer calls @ref


Let's remove this post condition. I think it's always possible to ensure the radio will still be working in the corresponding state after calling this function.

Otherwise it becomes hard for the implementation of some network stacks (e.g the OpenThread contrib would need to keep track of the last state when calling this function).

Yea the operating state should not change by calling this function.

benpicco · 2020-08-19T08:46:44Z

Want to do that last doc update (& squash) so we can merge this? 😉

jia200x · 2020-08-19T09:58:00Z

sure! I will :)

jia200x · 2020-08-19T10:06:32Z

Amended directly. Let's wait for Murdock

jia200x · 2020-08-19T11:06:15Z

Green :)

jia200x · 2020-08-19T11:09:42Z

thank you so much for the review @benpicco @PeterKietzmann @maribu @miri64 !!!!!

jia200x added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Jun 26, 2020

jia200x requested review from PeterKietzmann, benpicco, bergzand, fjmolinas, kaspar030, maribu and miri64 June 26, 2020 11:33

jia200x requested a review from smlng as a code owner June 26, 2020 11:33

miri64 reviewed Jun 29, 2020

View reviewed changes

sys/include/net/ieee802154/radio.h Outdated Show resolved Hide resolved

sys/include/net/ieee802154/radio.h Outdated Show resolved Hide resolved

PeterKietzmann reviewed Jun 30, 2020

View reviewed changes

jia200x mentioned this pull request Jul 13, 2020

RDM: The 802.15.4 Radio HAL #13943

Merged

jia200x mentioned this pull request Jul 29, 2020

[POC] ieee802154_hal: tester PR for #14371 #14655

Closed

jia200x force-pushed the pr/radio_hal branch 2 times, most recently from ebeb695 to 149349b Compare July 30, 2020 09:12

PeterKietzmann reviewed Jul 30, 2020

View reviewed changes

jia200x force-pushed the pr/radio_hal branch from bc2aceb to 2ce4091 Compare August 13, 2020 13:54

jia200x force-pushed the pr/radio_hal branch from 2ce4091 to 0e5bc0f Compare August 13, 2020 14:02

miri64 added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Aug 13, 2020

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Aug 13, 2020

jia200x commented Aug 14, 2020

View reviewed changes

ieee802154/radio: add radio HAL definition

b6e3afe

jia200x force-pushed the pr/radio_hal branch from 0e5bc0f to b6e3afe Compare August 19, 2020 10:06

benpicco merged commit 7f35961 into RIOT-OS:master Aug 19, 2020

jia200x mentioned this pull request Aug 19, 2020

[tracker] ieee802154_hal: implementation status #14792

Open

8 tasks

This was referenced Sep 4, 2020

ieee802154_submac: add initial support for common MAC sub layer #14950

Merged

Add other radios support for OpenThread pkg #10045

Closed

ieee802154_hal: add cap for register retention during off #14997

Merged

benpicco mentioned this pull request Nov 22, 2020

drivers/rail: efr32 radio driver #9212

Closed

12 tasks

fjmolinas mentioned this pull request Dec 11, 2020

sys/inet/ieee802154/radio: fix wrong rssi conversions treewide #15616

Merged

Conversation

jia200x commented Jun 26, 2020 • 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!

miri64 commented Jun 28, 2020

Uh oh!

miri64 commented Jun 28, 2020

Uh oh!

jia200x commented Jun 29, 2020

Uh oh!

jia200x commented Jun 29, 2020

Uh oh!

miri64 commented Jun 29, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PeterKietzmann 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!

Uh oh!

Uh oh!

jia200x commented Jul 29, 2020

Uh oh!

PeterKietzmann commented Jul 30, 2020

Uh oh!

jia200x commented Jul 30, 2020

Uh oh!

jia200x commented Jul 30, 2020

Uh oh!

PeterKietzmann commented Jul 30, 2020

Uh oh!

jia200x commented Jul 30, 2020

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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 Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jia200x commented Jun 26, 2020 •

edited

Loading

benpicco Aug 11, 2020 •

edited

Loading

jia200x commented Aug 13, 2020 •

edited

Loading

benpicco Aug 14, 2020 •

edited

Loading