Storage: Use internal flash for KVStore always if default config #12645

mtomczykmobica · 2020-03-18T09:28:10Z

…ion. TDB_INTERNAL is always for first choose.

Summary of changes

Chenged order of choosed KVStore to TDB_INTERNAL for default configuration.
Updated tests and documentation.

Impact of changes

Migration actions required

Documentation

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] 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)
[] Tests / results supplied as part of this PR

Reviewers

@SeppoTakalo
@VeijoPesonen
@0xc0170

VeijoPesonen · 2020-03-18T09:46:20Z

features/storage/TESTS/kvstore/direct_access_devicekey_test/main.cpp

+#if COMPONENT_FLASHIAP
+        internal_start_address =  MBED_CONF_STORAGE_TDB_INTERNAL_INTERNAL_BASE_ADDRESS;
+        internal_rbp_size =  MBED_CONF_STORAGE_TDB_INTERNAL_INTERNAL_SIZE;
+#elif COMPONENT_QSPIF || COMPONENT_SPIF || COMPONENT_DATAFLASH


I would say this configuration isn't even possible without FLASHIAP - please see TDB_External documentation. What I'm saying is that the TDB_External option should be dropped from default configurations. Only possibility would be to use TDB_External_no_RBP configuration (which means there isn't Root of Trust neither)

Removed @VeijoPesonen please check.

Pull request has been modified.

ciarmcom · 2020-03-18T12:00:33Z

@mtomczykmobica, thank you for your changes.
@VeijoPesonen @SeppoTakalo @0xc0170 @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

VeijoPesonen · 2020-03-18T12:14:07Z

features/storage/TESTS/kvstore/direct_access_devicekey_test/main.cpp

+#if COMPONENT_FLASHIAP
+        internal_start_address =  MBED_CONF_STORAGE_TDB_INTERNAL_INTERNAL_BASE_ADDRESS;
+        internal_rbp_size =  MBED_CONF_STORAGE_TDB_INTERNAL_INTERNAL_SIZE;
 #elif COMPONENT_SD


Actually we should touch this tests case. We of course want to test all combinations. We need to remember that there is a distinction between what are possible default configurations and what configurations we do test.

VeijoPesonen · 2020-03-18T12:20:33Z

features/storage/kvstore/conf/kv_config.cpp

-#if COMPONENT_QSPIF || COMPONENT_SPIF || COMPONENT_DATAFLASH
+#if COMPONENT_FLASHIAP
+    return _storage_config_TDB_INTERNAL();
+#elif COMPONENT_QSPIF || COMPONENT_SPIF || COMPONENT_DATAFLASH


This should be removed. If COMPONENT_FLASHIAP is not defined compiler error should be removed.

VeijoPesonen · 2020-03-18T12:20:56Z

features/storage/kvstore/conf/kv_config.cpp

+    return _storage_config_TDB_INTERNAL();
+#elif COMPONENT_QSPIF || COMPONENT_SPIF || COMPONENT_DATAFLASH
    return _storage_config_TDB_EXTERNAL();
 #elif COMPONENT_SD


This should be removed.

…ion. TDB_INTERNAL is always for first choose.

mtomczykmobica · 2020-03-18T13:11:25Z

From ticket:
"We discussed with Veijo Pesonen and decided that we should modify the scope of this ticket a little bit.

By default, the KVStore should go to internal flash. No other defaults should exist.

KVStorore should go to external device ONLY if requested by configuration values.

So you can simplify the code a lot."

Changes done.
@SeppoTakalo and @VeijoPesonen please review.

SeppoTakalo · 2020-03-18T13:56:47Z

@VeijoPesonen Please check also..

VeijoPesonen

Looks good to me

0xc0170 · 2020-03-18T15:02:09Z

CI started

mbed-ci · 2020-03-18T16:28:46Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_dynamic-memory-usage

mtomczykmobica · 2020-03-19T06:18:56Z

CI, failed first in PR: #12528

VeijoPesonen · 2020-03-19T08:15:06Z

@0xc0170 the bot doesn't care about the test results?

mtomczykmobica · 2020-03-19T08:44:36Z

@0xc0170 the bot doesn't care about the test results?

Rerun test on master and have the same error:
https://mbed-os.mbedcloudtesting.com/blue/organizations/jenkins/mbed-os-ci_dynamic-memory-usage/detail/mbed-os-ci_dynamic-memory-usage/4786/pipeline
Looks like some problem with CI.
Suspected commit:
https://github.com/ARMmbed/mbed-os-ci/commit/4c6c1d9f44dca2ec811f9164233bf948e2d6be3e

0xc0170 · 2020-03-19T12:57:59Z

@jamesbeyond @VeliMattiLahtela Can you review ^^ ? Looks like dynamic test is failing in PRs today

0xc0170 · 2020-03-19T12:58:42Z

As it was aborted, I restarted it to get the failure

mtomczykmobica · 2020-03-20T06:46:35Z

@0xc0170 Now CI works, thank you Martin.

SeppoTakalo previously approved these changes Mar 18, 2020

View reviewed changes

VeijoPesonen suggested changes Mar 18, 2020

View reviewed changes

mergify bot added the needs: CI label Mar 18, 2020

mtomczykmobica force-pushed the IOTSTOR-906 branch from 00f14b7 to c62df4c Compare March 18, 2020 10:30

ciarmcom requested review from 0xc0170, SeppoTakalo, VeijoPesonen and a team March 18, 2020 12:00

ciarmcom added the needs: review label Mar 18, 2020

VeijoPesonen reviewed Mar 18, 2020

View reviewed changes

[Storage] Use internal flash for KVStore always if default configurat…

6a21dfa

…ion. TDB_INTERNAL is always for first choose.

mtomczykmobica force-pushed the IOTSTOR-906 branch from c62df4c to 6a21dfa Compare March 18, 2020 13:06

SeppoTakalo approved these changes Mar 18, 2020

View reviewed changes

mergify bot removed the needs: review label Mar 18, 2020

VeijoPesonen approved these changes Mar 18, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Mar 18, 2020

0xc0170 removed the ready for merge label Mar 19, 2020

0xc0170 added the needs: work label Mar 19, 2020

0xc0170 changed the title ~~[Storage] Use internal flash for KVStore always if default configurat…~~ Storage: Use internal flash for KVStore always if default config Mar 19, 2020

0xc0170 added needs: CI and removed needs: work labels Mar 19, 2020

mergify bot added ready for merge and removed needs: CI labels Mar 19, 2020

0xc0170 added mergify skip needs: CI and removed ready for merge labels Mar 19, 2020

0xc0170 approved these changes Mar 20, 2020

View reviewed changes

0xc0170 added release-version: 6.0.0-beta-1 and removed mergify skip needs: CI labels Mar 20, 2020

0xc0170 merged commit 03dab7f into ARMmbed:master Mar 20, 2020

Storage: Use internal flash for KVStore always if default config #12645

Storage: Use internal flash for KVStore always if default config #12645

Uh oh!

Conversation

mtomczykmobica commented Mar 18, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

@SeppoTakalo @VeijoPesonen @0xc0170

Uh oh!

VeijoPesonen Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtomczykmobica Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

ciarmcom commented Mar 18, 2020

Uh oh!

VeijoPesonen Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeijoPesonen Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeijoPesonen Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

mtomczykmobica commented Mar 18, 2020

Uh oh!

SeppoTakalo commented Mar 18, 2020

Uh oh!

VeijoPesonen left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 18, 2020

Uh oh!

mbed-ci commented Mar 18, 2020

Test run: FAILED

Uh oh!

mtomczykmobica commented Mar 19, 2020

Uh oh!

VeijoPesonen commented Mar 19, 2020

Uh oh!

mtomczykmobica commented Mar 19, 2020

Uh oh!

0xc0170 commented Mar 19, 2020

Uh oh!

0xc0170 commented Mar 19, 2020

Uh oh!

mtomczykmobica commented Mar 20, 2020

Uh oh!

Uh oh!

@SeppoTakalo
@VeijoPesonen
@0xc0170

VeijoPesonen Mar 18, 2020 •

edited

Loading

VeijoPesonen Mar 18, 2020 •

edited

Loading

VeijoPesonen Mar 18, 2020 •

edited

Loading