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.

Sign up for GitHub

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

Jump to bottom

ieee802154/submac: calculate symbol time on demand #19668

Merged
merged 6 commits into from
Feb 20, 2025
Merged

ieee802154/submac: calculate symbol time on demand #19668

merged 6 commits into from
Feb 20, 2025

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented May 25, 2023 •
edited
Loading

Contribution description

This moves the ACK timeout/CSMA backoff period calculation from the at86rf215 driver to the radio HAL so it can be re-used by other IEEE 802.15.4g drivers.

To do so the ieee802154_phy_conf_t structure is extended depending on the used modulation. This allows to add alternative modulations with different properties (e.g. #19172).

Testing procedure

Currently there is no driver that makes use of this.

Issues/PRs references

alternative to #19198

@benpicco benpicco requested review from jia200x and kYc0o May 25, 2023 14:05
@github-actions github-actions bot added Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels May 25, 2023
@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch 2 times, most recently from 5377f37 to bd982d4 Compare May 25, 2023 16:06
@benpicco benpicco marked this pull request as ready for review May 25, 2023 16:10
@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from bd982d4 to d2b99fc Compare May 25, 2023 16:11
@benpicco benpicco mentioned this pull request May 25, 2023
Open
@benpicco benpicco requested a review from jeandudey May 25, 2023 16:12
@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from d2b99fc to 6fc9722 Compare May 25, 2023 16:42
@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from 6fc9722 to 494bb3b Compare June 5, 2023 12:33
@benpicco benpicco requested review from dylad and keestux as code owners June 5, 2023 12:33
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jun 5, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 19, 2024
@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from f135bbf to b248208 Compare February 19, 2024 18:23
@maribu
Copy link
Member

maribu commented Feb 19, 2024

This seems to need some MAYBE_UNUSED being sprinkled over some function declarations.

@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from b248208 to 2131868 Compare February 19, 2024 21:32
@benpicco benpicco requested review from jia200x and removed request for jia200x February 27, 2024 14:56
@benpicco benpicco requested a review from kfessel March 12, 2024 14:34
kfessel
kfessel reviewed Mar 12, 2024
View reviewed changes
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

looks good (better than before) and seems to pass the exhaustive test i will run that test again and if it succeeds i will approve unless someone else has objections (within reasonable time )

fabian18
fabian18 reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

I would like to understand the calculations. IEEE 802.15.4g-2012 should be sufficient, or do you recall any other document, because I think I have to figure out something similar for LoRa.

I'm glad to have this already.

@fabian18 fabian18 mentioned this pull request Feb 10, 2025
Open
maribu
maribu reviewed Feb 10, 2025
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Looks good to me. There are still some open points from @fabian18's comments.

@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from 2379296 to d977217 Compare February 10, 2025 13:52
@benpicco
Copy link
Contributor Author

Thank you for the ping!

@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from d977217 to ef72aae Compare February 10, 2025 13:56
@kYc0o
Copy link
Contributor

kYc0o commented Feb 12, 2025

Does this need any more testing?

@benpicco
Copy link
Contributor Author

looks like some testing was already provided in #21202 😃

@maribu
Copy link
Member

maribu commented Feb 17, 2025

@fabian18 Could you take another look?

@benpicco benpicco force-pushed the ieee802154_symbol_time_on_demand branch from ef72aae to 73e45dd Compare February 20, 2025 11:00
fabian18
fabian18 approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

looks like some testing was already provided in #21202 😃

Worked well for me.

Does this need any more testing?

For the multi rate modulation calculations I would trust benpiccos knowledge and experience with the at86rf215 transceiver.

@benpicco benpicco added this pull request to the merge queue Feb 20, 2025
Merged via the queue into RIOT-OS:master with commit f67055c Feb 20, 2025
26 checks passed
@benpicco benpicco deleted the ieee802154_symbol_time_on_demand branch February 20, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfessel kfessel kfessel left review comments

@maribu maribu maribu left review comments

@fabian18 fabian18 fabian18 approved these changes

@kYc0o kYc0o Awaiting requested review from kYc0o

@smlng smlng Awaiting requested review from smlng

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

@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 PeterKietzmann is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@jeandudey jeandudey Awaiting requested review from jeandudey

@dylad dylad Awaiting requested review from dylad

@keestux keestux Awaiting requested review from keestux

@jia200x jia200x Awaiting requested review from jia200x

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@benpicco @riot-ci @maribu @kYc0o @kfessel @fabian18