configurations: initial header file config and Doxygen group #10077

jia200x · 2018-09-28T16:49:40Z

Contribution description

This PR is a PoC of a header file for describing configurations and a new Doxygen group for exposing these configs, in the context of the Configurations Task Force.

We are starting with IEEE802.15.4 Default Channel configurations, because those are used by some examples.

Proposal

Configuration macro and default value declared under <module_name>/include/<module_name>_config.h
CONFIG_<MODULE_NAME>_ like macro

E.g ieee802154_config.h

// defines CONFIG_IEEE802154_DEFAULT_SUBGHZ_CHANNEL default to 5
#ifndef CONFIG_IEEE802154_DEFAULT_SUBGHZ_CHANNEL
#define CONFIG_IEEE802154_DEFAULT_SUBGHZ_CHANNEL   (5U)
#endif

Documentation (Doxygen) is also included in these defines. A new @configurations group is added and all configuration sub-groups are included there.

Configuration values can be modified with either:

CFLAGS (e.g -DCONFIG_IEEE802154_DEFAULT_SUBGHZ_CHANNEL=8)
Writting new <module_name>_config.h files in the application folder with all values
Defining a global header for overwriting configurations.

Implementation

This PR:

Adds CONFIG_ prefix to IEEE802.15.4 configurations and move them to a ieee802154_config.h file under sys/net/link_layer/ieee802154/include.
- Adds a USEMODULE_INCLUDES pointing to the introduced includes folder.
Adds a configurations Doxygen group in the root (directly under Modules)
Declares a config_ieee802154 group in ieee802154_config.h
- References all macros from ieee802154_config.h to both net_ieee802154 and config_ieee802154 group.

RFC

Are we OK with a CONFIG_ prefix to all RIOT configurations?
Are we OK to put configurations header files under module/include?
Are we OK with the configurations group in Doxygen?

Sub-groupings (e.g config_net_ieee802154 instead of config_ieee802154) is out of the scope of this PR. If the mechanism is accepted, we can discuss this on the go (it will never break the API but just the way how users see the documentation).

If this PR gets accepted, we will proceed to add a Tracker to gradually add configurations to other RIOT modules.

Testing procedure

make doc. Browse to Modules>Configurations
Compile gnrc_networking or any example using IEEE802154_DEFAULT_xxx variables

Issues/PRs references

#10058, #9856

miri64 · 2018-09-28T17:10:59Z

sys/net/link_layer/ieee802154/include/ieee802154_config.h

@@ -0,0 +1,54 @@
+/**
+* @defgroup config_ieee802154 IEEE802.15.4 configurations


Mh... Isn't there a way to keep them in their original file and just rename them?

I think it makes sense to have all the configurable parameter of each module on a separate header file. Makes it easier to find and edit. Also you can just tell what is the exposed configuration of the module without having to go to the documentation if every module implements a similar file structure.

You would have the same effect (though admitingly it requires extra knowledge) by having them just named CONFIG_… because you can always find them then by using

git grep "^#define\s\+\<CONFIG_"

I totally agree with @miri64 that being able to use grep/ag/rg to search for configuration parameters by looking for #defines with a common prefix like CONFIG_ could be a huge productivity boost.

NB there are some comments in the usage of something to fetch all config parameters (see TF status, expose default configs). Indeed the CONFIG_ prefix is a good idea.

We might still need to add other metadata to the options (type, allowed values, help message, etc) that might need some special format/parsing.

I like the idea of moving in the direction of putting everything relevant to a module within the module folder, in general. I think this is a somewhat separate issue to the focus of the PR, though, which is to just create a header file which exposes configurations. If we want to move in the direction of less shared files and directories between modules (which I gather there is appetite for), maybe it should be addressed and decided with a focus on that issue, elsewhere?

miri64 · 2018-09-28T17:11:50Z

sys/net/link_layer/ieee802154/include/ieee802154_config.h

+
+/**
+* @name    IEEE802.15.4 sub-GHZ channel.
+* @{


Having a group for just one macro seems overkill. Why not just use @brief and no group (or the @brief shorthand /**< ... */?

yes, sure. We can do that

jcarrano · 2018-10-01T08:23:28Z

Are we OK to put configurations header files under module/include?

It would be nice to make a distinction between configurations that change the interface of the module (i.e. affect the publicly accessible header) and those that don't (meaning binary compatibility is preserved).

An example of the latter ("internal config"????) would be enabling extra debug output on a module. No other module should be recompiled as a result of this. Our build system is not currently capable of preventing such unnecessary recompilations, but if we want to have that feature we must start preparing the ground.

smlng

One suggestions for consistency, otherwise I pretty much like the idea of having the *_config.h files to clearly separate and expose global config variables of a module. This would also make tooling (e.g. config GUI) and overriding values (and maybe even autogeneration in the future) very easy.

so big 👍

smlng · 2018-10-09T09:29:06Z

