Settings stream codec #9521

nvlsianpu · 2018-08-19T14:28:56Z

This patch reworks routines used to store and read the settings data.
Provide stream-style encoding and decoding to/from flash, so the the
API only requires a pointer to binary data, and the settings
implementation takes care of encoding/decoding to/from base64 and
writing/reading to/from flash on the fly. This would eliminate the
need of a separate base64 value buffer on the application-side, thereby
further contributing to the stack footprint reduction.

Above changes allows to remove:
256-byte value length limitation.
removing enum settings_type usage so all settings data are treated
now as a byte array (i.e. what's previously SETTINGS_BYTES)

Introduced routine settings_val_read_cb for read and decode the
settings data from storage inside h_set handler implementations.
h_set settings handler now provide persistent value's context
used along with read routine instead of immediately value.

Also the user code in higher modules will be aligned then.
For this patch all tests from path teste/subsys/settings/fcb pass.

Any new back-end need to implement two I/O handlers:

int (*read_cb)(void *ctx, off_t off, char *buf, size_t *len),
int (*write_cb)(void *ctx, off_t off, char const *buf, size_t len)
size_t (*get_len_cb)(void *ctx)

and register it using f. settings_line_io_init.
Above handler need only to provide RAW access to the back-end storage.
The Introduced codec take ceare about I/O access alignment.

remove Remove the tgt parameter to the export callback
The tgt parameter is unneeded and only complicates the API.

Signed-off-by: Andrzej Puzdrowski [email protected]

nvlsianpu · 2018-08-19T14:52:53Z

For sure the code still need some clean-up - so pleas comment on general conception and so one (no coding standard session for now).

codecov-io · 2018-08-19T15:00:43Z

Codecov Report

Merging #9521 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9521      +/-   ##
==========================================
+ Coverage   48.13%   48.19%   +0.06%     
==========================================
  Files         281      280       -1     
  Lines       43425    43259     -166     
  Branches    10406    10364      -42     
==========================================
- Hits        20902    20850      -52     
+ Misses      18368    18255     -113     
+ Partials     4155     4154       -1

Impacted Files	Coverage Δ
subsys/bluetooth/host/hci_core.c	`38.41% <ø> (ø)`	⬆️
subsys/bluetooth/host/gatt.c	`26.57% <ø> (ø)`	⬆️
subsys/bluetooth/host/keys.c	`10.22% <ø> (ø)`	⬆️
lib/cmsis_rtos_v1/cmsis_semaphore.c	`60% <0%> (-10%)`	⬇️
drivers/ethernet/eth_native_posix.c	`23.62% <0%> (-3.17%)`	⬇️
subsys/net/lib/http/http_server.c	`51.65% <0%> (-0.42%)`	⬇️
kernel/device.c	`95.34% <0%> (-0.21%)`	⬇️
kernel/sched.c	`92.78% <0%> (-0.05%)`	⬇️
subsys/net/lib/sockets/sockets_internal.h	`28.57% <0%> (ø)`	⬆️
include/net/socket.h	`100% <0%> (ø)`	⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e7c916...8b37f23. Read the comment docs.

nvlsianpu · 2018-08-31T10:26:06Z

@jhedberg @Laczen @carlescufi @lemrey Pleas review in yours convenience.

Laczen · 2018-09-04T16:35:48Z

Hi @nvlsianpu, sorry I did not respond any faster. I am tied up in other work. This is a great improvement to the settings module.

Maybe there is still an opening for a big simplification of the settings module. As I mentioned before the encoding could be removed. The encoding was imo needed for:

Storing binary information as string, when also other information (string, int, ...) was stored as string. If you now only have byte char storage this requirement has been removed.
Sending information over serial line (export to serial monitor). As this target has been removed encoding is also not required anymore.
Storing binary information in a ascii file with line ends. This is only required for nffs (I am not sure it is really required in this case). This requirement remains.

In fcb you could just store the items as "variable_name=" concatenated with the binary data. As you know the total length of an item in fcb getting the length of the binary data is the difference between the length and the location of the first "=". No need for encoding or streamed encoding. The advantage is of course the reduction in data size and the simplicity.

For nffs it might also be possible to do something like this (I have never used nffs). In the worst case nffs would still require encoding but the settings module would be a lot simpler for fcb.

nvlsianpu · 2018-09-06T09:50:38Z

@Laczen Thanks for comments - I received similar comments off-channel from my colleges. I will differentiate algorithm between FCB and FS.
Idea of this patch is to satisfy points 1,2,4,6 of #7413 .

Sending information over serial line (export to serial monitor). As this target has been removed encoding is also not required anymore.

What do you mean, what a serial monitor?

Laczen · 2018-09-06T11:10:37Z

Hi @nvlsianpu, what I meant by serial monitor was newt_mgr. The implementation in mynewt has the possibility to export to newt_mgr, this would require encoding to be able to do this.

nvlsianpu · 2018-09-07T06:56:04Z

@Laczen Sure - the idea is not to keep things like in the MyNewt it used to be. So zephyr-mcumgr-go-app will suport this (remote provisioning) in the appropriate shape.

include/settings/settings.h

rakons

This is a change in good direction. If the whole module is going to be rewriten it may be good moment to start using doxygen tags to describe structures here in correct way.

include/settings/settings.h

zephyrbot · 2018-11-22T15:26:23Z

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.

This Patch add tests for API in nffs and fcb back-ends. Signed-off-by: Andrzej Puzdrowski <[email protected]>

So far to deleting av existing key-value pair it was required to storing NULL value using setting_save_one(). This patch introduce more intuitive API which takes only the name key string. Signed-off-by: Andrzej Puzdrowski <[email protected]>

Added BT alisa for dumping binary data to the logg. The macro will be useful for BT as reworked settings supports r/w of raw data - which will be widely leveraged in bluetooth storage alignment to reworked settings. Signed-off-by: Andrzej Puzdrowski <[email protected]>

The Code need to be align after introduction of stream codec to setting serialization subsystem. Signed-off-by: Andrzej Puzdrowski <[email protected]>

nvlsianpu · 2018-12-07T16:01:52Z

@jhedberg updated

jhedberg

Looks generally good now, thanks! Mostly logging related comments still.

subsys/bluetooth/host/mesh/settings.c

The code need to be align after introduction of stream codec to setting serialization subsystem. Signed-off-by: Andrzej Puzdrowski <[email protected]>

The Code need to be align after introduction of stream codec to setting serialization subsystem. Signed-off-by: Andrzej Puzdrowski <[email protected]>

nvlsianpu · 2018-12-10T12:14:42Z

@jhedberg I hope I solved all problems you had found.

nvlsianpu · 2018-12-10T14:24:26Z

@Laczen @lemrey @Vudentz @sjanc Do any of you want to review this patch?

Laczen

Hi @nvlsianpu, very nice work, congrats.

Once this gets merged I will start working on adding a nvs backend. I would like to keep it as simple as possible, but with minimal flash wear:

Names are stored in id's 0x8001...0xBFFF
Corresponding values are stored in id's 0xC001..0xFFFF
id 0x8000 keeps track of the last name id in use.

This way:
a. Flash wear is minimal (only value updates are stored)
b. NVS direct use is possible for id's below 8000.

Settings_NVS would only need to keep track of:
a. settings_nvs_write_id (what id to use next, reusing deleted id's when possible),
b. settings_nvs_last_id (last id to use when reading at start).

carlescufi · 2018-12-10T17:54:33Z

@rakons could you please re-review?

rakons

I do not like the doxygen style used here to document structure - it might be corrected. But technically it is not an error. Some misssing return value documentation and also the parameters constantness requires thought. I am approving as any of the error here technically is not a something that would make the library unusable.

rakons · 2018-12-11T09:50:38Z

include/settings/settings.h

 */
 struct settings_handler {
 	sys_snode_t node;
+	/**< Linked list node info for module internal usage. */


Would not it be better to document it on top? Without < in the doxygen comment? This kind of comment is rather designed to be used in single name:

int val; /**< Super duper value field */

This looks ok.

int val; /**< Super duper value field */

Looks odd. Much better is:

/** Super duper value field */ int val;

rakons · 2018-12-11T09:51:35Z

include/settings/settings.h

+	int (*h_get)(int argc, char **argv, char *val, int val_len_max);
+	/**< Get values handler of settings items identified by keyword names.
+	 *
+	 * Parameters:


Use @param.

Return value documentation missing.

rakons · 2018-12-11T09:52:19Z

include/settings/settings.h

+	int (*h_set)(int argc, char **argv, void *value_ctx);
+	/**< Set value handler of settings items identified by keyword names.
+	 *
+	 * Parameters:


Use @param. Return value documentation missing.

rakons · 2018-12-11T09:52:40Z

include/settings/settings.h

 	int (*h_commit)(void);
-	int (*h_export)(int (*export_func)(const char *name, char *val),
-			enum settings_export_tgt tgt);
+	/**< This handler gets called after settings has been loaded in full.


Return value documentation.

rakons · 2018-12-11T09:53:05Z

include/settings/settings.h

+	/**< This gets called to dump all current settings items.
+	 *
+	 * This happens when @ref settings_save tries to save the settings.
+	 * Parameters:


Use @param. Document return value.

rakons · 2018-12-11T09:54:35Z

include/settings/settings.h

 * @return 0 on success, non-zero on failure.
 */
-int settings_set_value(char *name, char *val_str);
+int settings_set_value(char *name, void *value, size_t len);


I believe the value can be const here.

rakons · 2018-12-11T09:55:55Z

include/settings/settings.h

 *
 * @return 0 on success, non-zero on failure.
 */
 int settings_commit(char *name);


rakons · 2018-12-11T09:57:06Z

include/settings/settings.h

-
-#define SETTINGS_VALUE_SET(str, type, val)                                  \
-	settings_value_from_str((str), (type), &(val), sizeof(val))
+size_t settings_val_get_len_cb(void *value_ctx);


rakons · 2018-12-11T09:58:45Z

samples/boards/nrf52/mesh/onoff_level_lighting_vnd_app/src/storage.c

 }

-static int ps_set(int argc, char **argv, char *val)
+static int ps_set(int argc, char **argv, void *val_ctx)


For me it seems that we can have const char *const *argv here.

rakons · 2018-12-11T09:59:57Z

subsys/bluetooth/host/gatt.c

 }

-static int ccc_set(int argc, char **argv, char *val)
+static int ccc_set(int argc, char **argv, void *val_ctx)


char const * const *argv ?

carlescufi

Great job @nvlsianpu

Whenever a new key-value is about to be stored, the settings perform check whether the value really changes. This check after zephyrproject-rtos#9521 patch should work differently as `\0` is not the value terminator anymore. Because of above any value which starts from \0 will be treated mistakenly as a NULL. This patch uses check-length callback instead read-callback which fix the issue and simplify the code a little. Signed-off-by: Andrzej Puzdrowski <[email protected]>

Whenever a new key-value is about to be stored, the settings perform check whether the value really changes. This check after #9521 patch should work differently as `\0` is not the value terminator anymore. Because of above any value which starts from \0 will be treated mistakenly as a NULL. This patch uses check-length callback instead read-callback which fix the issue and simplify the code a little. Signed-off-by: Andrzej Puzdrowski <[email protected]>

Patch removes dead, replaced code which was accidentally not removed within the stream-codec PR zephyrproject-rtos#9521. Signed-off-by: Andrzej Puzdrowski <[email protected]>

Patch removes dead, replaced code which was accidentally not removed within the stream-codec PR #9521. Signed-off-by: Andrzej Puzdrowski <[email protected]>

nvlsianpu requested review from Laczen, carlescufi, jhedberg and lemrey August 19, 2018 14:32

nvlsianpu added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Aug 19, 2018

nvlsianpu mentioned this pull request Aug 19, 2018

settings subsystem enhancements #7413

Open

7 tasks

jhedberg requested changes Sep 10, 2018

View reviewed changes

include/settings/settings.h Outdated Show resolved Hide resolved

jhedberg mentioned this pull request Sep 20, 2018

Bluetooth API to access bond data #10122

Closed

rakons suggested changes Sep 20, 2018

View reviewed changes

include/settings/settings.h Outdated Show resolved Hide resolved

nashif added the DNM This PR should not be merged (Do Not Merge) label Sep 23, 2018

nvlsianpu force-pushed the settings_stream_codec branch from aebee6e to 753f97a Compare October 5, 2018 10:05

nvlsianpu force-pushed the settings_stream_codec branch 3 times, most recently from 7e4d791 to 9a8f0a0 Compare November 16, 2018 16:07

nvlsianpu changed the title ~~[RFC][DNM] Settings stream codec~~ [DNM] Settings stream codec Nov 16, 2018

This was referenced Nov 20, 2018

subsys: settings: stream encoding nvlsianpu/zephyr#3

Closed

subsys: settings: Invalidate entry with the same key in FCB #11574

Closed

nvlsianpu force-pushed the settings_stream_codec branch 2 times, most recently from 41c46ef to e86c420 Compare November 22, 2018 15:09

nvlsianpu requested review from Vudentz and sjanc as code owners November 24, 2018 15:33

nvlsianpu force-pushed the settings_stream_codec branch from a5f55ab to 85f9ea5 Compare November 27, 2018 09:29

nvlsianpu added 4 commits December 7, 2018 11:35

tests: subsys: settings: tests value get len API

c142970

This Patch add tests for API in nffs and fcb back-ends. Signed-off-by: Andrzej Puzdrowski <[email protected]>

settings: add delete key-value pair API

5076473

So far to deleting av existing key-value pair it was required to storing NULL value using setting_save_one(). This patch introduce more intuitive API which takes only the name key string. Signed-off-by: Andrzej Puzdrowski <[email protected]>

bluetooth: host: align code to reworked settings

05e9448

The Code need to be align after introduction of stream codec to setting serialization subsystem. Signed-off-by: Andrzej Puzdrowski <[email protected]>

nvlsianpu force-pushed the settings_stream_codec branch from 9152902 to 056c030 Compare December 7, 2018 11:15

jhedberg requested changes Dec 7, 2018

View reviewed changes

nvlsianpu added 2 commits December 10, 2018 11:56

bluetooth: host: mesh: align code to reworked settings

6e5cc2d

The code need to be align after introduction of stream codec to setting serialization subsystem. Signed-off-by: Andrzej Puzdrowski <[email protected]>

sample: host: mesh: onoff_level_lighting align code to reworked settings

8b37f23

The Code need to be align after introduction of stream codec to setting serialization subsystem. Signed-off-by: Andrzej Puzdrowski <[email protected]>

nvlsianpu force-pushed the settings_stream_codec branch from 056c030 to 8b37f23 Compare December 10, 2018 10:57

nvlsianpu mentioned this pull request Dec 10, 2018

[Coverity CID :189511]Code maintainability issues in /subsys/settings/src/settings_fcb.c #11095

Closed

jhedberg approved these changes Dec 10, 2018

View reviewed changes

Laczen approved these changes Dec 10, 2018

View reviewed changes

rakons approved these changes Dec 11, 2018

View reviewed changes

carlescufi approved these changes Dec 11, 2018

View reviewed changes

carlescufi merged commit 2267668 into zephyrproject-rtos:master Dec 11, 2018

nvlsianpu mentioned this pull request Dec 11, 2018

settings: fix key-value duplication check #12026

Merged

vikrant8052 mentioned this pull request Dec 12, 2018

samples: mesh: nrf52: removed unwanted code #12017

Closed

nvlsianpu mentioned this pull request Dec 18, 2018

settings: documenting of behaviour when settings_load is called #12166

Closed

nvlsianpu mentioned this pull request Mar 20, 2019

settings: remove dead code #14738

Merged

galak pushed a commit that referenced this pull request Mar 20, 2019

settings: remove dead code

6b62e5e

Patch removes dead, replaced code which was accidentally not removed within the stream-codec PR #9521. Signed-off-by: Andrzej Puzdrowski <[email protected]>

Settings stream codec #9521

Settings stream codec #9521

Uh oh!

Conversation

nvlsianpu commented Aug 19, 2018 • edited by himanshujha199640 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvlsianpu commented Aug 19, 2018

Uh oh!

codecov-io commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nvlsianpu commented Aug 31, 2018

Uh oh!

Laczen commented Sep 4, 2018

Uh oh!

nvlsianpu commented Sep 6, 2018

Uh oh!

Laczen commented Sep 6, 2018

Uh oh!

nvlsianpu commented Sep 7, 2018

Uh oh!

Uh oh!

rakons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zephyrbot commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvlsianpu commented Dec 7, 2018

Uh oh!

jhedberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nvlsianpu commented Dec 10, 2018

Uh oh!

nvlsianpu commented Dec 10, 2018

Uh oh!

Laczen left a comment

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Dec 10, 2018

Uh oh!

rakons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvlsianpu commented Aug 19, 2018 •

edited by himanshujha199640

Loading

codecov-io commented Aug 19, 2018 •

edited

Loading

zephyrbot commented Nov 22, 2018 •

edited

Loading