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.

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

gnrc/ipv6/nib: Expose configurations to Kconfig #13626

Merged

Conversation

leandrolanzieri
Copy link
Contributor

Contribution description

This PR moves configuration macros of GNRC NIB module to the CONFIG_ namespace and exposes them to Kconfig. To keep compatibility with boolean macros generated by Kconfig I modified the evaluation of many of them using IS_ACTIVE.

I think there are some cases where the #if could be changed to C conditionals, but as I was not so sure I decided not to change this in general.

Testing procedure

  • Examples using this module should still work
  • Unittests for the module should still pass
  • One should be able to configure NIB using menuconfig interface and it should be reflected on the application

Issues/PRs references

Part of #12888

@leandrolanzieri leandrolanzieri added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT TF: Config Marks issues and PRs related to the work of the Configuration Task Force Area: Kconfig Area: Kconfig integration labels Mar 12, 2020
@miri64
Copy link
Member

miri64 commented Mar 30, 2020

@leandrolanzieri please rebase.

@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_nib_rebased branch 2 times, most recently from 63cb8a1 to a68bb55 Compare March 30, 2020 08:25
sys/include/net/gnrc/ipv6/nib/conf.h Outdated Show resolved Hide resolved
@leandrolanzieri
Copy link
Contributor Author

I wrapped lines to 80 chars on files where I was getting the 100 char warning

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Mostly style and documentation stuff, as this is just renaming the macros and introduces the Kconfig file. Everything else should be uncovered by Murdock. Please squash immediately. I can't keep track of >20 commits anyways ;-P

sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved
sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved
sys/include/net/gnrc/ipv6/nib/conf.h Outdated Show resolved Hide resolved
sys/net/gnrc/Kconfig Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/Kconfig Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/Kconfig Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/Kconfig Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/Kconfig Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/Kconfig Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/Kconfig Show resolved Hide resolved
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_nib_rebased branch 3 times, most recently from 3df65ce to aa8319e Compare March 30, 2020 20:22
@leandrolanzieri
Copy link
Contributor Author

@miri64 I think I addressed all comments. I squashed directly.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 31, 2020
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Minor error picked up by Murdock.

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h Outdated Show resolved Hide resolved
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

While you are fixing this anyway: how about some more warnings about experimental options.

sys/net/gnrc/network_layer/ipv6/nib/Kconfig Show resolved Hide resolved
Also evaluate it using IS_ACTIVE macro.
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_nib_rebased branch from aa8319e to 5833a3f Compare March 31, 2020 16:07
@leandrolanzieri
Copy link
Contributor Author

Added a fixup to guard all configuration parameters from the header when using Kconfig, so it does not override booleans.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

I tested with native and samr21-xpro:

  • examples/gcoap to test hosts (standard with native and 6LN with samr21-xpro)
  • examples/gnrc_networking to test routers (standard with native and 6LR with samr21-xpro)
  • examples/gnrc_border_router to test 6LBRs

Then I pinged both link-local addresses and global addresses. I did this without Kconfig and with Kconfig. To check the configuration with Kconfig, I deactivated router solicitations with examples/gcoap using CONFIG_GNRC_IPV6_NIB_NO_RTR_SOL=1 (which flashed the nice new warning).

It all worked. With the changed configuration the gcoap example did not send any router solicitation (making the global ping impossible on the samr21-xpro, as was expected).

ACK. Please squash your final change.

@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_nib_rebased branch from f9b33d4 to aa2ec68 Compare March 31, 2020 17:03
@miri64 miri64 merged commit eca9d3d into RIOT-OS:master Mar 31, 2020
@miri64
Copy link
Member

miri64 commented Mar 31, 2020

🎉

@leandrolanzieri leandrolanzieri deleted the pr/kconfig_migrate/gnrc/ipv6_nib_rebased branch March 31, 2020 18:15
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Kconfig Area: Kconfig integration Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR TF: Config Marks issues and PRs related to the work of the Configuration Task Force Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants