Skip to content

drivers/kw2xrf: add support for IEEE 802.15.4 Radio HAL#18383

Merged
benpicco merged 9 commits intoRIOT-OS:masterfrom
jia200x:pr/kw2xrf_radio_hal
Aug 16, 2022
Merged

drivers/kw2xrf: add support for IEEE 802.15.4 Radio HAL#18383
benpicco merged 9 commits intoRIOT-OS:masterfrom
jia200x:pr/kw2xrf_radio_hal

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Jul 28, 2022

Contribution description

This PR is a take over of original work by @MichelRottleuthner to port KW2XRF radios to the IEEE 802.15.4 Radio HAL.
The port should be complete and it already improves PRR a lot:

ping fe80::dc01:176a:c1a8:e030 -s 1024 -i 250 -c 1000

2022-07-28 13:28:11,515 # --- fe80::dc01:176a:c1a8:e030 PING statistics ---
2022-07-28 13:28:11,521 # 1000 packets transmitted, 1000 packets received, 0% packet loss
2022-07-28 13:28:11,525 # round-trip min/avg/max = 146.356/176.099/222.387 ms

Strictly speaking this PR changes the API of kw2xrf_init, but IMO adding a deprecation at this level does not make sense.
It also removes the old netdev implementation.

Testing procedure

Try tests/ieee802154_* tests and examples/gnrc_networking for any of the compatible kw2xrf boards.

Issues/PRs references

Depends on #18382

@jia200x jia200x added Area: network Area: Networking Area: tests Area: tests and testing framework Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers labels Jul 28, 2022
@jia200x jia200x requested review from benpicco and maribu July 28, 2022 16:54
@github-actions github-actions bot added the Area: sys Area: System label Jul 28, 2022
@benpicco benpicco requested a review from benemorius July 28, 2022 16:56
benpicco
benpicco reviewed Jul 28, 2022
View reviewed changes
benpicco
benpicco reviewed Jul 28, 2022
View reviewed changes
benpicco
benpicco reviewed Jul 28, 2022
View reviewed changes
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jul 29, 2022

This already needs a rebase

@jia200x jia200x force-pushed the pr/kw2xrf_radio_hal branch from a2b33ac to dc15c18 Compare July 29, 2022 08:45
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jul 29, 2022

This already needs a rebase

done!

benpicco
benpicco reviewed Aug 8, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Code looks good, I don't have the hardware but I trust your testing.

Great to see that this now allows to use the radio HAL with SPI based radios too!

sys/net/gnrc/netif/init_devs/auto_init_kw2xrf.c

}
}
/** @} */
Copy link
Copy Markdown
Contributor

@benpicco benpicco Aug 8, 2022

Choose a reason for hiding this comment

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

Does LWIP also need an update?

Copy link
Copy Markdown
Member Author

@jia200x jia200x Aug 8, 2022

Choose a reason for hiding this comment

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

there's not yet support for this radio there :(. I can provide a port in a follow up

Copy link
Copy Markdown
Contributor

@benpicco benpicco Aug 8, 2022

Choose a reason for hiding this comment

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

Would be good to make sure the new bottom-half processing works with either stack

Copy link
Copy Markdown
Member Author

@jia200x jia200x Aug 9, 2022

Choose a reason for hiding this comment

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

Probably a middle layer could do the trick

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 8, 2022

With the last commits I consistently get:

2022-08-08 17:48:59,263 # --- fe80::dc01:176a:c1a8:e030 PING statistics ---
2022-08-08 17:48:59,269 # 1000 packets transmitted, 1000 packets received, 0% packet loss
2022-08-08 17:48:59,275 # round-trip min/avg/max = 146.812/175.536/208.455 ms

For 1 kB ping packets

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 9, 2022

I think I addresses all comments

benpicco
benpicco reviewed Aug 9, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Please squash if everything still works.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 9, 2022

Please squash if everything still works.

It does:

2022-08-09 12:00:45,748 # --- fe80::dc01:176a:c1a8:e030 PING statistics ---
2022-08-09 12:00:45,753 # 100 packets transmitted, 100 packets received, 0% packet loss
2022-08-09 12:00:45,758 # round-trip min/avg/max = 155.190/176.692/198.572 ms

@jia200x jia200x force-pushed the pr/kw2xrf_radio_hal branch 2 times, most recently from b9363d5 to 679342a Compare August 9, 2022 10:06
@jia200x jia200x force-pushed the pr/kw2xrf_radio_hal branch 2 times, most recently from d3f4d80 to b123abe Compare August 12, 2022 09:14
@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Aug 12, 2022
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 12, 2022

squashed and still everything seems to work.

@jia200x jia200x force-pushed the pr/kw2xrf_radio_hal branch from b123abe to 022c757 Compare August 12, 2022 09:22
@jia200x jia200x added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 12, 2022
@jia200x jia200x force-pushed the pr/kw2xrf_radio_hal branch from 022c757 to 3fdc669 Compare August 15, 2022 08:24
@jia200x jia200x force-pushed the pr/kw2xrf_radio_hal branch from 3fdc669 to 8eb17c8 Compare August 15, 2022 10:11
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 15, 2022

unrelated murdock error...

@jia200x jia200x removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 15, 2022
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Aug 15, 2022
@benpicco benpicco merged commit 0aff42c into RIOT-OS:master Aug 16, 2022
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 16, 2022

thanks for the review!!

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 17, 2022

Just FYI, nightlies are broken. It would be good to at least confirm the build tests are complete after a rebase. I have informed @jia200x and he is working on a fix shortly.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 17, 2022

Though... Other PRs are passing now. One would assume they would be failing if this was a real problem. We really need to sort this stuff out.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 17, 2022

Is it possible that the CI no longer autorebases?

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 17, 2022

ping @kaspar030 could this be? Should I try to test it by opening an PR with an old base using a new feature to show it?

I assumed that when the CI build label is set it would rebase to master.

@MrKevinWeiss MrKevinWeiss mentioned this pull request Aug 17, 2022
Closed
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 17, 2022

Is it possible that the CI no longer autorebases?

Not that I know of.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 17, 2022

Sorry for the noise. This test/board combo is just not tested on the PRs and only the nightlies. This is why other PRs don't see this problem. We can either add it to the list in the .murdock file or live with testing a subset and sometime things getting through (and then applying a quick fix).

Good to see things are at least behaving as expected!

Something like

/bin/bash -c "source .murdock; JOBS=4 compile tests/ieee802154_hal pba-d-01-kw2x:gnu"

Would have found it as well.

This was referenced Aug 18, 2022
Merged
Open
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@benpicco benpicco benpicco approved these changes

@maribu maribu Awaiting requested review from maribu

@smlng smlng Awaiting requested review from smlng

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

@benemorius benemorius Awaiting requested review from benemorius

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

Assignees

No one assigned

Labels

Area: boards Area: Board ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

6 participants

@jia200x @benpicco @MrKevinWeiss @kaspar030 @leandrolanzieri @maribu