My two cents on your questions. BTW, I do agree that it is tricky to implement application code with the config so I think the work you have done is worthwhile
1. No strong opinion here. 8 bytes per group is not the worst and hopefully an application will not have many groups, and if it does, one would suspect it would have the space to support them. 2. I agree: no. 3. It would be nice but not "needed". Will On Fri, Feb 7, 2020 at 2:25 PM Christopher Collins <ccollins47...@gmail.com> wrote: > Hello all, > > The `sys/config` package lets an app persist and restore data in > permanent storage. It is documented here: > <http://mynewt.apache.org/latest/os/modules/config/config.html>. > > This package does its job well and its design is sound. It has one > minor problem though (in my opinion, in case that needs saying) - it is > nearly impossible to use! Here is an example: > < > https://github.com/apache/mynewt-core/blob/master/boot/split/src/split_config.c > >. > This file implements a single 8-bit setting called `split/status`. This > is not an extreme example; it is how all `sys/config` client code looks. > Speaking for myself, it is a major implementation effort whenever I need > to add a new setting, or worse, a new setting group. As basic and > fundamental as data persistence is to an embedded application, this > should be an easier task. > > The problem is obvious: the config package is very powerful, and with > that power comes a very complex API. > > I think most application code would be fine with a less powerful > library. Such a library would allow all settings to be loaded, > individual settings to be saved on demand, and very little else. It > would not be possible to commit several settings at once or to execute > custom code when a setting is saved or loaded. With this simplified set > of requirements in mind, I came up with a wish list for an API: > > 1. It should be easy to create a new setting group. Ideally you could > just copy and paste from existing code. > > 2. Creating a new setting should be as simple as appending an element to > an array. > > 3. Persisting a setting should be possible with a single function call. > > So I took that and implemented... something. The library is called > "scfg" (short for "simple config"). I'll go into more details below, > but first, here is how that `split/status` configuration would be done > using the new library: <https://github.com/apache/mynewt-core/pull/2183> > [1]. I think this is a marked improvement, but please judge for > yourselves. > > ### Implementation > > I had the idea that we could just build a simple library on top of the > existing `sys/config` library. Unfortunately, `sys/config` doesn't lend > itself well to this kind of a wrapper library. The problem is that the > `conf_handler` callbacks don't accept a `void *arg` parameter, meaning > every config group must define a dedicated set of callback functions. > This is an issue because scfg needs to define a generic set of callbacks > for all config groups. So I had to modify `sys/config` so that the > callbacks accepted an extra generic argument, and this had to be done in > a backwards compatible way. It's not pretty, but this is what I came up > with: <https://github.com/apache/mynewt-core/pull/2181>. > > The scfg library required a second change to `sys/config`: Support for > unsigned integer types. Only signed integer types are currently > supported. Before my change, when a config setting needed to use an > unsigned type, the handler would work around this limitation by > specifying a signed integer type that is slightly wider than the actual > data. Then the conf handler callbacks would manually convert between > the unsigned and signed types. Since scfg does not allow for arbitrary > code to run during save and restore operations, the library needs to > handle unsigned types natively. I made this change here: > <https://github.com/apache/mynewt-core/pull/2180>. > > Finally, here is the PR for the scfg library itself: > <https://github.com/apache/mynewt-core/pull/2182>. > > ### Questions > > 1. Do we want to enable extended callbacks in `sys/config` by default, > or should this require a syscfg setting to be enabled? This change adds > eight bytes to every config group which is a bummer. I prefer that we > enable this change unconditionally. I think a simplified config > interface is valuable enough that developers should be able to assume it > is available. > > 2. Does scfg need some sort of "on-loaded" callback for each setting? > My feeling is no, we don't need that. If applications need to know when > settings are restored, then we can modify `sys/config` to allow > callbacks to be registered. These callbacks would be executed after > `conf_load()` completes. > > 3. Do we need support for binary data? `sys/config` only allows text > values for settings. Binary blobs must be converted to text using > something like base64 before they are saved. This is a nuissance for > large settings (e.g., nimble host security data). I would love it if we > could support raw binary values, but my impression is that this would > require massive changes to the sys/config library. I think this change > would be quite valuable, but it is a feature in itself and it should be > considered separately. > > All comments are welcome. > > [1] It seems the split image feature is broken again. In light of that, > this probably wasn't the best group to use as an example! > > Thanks for reading, > Chris > >
