configurations: initial header file config and Doxygen group#10077
configurations: initial header file config and Doxygen group#10077jia200x wants to merge 2 commits intoRIOT-OS:masterfrom
Conversation
| @@ -0,0 +1,54 @@ | |||
| /** | |||
| * @defgroup config_ieee802154 IEEE802.15.4 configurations | |||
There was a problem hiding this comment.
Mh... Isn't there a way to keep them in their original file and just rename them?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_"There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
|
||
| /** | ||
| * @name IEEE802.15.4 sub-GHZ channel. | ||
| * @{ |
There was a problem hiding this comment.
Having a group for just one macro seems overkill. Why not just use @brief and no group (or the @brief shorthand /**< ... */?
There was a problem hiding this comment.
yes, sure. We can do that
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
left a comment
There was a problem hiding this comment.
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 👍
| #include "periph/gpio.h" | ||
| #include "net/netdev.h" | ||
| #include "net/netdev/ieee802154.h" | ||
| #include "ieee802154_config.h" |
There was a problem hiding this comment.
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?
|
|
||
| #include "net/netdev.h" | ||
| #include "net/netdev/ieee802154.h" | ||
| #include "ieee802154_config.h" |
I'm also with this. Besides helping with the tooling, IMO it's quite clean to expose the interface that way |
|
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. |
Make sure to not use a hash table in the implementation, because hash functions might lead to collisions. |
???? |
|
I guess this is superseded by #10626. |
|
(feel free to re-open if you disagree). |
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 Channelconfigurations, because those are used by some examples.Proposal
<module_name>/include/<module_name>_config.hCONFIG_<MODULE_NAME>_like macroE.g
ieee802154_config.h@configurationsgroup is added and all configuration sub-groups are included there.Configuration values can be modified with either:
<module_name>_config.hfiles in the application folder with all valuesImplementation
This PR:
CONFIG_prefix to IEEE802.15.4 configurations and move them to aieee802154_config.hfile undersys/net/link_layer/ieee802154/include.USEMODULE_INCLUDESpointing to the introduced includes folder.configurationsDoxygen group in the root (directly under Modules)config_ieee802154group inieee802154_config.hieee802154_config.hto bothnet_ieee802154andconfig_ieee802154group.RFC
CONFIG_prefix to all RIOT configurations?module/include?configurationsgroup in Doxygen?Sub-groupings (e.g
config_net_ieee802154instead ofconfig_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 toModules>Configurationsgnrc_networkingor any example usingIEEE802154_DEFAULT_xxxvariablesIssues/PRs references
#10058, #9856