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
[RFC] [PoC] Configuration generation #10058
[RFC] [PoC] Configuration generation #10058
Conversation
maybe @kaspar030, @kYc0o , @aabadie and @cladmi are interested in this |
If I understand that correctly, do you propose to use both .yaml and .h? |
@kYc0o we forgot to indicate the header file here is just to show the output. We will update the description. (EDIT: Done) We are originally proposing to generate the documentation with YAML, and produce this The question is: should this generated C files with default values be included in the upstream as well? (e.g put a at86rf2xx_config.h in the at86rf2xx.h). The current PR assumes this file doesn't exist until you generate them. I've seen this in other OSs, and although the information is duplicated in the YAML and header file, the header file is never meant to be touched by developers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of my comments were discussed separately with @leandrolanzieri and @jia200x . I'm putting them here so that we can all consider the alternatives.
I have not looked at the implementation of the tool (and I hope others don't either) because I think is beyond the point (it's a proof of concept).
I think we should write a doc explaining the format.
@@ -0,0 +1,23 @@ | |||
/* This is an automatically generated configuration file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify: this file is for demonstration purposes, so that the app can be compiled without the tool, but would normally not be committed.
/* This is an automatically generated configuration file | ||
* DO NOT EDIT, the content will be overwritten. | ||
*/ | ||
#ifndef APP_CONFIG_CONFIG_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need #ifndef
if the config format already includes a way for overriding / changing variables?
|
||
RIOT_CONFIG_FILE = riot.yaml | ||
|
||
APP_H = app_config.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app_config.h, being a build artifact, should be placed somewhere in the build
dir. @cladmi can you suggest something?
return value | ||
|
||
|
||
if __name__ == '__main__': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could integrate lazysponge (#9634) functionality in this thing.
@@ -32,8 +32,7 @@ | |||
#include "net/loramac.h" | |||
#include "semtech_loramac.h" | |||
|
|||
/* Messages are sent every 20s to respect the duty cycle on each channel */ | |||
#define PERIOD (20U) | |||
#include "app_config.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for requiring an explicit #include
if the config header and not having defines magically rain from the heavens.
name: lorawan | ||
parameters: | ||
- name: appkey | ||
macro: APPKEY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macro
field can be optional, and default to the name
in caps.
macro: APPEUI | ||
description: LoRaWAN Application EUI | ||
value: 0xBBBBBBBBBBBBBBBB | ||
type: byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to stick to names defined in stdint.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
description: LoRaWAN Application Key | ||
value: 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA | ||
type: byte | ||
properties: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not shown here, but max
and min
are also properties.
What do people think about the For example, if you want to select the region for LoRa, you have to choose from a handful of values and the number used to represent them does not matter. |
The point in including generated files is so that the user does not need to run the tool again (maybe he doesn't have it). For example, in Python packages, if you are using Cython it is common to include the generated files so that the package can be compiled without having Cython installed. I don't think this will be the case in RIOT, as the tool will always be available and in any case it should be a super easy to install tool. |
license: LGPL-2.1 | ||
copyright: [2018 Inria] | ||
configuration: | ||
- group: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number of nesting levels and the verbosity can be reduced by using dicts of dicts instead of lists of dicts:
version: 0.1.0
name: lorawan-example
authors:
- name: Alexandre Abadie
email: alexandre.abadie@inria.fr
license: LGPL-2.1
copyright: [2018 Inria]
configuration:
lorawan: # group disappears
appkey: # name disappears and so does macro
description: LoRaWAN Application Key
value: 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
type: byte
properties:
size: 16
appeui:
description: LoRaWAN Application EUI
value: 0xBBBBBBBBBBBBBBBB
type: byte
properties:
size: 8
deveui:
description: LoRaWAN Device EUI
value: 0xCCCCCCCCCCCCCCCC
type: byte
properties:
size: 8
period:
description: Seconds to wait between messages
type: int
value: 20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open question with this approach: how do you know some name is a an option and not a namespace for other options without complicated guessing. Maybe option containers can be prefixed with a character (@ ??) or start with a capital letter.
Are we forced to use doxygen on this header files? I think doc can still be there and the parsers (yes, very probable you'll need to write your parser by yourself) may skip it.
The advantage of using header files is that you can do
You mean that the variable might be defined in two places? If that occurs, then we have another problem that cannot be solved by a "conflict detector".
This would slow down a lot the CI. The proposal was to create them one by one by hand, which takes a lot of time but it only need to be done once.
This is interesting indeed, but I'd say is really complicated to do it at low level. I'd say, if you're touching the files directly you know what you're doing. If not, use the IDE which already restrict the parameters. I don't know all the possibilities but I guess there should be a way to do it, but not straightforward.
Please don't! We trying here to separate concerns, and here you're mixing things. Configuration values should be independent of anything, only restricted, if anything, as you stated before.
Again, please don't, that's another topic which needs a different approach (probably yaml but in a different way).
+1! (can be done in headers)
That IMHO needs another approach, some kind of config inheritance but always through header files. Using an external tool (yes, yaml it's a tool after all) makes things more complex.
This reduces a lot the scope of the variables and are less easy to handle as configuration options. Overall, I think there's still a misunderstanding on what are module configurations and an application variant.
So, I propose to stay on the configuration dilemma and not mix with buildsystem config, which will need some love but in a separated issue. |
@jcarrano I suggest first to reach consensus on the approach before tackling the approach in itself. I don't see any advantages of discussing what's done right or wrong in the implementation if the implementation can be changed anyways. |
hi @kYc0o
This can be solved with keeping the
If we follow the approach from above, we can still do this. The YAML file would only define how the
I think this is accurate and probably we shouldn't mix build system config, dependencies, etc yet.
I think this is more in the direction of global configurations. E.g, a lot of modules might need to read "THREAD_STACKSIZE" kind of variables. Maybe is more accurate to rename it to "read global configurations". What do you think? My whole point is:
Instead of manually adding So, header files declaring params are never touched by the developer.
Sure, let's try to find consensus in the usage of a explicit Config File for generating header files. |
one more thing... if we add other layers on top (validation of params, CLI, etc) it's way easier to expand a YAML file than a header file. I can agree with some benefits of having header files with macro preprocessors and override values with new #defines. But also think parsing a header file for exposing params, validations, etc is not the best way to go |
This looks promising but how are we suppose to use it and what are the Python dependencies required ? |
With this simple implementation you would just place your app configurable parameters on the YAML file, add the Makefile dependency for de xx_config.h file, and the generator is called on |
No, but then there is even MORE work to be done in parsing.
Same answer as @jia200x. Let me add that it's better if one does not try to circumvent the mechanism.
The idea is not to solve the conflict, only to detect and report it and avoid hard-to-find bugs.
If C had static asserts we would not need it so much. In any case, the limits also act as documentation.
I'm not very convinced of this either, but we wanted to put it under consideration.
The idea with this is:
We are already stretching the limits of what can be done with Make (sometimes because what we are doing is inherently complex, sometimes because make is an awkward language and people tend to get things wrong). If we use headers (that means the C preprocessor) I'm afraid we will run into the same situation, and end up with something that looks more like a mashup of hacks. That for me is the main argument against a system using header files. The thing is, I still have to hear a good argument for them. That they don't need any extra tooling is an illusion. What is the RIOT make system but a giant tool distributed in many files and directories. And the current system of "config.h" does not work without this tooling without some non-trivial manual work.
I'd not rush any performance prediction yet. And even if it slows down the CI at first, it has the potential to speed up things in the long run by preventing unnecessary rebuilds and by isolating which parts of the code could have possibly been affected by a PR.
How is a module different from an application? Similarities
Differences
Ok, but what do you do with the stuff that is configured at build time?
Agreed. |
I know this is not about implementation, but I want to clear up any doubts regarding build times. Hopefully I can convince everyone it is not an issue and it's not worth discussing it now. Parsing the config files and generating the H files can be done extremely fast:
If anything should be of concern, that's the last point. Solving it is not impossible: make a single invocation of the script process several files. Not trivial, but not impossible either. |
After some offline discussions, we will open a PR with the proposal for the |
As an example of what are our goals with a configuration system, consider this (from #10075)
I would like not to have to touch board code to change a setting. |
How do we proceed, @leandrolanzieri @jia200x, whith #10626 merged and #10077 closed? |
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. |
@leandrolanzieri @jia200x with all the work going on in kconfig I'm thinking this can be closed? Current work seems to have diverged... |
Yes, this can be closed now. Thanks! |
What's the goal of this PR and its scope?
This PR to proposes a first workaround on "How to handle the configuration of modules in RIOT?".
This work is in conjuction with @jia200x
Here we are presenting an idea of what a solution to some of the issues found at the RIOT summit might look like.
This is a PoC intended to find consensus on the usage of configuration files (which generate header files as output), the YAML serialization format, and the proposed schema.
The scope of this PR is configuration format and mechanism (not how to integrate it to the build system), and only defines application configurations (but can be extended to all RIOT modules). We took the LoRaWAN example as a reference.
This PR is not by any means intended to be merged.
What's the proposal?
Draft 2:
Declare and describe configurations in a well-known data structure (YAML is proposed):
Define name, default values, description, types and properties. See here for an example. (The purpose of the header file on the PR is just to show the output of the configuration generator)
Generate
xxx_config.h
files from these Configuration file with#ifndef CONFIG_XXX #define CONFIG_XXX VALUE #endif
.Either:
3a. RIOT is shipped with these
xxx_config.h
files in modules directories: They are regenerated only if the configuration file changes, and the config header is never meant to be edited by developers.3b.
xxx_config.h
files are generated on every build (make all
) in thebuild
directory .Please note adding tools on top (e.g CLI/GUI for exposing configurations) is almost straightforward if we parse from Configuration Files instead of header files.
How to override default configuration values is the next discussions. With 3a and 3b is possible to use CFLAGS, a standard header file,
riotbuild.h
, or any other mechanism.Important:
xxx_config.h
are a subset of the information in the Configuration files (YAML). All metadata should be included in the YAML file, and thexxx_config.h
file is only the interface for injecting configurations in the corresponding C files.FAQ
What's the problem with declaring configurations in header files?
We started working with the following proposal:
We were planning to add these configurations in
xxx_config.h
files on each module, so they can be included in C files that require these configurations.These files were supposed to be used to extract information about params (name, default values, restrictions) and override values via CFLAGS or another header file.
However, we found some issues:
fmt
line with the configuration description, and probably add| defined(DOXYGEN)
. Also, the proposed format is dependent to Doxygen.Why YAML?
YAML is a serialization standard across multiple languages, and wide used on different projects. Has multiple features such as comments, support for many data types and field reference. It can be easily transform to JSON.
There is support for: C/C++/Crystal/Ruby/Python/Java/Pearl/C#/.NET/Golang/PHP/JavaScript/OCaml/ActionScript/Haskell/Dart/Rust/Nim
By defining a SCHEMA (on another YAML of JSON file) you can validate the format of the configuration, document and version any changes that may by applied in the future. There are standard tools like Rx.
Can be easily read and modified. Also easy to generate.
Some possible features (for further discussions)
Indicating constraints on the accepted parameters.
value: othermodule.parameter
Also any configuration options for those dependencies.
This way it's easy to declare metadata such as versions.
static const
instead of preprocessor#defines
.Archived drafts:
Draft 1
Define name, default values, description, types and properties. See here for an example. (The purpose of the header file on the PR is just to show the output of the configuration generator)
By using an
override
parent instead ofconfiguration
(could taste likeoverride: gnrc.ieee802154.default_channel=11
). The inheritance priority is not in the scope of this PR. The key feature, however, is that we will be able to handle coflicts in a well-defined and safe way.make all
:We were thinking of keeping
module_config.h
format, and these files should be imported with#include module_config.h
.Such as ncurses, ENV vars, etc. Here we are limiting ourselves to defining a core mechanism, not an interface.