Skip to content

Allows placing KVStore and update images on separate storage devices - to internal and external flash #11165

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 2 commits into from
Aug 19, 2019
Merged

Allows placing KVStore and update images on separate storage devices - to internal and external flash #11165

merged 2 commits into from
Aug 19, 2019

Conversation

VeijoPesonen
Copy link
Contributor

@VeijoPesonen VeijoPesonen commented Aug 5, 2019 •
edited
Loading

Allows having KVStore in internal and update image in external flash

Fixes a bug where it has not been possible to have KVStore in internal
flash while an update image image has been kept in external storage.

[TDBStore] changes the default TDBStore location. Instead of placing the TDBStore after the application - plus two spare sectors if a new app image is bigger than the original - now it's kept at the end of the flash

Description

Pull request type

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

Reviewers

@SeppoTakalo
@yossi2le
@teetak01

Release Notes

Fixes TDB_INTERNAL configuration - makes possible to have the KVStore inside internal flash while update images are kept in an external storage.

@0Grit
Copy link

0Grit commented Aug 5, 2019

This is needed.

@ciarmcom ciarmcom requested review from SeppoTakalo, teetak01, yossi2le and a team August 5, 2019 13:00
@ciarmcom
Copy link
Member

ciarmcom commented Aug 5, 2019

@VeijoPesonen, thank you for your changes.
@SeppoTakalo @teetak01 @yossi2le @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@teetak01
Copy link
Contributor

teetak01 commented Aug 6, 2019

@VeijoPesonen can you clarify the commit and fix description? There is no presumption that KVstore is always in external flash. We do already have configurations with do not use external storage at all.

Instead the KVStore always has at least one part in internal flash.

@teetak01
Copy link
Contributor

teetak01 commented Aug 6, 2019

@davidsaada @alzix

Allows having KVStore in internal and update image in external flash
9222e15 
Fixes a bug where it has not been possible to have KVStore in internal
flash while an update image image has been kept in external storage.
teetak01
teetak01 approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@teetak01 teetak01 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. You could update also the title.

@davidsaada
Copy link
Contributor

I'm not assigned as a reviewer, but this looks good to me.

@0Grit
Copy link

0Grit commented Aug 6, 2019

@ARMmbed/team-embeddedplanet

@VeijoPesonen VeijoPesonen changed the title [KVStore] Allows KVStore-in-internal-flash-only-configuration Allows placing KVStore and update images on separate storage devices - to internal and external flash Aug 6, 2019
@mbed-ci
Copy link

mbed-ci commented Aug 7, 2019

Test run: FAILED

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

Failed test jobs:

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

@VeijoPesonen
Copy link
Contributor Author

Test run: FAILED

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

Failed test jobs:

* jenkins-ci/mbed-os-ci_greentea-test

Seems to fail with NUCLEO_F429ZI, with all compilers.

[1565180603.94][CONN][RXD] >>> Running case #1: 'Testing direct access to devicekey with tdb over flashiap remainder'...
[1565180604.04][CONN][RXD] Test Direct Access To DeviceKey Test Entire FlashIAP Remainder
[1565180604.04][CONN][INF] found KV pair in stream: {{__testcase_start;Testing direct access to devicekey with tdb over flashiap remainder}}, queued...
[1565180605.35][CONN][INF] found KV pair in stream: {{__testcase_finish;Testing direct access to devicekey with tdb over flashiap remainder;1;0}}, queued...
[1565180605.45][CONN][RXD] >>> 'Testing direct access to devicekey with tdb over flashiap remainder': 1 passed, 0 failed
[1565180605.45][CONN][RXD]
[1565180605.55][CONN][RXD] >>> Running case #2: 'Testing direct access to devicekey with tdb over last two sectors'...
[1565180605.65][CONN][RXD] Test Direct Access To DeviceKey Test Last Two Sectors
[1565180605.65][CONN][INF] found KV pair in stream: {{__testcase_start;Testing direct access to devicekey with tdb over last two sectors}}, queued...
[1565180607.05][CONN][INF] found KV pair in stream: {{__testcase_finish;Testing direct access to devicekey with tdb over last two sectors;1;0}}, queued...
[1565180607.15][CONN][RXD] >>> 'Testing direct access to devicekey with tdb over last two sectors': 1 passed, 0 failed
[1565180607.15][CONN][RXD]
[1565180607.25][CONN][RXD] >>> Running case #3: 'Testing direct access to injected devicekey '...
[1565180607.35][CONN][INF] found KV pair in stream: {{__testcase_start;Testing direct access to injected devicekey }}, queued...
mbedgt: :314::FAIL: Expected 136052736 Was 134610944
[1565180608.56][CONN][RXD] :314::FAIL: Expected 136052736 Was 134610944
[1565180608.66][CONN][INF] found KV pair in stream: {{__testcase_finish;Testing direct access to injected devicekey ;0;1}}, queued...
[1565180608.75][CONN][RXD] >>> 'Testing direct access to injected devicekey ': 0 passed, 1 failed with reason 'Test Cases Failed'

