KVStore & derived classes: design docs, implementation & configuration #8667

davidsaada · 2018-11-07T16:34:10Z

Description

This PR implements KVStore, the new Key Value based storage framework. It includes the following:

KVStore - Key Value store base class.

Derived classes:

TDBStore - Tiny Database store over a flash device
FileSystemStore - Key Value store over file system
SecureStore - A Security Key Value store implementation over an underlying key value store

Global APIs - A set of APIs to access all KVStore classes in a C style manner.

Configuration framework - Means to generate configurations for the above, covering most of the typical applications.

Device Key adaptations.

Design documents for all of the above.

Implemented by @yossi2le, @offirko, @theamirocohen & myself, supervised by @dannybenor.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

dannybenor · 2018-11-08T07:04:32Z

This PR will be updated with small changes while reviewed. We already reviewed internally and therefore I will approve it

0xc0170 · 2018-11-08T09:28:24Z

travis-ci/astyle — Passed, 556 files (+11 files)

Can you run astyle on new added files here? #8591 is addressing storage files in the current codebase, so only new files in this PR should have style fix and we should be good for storage files

0xc0170

Initial screen review to provide early feedback

0xc0170 · 2018-11-08T09:32:05Z

features/storage/kvstore/kv_map/KVMap.h

+ *
+ * @param partition partition configuration struct.
+ */
+ void deinit_partition(kv_map_entry_t * partition);


#if !defined(DOXYGEN_ONLY) for private/protected members/methods in new files? so doxygen is not generated for our public docs (see #8670)

0xc0170 · 2018-11-08T09:34:36Z

docs/design-documents/features/storage/Configuration/CONFIGURATION.md

@@ -0,0 +1,388 @@
+# Storage configuration 


design docs - were they reviewed by docs team or we request it now here? Let us know, we will add docs reviewers

We had an internal review, but not by the docs team. Please add them.

0xc0170 · 2018-11-08T09:37:18Z

features/storage/kvstore/conf/kv_config.cpp

+ return &fss;
+}
+
+FileSystem *_get_filesystem_default(BlockDevice *bd, const char *mount)


_get_filesystem_default vs _get_fileSystemStore - we follow the former (no camelcase for function names). Similar in other functions in this file, like _calculate_blocksize_match_TDBStore -> _calculate_blocksize_match_tbdstore. or I even noticed _storage_config_TDB_EXTERNAL, please consolidate

(this might be normal in the storage files?)

The following cannot be changed cause the calling function will build the correct function name using configuration macro:
_get_filesystem_FAT
_get_filesystem_LITTLE
_get_blockdevice_FLASHIAP
_get_blockdevice_SPIF
_get_blockdevice_QSPIF
_get_blockdevice_DATAFLASH
_get_blockdevice_SD
_storage_config_TDB_INTERNAL
_storage_config_TDB_EXTERNAL
_storage_config_TDB_EXTERNAL_NO_RBP
_storage_config_FILESYSTEM
_storage_config_FILESYSTEM_NO_RBP

the rest I have changed to lower case.

That one I have missed but noticed some string macros. How does this work (this function_MACRO) here?

Yes, by design the mbed_lib.json value of default is in lower case.

davidsaada · 2018-11-08T12:12:42Z

Can you run astyle on new added files here?

Done.

0xc0170 · 2018-11-08T12:23:29Z

@AnotherButler There is new API (doxygen) and new design documents here, please review

davidsaada · 2018-11-11T12:44:26Z

Added dependency on #8703 (as without it, TDBStore performance is bad).

AnotherButler · 2018-11-12T02:46:12Z

features/storage/kvstore/kv_map/KVMap.h

