Skip to content

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

Closed
jia200x wants to merge 13 commits intoRIOT-OS:masterfrom
jia200x:pr/Kconfig/ieee802154
Closed

{dev,net}/ieee802154_*: add initial Kconfig modeling#16842
jia200x wants to merge 13 commits intoRIOT-OS:masterfrom
jia200x:pr/Kconfig/ieee802154

Conversation

@jia200x
Copy link
Copy Markdown
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
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

I think this needs a rebase before review

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Nov 2, 2021

@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
Copy Markdown
Member Author

jia200x commented Nov 10, 2021

is there anything missing here?

@fjmolinas
Copy link
Copy Markdown
Contributor

is there anything missing here?

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

Comment on lines +45 to +46
select HAVE_IEEE802154_RADIO
bool
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

done

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
Copy Markdown
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
Copy Markdown
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.


menuconfig MODULE_IEEE802154
bool "IEEE 802.15.4 support"
menuconfig FEATURE_IEEE802154
Copy link
Copy Markdown
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
Copy Markdown
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,,,

menuconfig FEATURE_IEEE802154
bool "Enable IEEE 802.15.4 support"
depends on TEST_KCONFIG
default y if HAVE_IEEE802154_RADIO && MODULE_NETDEV_DEFAULT
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

done

if FEATURE_IEEE802154

config MODULE_IEEE802154
bool "IEEE 802.15.4 helpers"
Copy link
Copy Markdown
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?

menuconfig MODULE_IEEE802154
bool "IEEE 802.15.4 support"
menuconfig FEATURE_IEEE802154
bool "Enable IEEE 802.15.4 support"
Copy link
Copy Markdown
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
Copy Markdown
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

bool "IEEE 802.15.4 security"
select MODULE_CRYPTO
select MODULE_CIPHER_MODES
depends on TEST_KCONFIG
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done

Comment on lines +32 to +33
depends on FEATURE_IEEE802154
depends on TEST_KCONFIG
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done

config HAS_RADIO_NRF802154
bool
select HAVE_NRF5X_RADIO
select MODULE_NRF802154 if MODULE_NETDEV_DEFAULT
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

I rebased and fixed some of the comments from @leandrolanzieri

@fjmolinas
Copy link
Copy Markdown
Contributor

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

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

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

@stale
Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

Yup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

5 participants