Enable storage tests to all targets #12638

tymoteuszblochmobica · 2020-03-17T11:03:12Z

Storage tests is now enabled to all targets

Summary of changes

Many storage tests was enabled only to
-K64F
-Cypress PSOC6 targets
-ARM FM targets
This PR removes this limitation.

Impact of changes

No impact

Migration actions required

Not needed

Documentation

Not needed

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

@VeijoPesonen
@SeppoTakalo

tymoteuszblochmobica · 2020-03-17T11:04:50Z

#if !SECURESTORE_ENABLED still is needed to avoid runtime crash if TRNG is not available on particular target.

SeppoTakalo · 2020-03-17T11:11:13Z

Can you estimate what new boards are now under tests, if we remove these flags?

tymoteuszblochmobica · 2020-03-17T11:17:08Z

I didnt check all targets but now most STM based, Cypress and Freescale NXP should pass or skip if TRNG is not available.
Next step is add of fix Jenkins job to verify all.

VeijoPesonen

Looks good to me

0xc0170 · 2020-03-17T14:52:27Z

CI started

mbed-ci · 2020-03-17T18:47:49Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

VeijoPesonen · 2020-03-18T08:01:04Z

Test run: FAILED

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

Failed test jobs:
* jenkins-ci/mbed-os-ci_greentea-test

I guess we now got the list of targets which needs to be checked against those newly enabled test cases.

NRF52840_DK
DISCO_L475VG_IOT01A
LPC55S69

In other words we need preceding PR - or multiple - which makes the test cases pass on those boards. The necessary changes can also be part of this PR from my point of view.

tymoteuszblochmobica · 2020-03-18T09:09:36Z

Im fixing this test fails.

Pull request has been modified.

VeijoPesonen · 2020-03-27T08:15:03Z

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

        // but minimum of 2 erase sectors, so that the garbage collection way work
-        ul_bd_size  = align_up(program_size * PAGES_ESTIMATE, erase_size * 2);
-        rbp_bd_size = align_up(program_size * PAGES_ESTIMATE, erase_size * 2);
+        ul_bd_size  = align_up(program_size * PAGES_ESTIMATE, erase_size * 4);


Should we actually set some kind of upper limit for the size, maybe 32KiB?

Why PAGES_ESTIMATE is not a good limit?

It was 8kB I had to increase to 16kB because set_add_data_set_key_value_five_Kbytes fails.
It seems that now is enough.

PAGES_ESTIMATE is 40

program_size = sec_bd->get_program_size();
For QSPI this return MBED_CONF_QSPIF_QSPI_MIN_PROG_SIZE is 4

Maybe we should modify this ?

erase_size is 4kB

I guess @kyle-cypress would disagree about dropping the estimate, at least some of the Cypress boards have really small(512B) sector size

Indeed, many of the Cypress boards with internal flash only have sector size = page size = 512K.
Another comment:
Why does this change the size to be aligned up to 4 * erase_size instead of 2. The intent of the current code is that PAGES_ESTIMATE reflects the main space requirement for the test. The align up to erase_size * 2 is just there because garbage collection requires two sectors in order to function properly, and on devices with a high sector to page size ratio PAGES_ESTIMATE may fit entirely in one sector.
If PAGES_ESTIMATE proves to be too low, I think the correct thing to do is to to increase PAGES_ESTIMATE. This will enable the size of the pre-test erase to adjust correspondingly.

Reverted alignment. Fixed page size for STM and Nordic boards Qspi flash page size

kyle-cypress · 2020-03-27T18:36:05Z

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


-#if !defined(TARGET_K64F) && !defined(TARGET_ARM_FM) && !defined(TARGET_MCU_PSOC6)
-#error [NOT_SUPPORTED] Kvstore API tests run only on K64F devices, Fastmodels, and PSoC 6
+#if !SECURESTORE_ENABLED


Is this intended to be a new hard requirement? The existing test checks for and disables the securestore-specific tests when securestore is not enabled.

If this test is being made completely dependent on SecureStore, the other securestore related ifdefs (e.g. in kvstore_init()) should be removed.

Yes. The purpose o this PR is to enable all storage tests if possible.
However if particular target doesn't support all necessary features test must be skipped and we can do nothing with this.

The desire to enable all storage tests whenever possible doesn't seem to align with the check here.
Previously the test was structured so that it could run the FSStore and TDBStore tests on targets without SECURESTORE_ENABLED (if they were part of an otherwise limited list). Only the SecureStore dependent tests were disabled.
Now, the FSStore and TDBStore tests are also disabled when SecureStore is not enabled.

