Restructure KVStore to one library per store type #13908

LDong-Arm · 2020-11-13T14:05:40Z

Summary of changes

Fixes: #13657
Mutually depends on: ARMmbed/mbed-os-example-kvstore#60

In #13307 we combined multiple KVStore types (TDBStore, FileSystemStore) and the global API into one library, kv-global-api. This is inconvenient for users of bare metal: if they only want one store type, they need to add dependencies for other unused types and the global API to "requires". This becomes cumbersome as FileSystem has many dependencies.

This PR restructures KVStore to follow the same logic of BlockDevice: declare the interface at the top level, and treat each type/implementation as a "sub-" library. Users only need to enable the interface + the type they need. So each of the following is a library (containing mbed_lib.json):

storage/kvstore: KVStore interface
storage/kvstore/tdbstore: TDBStore : KVStore class
storage/kvstore/filesystemstore: FileSystemStore : KVStore class
storage/kvstore/securestore: SecureStore : KVStore class
storage/kvstore/kvstore_global_api: optional API to globally manage instances of KVStore, containing kvstore_global_api functions and KVMap which is a helper class for the global API
storage/kvstore/kvconfig: configurations

Greentea and unit tests have also been moved into subdirectories of corresponding KVStore types.

Note: The CMake support for KVStore has not been componentised yet - compilation with CMake still brings in all the unused types and dependencies, because everything is under one mbed-storage-kvstore target. Hopefully the CMake team will later componentise each type of KVStore into mbed-storage-kvstore-<type>. similar to what they've already done for BlockDevices. @0xc0170 (Update: required CMake changed included in this PR)

Impact of changes

Users can enable one type of KVStore (e.g. TDBStore) without having to bring in dependencies for unused types of KVStore.

Migration actions required

For a bare metal application, add "kvstore" and the type of KVStore to use (e.g. "tdbstore") to "requires" in mbed_app.json. Note that the KVStore interface library is "kvstore", not "kv-global-api" which now contains only the global API.
To use CMake (full profile and bare metal profile): Add the TDBStore sub-component you need (e.g. mbed-storage-tdbstore) and dependencies (e.g. TDBStore requires mbed-storage-blockdevice) to your app's CMakeLists.txt.

Documentation

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Note: For the CMake support, I tested the "feature-cmake" branch of mbed-os-example-blinky-baremetal, with TDBStore store(BlockDevice::get_default_instance()); (and headers) added to main.cpp and the following dependencies added to the app's CMakeLists.txt: mbed-storage-kvstore, mbed-storage-blockdevice, mbed-storage (for the default block device).

Reviewers

@ARMmbed/mbed-os-core @evedon @boraozgen

storage/kvstore/filesystemstore/CMakeLists.txt

ciarmcom · 2020-11-13T14:30:25Z

@LDong-Arm, thank you for your changes.
@evedon @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-test @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2020-11-16T12:00:48Z

Note: The CMake support for KVStore has not been componentised yet - compilation with CMake still brings in all the unused types and dependencies, because everything is under one mbed-storage-kvstore target. Hopefully the CMake team will later componentise each type of KVStore into mbed-storage-kvstore-. similar to what they've already done for BlockDevices. @0xc0170

@hugueskamba @rajkan01 Please review the layout here

storage/kvstore/filesystemstore/CMakeLists.txt

storage/kvstore/kvstore_global_api/CMakeLists.txt

Pull request has been modified.

LDong-Arm · 2020-11-16T15:11:11Z

@0xc0170 @hugueskamba
I've changed the CMake layout (one CMake target per sub-component of KVStore) as you requested, please review again, thanks.

I've tested my change with mbed-os-example-blinky-baremetal:

checkout feature-cmake branch, and remove target.c_lib and target.printf_lib overrides from mbed_app.json, due to a known issue in mbed-tools
Add mbed-storage, mbed-storage-blockdevice and mbed-storage-tdbstore to the example's CMakeLists.txt
In main, include headers for BlockDevice and TDBStore, and add to main()
```
TDBStore store(BlockDevice::get_default_instance());
```

To make it work, I needed to fix the declaration of SFDP.cpp (this commit).

PR ARMmbed#13908 replaces the umbrella library for all KVStore from "kvstore_global_api" to "kvstore". Now "kvstore_global_api" still exists but is restricted to the global API part only. By adding "kvstore" as a dependency of "kvstore_global_api", existing bare metal projects (e.g. mbed-bootloader) that use the latter library name to pull in the KVStore prototype class will continue to work.

Pull request has been modified.

PR ARMmbed#13908 replaces the umbrella library for all KVStore from "kvstore_global_api" to "kvstore". Now "kvstore_global_api" still exists but is restricted to the global API part only. By adding "kvstore" as a dependency of "kvstore_global_api", existing bare metal projects (e.g. mbed-bootloader) that use the latter library name to pull in the KVStore prototype class will continue to work.

LDong-Arm · 2020-12-09T17:39:13Z

The best timing would be to do it tomorrow before the code freeze (the rest of Prs are in)

Rebased, tested everything locally and they still work.

