Skip to content

Settings reworked to const char processing #16711

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 18, 2019
Merged

Settings reworked to const char processing #16711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b0fb2c
subsys/settings: change processing to const char
Laczen Jun 8, 2019
791309a
subsys/settings: Update client modules
Laczen Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 82 additions & 22 deletions include/settings/settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ extern "C" {
#define SETTINGS_MAX_DIR_DEPTH 8 /* max depth of settings tree */
#define SETTINGS_MAX_NAME_LEN (8 * SETTINGS_MAX_DIR_DEPTH)
#define SETTINGS_MAX_VAL_LEN 256
#define SETTINGS_NAME_SEPARATOR "/"
#define SETTINGS_NAME_SEPARATOR '/'
#define SETTINGS_NAME_END '='

/* pleace for settings additions:
* up to 7 separators, '=', '\0'
Expand All @@ -47,26 +48,27 @@ struct settings_handler {
char *name;
/**< Name of subtree. */

int (*h_get)(int argc, char **argv, char *val, int val_len_max);
int (*h_get)(const char *key, char *val, int val_len_max);
/**< Get values handler of settings items identified by keyword names.
*
* Parameters:
* - argc - count of item in argv.
* - argv - array of pointers to keyword names.
* - val - buffer for a value.
* - val_len_max - size of that buffer.
* - key[in] the name with skipped part that was used as name in
* handler registration
* - val[out] buffer to receive value.
* - val_len_max[in] size of that buffer.
*/

int (*h_set)(int argc, char **argv, size_t len,
settings_read_cb read_cb, void *cb_arg);
int (*h_set)(const char *key, size_t len, settings_read_cb read_cb,
void *cb_arg);
/**< Set value handler of settings items identified by keyword names.
*
* Parameters:
* - argc - count of item in argv.
* - argv - array of pointers to keyword names.
* - len - the size of the data found in the backend.
* - read_cb - function provided to read the data from the backend.
* - cb_arg - arguments for the read function provided by the backend.
* - key[in] the name with skipped part that was used as name in
* handler registration
* - len[in] the size of the data found in the backend.
* - read_cb[in] function provided to read the data from the backend.
* - cb_arg[in] arguments for the read function provided by the
* backend.
*/

int (*h_commit)(void);
Expand Down Expand Up @@ -121,8 +123,18 @@ int settings_register(struct settings_handler *cf);
int settings_load(void);

/**
* Save currently running serialized items. All serialized items which are different
* from currently persisted values will be saved.
* Load limited set of serialized items from registered persistence sources.
* Handlers for serialized item subtrees registered earlier will be called for
* encountered values that belong to the subtree.
*
* @param[in] subtree name of the subtree to be loaded.
* @return 0 on success, non-zero on failure.
*/
int settings_load_subtree(const char *subtree);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


/**
* Save currently running serialized items. All serialized items which are
* different from currently persisted values will be saved.
*
* @return 0 on success, non-zero on failure.
*/
Expand Down Expand Up @@ -161,6 +173,16 @@ int settings_delete(const char *name);
*/
int settings_commit(void);

/**
* Call commit for settings handler that belong to subtree.
* This should apply all settings which has been set, but not applied yet.
*
* @param[in] subtree name of the subtree to be committed.
*
* @return 0 on success, non-zero on failure.
*/
int settings_commit_subtree(const char *subtree);

/**
* @} settings
*/
Expand Down Expand Up @@ -191,8 +213,9 @@ struct settings_store {
* Destinations are registered using a call to @ref settings_dst_register.
*/
struct settings_store_itf {
int (*csi_load)(struct settings_store *cs);
/**< Loads all values from storage.
int (*csi_load)(struct settings_store *cs, const char *subtree);
/**< Loads values from storage limited to subtree defined by subtree. If
* subtree = NULL loads all values.
*
* Parameters:
* - cs - Corresponding backend handler node
Expand Down Expand Up @@ -248,16 +271,53 @@ void settings_dst_register(struct settings_store *cs);
/**
* Parses a key to an array of elements and locate corresponding module handler.
*
* @param name Key in string format
* @param name_argc Parsed number of elements.
* @param name_argv Parsed array of elements.
* @param[in] name in string format
* @param[out] next remaining of name after matched handler
*
* @return settings_handler node on success, NULL on failure.
*/
struct settings_handler *settings_parse_and_lookup(char *name, int *name_argc,
char *name_argv[]);
struct settings_handler *settings_parse_and_lookup(const char *name,
const char **next);


/*
* API for const name processing
*/

/**
* Compares the start of name with a key
*
* @param[in] name in string format
* @param[in] key comparison string
* @param[out] next pointer to remaining of name, when the remaining part
* starts with a separator the separator is removed from next
*
* Some examples:
* settings_name_steq("bt/btmesh/iv", "b", &next) returns 1, next="t/btmesh/iv"
* settings_name_steq("bt/btmesh/iv", "bt", &next) returns 1, next="btmesh/iv"
* settings_name_steq("bt/btmesh/iv", "bt/", &next) returns 0, next=NULL
* settings_name_steq("bt/btmesh/iv", "bta", &next) returns 0, next=NULL
*
* REMARK: This routine could be simplified if the settings_handler names
* would include a separator at the end.
*
* @return 0: no match
* 1: match, next can be used to check if match is full
*/
int settings_name_steq(const char *name, const char *key, const char **next);

/**
* determine the number of characters before the first separator
*
* @param[in] name in string format
* @param[out] next pointer to remaining of name (excluding separator)
*
* @return index of the first separator, in case no separator was found this
* is the size of name
*
*/
int settings_name_next(const char *name, const char **next);

/*
* API for runtime settings
*/
Expand Down
6 changes: 4 additions & 2 deletions samples/bluetooth/peripheral_dis/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ static struct bt_conn_cb conn_callbacks = {
.disconnected = disconnected,
};

static int zephyr_settings_fw_load(struct settings_store *cs);
static int zephyr_settings_fw_load(struct settings_store *cs,
const char *subtree);

static const struct settings_store_itf zephyr_settings_fw_itf = {
.csi_load = zephyr_settings_fw_load,
Expand All @@ -55,7 +56,8 @@ static struct settings_store zephyr_settings_fw_store = {
.cs_itf = &zephyr_settings_fw_itf
};

static int zephyr_settings_fw_load(struct settings_store *cs)
static int zephyr_settings_fw_load(struct settings_store *cs,
const char *subtree)
{

#if defined(CONFIG_BT_GATT_DIS_SETTINGS)
Expand Down
22 changes: 13 additions & 9 deletions samples/boards/nrf52/mesh/onoff_level_lighting_vnd_app/src/storage.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,48 +114,52 @@ void save_on_flash(u8_t id)
k_work_submit(&storage_work);
}

static int ps_set(int argc, char **argv, size_t len_rd,
static int ps_set(const char *key, size_t len_rd,
settings_read_cb read_cb, void *cb_arg)
{
ssize_t len = 0;
int key_len;
const char *next;

if (argc == 1) {
if (!strcmp(argv[0], "rc")) {
key_len = settings_name_next(key, &next);

if (!next) {
if (!strncmp(key, "rc", key_len)) {
len = read_cb(cb_arg, &reset_counter,
sizeof(reset_counter));
}

if (!strcmp(argv[0], "gdtt")) {
if (!strncmp(key, "gdtt", key_len)) {
len = read_cb(cb_arg,
&gen_def_trans_time_srv_user_data.tt,
sizeof(gen_def_trans_time_srv_user_data.tt));
}

if (!strcmp(argv[0], "gpo")) {
if (!strncmp(key, "gpo", key_len)) {
len = read_cb(cb_arg,
&gen_power_onoff_srv_user_data.onpowerup,
sizeof(gen_power_onoff_srv_user_data.onpowerup));
}

if (!strcmp(argv[0], "ltd")) {
if (!strncmp(key, "ltd", key_len)) {
len = read_cb(cb_arg,
&light_ctl_srv_user_data.lightness_temp_def,
sizeof(light_ctl_srv_user_data.lightness_temp_def));
}

if (!strcmp(argv[0], "ltl")) {
if (!strncmp(key, "ltl", key_len)) {
len = read_cb(cb_arg,
&light_ctl_srv_user_data.lightness_temp_last,
sizeof(light_ctl_srv_user_data.lightness_temp_last));
}

if (!strcmp(argv[0], "lr")) {
if (!strncmp(key, "lr", key_len)) {
len = read_cb(cb_arg,
&light_lightness_srv_user_data.lightness_range,
sizeof(light_lightness_srv_user_data.lightness_range));
}

if (!strcmp(argv[0], "tr")) {
if (!strncmp(key, "tr", key_len)) {
len = read_cb(cb_arg,
&light_ctl_srv_user_data.temperature_range,
sizeof(light_ctl_srv_user_data.temperature_range));
Expand Down
29 changes: 16 additions & 13 deletions subsys/bluetooth/host/gatt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3558,26 +3558,29 @@ static u8_t ccc_load(const struct bt_gatt_attr *attr, void *user_data)
return load->count ? BT_GATT_ITER_CONTINUE : BT_GATT_ITER_STOP;
}

static int ccc_set(int argc, char **argv, size_t len_rd,
settings_read_cb read_cb, void *cb_arg)
static int ccc_set(const char *name, size_t len_rd, settings_read_cb read_cb,
void *cb_arg)
{
struct ccc_store ccc_store[CCC_STORE_MAX];
struct ccc_load load;
bt_addr_le_t addr;
int len, err;
const char *next;

if (argc < 1) {
settings_name_next(name, &next);

if (!name) {
BT_ERR("Insufficient number of arguments");
return -EINVAL;
} else if (argc == 1) {
} else if (!next) {
load.addr_with_id.id = BT_ID_DEFAULT;
} else {
load.addr_with_id.id = strtol(argv[1], NULL, 10);
load.addr_with_id.id = strtol(next, NULL, 10);
}

err = bt_settings_decode_key(argv[0], &addr);
err = bt_settings_decode_key(name, &addr);
if (err) {
BT_ERR("Unable to decode address %s", argv[0]);
BT_ERR("Unable to decode address %s", name);
return -EINVAL;
}

Expand Down Expand Up @@ -3614,21 +3617,21 @@ static int ccc_set(int argc, char **argv, size_t len_rd,
BT_SETTINGS_DEFINE(ccc, ccc_set, NULL, NULL);

#if defined(CONFIG_BT_GATT_CACHING)
static int cf_set(int argc, char **argv, size_t len_rd,
settings_read_cb read_cb, void *cb_arg)
static int cf_set(const char *name, size_t len_rd, settings_read_cb read_cb,
void *cb_arg)
{
struct gatt_cf_cfg *cfg;
bt_addr_le_t addr;
int len, err;

if (argc < 1) {
if (!name) {
BT_ERR("Insufficient number of arguments");
return -EINVAL;
}

err = bt_settings_decode_key(argv[0], &addr);
err = bt_settings_decode_key(name, &addr);
if (err) {
BT_ERR("Unable to decode address %s", argv[0]);
BT_ERR("Unable to decode address %s", name);
return -EINVAL;
}

Expand Down Expand Up @@ -3662,7 +3665,7 @@ BT_SETTINGS_DEFINE(cf, cf_set, NULL, NULL);

static u8_t stored_hash[16];

static int db_hash_set(int argc, char **argv, size_t len_rd,
static int db_hash_set(const char *name, size_t len_rd,
settings_read_cb read_cb, void *cb_arg)
{
int len;
Expand Down
19 changes: 11 additions & 8 deletions subsys/bluetooth/host/keys.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,17 +277,18 @@ int bt_keys_store(struct bt_keys *keys)
return 0;
}

static int keys_set(int argc, char **argv, size_t len_rd,
settings_read_cb read_cb, void *cb_arg)
static int keys_set(const char *name, size_t len_rd, settings_read_cb read_cb,
void *cb_arg)
{
struct bt_keys *keys;
bt_addr_le_t addr;
u8_t id;
size_t len;
int err;
char val[BT_KEYS_STORAGE_LEN];
const char *next;

if (argc < 1) {
if (!name) {
BT_ERR("Insufficient number of arguments");
return -EINVAL;
}
Expand All @@ -298,18 +299,20 @@ static int keys_set(int argc, char **argv, size_t len_rd,
return -EINVAL;
}

BT_DBG("argv[0] %s val %s", argv[0], (len) ? val : "(null)");
BT_DBG("name %s val %s", name, (len) ? val : "(null)");

err = bt_settings_decode_key(argv[0], &addr);
err = bt_settings_decode_key(name, &addr);
if (err) {
BT_ERR("Unable to decode address %s", argv[0]);
BT_ERR("Unable to decode address %s", name);
return -EINVAL;
}

if (argc == 1) {
settings_name_next(name, &next);

if (!next) {
id = BT_ID_DEFAULT;
} else {
id = strtol(argv[1], NULL, 10);
id = strtol(next, NULL, 10);
}

if (!len) {
Expand Down
Loading