@VeijoPesonen
Copy link
Contributor Author

VeijoPesonen commented Aug 9, 2019 •
edited
Loading

The test case seems to assume that the flash region is contiguous on F429ZI's memory map, which it isn't. Please see "Reference manual - STM32F405/415, STM32F407/417, STM32F427/437 andSTM32F429/439 advanced Arm®-based 32-bit MCUs", Chapter 3.4, Table 6.

@VeijoPesonen
Copy link
Contributor Author

TDBStore's default location has been changed and test cases adjusted accordingly as a fix for the earlier PR check issues. Please re-review.

[TDBStore] changes the default TDBStore location
722628b 
Thus far the default position has been after the application plus two
spare sectors. For simplicity and to have a predictable location for the
TDBStore with the default configuration the location is now switched to
the end of the flash. Two last sectors to be exact.
@mbed-ci
Copy link

mbed-ci commented Aug 15, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@VeijoPesonen
Copy link
Contributor Author

Script failure, unit tests are passing.

[2019-08-15T08:14:50.436Z] 100% tests passed, 0 tests failed out of 45

[2019-08-15T08:14:50.436Z] 

[2019-08-15T08:14:50.436Z] Total Test time (real) =   0.10 sec

[2019-08-15T08:14:50.436Z] 

[2019-08-15T08:14:50.436Z] 

[2019-08-15T08:14:53.698Z] 

[2019-08-15T08:14:53.698Z] 

[2019-08-15T08:14:53.698Z] HTML code coverage report created.

[2019-08-15T08:14:56.211Z] 

[2019-08-15T08:14:56.211Z] 

[2019-08-15T08:14:56.211Z] XML code coverage report created.

[2019-08-15T08:14:56.211Z] + lcov -C -D . --output-file unittest-coverage.info

[2019-08-15T08:14:56.211Z] /builds/ws/mbed-os-ci_unittests/unitTest-3607/mbed-os@tmp/durable-3248b3ac/script.sh: 7: /builds/ws/mbed-os-ci_unittests/unitTest-3607/mbed-os@tmp/durable-3248b3ac/script.sh: lcov: not found

@mbed-ci
Copy link

mbed-ci commented Aug 16, 2019

Test run: SUCCESS

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

@SeppoTakalo
Copy link
Contributor

@0xc0170 Please remove the "Needs Work" label and add "Ready for Merge"

This has now been reviewed and tests have passed. (Yossi will probably not spend time anymore for this).

@0xc0170 0xc0170 merged commit 2e09a27 into ARMmbed:master Aug 19, 2019
@VeijoPesonen VeijoPesonen deleted the fix_internal_kvstore_config branch August 27, 2019 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@teetak01 teetak01 teetak01 approved these changes

@yossi2le yossi2le Awaiting requested review from yossi2le

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

Successfully merging this pull request may close these issues.
8 participants
@VeijoPesonen @0Grit @ciarmcom @teetak01 @davidsaada @mbed-ci @SeppoTakalo @0xc0170