Skip to content

ieee802154/submac: handle non standard modulations #21527

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fabian18
Copy link
Contributor

@fabian18 fabian18 commented Jun 3, 2025

Contribution description

The key feature of this PR is to reuse the conf parameter as an output in

static int _config_phy(ieee802154_dev_t *dev, ieee802154_phy_conf_t *conf)

So the driver that is using an unknown IEEE.802.15.4 modulation can set ACK timeout and CSMA backoff.

This is crucial for #21202.

Testing procedure

Issues/PRs references

Follow up from #19668.
Split out from #21202

@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: network Area: Networking Area: drivers Area: Device drivers Area: cpu Area: CPU/MCU ports Area: sys Area: System labels Jun 3, 2025
@crasbe crasbe added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 3, 2025
@@ -692,6 +822,9 @@ int ieee802154_submac_init(ieee802154_submac_t *submac, const network_uint16_t *

submac->phy_mode = ieee802154_cap_to_phy_mode(1 << bit);
}
else {
submac->phy_mode = IEEE802154_PHY_DISABLED; /* non standard */
Copy link
Contributor

@benpicco benpicco Jun 3, 2025

Choose a reason for hiding this comment

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

You can just add IEEE802154_PHY_LORA if you want to.
No need to abuse the IEEE802154_PHY_DISABLED for that - this is our enum, we can add to it what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ifconfig showing phy: DISABLED already confused some people.
However something with IEEE802154_...._LORA is not really something that I would like to introduce.
I would like that the radio HAL could be used for LoRa, but not that I put LoRa stuff in the IEEE 802154 radio hal, because it is too much experimental.

Bottomline: IEEE802154_PHY_UNKNOWN is something I would propose instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we also want to add IEEE802154_PHY_NRF24LP?

Just because it isn't a IEEE standard doesn't mean we can add it to our enum - 6LoWPAN over nRF24L01+ isn't a standard either.

If it eases your worries, how about IEEE802154_PHY_NSTD_LORA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IEEE802154_PHY_NSTD_LORA would not make a big difference to me.
If you are certain I will just add IEEE802154_PHY_LORA.

@riot-ci
Copy link

riot-ci commented Jun 3, 2025

Murdock results

FAILED

c2d579e fixup! ieee802154: add LoRa PHY mode

Success Failures Total Runtime
9811 0 10310 10m:49s

Artifacts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: native Platform: This PR/issue effects the native platform Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants