Skip to content

KVStore: Support external storage out of mbed-os tree #10355

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

Jump to bottom
Merged
merged 1 commit into from
May 7, 2019
Merged

KVStore: Support external storage out of mbed-os tree #10355

merged 1 commit into from
May 7, 2019

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 commented Apr 9, 2019 •
edited
Loading

Description

This PR tries to support external storage out of mbed-os tree in KVStore. To enable it, user needs to:

  1. Configure blockdevice to other.
  2. Override get_other_blockdevice() to provide block device out of mbed-os tree.

Pull request type

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

Release issue

#10300

Release Notes

Support external storage out of mbed-os tree in KVStore. To enable it, user needs to:

  1. Configure KVStore configuration parameter blockdevice to other.
  2. Override get_other_blockdevice() to provide block device out of mbed-os tree. 
    BlockDevice *get_other_blockdevice();

@ciarmcom ciarmcom requested review from a team April 9, 2019 11:00
@ciarmcom
Copy link
Member

ciarmcom commented Apr 9, 2019

@ccli8, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@bulislaw
Copy link
Member

@davidsaada please review. Are there any docs that need to be updated?

davidsaada
davidsaada suggested changes Apr 24, 2019
View reviewed changes
Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

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

Looks OK all in all. Just got one comment regarding the classification of the device as a flash one.

features/storage/kvstore/conf/kv_config.cpp Outdated
*
* @returns true for flash type or false for non-flash type.
*/
bool is_blockdevice_flash_type(BlockDevice *bd);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for this function. One can call the BD's get_erase_value method. A non flash device would return a -1 value, telling that that this block device is incapable of erasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidsaada Yes. BD's get_erase_value method can help check flash type or not. I've updated it and related config description.

@ccli8
KVStore: Support block device out of mbed-os tree
63d9cde 
To support block device out of mbed-os tree in KVStore, user needs to:
1. Configure blockdevice to "other".
2. Override get_other_blockdevice() to provide block device out of mbed-os tree.
@ccli8 ccli8 force-pushed the nuvoton_kvstore_other_bd branch from 8b885d3 to 63d9cde Compare April 25, 2019 03:12
@ccli8 ccli8 changed the title Support external storage out of Mbed OS tree in KVStore KVStore: Support external storage out of mbed-os tree Apr 25, 2019
@adbridge
Copy link
Contributor

@davidsaada could you approve now please if you are happy with the changes ?

davidsaada
davidsaada approved these changes Apr 25, 2019
View reviewed changes
Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks.

@adbridge
Copy link
Contributor

adbridge commented May 1, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented May 1, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
  • jenkins-ci/mbed-os-ci_greentea-test

@ccli8 ccli8 mentioned this pull request May 2, 2019
Closed
@0xc0170
Copy link
Contributor

0xc0170 commented May 2, 2019

CI restarted (internal license server fault)

@0xc0170
Copy link
Contributor

0xc0170 commented May 2, 2019 •
edited
Loading

@davidsaada please review. Are there any docs that need to be updated?

@davidsaada
Is there any docs for these change that needs updating? Should this have release notes included? To illustrate how to add this to own project (using own BD implementation).

@mbed-ci
Copy link

mbed-ci commented May 2, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@davidsaada
Copy link
Contributor

@davidsaada please review. Are there any docs that need to be updated?

@davidsaada
Is there any docs for these change that needs updating? Should this have release notes included? To illustrate how to add this to own project (using own BD implementation).

Our KVStore documentation doesn't go deep as in specifying how to implement block devices. I think that adding a description in the release notes is the way to go.

@0xc0170
Copy link
Contributor

0xc0170 commented May 2, 2019

@ccli8 Set to 5.13 as functionality change (PR type should be fixed above). If that is correct, release notes section should be filled.

@ccli8
Copy link
Contributor Author

ccli8 commented May 3, 2019

@0xc0170 Changed PR type to Functionality change and added Release Notes section.

@adbridge adbridge merged commit 0ac1c97 into ARMmbed:master May 7, 2019
@ccli8 ccli8 deleted the nuvoton_kvstore_other_bd branch May 8, 2019 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adbridge adbridge adbridge approved these changes

@davidsaada davidsaada davidsaada approved these changes

Assignees
No one assigned
Labels
release-version: 5.13.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ccli8 @ciarmcom @bulislaw @adbridge @mbed-ci @0xc0170 @davidsaada @cmonr