Are there not meaningful cases there SECURESTORE_ENABLED is false but TDBStore and FSStore do work?

Also, can you address this part of my previous comment?

If this test is being made completely dependent on SecureStore, the other securestore related ifdefs (e.g. in kvstore_init()) should be removed.

You right. I will skip only SecureStore.
I need to fix TDBStore because there are crashes on targets without TRNG

0xc0170 · 2020-03-31T13:46:12Z

@tymoteuszblochmobica please be aware you started lts job instead. This is targeting master, so needs regular master PR tests.

0xc0170 · 2020-03-31T13:53:43Z

I aborted it, will start regular CI now

mbed-ci · 2020-03-31T18:24:45Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_cloud-client-pytest

0xc0170 · 2020-04-01T08:33:28Z

CI passed, I'll clear old lts run from here now

0xc0170 · 2020-04-01T08:50:14Z

CI passed.

@tymoteuszblochmobica Is this ready for integration? I see one review comment left : #12638 (comment)

@kyle-cypress please review

tymoteuszblochmobica · 2020-04-01T09:17:07Z

0xc0170 Not yet. I must correct flash page size and revert one test change. Will do it today

tymoteuszblochmobica · 2020-04-01T10:10:04Z

Updated flash page size and align reverted.
@0xc0170 You can restart CI

0xc0170 · 2020-04-01T12:19:56Z

CI started

mbed-ci · 2020-04-01T14:29:26Z

Test run: SUCCESS

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

kyle-cypress

Thanks for fixing the page size. Still some outstanding comments on on the way execution is conditioned on SECURESTORE_ENABLED but those are not critical from my side.

SeppoTakalo previously approved these changes Mar 17, 2020

View reviewed changes

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

VeijoPesonen approved these changes Mar 17, 2020

View reviewed changes

0xc0170 previously approved these changes Mar 17, 2020

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Mar 17, 2020

tymoteuszblochmobica force-pushed the testall branch from 0217460 to 1ebc8b1 Compare March 26, 2020 16:25

VeijoPesonen reviewed Mar 27, 2020

View reviewed changes

kyle-cypress reviewed Mar 27, 2020

View reviewed changes

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

0xc0170 added release-version: 6.0.0-beta-1 needs: review and removed needs: CI labels Apr 1, 2020

Enable storage tests to all targets

2ca832c

tymoteuszblochmobica force-pushed the testall branch from 1ebc8b1 to 2ca832c Compare April 1, 2020 10:08

SeppoTakalo approved these changes Apr 1, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Apr 1, 2020

mergify bot added ready for merge and removed needs: CI labels Apr 1, 2020

kyle-cypress approved these changes Apr 1, 2020

View reviewed changes

0xc0170 merged commit 5539d65 into ARMmbed:master Apr 3, 2020

mergify bot removed the ready for merge label Apr 3, 2020

jamesbeyond mentioned this pull request Apr 14, 2020

NUCLEO_F767ZI failed with features-storage-tests-kvstore-general_tests_phase_1&2 #12806

Closed

Enable storage tests to all targets #12638

Enable storage tests to all targets #12638

Uh oh!

Conversation

tymoteuszblochmobica commented Mar 17, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

tymoteuszblochmobica commented Mar 17, 2020

Uh oh!

SeppoTakalo commented Mar 17, 2020

Uh oh!

tymoteuszblochmobica commented Mar 17, 2020

Uh oh!

VeijoPesonen left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 17, 2020

Uh oh!

mbed-ci commented Mar 17, 2020

Uh oh!

VeijoPesonen commented Mar 18, 2020

Uh oh!

tymoteuszblochmobica commented Mar 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 31, 2020

Uh oh!

0xc0170 commented Mar 31, 2020

Uh oh!

mbed-ci commented Mar 31, 2020

Uh oh!

0xc0170 commented Apr 1, 2020

Uh oh!

0xc0170 commented Apr 1, 2020

Uh oh!

tymoteuszblochmobica commented Apr 1, 2020

Uh oh!

tymoteuszblochmobica commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 1, 2020

Uh oh!

mbed-ci commented Apr 1, 2020

Uh oh!

kyle-cypress left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

tymoteuszblochmobica commented Apr 1, 2020 •

edited

Loading