ieee802154/radio_hal: detach hal descriptor from driver by jia200x · Pull Request #16534 · RIOT-OS/RIOT

jia200x · 2021-06-07T13:25:53Z

Contribution description

This PR re-arranges the Radio HAL descriptor to be allocated outside of the private descriptor of radios.

In our current scheme, we usually allocate the upper layer descritptor (netdev, hal, etc) inside the private descriptor of the driver.
Therefore we assume it's possible to cast the private descriptor to the interface (and back). However, this approach has some disadvantages:

It requires more RAM than allocating the HAL descriptor close to the network stack. The upper layer descriptor requires a pointer to the "context" of the upper layer (e.g the radio HAL context points to the SubMAC if used within GNRC). At the same time, the network stack require to store pointers to the HAL descriptor, because the network stack descriptors and the HAL don't share the same location. If the HAL is allocated closed to the network stack (e.g in the SubMAC), only a pointer to the private descriptor is needed. After all, all ieee802154_rf_ops functions simply use the HAL descriptor (ieee802154_dev_t) to fetch the private descriptor. The upper layer can always retrieve the network stack descriptor if they were allocated together (e.g container_of(...)).
The relation of HAL <-> descriptor is not one to one. E.g there are devices that implement 2 radios (at86rf215). Therefore we can simplify a lot the code if we allocate one private descriptor and point it from 2 HAL instances.
EDIT: Also, there could be cases were several HAL types point to the same device descriptor. An example of this is radios that support crypto acceleration.

From now on, ieee802154_dev_t's ctx was renamed to priv in order to store the private descriptor of the radios. All Radio HAL functions can access the private descriptor simply following the pointer. Radio events must be boostrapped in order to pass the pointer to the HAL on ISR (this is already done in the current Radio HAL implementations, but it will require to pass the HAL pointer to the args of the ISR in SPI radios).

A follow-up PR with the WIP HAL implementation of at86rf2xx will show this mechanism.

Testing procedure

Make sure all radio HAL radios still work properly (cc2538, nrf52840). Use e.g examples/gnrc_networking.

Issues/PRs references

Depends on #16533

jia200x · 2021-08-16T14:24:22Z

Ping results between cc2538 and nrf802154:

2021-08-16 16:23:17,957 # ping fe80::ac8d:fee1:6089:34c4
2021-08-16 16:23:17,972 # 12 bytes from fe80::ac8d:fee1:6089:34c4%6: icmp_seq=0 ttl=64 rssi=-24 dBm time=7.501 ms
2021-08-16 16:23:18,971 # 12 bytes from fe80::ac8d:fee1:6089:34c4%6: icmp_seq=1 ttl=64 rssi=-24 dBm time=6.099 ms
2021-08-16 16:23:19,973 # 12 bytes from fe80::ac8d:fee1:6089:34c4%6: icmp_seq=2 ttl=64 rssi=-24 dBm time=8.386 ms
2021-08-16 16:23:19,974 # 
2021-08-16 16:23:19,978 # --- fe80::ac8d:fee1:6089:34c4 PING statistics ---
2021-08-16 16:23:19,982 # 3 packets transmitted, 3 packets received, 0% packet loss
2021-08-16 16:23:19,987 # round-trip min/avg/max = 6.099/7.328/8.386 ms

benpicco · 2021-08-16T14:28:11Z

Looks like this needs a rebase

jia200x · 2021-08-16T14:29:45Z

I just rebased it. I will post the results again just to be sure

jia200x · 2021-08-16T14:30:48Z

ping results are the same:

ping fe80::ac8d:fee1:6089:34c4
2021-08-16 16:29:51,602 # ping fe80::ac8d:fee1:6089:34c4
2021-08-16 16:29:51,619 # 12 bytes from fe80::ac8d:fee1:6089:34c4%6: icmp_seq=0 ttl=64 rssi=-24 dBm time=9.264 ms
2021-08-16 16:29:52,619 # 12 bytes from fe80::ac8d:fee1:6089:34c4%6: icmp_seq=1 ttl=64 rssi=-24 dBm time=9.536 ms
2021-08-16 16:29:53,617 # 12 bytes from fe80::ac8d:fee1:6089:34c4%6: icmp_seq=2 ttl=64 rssi=-24 dBm time=7.462 ms
2021-08-16 16:29:53,618 # 
2021-08-16 16:29:53,622 # --- fe80::ac8d:fee1:6089:34c4 PING statistics ---
2021-08-16 16:29:53,627 # 3 packets transmitted, 3 packets received, 0% packet loss
2021-08-16 16:29:53,631 # round-trip min/avg/max = 7.462/8.754/9.536 ms
>

MrKevinWeiss · 2021-08-16T14:38:40Z

Just to state a design decision that was discuss in as IRL as we can get with Corona.

Moving the ieee802154_dev_t *dev to ieee802154_dev_t dev instantiates the in ieee802154_submac struct, which, in this case, must be in RAM. Theoretically one could have had a const ieee802154_dev_t and saved the 12 bytes of RAM similar how we have *params in our device drivers. With this update it is no longer possible but we gain simplicity by eliminated an unneeded dependency. Since we don't keep it in ROM anyways we decided this was the best way forward.

