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

{dev,net}/ieee802154_*: add initial Kconfig modeling #16842

Closed
wants to merge 13 commits into from
Closed

{dev,net}/ieee802154_*: add initial Kconfig modeling #16842

wants to merge 13 commits into from

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Sep 13, 2021

Contribution description

This PR add the initial Kconfig modeling for the lower layers of IEEE 802.15.4. This includes:

  • Adding feature symbols to indicate presence of Radio HAL or Netdev Driver based driver.
  • Modeling of SubMAC and netdev_ieee802154_submac
  • Adding tests/ieee802154_submac to the list of Kconfig test executed by Murdock

This PR also refactors the IEEE 802.15.4 menu, because MODULE_IEEE802154 just provides helper functions for processing IEEE 802.15.4 frames. Last but not least, it removes unused configurations in nrf802154.

Testing procedure

Murdock should be enough I guess.

Issues/PRs references

Depends on #16837

@github-actions github-actions bot added Area: boards Area: Board ports Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU 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 Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: native Platform: This PR/issue effects the native platform labels Sep 13, 2021
@leandrolanzieri leandrolanzieri mentioned this pull request Sep 22, 2021
Closed
@MrKevinWeiss
Copy link
Contributor

I think this needs a rebase before review

@fjmolinas
Copy link
Contributor

fjmolinas commented Nov 2, 2021 •
edited
Loading

@jia200x this needs a rebase now(sorry the now made it sound like an order)!

@jia200x jia200x force-pushed the pr/Kconfig/ieee802154 branch from 0625b1f to 4b09a3d Compare November 2, 2021 12:22
@github-actions github-actions bot removed Area: boards Area: Board ports Area: tests Area: tests and testing framework Platform: native Platform: This PR/issue effects the native platform labels Nov 2, 2021
@jia200x jia200x force-pushed the pr/Kconfig/ieee802154 branch from 4b09a3d to a8590f1 Compare November 2, 2021 12:29
@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 2, 2021
@jia200x
Copy link
Member Author

jia200x commented Nov 10, 2021

is there anything missing here?

@fjmolinas
Copy link
Contributor

is there anything missing here?

Yes, I have some unanswered comments, I think currently the submac test does not build

leandrolanzieri
leandrolanzieri reviewed Dec 7, 2021
View reviewed changes
drivers/Kconfig.net Outdated
Comment on lines 45 to 46
select HAVE_IEEE802154_RADIO
bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: can you invert the order of these two lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

done

drivers/Kconfig.net
Comment on lines +25 to +30
config MODULE_NETDEV_IEEE802154
bool
help
Netdev IEEE 802.15.4 module, provides common functionalities for
netdev based IEEE 802.15.4 devices.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this and the feature better be placed in drivers/netdev?

Copy link
Contributor

Choose a reason for hiding this comment

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

Things get a bit spread out. I will have to take a closer look what belongs where.

sys/net/link_layer/ieee802154/Kconfig Outdated
@@ -4,26 +4,38 @@
# General Public License v2.1. See the file LICENSE in the top level
# directory for more details.

menuconfig MODULE_IEEE802154
bool "IEEE 802.15.4 support"
menuconfig FEATURE_IEEE802154
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just call this IEEE802154

Copy link
Contributor

Choose a reason for hiding this comment

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

done, though I think we should rename the module to IEEE802154_HELPERS. A networking newbie like myself can get easily confused,,,

sys/net/link_layer/ieee802154/Kconfig
depends on TEST_KCONFIG
default y if HAVE_IEEE802154_RADIO && MODULE_NETDEV_DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want MODULE_NETDEV_DEFAULT to enable this or only the drivers? (similar to SAUL).
If we are keeping this I would suggest to hide the prompt, as it is no longer optional not to have it:

bool
prompt "IEEE 802.15.4 support" if !(HAVE_IEEE802154_RADIO && MODULE_NETDEV_DEFAULT)

Copy link
Contributor

Choose a reason for hiding this comment

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

done

sys/net/link_layer/ieee802154/Kconfig
if FEATURE_IEEE802154

config MODULE_IEEE802154
bool "IEEE 802.15.4 helpers"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this default y? Moreover, does it make sense that the user disables this when they want IEEE 802.15.4 support?

sys/net/link_layer/ieee802154/Kconfig Outdated
menuconfig MODULE_IEEE802154
bool "IEEE 802.15.4 support"
menuconfig FEATURE_IEEE802154
bool "Enable IEEE 802.15.4 support"
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have some documentation as this is no minor module.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will leave that to @jia200x

sys/net/link_layer/ieee802154/Kconfig Outdated

config MODULE_IEEE802154_SECURITY
bool "IEEE 802.15.4 security"
select MODULE_CRYPTO
select MODULE_CIPHER_MODES
depends on TEST_KCONFIG
Copy link
Contributor

Choose a reason for hiding this comment

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

It already depends on FEATURE_IEEE802154 because it's inside the if, also you can drop the dependency on TEST_KCONFIG.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

sys/net/link_layer/ieee802154/Kconfig Outdated
Comment on lines 32 to 33
depends on FEATURE_IEEE802154
depends on TEST_KCONFIG
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

cpu/nrf52/radio/nrf802154/Kconfig Outdated
@@ -8,5 +8,6 @@
config HAS_RADIO_NRF802154
bool
select HAVE_NRF5X_RADIO
select MODULE_NRF802154 if MODULE_NETDEV_DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also select the symbol that indicates that there is a 802.15.4 radio present?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this cannot actually select the module here.

As MODULE_NRF802154 is part of a choice I added that to be the default choice. If the bluetooth stack is used then that choice can be overridden to select the BLE one first... I think.

@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 15, 2022
@MrKevinWeiss
Copy link
Contributor

I rebased and fixed some of the comments from @leandrolanzieri

@fjmolinas
Copy link
Contributor

Still some issues @MrKevinWeiss (assuming you are taking over)

@MrKevinWeiss
Copy link
Contributor

I think @leandrolanzieri is working on a broader solution that will make this obsolete...

@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@Teufelchen1
Copy link
Contributor

@MrKevinWeiss this now outdated, right? Can it be closed?

@MrKevinWeiss
Copy link
Contributor

Yup

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

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

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

@cgundogan cgundogan Awaiting requested review from cgundogan

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner

@smlng smlng Awaiting requested review from smlng

Assignees
No one assigned
Labels
Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jia200x @MrKevinWeiss @fjmolinas @Teufelchen1 @leandrolanzieri