-
Notifications
You must be signed in to change notification settings - Fork 2k
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
sys: runtime configuration module with persistent storage backends #19557
base: master
Are you sure you want to change the base?
Conversation
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.
I think I don't yet fully understand how this works: Are config values always read from storage or is there some caching going on? If so, where is the memory for this allocated and can config values be of arbitrary length?
pkg/flashdb/include/fal_cfg.h
Outdated
@@ -83,7 +101,7 @@ extern struct fal_flash_dev mtd_flash0; | |||
* @brief Partition 3 | |||
*/ | |||
#ifdef FAL_PART3_LABEL | |||
#define FAL_ROW_PART3 { FAL_PART_MAGIC_WORD, FAL_PART2_LABEL, "fal_mtd", | |||
#define FAL_ROW_PART3 { FAL_PART_MAGIC_WORD, FAL_PART3_LABEL, "fal_mtd", | |||
FAL_PART2_LENGTH, FAL_PART3_LENGTH, 0 }, |
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 copy & paste error extended to the next line too
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.
I somehow don't see what else is a copy paste error. But I am not sure whether the partition offset should be FAL_PART0_LENGTH + FAL_PART1_LENGTH + FAL_PART2_LENGTH
for partition 3
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.
I think you are right!
pkg/flashdb/mtd/fal_mtd_port.c
Outdated
@@ -75,11 +75,12 @@ struct fal_flash_dev mtd_flash0 = { | |||
void fdb_mtd_init(mtd_dev_t *mtd) | |||
{ | |||
unsigned sector_size; | |||
if (!_mtd) { |
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.
if (!_mtd) { | |
if (_mtd) { | |
return; | |
} |
is always nicer
sys/include/auto_init_utils.h
Outdated
* | ||
* @param[in] module Module to be initialized | ||
*/ | ||
void auto_init_module(const volatile auto_init_module_t *module); |
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.
as this still only calls module->init();
it could still be static inline
here
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.
Oh I think DEBUG()
is a bit in the way to this. Because I would have to #include debug.h
in auto_init_utils.h
and ... if this include passes through to some C file which include auto_init_utils.h
before debug.h
it could cause confusion.
Or if I don't include debug.h
I would insist on including debug.h
before auto_init_utils.h
.
Or .... I simply change DEBUG()
to printf()
.
{"/food/bread/white", &persist_conf.food.food_bread[0]}, | ||
{"/food/bread/whole grain", &persist_conf.food.food_bread[1]}, | ||
|
||
{"/food/cake/cheesecake", &persist_conf.food.food_cake[0]}, | ||
{"/food/cake/donut", &persist_conf.food.food_cake[1]}, | ||
|
||
{"/drinks/coffee", &persist_conf.drinks[0]}, | ||
{"/drinks/tea", &persist_conf.drinks[1]}, | ||
{"/drinks/cocoa", &persist_conf.drinks[2]}, |
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.
this very much reminds me about the config system @bergzand once proposed :D
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 you have a PR or branch link?
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.
I'm not sure, but https://github.com/bergzand/RIOT/tree/wip/coreconf might be it
(but that might just be config serialization for transmission, not storage)
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 more about configuration-in-transit, but if our config-at-rest were compatible with that, it'd be a plus (that's actually the last item of #19557 (comment))
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.
I looked at the coreconf
and I definitely see parallels. There are different node types like inner container type node (subtree) and leaf nodes (data handler implementation). Remote configuration will be a future requirement for us too. I think there can be a mapping between CoAP URIs "/c/..." and internal configuration path. And the CoAP handler could know which configuration API to use. Whatever is left from the URI in the request after the CoAP handler was found, could be the next
in the configuration API and the configuration handler could handle that. The CoAP handler would have to parse the CBOR payload which should map to the internal configuration object, the internal struct. The XFA and SID modeling is a bit hard to understand.
I have not heard of YANG before and will try to look into it.
sys/include/configuration.h
Outdated
/** | ||
* @brief Static initializer for an intermediate handler node | ||
* | ||
* @param path Configuration path segment, e.g. "bar" in "foo/bar/bazz" |
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.
So this stacks and "foo" would be the parent and "bazz" the child node?
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.
Yes foo
could be the parent and bazz
could be one child.
The decision to not put the full path foo/bar/bazz
in the handler node I made because I wanted to avoid to store the full key foo/bar/bazz/
for example for foo/bar/bazz/a
, foo/bar/bazz/b
, foo/bar/bazz/c
. I also had the idea that the tree could in fact be a list of only handler nodes which then must store the full key. This would simplify the handler lookup procedure but depending on the number of items with a common subkey string I think we would have to store many const char *
with a common prefix substring.
sys/include/configuration.h
Outdated
list_node_t node; /**< Every node is in a list, managed by its parent */ | ||
struct conf_handler_node *sub_nodes; /**< Every node has a list of subnodes */ | ||
const char *subtree; /**< A segment in a key to a configuration item */ | ||
unsigned level; /**< Level in the configuration tree (root = 0) */ |
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.
I'm trying to understand how this works: What do we need the level for?
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.
A node has a list of his child nodes. While it self is in the list of child nodes of his parent.
sub_nodes
is the list of child nodes of this nodes. And node
is the node in the list of the node's parent.
The level is stored to be able to iterate over the tree with a stack, which is the usual approach to traverse a tree by not using recursion. Imagine, when we do a depth first search we go preferably deeper and deeper in the tree. But on our way, when we see another child node on the same level, like the current node, we save it on the stack to visit it later.
Without the level, we would not know if the node which we pop next from the stack was on level maybe 1, 5, or 6.
Imagine we are on level 1 and push a neighbor node to the stack, then we go deeper to level 4 and did not see another node on the same level. So we visited the node on level 4, and pop next the node we saw on level one. But we could not distinguish whether we pushed the node on level 1 or maybe level 3. And By the level we know how many segments in the key correspond to the node on the stack. If it was level 1, then /foo/
would be the part of the key from which to continue when the key was /foo/bar/bazz/whatever/.../whatsoever
.
|
||
/** | ||
* @brief Handler prototype to import a configuration value from persistent storage to | ||
* the internal representation location of the configuration item |
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.
Where is this value stored then?
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.
I would say a configuration subsystem is a set of handlers which operate on an internal data structure of a struct
.
The subsystem allocates a struct type variable of their configuration. This internal struct allocation is modified and read with set()
and get()
. The "internal representation location" is the struct of the configuration subsystem. The import()
.... imports (fills) the internal struct with data from persistent storage, and export()
.... exports the struct to persistent storage. verify()
is called after import and before export.
sys/include/configuration.h
Outdated
* | ||
* @param[in] handler Reference to the handler | ||
* @param[in] key Configuration key which belongs to the configuration handler | ||
* @param[in] next Comes after the key and the handler knows how to process |
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.
This I don't understand
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.
If key
in configuration_set()
is foo/bar/bazz/length
and foo
, bar
, bazz
are nodes in the tree where bazz
implements a handler, key
in the handler callback would be foo/bar/bazz
and next
would be length
.
Also we could maybe reach the and of the tree already by /foo/bar
in which case, next
would be bazz/length
.
I would maybe assume that /foo/bar/bazz
would be the key in persistent storage to import/export the full object and foo/bar/bazz/length
could be used to just set/get the length of bazz.
I'd be glad to have a config system in the style of this; before I jump into code, but do have questions:
(I'm setting this off visibly because while I hope the above can all be answered easily, this might be a rabbit hole you may prefer not to go into:)
|
Hello everyone, I just saw this pull request and would like to add my 2 cents. Also I think maybe this is a good opportunity to start a collaboration between the author of this PR and myself, if interest exists, as we both have put a lot of thinking in solving this issue of runtime configuration in RIOT OS. I also started to build my architecture the same way as the Handler =>
|
@chrysn I have spent some time to reason about and answer your questions.
"Different back-ends mounted at different paths", so for example
Firmware 1.0.0 could for example seek for "conf/FW1.0.0/objects" and Firmware 2.0.0 "conf/FW2.0.0/objects".
For the backend it is just bytes. For the internal representation in RAM it is a struct which is parsed from the backend bytes.
We could drop the FlashDB VFS backend if you want. It was good for me to see that it works. However it is notably slower than the FAL backend.
Ok I can improve the documentation. I would call the internal struct a cache which is modified with get/set. And when desired can be exported to storage.
Thats a heavy task. I would pass the problem to the backend :D
I agree the string code is ugly but could be good if it comes to URIs for remote confiuration.
I did a configuration array of WiFi access points with it, and yes it resulted in an enumeration
That the backend knows the struct type or at least the maximum size of a configuration item would ease implementation of it.
Let's say there are configuration structs A, B and C and they fit on a flash page. There would be a handler and an internal allocated instance for each of struct A B and C. |
On Mon, May 15, 2023 at 08:22:03AM -0700, fabian18 wrote:
"Different back-ends mounted at different paths", so for example
`/my/large/config/foo` I would put maybe on the FlashDB which should
not be exported so frequently but the frame counter which increments
for sending an IEEE.802.15.4 frame I would store on a location which
is fast to read from and write to.
I agree there are different desirable characteristics, but the frame
counter example shows that the mount point paradigm may not be ideal.
Take for example the state of an OSCORE security context. Its key is
immutable, the sequence number is changed often (and committed to flash
every 256-or-so messages), and the replay window is only ever committed
at shutdown (to some value) and at startup (before processing the first
message, to an "uninitialized" value).
Managing them at three points in the tree (say,
`/frozen/oscore/ctx/0/key`, `/once-in-a-while/oscore/ctx/0/ssn`,
`/at-boot/oscore/ctx/0/replay` sounds hard to manage consistently.
Firmware 1.0.0 could for example seek for "conf/FW1.0.0/objects" and Firmware 2.0.0 "conf/FW2.0.0/objects".
I would assume the firmware knows its firmware version and is able to construct the right key.
I was rather thinking in terms of smaller changes -- a 2.0 upgrade would
likely convert and erase the old config. But an incremental upgrade
might change the interpretation of some key, or the permitted range.
Would a version 1.1 that replaces the /foo/replay scalar with a struct
remove the replay key and write a /foo/replay2 key in the same
transaction?
> I understand this to not have any typing; all data items are
> arbitrarily long byte sequences. Should there be helpers for the
> typical case of accessing a u32 or a bool?
For the backend it is just bytes. For the internal representation in
RAM it is a struct which is parsed from the backend bytes.
That conversion sounds like a task that could gain efficiency from some
common tools -- but maybe that's better deferred to later in the thread.
> I'd appreciate a note on whose responsibility it is to fail safe in
> case of power interruptions.
Thats a heavy task. I would pass the problem to the backend :D
That's totally fine! I wasn't expecting any mechanism here, just a clear
statement what the backend needs to provide.
I agree the string code is ugly but could be good if it comes to URIs for remote confiuration.
At least they come from CORECONF URIs, then these URIs are just
encodings for numeric keys (if at all; in the latest revision it'll
mainly be CBOR in FETCH payloads IIUC).
With enums I would not know how to build a tree.
I don't have a clear suggestion here; could be that there are enums for
the individual path components (say, mapping every string between
slashes to an int16_t) and it's resolved like that, could be something
external like CORECONF's ("huge": 63bit -- still shorter than most
strings, and readable quicky as two aligned reads on most platforms)
SIDs.
Could just as well be names boiled by some preprocessor system into
(driver, struct offset) pairs. Probably depends on the answers to other
questions, and on the amount of descriptive pre-processing one is
willing to accept. (Like, would it be fine if the mount points / config
structure were described in some TOML or YANG file, which then gets
preprocesed into a .h file that has all the right macros?).
> Is encoding child names as ASCII numbers really the way to go here?
I did a configuration array of WiFi access points with it, and yes it
resulted in an enumeration `/wifi/ap/0` ...`/wifi/ap/1`.
There are two things that worry me about this approach:
* It's going back and forth between numerics and ASCII strings a lot.
(Right now I can't see clearly how often).
* If there should be semantic operations (like, "add a new item with the
next number") that need support from the back-end, this is one more
point where back-end struct and front-end usage need to be aligned.
(Frankly, I'm still not sure whether the idea here is that there would
be generic key-value backends, or fixed-structure backends, or an
application-specific mix, or a mix that's USEMODULE dependent).
That the backend knows the struct type or at least the maximum size of
a configuration item would ease implementation of it.
That the backend knows all the keys [...]
What are the expectations users could work with here?
Can a user start implementing their application on a generic back-end,
say, flashdb-on-mtd, and switch to a more bespoke back-end when
benchmarking indicates the need for it? Will that require changes to the
application? Do you expect to see a toolkit for building a more custom
back-end for applications that know their data structure?
Let's say there are configuration structs A, B and C and they fit on a flash page. There would be a handler and an internal allocated instance for each of struct A B and C.
The `get()` and `set()` would modify the internal instance and `export()` would call store() to sync the structs to the flash page. Maybe on another flashpage, the backend would need to store meta information like on which offset is which key stored, and add such an entry for every key that is added. This PR is not intended to facilitate backend implementation. It is expected that the backend is capable to allocate memory for new keys and values. The test backend is static though.
What determines the granularity of commits (exports?), and how is that
communicated?
|
Taking 2.) Maybe to facilitate efficient update, the backend 2: If the backend supports bytewise access it could use the offset parameter for optimized access. A mount path to a configuration object of which one member must be updated very frequently could store more than one backend pointer in the The export handler could allocate a struct which looks like the |
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.
Would a version 1.1 that replaces the /foo/replay scalar with a struct
remove the replay key and write a /foo/replay2 key in the same
transaction?
I think a new software be it a major or minor release should never delete a previous configuration by itself for backwards compatibility. Our choice to roll out a configuration would be via SUIT. So a new firmware comes with a new configuration. And storing at least 2 configurations should make it possible to roll back.
This convinces me that the configuration must be prefixed with the RIOT release version.
For example /conf/RIOT/202304/...
And then honestly configurations format changes in RIOT should only happen on new releases.
And the API should prepend the firmware string to the key.
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.
Would a version 1.1 that replaces the /foo/replay scalar with a struct
remove the replay key and write a /foo/replay2 key in the same
transaction?
I think a new software be it a major or minor release should never delete a previous configuration by itself for backwards compatibility. Our choice to roll out a configuration would be via SUIT. So a new firmware comes with a new configuration. And storing at least 2 configurations should make it possible to roll back.
This convinces me that the configuration must be prefixed with the RIOT release version.
For example /conf/RIOT/202304/...
And then honestly configurations format changes in RIOT should only happen on new releases.
And the API should prepend the firmware string to the key.
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.
Like, would it be fine if the mount points / config
structure were described in some TOML or YANG file, which then gets
preprocesed into a .h file that has all the right macros?
Having a TOML file for a subsystem in a /conf/RIOT/202304 folder would be an easy file system backend.
The import() function would read the file and parse it into a struct.
Regarding enums, the static path component to identify a handler could be easily mapped to an arbitrary enum.
But the dynamic part ... And there is always a finite number of enums in between two enums. For strings it is more flexible IMO. It could be overhead to manage the enum spaces.
The integer key thing also has no enforced for zephyr.
The concept for a configuration array is that we can know if an index is free or not. |
I think we're talking different things here -- I meant describing on the PC side in a TOML file the mount points and possibly even the keys in there. Parsing a TOML file at runtime sounds like it'd exceed the capacities of most RIOT systems. Sure it can be done, at least on the larger ones, but it'd occupy way more flash and stack than I'm comfortable allocating for an embedded configuration system, especially if it is supposed to be used by the OS. (For what it's worth, I think that any text-string processing function on an embedded device indicates a badly designed protocol underneath it, but that's an extremist PoV. But not doing TOML-style complexity on the microprocessor is probably a widely supported approach). |
To better understand the requirements on this, could we compare the proposed interface to the EEPROM "registry"? (Not that I'd be perfectly happy with it either, but to get a few properties straight).
Is that about it, or did I miss something? |
184efa7
to
2f7b2a0
Compare
c7a93ba
to
bafe126
Compare
a12eb80
to
a5657bb
Compare
Using 64 bit SIDs now as keys. A string path is constructed on handler lookup when |
54349dd
to
5f103c9
Compare
SID assignmentAn SID is a unique identifier for any configuration value. The global SID space ranges from 0x0000000000000000 to 0xffffffffffffffff. SID assignment rules for different types of configuration nodesSingle value nodeA simple node which represents one integer, string or byte array value is assigned a single SID within the SID range of its parent node. Compound value nodeA compound object must be assigned an appropriate SID range [ Array vale nodesAn array is also just a kind of container type and must be assigned an SID space. SID assignment exampleGiven is the schema from the current CORECONF draft:
Let the SID range That means
Having a at most 5 interfaces is a bit low though. Let the SID range In the context of this implementation, node types for all configuration targets must be allocated.
|
91a210b
to
1e7b0bd
Compare
1e7b0bd
to
6c4ef91
Compare
bd74ee5
to
06c41fd
Compare
c9f1c37
to
9b8a3e2
Compare
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.
This now needs a rebase to resolve a merge conflict due to an API change in XFA (which was required to fix a nasty memory alignment bug).
... and define a general _AUT_INIT() macro taking an XFA as parameter to allow for a second level auto-init XFA
9b8a3e2
to
5bac862
Compare
Contribution description
This provides a generic configuration module for setting/getting runtime configuration parameters of modules which require to store configuration parameters on persistent storage.
It should serve the same purpose as Zephyres settings subsystem. It is maybe a bit more limited because the zephyre module seems way more complex to me.
Multiple backend can be implemented in the future. The first backend implementation is included and uses the
FlashDB
.Read the documentation in
configuration.h
for more details.New modules:
configuration
: base moduleconfiguration_backend_flashdb_vfs
: enable FlashDB VFS mode as a configuration backendconfiguration_backend_flashdb_mtd
: enable FlashDB FAL mode as a configuration backendconfiguration_backend_reset_flashdb
: erase configuration fromFlashDB
backendauto_init_configuration
: 2nd level of auto-initXFA
calling all auto-init functions of configuration subsystemsauto_init_configuration_backend_flashdb
: auto-initFlashDB
backendTesting procedure
There is a test in
tests/configuration
. By default it uses a pseudo backendconfiguration_backend_ram
.$ make
You can also try the test with FlashDB:
$ TEST_CONFIGURATION_BACKEND=configuration_backend_flashdb_mtd make
When switching from FlashDB MTD to FlashDB VFS a format is required.
$ USEMODULE+=vfs_auto_format TEST_CONFIGURATION_BACKEND=configuration_backend_flashdb_vfs make
Issues/PRs references
It can help to solve #16844