Settings reworked to const char processing

[Laczen]

This is an alternative implementation of #16609.

The PR provides:
a. Working with const char names instead of argc, argv list
b. The ability to register settings_handler names containing one or multiple separators
c. The ability to register a handler for each variable
d. Loading of variables belonging to a subtree and by this allowing RAM reduction in the
module/application using settings.

[Laczen]

@dbkinder, don't review this yet.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[Laczen]

@fnde-ot, this would be a breaking change for your custom backend: the load function is changed (the same for #16609).

[rakons]

Looks good. I did not analyse all the corner cases for the new way of registration and key searching - request for unit tests for that.

Currently implemented unit tests fails in many places - I have run only fcb/raw and 9 tests fail.

[rakons]

Just a side note (nitpicking) - should not it be done on a private fork? I understand that now it would be a waste of time and resources to move it, but just for the future...

[Vudentz]

Is this something I could call on bt_init? What if the application is also calling settings_load by itself then? Would that cause the handler to be called once again? I guess we should clearly state on the documentation this sort of behavior.

[Laczen]

Yes you could, the mutex would not stop the handler to be called again. For the documentation it should be clear:
settings_load(): loads all entries,
settings_load_subtree(): loads limited set of entries (subtree is not limited to handler names, it can be a subset of a handler name).

But maybe your are asking something else: I have a handler that I want to call myself, and nobody else, how do I do this ?
Option 1: Add a flag to the set routine and only allow processing of the set routine if this flag is set. Do the loading by: enabling the flag, call settings_load_subtree, disable the flag. This is not thread-safe but might be sufficient.
Option 2: Don't register the settings_handler yet, do the loading by register the handler, call settings_load_subtree, deregister the handler. At the moment this is not yet thread-safe, but this could be converted into one routine and made thread-safe.

[Vudentz]

My question was that if this would cause the handler to be called multiple times, one by load and another by load_subtree that may cause extra loading from no reason, I guess the idea is that each subsystem calls load_substree and load would take care of the remaining settings which most likely are just app specific, this way the more natural sequence would be subsystem blah settings_load_subtree("blah") (marked it as loaded)... -> app settings_load()

[Laczen]

oops, sorry forgot to answer the question: Yes, it would be called several times.

There is no requirement on how to use it, the idea is that it would be needed to call a settings_load_subtree() as a result of a connection request so that only limited RAM would be occupied for storing this information.

Your natural sequence could also be done: using option 2 from above settings_load_handler(&blah_handler): registers blah, loads blah elements, unregister blah (this is the marked as loaded). Do this for each subsystem and finally call settings_load for all app related stuff.

Should I add a settings_load_handler() like defined above ?

[rakons]

It depends on how we wish the system to be designed. I believe that the full settings load should be called only once - on system start. And it should be left to the module implementation to decide how we wish to tread incoming data (for example, any CCCD data may be ignored if we do not have an active connection already).

Then when the system works, we can call settings_load_subtree (or load_selected from #16609) whenever needed.

[fnde-ot]

Why do we even need a settings_load()? Couldn't each module just call settings_load_subtree() when appropriate on init (or on demand)? The app can always have its own subtree (fx. "app/") and use settings_load_subtree().

[Laczen]

@fnde-ot, completely agree, we could remove the settings_load() completely. Combining this with the remark from @Vudentz in #16676 would open a lot of opportunities. There of course would be
a disadvantage that all entries in the backend are read multiple times, but maybe even this can be avoided (e.g. for the bluetooth system): let us suppose there are multiple subsystems defined (bt/btmesh, bt/ccc, bt/gatt ... these are just examples I'm not a bt expert) each of these would define their own set routines. To load all bt related settings we would define bt_set routines as:

int bt_set(const char *name, ...) {
  char *next;
#ifdef(BTMESH)
  if (settings_name_cmp(name,"btmesh", &next)) {
    return btmesh_set(next, ...);
  }
#endif
#ifdef(BTCCC)
  if (settings_name_cmp(name, "ccc", &next)) {
    return ccc_set(next, ....);
  }
#endif
}

int bt_load() {
  settings_load_subtree("bt", bt_set);
}

[rakons]

And you cannot agree with my proposal as it requires too much of rework inside modules. I ask you not to go that direction here - separate this idea and offer it in another PR.

[fnde-ot]

@Laczen @rakons, Sounds to me more like a backend problem. On-demand or subtree loading should not require significant changes to existing modules, appart from adding calls to settings_load_subtree(...) in the appropriate init functions.

The multiple-read would not be an issue if the backend properly separated settings from different modules (not a flash expert, but maybe using multiple files or grouping settings from a single module together and reading from a specific offset). But then again, this could require to rethink the way existing backends work, which would be out of scope for this PR.

We can still remove settings_load from this PR and rework the backends in another one.

[Laczen]

@rakons, I cannot agree with your proposal because:
a. The pattern search is to much code and not needed
b. The split up to get a dynamic generated name is not done and is relying on string conversion routines to do the split up. This only works if there is a conversion routine available.

I have accepted doing the rework inside the modules by not introducing the setiings_name_separate(), but there is a very specific reason for this: reducing RAM usage in the case where a dynamic name would be used at the top of some tree where most entries under this would have a fixed part (e.g. bt/ccc/0xfffe/info/data/val1, bt/ccc/0xffe/info/data/val2, bt/ccc/0xffe/id, and the same entries for bt/ccc/0xfffd...).

This however has nothing to do with the discussion above: the proposal has triggered other (new) ideas to make the proposal even simpler (remove registering of handlers and looking up handlers) and it would be a pity not to take these ideas into account.

[nvlsianpu]

I put my comments. I'm also inline with @rakons comments.

[nvlsianpu]

@Laczen @rako @Vudentz We really want to proces this PR quickly - so It would be great to limit its scope only to things we are agreed on them, and not escalate discussion here.

[nvlsianpu]

I wish we can merge it tomorrow.

[Laczen]

@nvlsianpu, I am updating the tests. Everything is ok for fcb, for nffs I get a compilation error (for nrf52_pca10040): #include <nffs/nffs.h> cannot be found -> has something changed here ?

[Laczen]

@rakons, @nvlsianpu, I am doubting if we need to expose settings_name_steq() to the modules/user. settings_name_next() provides all the needed functionality when used in combination with strncmp(). It would also force modules into avoiding using the string comparison multiple times.

[nvlsianpu]

Maybe worth to align settings's client modules to code changes - so It will be visible whether it make sense.

I mean I reali appreciated user experience - which comes from using subtree name explicit - zephyr modules might not use it at all, but for high level application development it might be useful. I think settings_name_steq() might gives good user experience - I would not resign form it in this PR.

[rakons]

Accepting it now - it seems to go in the right direction and I wish not to block it while I am on my leave.

[Laczen]

• Update the settings users to the new API (also already ported)

done

@jhe, @Vudentz, please review. @Vudentz, I have discovered that "bt/hash" is written twice during startup of an app with bt, even when the handlers for "bt/hash" are disabled, maybe there is something wrong.

[nvlsianpu]

I'm doing one more review round (I believe the last on:D)

[Laczen]

Hi @nvlsianpu, I have prepared a new commit that adds the possibility to have rom registered handles without any (extra) API change. It just adds a function SETTINGS_REGISTER_ROM(handler) and the handler needs to be const. Two Kconfig option are added: SETTINGS_RAM_HANDLERS and SETTINGS_ROM_HANDLERS both set to y by default.

I would add this to the PR as a separate commit, so it can be cherrypicked. With this last commit I would stop working on the settings module.

[nvlsianpu]

^^ better open new PR

[nvlsianpu]

Tests passes (verified nrf52_10040.)
functional tests LGTM
settings client adaptations looks ok
f. settings_name_steq() is kept (appreciated)
I had just 2 nips which can be fixed even later

[nvlsianpu]

just nip: not inserted anymore

[nvlsianpu]

nip: looks like this test doesn't ensure storage clearance - it fails therefore in second run in line 222

[nvlsianpu]

@Laczen Can DNM label be removed?

[nvlsianpu]

@jhedberg, @Vudentz Can you look at relevant client modules changes?

[jhedberg]

I've gone through the Bluetooth-related changes, and they look fine to me. I haven't tested anything however, so it'd be good if someone would find time to do this.

[nvlsianpu]

I've gone through the Bluetooth-related changes, and they look fine to me. I haven't tested anything however, so it'd be good if someone would find time to do this.

Checked that bond info and keys are stored/loaded properly, ccc storage works well as well.

[nvlsianpu]

@carlescufi This PR can be merged.

[aescolar]

@Laczen , @nvlsianpu : about @dbkinder negative review, regarding documentation, is this PR not meant to add/change documentation anymore?

[Laczen]

@aescolar, I think this negative review was the result of a sample that was missing documentation. This sample was never meant to be merged but to allow discussion. The sample is not there anymore so I am unsure about the negative review.

not applicable