drivers/include/at86rf2xx.h

@@ -37,6 +37,7 @@
 #include "periph/gpio.h"
 #include "net/netdev.h"
 #include "net/netdev/ieee802154.h"
+#include "ieee802154_config.h"


wouldn't it be more in the spirit of this PR to introduce a at86rf2xx_config.h to set all the values below, and include that here and use ieee802154_config.h only in that new header file?

I like the idea!

smlng · 2018-10-09T09:29:16Z

drivers/include/cc2420.h

@@ -28,6 +28,7 @@

 #include "net/netdev.h"
 #include "net/netdev/ieee802154.h"
+#include "ieee802154_config.h"


same as above

jia200x · 2018-10-09T14:10:20Z

otherwise I pretty much like the idea of having the *_config.h files to clearly separate and expose global config variables of a module. This would also make tooling (e.g. config GUI) and overriding values (and maybe even autogeneration in the future) very easy.

I'm also with this. Besides helping with the tooling, IMO it's quite clean to expose the interface that way

jcarrano · 2018-10-09T14:59:28Z

Using only one underscore leads to some ambiguity, eg:

CONFIG_A_THE_THING can refer to a.the_thing, a.the.thing, a_the.thing, etc.

If, at some point we use something in the style of #10058, we may run into problems.
If we use two underscores, we can easily define a "macroification" and "demacroification" function (not allowing two successive underscores in a name component may be a good idea too.)

kaspar030 · 2018-10-09T15:10:58Z

Using only one underscore leads to some ambiguity, eg:

CONFIG_A_THE_THING can refer to a.the_thing, a.the.thing, a_the.thing, etc.

Make sure to not use a hash table in the implementation, because hash functions might lead to collisions.

jcarrano · 2018-10-10T11:53:11Z

@kaspar030

Make sure to not use a hash table in the implementation, because hash functions might lead to collisions.

????

miri64 · 2018-12-18T13:24:08Z

I guess this is superseded by #10626.

miri64 · 2018-12-18T13:24:56Z

(feel free to re-open if you disagree).

leandrolanzieri added 2 commits September 28, 2018 18:14

Move configuration parameters to config file

01e47d6

Include config file

b246d3b

jia200x added the TF: Config Marks issues and PRs related to the work of the Configuration Task Force label Sep 28, 2018

jia200x self-assigned this Sep 28, 2018

jia200x requested review from jcarrano, kaspar030, kYc0o and miri64 September 28, 2018 16:49

miri64 requested changes Sep 28, 2018

View reviewed changes

jia200x mentioned this pull request Oct 8, 2018

Architecture: APP specific configuration files required #9825

Closed

jia200x requested a review from smlng October 8, 2018 16:21

smlng added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Oct 9, 2018

smlng suggested changes Oct 9, 2018

View reviewed changes

jia200x requested a review from cladmi October 9, 2018 14:55

jia200x mentioned this pull request Oct 9, 2018

[CTF]: Compile-time configuration of components #10138

Open

3 tasks

miri64 closed this Dec 18, 2018

tcschmidt mentioned this pull request Jul 2, 2019

[RFC] [PoC] Configuration generation #10058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

configurations: initial header file config and Doxygen group #10077

configurations: initial header file config and Doxygen group #10077

jia200x commented Sep 28, 2018 •

edited

Loading

miri64 Sep 28, 2018

leandrolanzieri Oct 1, 2018

miri64 Oct 1, 2018

maribu Oct 8, 2018

jia200x Oct 8, 2018

danpetry Nov 13, 2018 •

edited

Loading

miri64 Sep 28, 2018

jia200x Oct 8, 2018

jcarrano commented Oct 1, 2018

smlng left a comment

smlng Oct 9, 2018

jia200x Oct 9, 2018

smlng Oct 9, 2018

jia200x commented Oct 9, 2018

jcarrano commented Oct 9, 2018

kaspar030 commented Oct 9, 2018

jcarrano commented Oct 10, 2018 •

edited

Loading

miri64 commented Dec 18, 2018

miri64 commented Dec 18, 2018

		@@ -0,0 +1,54 @@
		/**
		* @defgroup config_ieee802154 IEEE802.15.4 configurations

configurations: initial header file config and Doxygen group #10077

configurations: initial header file config and Doxygen group #10077

Conversation

jia200x commented Sep 28, 2018 • edited Loading

Contribution description

Proposal

Implementation

RFC

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpetry Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcarrano commented Oct 1, 2018

smlng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jia200x commented Oct 9, 2018

jcarrano commented Oct 9, 2018

kaspar030 commented Oct 9, 2018

jcarrano commented Oct 10, 2018 • edited Loading

miri64 commented Dec 18, 2018

miri64 commented Dec 18, 2018

jia200x commented Sep 28, 2018 •

edited

Loading

danpetry Nov 13, 2018 •

edited

Loading

jcarrano commented Oct 10, 2018 •

edited

Loading