LDong-Arm · 2020-12-09T20:54:06Z

Oh wait didn't know #13917 isn't merged yet. The CMake part depends on that PR...

0xc0170 · 2020-12-10T14:24:42Z

Oh wait didn't know #13917 isn't merged yet. The CMake part depends on that PR...

Its in. I'll mark this as priority high so we do not forget about this one for the upcoming release.

LDong-Arm · 2020-12-10T15:50:45Z

Oh wait didn't know #13917 isn't merged yet. The CMake part depends on that PR...

Its in. I'll mark this as priority high so we do not forget about this one for the upcoming release.

Again we need ARMmbed/mbed-os-example-kvstore#65 otherwise CI will fail...

LDong-Arm · 2020-12-11T12:21:25Z

@adbridge @0xc0170 Dependency merged, ready for CI

adbridge · 2020-12-11T12:37:10Z

CI started

mbed-ci · 2020-12-11T13:20:05Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 6 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

LDong-Arm commented Nov 13, 2020

View reviewed changes

storage/kvstore/filesystemstore/CMakeLists.txt Outdated Show resolved Hide resolved

mergify bot added the needs: work label Nov 13, 2020

LDong-Arm mentioned this pull request Nov 13, 2020

Bare metal: Adding TDBStore API creates many unrelated dependencies #13657

Closed

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Nov 13, 2020

ciarmcom requested review from evedon and a team November 13, 2020 14:30

ciarmcom added the needs: review label Nov 13, 2020

evedon previously approved these changes Nov 16, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review needs: work labels Nov 16, 2020

0xc0170 requested a review from hugueskamba November 16, 2020 12:00

0xc0170 requested a review from rajkan01 November 16, 2020 12:00

hugueskamba previously requested changes Nov 16, 2020

View reviewed changes

storage/kvstore/filesystemstore/CMakeLists.txt Outdated Show resolved Hide resolved

mergify bot added needs: work and removed needs: CI labels Nov 16, 2020

0xc0170 reviewed Nov 16, 2020

View reviewed changes

storage/kvstore/kvstore_global_api/CMakeLists.txt Outdated Show resolved Hide resolved

LDong-Arm force-pushed the kvstore_libraries branch from da4cace to eb520c2 Compare November 16, 2020 15:03

LDong-Arm requested review from 0xc0170, hugueskamba and evedon November 16, 2020 15:04

hugueskamba previously approved these changes Nov 16, 2020

View reviewed changes

CMake fix: location of MBED_CONF_FILESYSTEM_PRESENT=1

0f5957d

LDong-Arm force-pushed the kvstore_libraries branch from ce59f29 to a80fb9f Compare December 9, 2020 17:21

LDong-Arm added 5 commits December 9, 2020 17:26

CMake fix: SFDP already moved to BlockDevice

3869266

Fix astyle in KVStore unit tests

34bbcbf

KVStore: drop './' from CMake paths

170c07c

CMake: add missing linkages of storage libraries

c2cc438

LDong-Arm force-pushed the kvstore_libraries branch from a80fb9f to 479b704 Compare December 9, 2020 17:28

0xc0170 added the priority: high label Dec 10, 2020

evedon approved these changes Dec 11, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Dec 11, 2020

mergify bot added ready for merge and removed needs: CI labels Dec 11, 2020

adbridge merged commit 9f18c44 into ARMmbed:master Dec 11, 2020

adbridge removed the ready for merge label Dec 11, 2020

mbedmain added release-version: 6.6.0 Release-pending and removed release-type: feature Release-pending labels Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure KVStore to one library per store type #13908

Restructure KVStore to one library per store type #13908

LDong-Arm commented Nov 13, 2020 •

edited by 0xc0170

Loading

ciarmcom commented Nov 13, 2020

0xc0170 commented Nov 16, 2020 •

edited

Loading

LDong-Arm commented Nov 16, 2020

LDong-Arm commented Dec 9, 2020

LDong-Arm commented Dec 9, 2020

0xc0170 commented Dec 10, 2020 •

edited

Loading

LDong-Arm commented Dec 10, 2020

LDong-Arm commented Dec 11, 2020

adbridge commented Dec 11, 2020

mbed-ci commented Dec 11, 2020

Restructure KVStore to one library per store type #13908

Restructure KVStore to one library per store type #13908

Conversation

LDong-Arm commented Nov 13, 2020 • edited by 0xc0170 Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Nov 13, 2020

0xc0170 commented Nov 16, 2020 • edited Loading

LDong-Arm commented Nov 16, 2020

LDong-Arm commented Dec 9, 2020

LDong-Arm commented Dec 9, 2020

0xc0170 commented Dec 10, 2020 • edited Loading

LDong-Arm commented Dec 10, 2020

LDong-Arm commented Dec 11, 2020

adbridge commented Dec 11, 2020

mbed-ci commented Dec 11, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 6 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

LDong-Arm commented Nov 13, 2020 •

edited by 0xc0170

Loading

0xc0170 commented Nov 16, 2020 •

edited

Loading

0xc0170 commented Dec 10, 2020 •

edited

Loading