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

Kconfig: Expose gnrc/ipv6/ext/frag configurations #12958

Merged

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Dec 16, 2019

Contribution description

This PR moves the configuration macros of gnrc_ipv6_ext_frag to the CONFIG_ namespace, and exposes them to Kconfig.

In the test application a check was added, so the parameters are not changed via CFLAGS when Kconfig is being used, that's why this depends on #12913.

Testing procedure

  • tests/gnrc_ipv6_ext_frag should still work as usual.
  • You should be able to configure the module's parameters via make menuconfig in the test application.

Issues/PRs references

Part of #12888
Depends on #12913

@leandrolanzieri leandrolanzieri added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first TF: Config Marks issues and PRs related to the work of the Configuration Task Force labels Dec 16, 2019
@leandrolanzieri leandrolanzieri added this to the Release 2020.01 milestone Dec 16, 2019
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_ext_hdr branch from e8da247 to f22d1d2 Compare December 20, 2019 10:11
@leandrolanzieri
Copy link
Contributor Author

Rebased to master, this has no dependencies now.

@leandrolanzieri leandrolanzieri removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 20, 2019
@fjmolinas
Copy link
Contributor

@leandrolanzieri this one needs rebase.

@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_ext_hdr branch from f22d1d2 to aeffa05 Compare January 9, 2020 07:50
@leandrolanzieri
Copy link
Contributor Author

@leandrolanzieri this one needs rebase.

@fjmolinas Thanks, rebased

@tcschmidt
Copy link
Member

@miri64 @leandrolanzieri do we get last issues 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.

When setting CONFIG_GNRC_IPV6_EXT_FRAG_LIMIT_POOL_SIZE to 2 I can't compile anymore tests/gnrc_ipv6_ext_frag:

$ make menuconfig
Using default symbol values (no '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/bin/native/generated/merged.config')
No changes to save (for '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/bin/native/generated/merged.config')
$ make
Building application "tests_gnrc_ipv6_ext_frag" for "native" with MCU "native".

In file included from <command-line>:
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/bin/native/generated/autoconf.h:72: error: "CONFIG_GNRC_IPV6_EXT_FRAG_LIMITS_POOL_SIZE" redefined [-Werror]
   72 | #define CONFIG_GNRC_IPV6_EXT_FRAG_LIMITS_POOL_SIZE 2
      | 
<command-line>: note: this is the location of the previous definition
In file included from <command-line>:
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/bin/native/riotbuild/riotbuild.h:4: error: "CONFIG_GNRC_IPV6_EXT_FRAG_LIMITS_POOL_SIZE" redefined [-Werror]
    4 | #define CONFIG_GNRC_IPV6_EXT_FRAG_LIMITS_POOL_SIZE 3
      | 
In file included from <command-line>:
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/bin/native/generated/autoconf.h:72: note: this is the location of the previous definition
   72 | #define CONFIG_GNRC_IPV6_EXT_FRAG_LIMITS_POOL_SIZE 2
      | 
cc1: all warnings being treated as errors
make[1]: *** [/home/mlenders/Repositories/RIOT-OS/RIOT/Makefile.base:103: /home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/bin/native/application_tests_gnrc_ipv6_ext_frag/main.o] Error 1
make: *** [/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/../../Makefile.include:540: /home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext_frag/bin/native/application_tests_gnrc_ipv6_ext_frag.a] Error 2

Regarding the wording, I know that you mostly just copied what I wrote, but I have the feeling in isolation this has to be more precise.

sys/net/gnrc/network_layer/ipv6/ext/frag/Kconfig Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/ext/frag/Kconfig Outdated Show resolved Hide resolved
@leandrolanzieri
Copy link
Contributor Author

When setting CONFIG_GNRC_IPV6_EXT_FRAG_LIMIT_POOL_SIZE to 2 I can't compile anymore tests/gnrc_ipv6_ext_frag:

I placed the Kconfig symbol check wrongly in the Makefile, now this should be fixed.

@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_ext_hdr branch from b868cc7 to 2a7b9a9 Compare January 29, 2020 11:07
@leandrolanzieri
Copy link
Contributor Author

Rebased to current master.

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.

My comments were addressed. I tested this PR using tests/gnrc_ipv6_ext_frag on native and samr21-xpro, the tests still succeed.

@miri64
Copy link
Member

miri64 commented Jan 30, 2020

Please squash!

Macros that changed:
GNRC_IPV6_EXT_FRAG_SEND_SIZE -> CONFIG_GNRC_IPV6_EXT_FRAG_SEND_SIZE
GNRC_IPV6_EXT_FRAG_RBUF_SIZE -> CONFIG_GNRC_IPV6_EXT_FRAG_RBUF_SIZE
GNRC_IPV6_EXT_FRAG_LIMITS_POOL_SIZE -> CONFIG_GNRC_IPV6_EXT_FRAG_LIMITS_POOL_SIZE
GNRC_IPV6_EXT_FRAG_RBUF_TIMEOUT_US -> CONFIG_GNRC_IPV6_EXT_FRAG_RBUF_TIMEOUT_US
This test needs the pool size for limit objects set to 3 by default so
it does not fail. As this is done with the 'app.config' file, we
explicitly disable Kconfig by default.
@leandrolanzieri leandrolanzieri force-pushed the pr/kconfig_migrate/gnrc/ipv6_ext_hdr branch from 2a7b9a9 to 6481076 Compare January 30, 2020 16:43
@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 30, 2020
@cgundogan
Copy link
Member

Murdock shows green light! GO

@cgundogan cgundogan merged commit 0cf8bf1 into RIOT-OS:master Jan 30, 2020
@leandrolanzieri leandrolanzieri deleted the pr/kconfig_migrate/gnrc/ipv6_ext_hdr branch January 31, 2020 08:02
@leandrolanzieri leandrolanzieri added the Area: Kconfig Area: Kconfig integration label Feb 20, 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.

5 participants