benpicco · 2021-08-16T16:32:13Z

Murdock is not happy

jia200x · 2021-08-18T10:19:18Z

More test results after the last push:

`tests/ieee802154_hal` between `remote-revb` and `nrf52840`

> 2021-08-18 11:42:30,362 # main(): This is RIOT! (Version: 2021.10-devel-375-g77c659-pr/detach_radio_hal_descriptor)
2021-08-18 11:42:30,364 # Trying to register nrf52840.
2021-08-18 11:42:30,365 # Success
2021-08-18 11:42:30,370 # Initialization successful - starting the shell now
txtsnd ae8dfee1608937c4 10
2021-08-18 11:42:38,483 # txtsnd ae8dfee1608937c4 10
2021-08-18 11:42:38,487 # Transmission succeeded
2021-08-18 11:42:38,488 # 
2021-08-18 11:42:38,488 # 
2021-08-18 11:42:38,491 # Received valid ACK with sqn 0
2021-08-18 11:42:38,492 # 
2021-08-18 11:42:38,494 # LQI: 152, RSSI: 86
2021-08-18 11:42:38,494 # 
> 

> 2021-08-18 11:42:37,167 # main(): This is RIOT! (Version: 2021.10-devel-375-g77c659-pr/detach_radio_hal_descriptor)
2021-08-18 11:42:37,169 # Trying to register cc2538_rf.
2021-08-18 11:42:37,170 # Success
2021-08-18 11:42:37,172 # Initialization successful - starting the shell now
> 2021-08-18 11:42:38,488 # Packet received:
2021-08-18 11:42:38,502 # 61 dc 00 23 00 c4 37 89 60 e1 fe 8d ae da af f6 22 11 08 64 6a 4c 6f 72 65 6d 20 69 70 73 75 
2021-08-18 11:42:38,503 # LQI: 233, RSSI: 119
2021-08-18 11:42:38,503 #

`tests/ieee802154_submac` between `remote-revb` and `nrf52840`

2021-08-18 11:52:32,639 # main(): This is RIOT! (Version: 2021.10-devel-375-g77c659-pr/detach_radio_hal_descriptor)
2021-08-18 11:52:32,640 # Trying to register cc2538_rf.
2021-08-18 11:52:32,641 # Success
2021-08-18 11:52:32,646 # Initialization successful - starting the shell now
> 2021-08-18 11:52:34,305 # 
2021-08-18 11:52:34,306 # DATA
2021-08-18 11:52:34,313 # Dest. PAN: 0x0023, Dest. addr.: ae:8d:fe:e1:60:89:37:c4
2021-08-18 11:52:34,318 # Src. PAN: 0x0023, Src. addr.: 6a:64:08:11:22:f6:af:da
2021-08-18 11:52:34,332 # Security: 0, Frame pend.: 0, ACK req.: 1, PAN comp.: 1
2021-08-18 11:52:34,333 # Version: 1, Seq.: 0
2021-08-18 11:52:34,336 # 00000000  4C  6F  72  65  6D  20  69  70  73  75
2021-08-18 11:52:34,338 # txt: Lorem ipsu
2021-08-18 11:52:34,340 # RSSI: -56, LQI: 233
2021-08-18 11:52:34,340 #

OpenThread seems to be broken for one of these radios on master... So it also doesn't work here. I will investigate but I propose to leave it out of this PR.

`tests/lwip` between `samr21-xpro` and `nrf52840`

2021-08-18 12:15:13,715 # RIOT lwip test application
> udp send [fe80:0:0:0:6864:811:22f6:a7da]:80 AA
2021-08-18 12:15:28,693 # udp send [fe80:0:0:0:6864:811:22f6:a7da]:80 AA
2021-08-18 12:15:28,701 # Success: send 1 byte over UDP to [fe80:0:0:0:6864:811:22f6:a7da]:80
udp send [fe80:0:0:0:6864:811:22f6:a7da]:80 AA
> 2021-08-18 12:15:29,711 # udp send [fe80:0:0:0:6864:811:22f6:a7da]:80 AA
2021-08-18 12:15:29,718 # Success: send 1 byte over UDP to [fe80:0:0:0:6864:811:22f6:a7da]:80
udp send [fe80:0:0:0:6864:811:22f6:a7da]:80 AA
> 2021-08-18 12:15:30,728 # udp send [fe80:0:0:0:6864:811:22f6:a7da]:80 AA
2021-08-18 12:15:30,735 # Success: send 1 byte over UDP to [fe80:0:0:0:6864:811:22f6:a7da]:80

2021-08-18 12:15:10,155 # RIOT lwip test application
udp server start 80
2021-08-18 12:15:26,859 # udp server start 80
2021-08-18 12:15:26,862 # Success: started UDP server on port 80
> 2021-08-18 12:15:28,713 # Received UDP data from [fe80::204:2519:1801:bddb]:49153
2021-08-18 12:15:28,714 # 00000000  AA
2021-08-18 12:15:29,722 # Received UDP data from [fe80::204:2519:1801:bddb]:49154
2021-08-18 12:15:29,723 # 00000000  AA
2021-08-18 12:15:30,738 # Received UDP data from [fe80::204:2519:1801:bddb]:49155
2021-08-18 12:15:30,739 # 00000000  AA

I also squashed all commits to one because as it was it was not possible to ensure it would compile in all cases.

EDIT: The previous ping tests are still valid because I didn't touch any radio/submac files.

MrKevinWeiss · 2021-08-18T12:53:20Z

murdock says there are some problems with openwsn stuff,please look into that.

jia200x · 2021-08-18T13:24:42Z

it should work now

benpicco · 2021-08-18T15:43:20Z

Is tests/ieee802154_hal still working as expected?

MrKevinWeiss · 2021-08-18T15:58:47Z

There we go, murdock is passing.

jia200x · 2021-08-18T16:07:03Z

Is tests/ieee802154_hal still working as expected?

I posted the tests results in #16534 (comment). I changed the initialization.
I was thinking we might use a similar pattern to centralize device descriptors allocation.

jia200x added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: drivers Area: Device drivers labels Jun 7, 2021

jia200x requested review from benpicco, fjmolinas, jeandudey, maribu and miri64 June 7, 2021 13:25

jia200x requested review from PeterKietzmann, cgundogan and haukepetersen as code owners June 7, 2021 13:25

github-actions bot added the Area: sys Area: System label Jun 7, 2021

jia200x mentioned this pull request Jun 7, 2021

[RFC] at86rf2xx: implement radio HAL #16535

Draft

4 tasks

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

jia200x force-pushed the pr/detach_radio_hal_descriptor branch from 483b6cf to 5598c11 Compare August 16, 2021 13:51

jia200x requested review from aabadie, bergzand and smlng as code owners August 16, 2021 13:51

github-actions bot added Area: cpu Area: CPU/MCU ports Area: pkg Area: External package ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Aug 16, 2021

jia200x force-pushed the pr/detach_radio_hal_descriptor branch from 5598c11 to 9a11540 Compare August 16, 2021 14:28

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 16, 2021

jia200x force-pushed the pr/detach_radio_hal_descriptor branch from 9a11540 to 77c6599 Compare August 16, 2021 14:29

benpicco approved these changes Aug 16, 2021

View reviewed changes

jia200x force-pushed the pr/detach_radio_hal_descriptor branch from 77c6599 to dd9e916 Compare August 18, 2021 10:19

jia200x requested review from MichelRottleuthner and leandrolanzieri as code owners August 18, 2021 10:19

github-actions bot added the Area: tests Area: tests and testing framework label Aug 18, 2021

jia200x force-pushed the pr/detach_radio_hal_descriptor branch 2 times, most recently from 321d699 to 2801ade Compare August 18, 2021 11:20

ieee802154/radio_hal: detach hal descriptor from driver

ddc9c7c

jia200x force-pushed the pr/detach_radio_hal_descriptor branch from 2801ade to ddc9c7c Compare August 18, 2021 13:24

benpicco merged commit 71953c9 into RIOT-OS:master Aug 18, 2021

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ieee802154/radio_hal: detach hal descriptor from driver#16534

ieee802154/radio_hal: detach hal descriptor from driver#16534
benpicco merged 1 commit intoRIOT-OS:masterfrom
jia200x:pr/detach_radio_hal_descriptor

jia200x commented Jun 7, 2021 •

edited

Loading

Uh oh!

jia200x commented Aug 16, 2021

Uh oh!

benpicco commented Aug 16, 2021

Uh oh!

jia200x commented Aug 16, 2021

Uh oh!

jia200x commented Aug 16, 2021

Uh oh!

MrKevinWeiss commented Aug 16, 2021

Uh oh!

benpicco commented Aug 16, 2021

Uh oh!

jia200x commented Aug 18, 2021 •

edited

Loading

Uh oh!

MrKevinWeiss commented Aug 18, 2021

Uh oh!

jia200x commented Aug 18, 2021

Uh oh!

benpicco commented Aug 18, 2021

Uh oh!

MrKevinWeiss commented Aug 18, 2021

Uh oh!

jia200x commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

jia200x commented Aug 16, 2021

Uh oh!

benpicco commented Aug 16, 2021

Uh oh!

jia200x commented Aug 16, 2021

Uh oh!

jia200x commented Aug 16, 2021

Uh oh!

MrKevinWeiss commented Aug 16, 2021

Uh oh!

benpicco commented Aug 16, 2021

Uh oh!

jia200x commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrKevinWeiss commented Aug 18, 2021

Uh oh!

jia200x commented Aug 18, 2021

Uh oh!

benpicco commented Aug 18, 2021

Uh oh!

MrKevinWeiss commented Aug 18, 2021

Uh oh!

jia200x commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jia200x commented Jun 7, 2021 •

edited

Loading

jia200x commented Aug 18, 2021 •

edited

Loading