+ * @brief As a singleton, return the single instance of the class.
+ * This class is a singleton for the following reasons:
+ * - Ease of use, so you don't have to coordinate instantiations.
+ * - Lazy instantiation of internal data, (which we can't achieve with simple static classes).


Query: Is "lazy instantiation" a common term I haven't heard of, or should we rephrase this?

It's a common term.

AnotherButler · 2018-11-12T03:11:25Z

docs/design-documents/features/storage/TDBStore/TDBStore_design.md

+
+### Sequential writes
+
+All writes occur sequentially on the physical storage as records, superseding the previous ones for the same key. Each data record is written right after the last written one. If a key is updated, a new record with this key is written, overriding the previous value of this key. If a key is deleted, a new record with a "deleted" flag is added.


A lot of this document is in passive voice, and I'm not sure what's doing the action. What's writing data records? What's updating keys? What's deleting keys? Is it you, the user? Please clarify.

Thanks. Rephrased these lines.

Your changes look good. I came across a couple design docs I missed in my first round of edits, so I'm finishing those up, and then I'll be finished.

0xc0170 · 2018-11-22T09:18:10Z

^^ @OPpuolitaival Please review the latest test results and help us restarted (make it reported here)

0xc0170 · 2018-11-22T10:13:50Z

Restarted the CI

mbed-ci · 2018-11-22T18:02:41Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 26
Build artifacts
Build logs

0xc0170 · 2018-11-22T18:07:35Z

@ARMmbed/mbed-os-test @davidsaada CI reports 3 failues (check test failed status), tehre are timeouts, like targets did not even start (and only for kvstore tests)
Please review

Exporters timeouted, we will restart now

0xc0170 · 2018-11-22T21:21:15Z

Looks like exporter again time outed for IAR. @mbed-os-test Please review

0xc0170 · 2018-11-23T09:07:47Z

I run locally EV_COG_AD3029LZ | features-storage-tests-kvstore-general_tests_phase_1 for GCC_ARM and IAR - all passing.

davidsaada · 2018-11-23T09:10:40Z

0xc0170 those failures are sporadic, but all appear in our kvstore general test, so we think there's something to it and investigate it now.

0xc0170 · 2018-11-23T19:03:17Z

I restarted tests to confirm there is an issue with EV_COG devices

0xc0170 · 2018-11-24T07:18:46Z

Confirmed the failures, latest CI test run (13h ago)

ARCH_PRO iar_arm / ARCH_PRO-IAR.features-storage-tests-kvstore-general_tests_phase_1.features-storage-tests-kvstore-general_tests_phase_1
EV_COG_AD3029LZ iar_arm / EV_COG_AD3029LZ-IAR.features-storage-tests-kvstore-general_tests_phase_1.features-storage-tests-kvstore-general_tests_phase_1
EV_COG_AD3029LZ arm-none-eabi-gcc / EV_COG_AD3029LZ-GCC_ARM.features-storage-tests-kvstore-general_tests_phase_1.features-storage-tests-kvstore-general_tests_phase_1

dannybenor · 2018-11-24T09:50:01Z

The problem we have with this test is related to targets that do not support storage because they do not have any component enabled for block devices. We will continue the investigation tomorrow.

davidsaada · 2018-11-24T15:42:35Z

Pushed an update that may work. It's a long shot which I can't tell whether will work for sure, as I wasn't able to reproduce the problem on ARCH_PRO and have no EV_COG_AD3029LZ to check the fix with on any raas node.

Implement the following: KVStore base class TDBStore class FileSystemStore class SecureStore class Global APIs Configuration framework Design documentation

0xc0170 · 2018-11-24T16:13:54Z

CI triggered

0xc0170 · 2018-11-24T20:34:01Z

Looks like it passed testing (the latest update fixed the issue).

mbed-ci · 2018-11-24T22:55:07Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 27
Build artifacts
Build logs

jeromecoutant · 2019-01-04T11:08:39Z

targets/targets.json

@@ -3504,7 +3504,7 @@
 }
 },
 "DISCO_L475VG_IOT01A": {
- "components_add": ["QSPIF"],
+ "components_add": ["QSPIF", "FLASHIAP"],


Hi
Did you check this new component with this DISCO_L475 target ?
I got all components-storage-blockdevice-component_flashiap-tests FAIL with 5.11.0 release... :-(

Please create also a new issue for this

Sorry about that. We have tested the block device with the general_test under feature/storage/TESTS/blockdevice and it uses get default instance and calculate the correct address and size. The component test fails because the base address and size are not set in the COMPONENT_FLASHIAP mbed_lib target overwrites. I run this with start address set to 0x801A800 and the size to 0xE5800 and it works. @jeromecoutant do you like me to add those to the mbed_lib.json permanently or you have some other numbers you prefer?

Second thought that we will change the current implementation of the test and if no parameters will be provided in mbed_lib.app the test will calculate the start address and the size by itself as the get default instance does. Take a look at PR #9268

davidsaada force-pushed the kvstore_integration branch from 1cc4d60 to 9d8debf Compare November 7, 2018 21:25

cmonr requested a review from a team November 8, 2018 04:42

cmonr added the needs: review label Nov 8, 2018

cmonr requested a review from kjbracey November 8, 2018 04:42

cmonr added the release-version: 5.11.0-rc1 label Nov 8, 2018

dannybenor approved these changes Nov 8, 2018

View reviewed changes

0xc0170 added the needs: preceding PR label Nov 8, 2018

0xc0170 requested changes Nov 8, 2018

View reviewed changes

davidsaada force-pushed the kvstore_integration branch from 9d8debf to ff6688a Compare November 8, 2018 12:10

davidsaada force-pushed the kvstore_integration branch from ff6688a to 4d646c2 Compare November 8, 2018 12:21

0xc0170 requested a review from AnotherButler November 8, 2018 12:23

davidsaada force-pushed the kvstore_integration branch 4 times, most recently from 0abfad5 to d414914 Compare November 9, 2018 12:17

geky mentioned this pull request Nov 9, 2018

Standardized FileSystem init/deinit/reset to match KVStore API #8700

Closed

geky requested a review from SenRamakri November 9, 2018 22:31

davidsaada force-pushed the kvstore_integration branch from d414914 to e9948d7 Compare November 11, 2018 12:35

davidsaada force-pushed the kvstore_integration branch 3 times, most recently from 4fc3aee to 769c0e3 Compare November 11, 2018 16:09

yossi2le mentioned this pull request Nov 11, 2018

KVStore based DeviceKey - Device key and KVStore integration. #8707

Closed

AnotherButler reviewed Nov 12, 2018

View reviewed changes

davidsaada force-pushed the kvstore_integration branch from f13ed2b to 645f219 Compare November 12, 2018 09:21

davidsaada force-pushed the kvstore_integration branch from f0f8462 to f26c277 Compare November 22, 2018 11:48

0xc0170 mentioned this pull request Nov 23, 2018

Import Mbed TLS v2.15.0 with Mbed Crypto #8859

Merged

davidsaada force-pushed the kvstore_integration branch from f26c277 to 89cde0d Compare November 24, 2018 15:37

David Saada added 2 commits November 24, 2018 17:43

KVStore & derived classes: design docs, implementation & configuration

e9c25e2

Implement the following: KVStore base class TDBStore class FileSystemStore class SecureStore class Global APIs Configuration framework Design documentation

Change Device key to work with KVStore

e073fb6

davidsaada force-pushed the kvstore_integration branch from 89cde0d to e073fb6 Compare November 24, 2018 15:43

NirSonnenschein added ready for merge and removed needs: CI labels Nov 25, 2018

NirSonnenschein merged commit 0283bb8 into ARMmbed:master Nov 25, 2018

NirSonnenschein removed the ready for merge label Nov 25, 2018

cmonr removed risk: A priority: high labels Nov 26, 2018

davidsaada deleted the kvstore_integration branch December 6, 2018 13:04

jeromecoutant reviewed Jan 4, 2019

View reviewed changes

jeromecoutant mentioned this pull request Jan 4, 2019

FLASHIAP component tests failed #9255

Closed

VeijoPesonen mentioned this pull request Aug 28, 2019

TDBStore bugfix: won't rely on flash erase value to detect is a sector erased #11349

Merged


		### Sequential writes

		All writes occur sequentially on the physical storage as records, superseding the previous ones for the same key. Each data record is written right after the last written one. If a key is updated, a new record with this key is written, overriding the previous value of this key. If a key is deleted, a new record with a "deleted" flag is added.

KVStore & derived classes: design docs, implementation & configuration #8667

KVStore & derived classes: design docs, implementation & configuration #8667

Conversation

davidsaada commented Nov 7, 2018 • edited Loading

Description

Pull request type

dannybenor commented Nov 8, 2018

0xc0170 commented Nov 8, 2018 • edited Loading

0xc0170 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsaada commented Nov 8, 2018

0xc0170 commented Nov 8, 2018

davidsaada commented Nov 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Nov 22, 2018

0xc0170 commented Nov 22, 2018

mbed-ci commented Nov 22, 2018

0xc0170 commented Nov 22, 2018

0xc0170 commented Nov 22, 2018

0xc0170 commented Nov 23, 2018

davidsaada commented Nov 23, 2018

0xc0170 commented Nov 23, 2018

0xc0170 commented Nov 24, 2018 • edited Loading

dannybenor commented Nov 24, 2018

davidsaada commented Nov 24, 2018

0xc0170 commented Nov 24, 2018

0xc0170 commented Nov 24, 2018

mbed-ci commented Nov 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsaada commented Nov 7, 2018 •

edited

Loading

0xc0170 commented Nov 8, 2018 •

edited

Loading

0xc0170 Nov 8, 2018 •

edited

Loading

0xc0170 commented Nov 24, 2018 •

edited

Loading