TDBStore refactoring #11987

SeppoTakalo · 2019-11-29T10:32:38Z

Description

Summary of change

Small fixes, refactoring and one functionality change to TDBStore:

TDBStore: Don't copy more data than what we can hold
TDBStore: Move Assert to init(), so Block parameter are initialised correctly
Do not require Flash device for TDBStore <-- Functional change, but not API change
TDBStore: Do no garbage_collect() on init()
TDBStore: Keep copy of reserved data on both areas.
TDBStore: Erase one program unit more, when cleaning areas

Assuming underlying device was FLASH device was error prone, as we cannot trust erase values. Logic has changed to that we always erase before writing to new area.

Documentation

We can remove then requirements from documentation that TDBStore would require FLASH device. After these changes, it does not anymore.

More module+unit tests have been implemented but can only be submitted once Mbed CRC changes are submitted into master.

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)
[] Tests / results supplied as part of this PR

Reviewers

@VeijoPesonen

Release Notes

Summary of changes

TDBStore does not anymore require Flash based block device.

Impact of changes

Migration actions required

VeijoPesonen

Looks good to me

mbed-ci · 2019-11-29T12:00:33Z

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

ciarmcom · 2019-11-29T12:00:38Z

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

Change the "reserved data" logic so that every time we erase and area, the content of reserved data is then immediately copied to newly erased area. This keeps two copies of the data. When data is requested, return only if checksum is matching. When data is written, only allow if BOTH checksums are incorrect, meaning that areas are either corrupted or erased. Only exception is TDBStore::reset() which erases all keys and reserved data. Removed all logic that tried to detect, if reserved are was erased or corrupted. Rely entirely on checksum. Add moduletest for reserved data.

Previous logic caused garbage collection to kick in, if the init() was called on empty storage. This has effect of erasing areas twice, if both areas were empty. Re-write logic so that we erase areas only on garbage_collect() or reset(). The init() logic already chooses the active area, so no need to touch, until keys are modified. Removed also the is_erase_unit_erased() as this is working only on FLASH devices, and TDBStore should be refactored to work on all storages.

TDBStore used to rely on Flash devices erase value. This logic has been removed, and TDBStore can do the entire erase logic itself, in case the given BlockDevice does not offer erase(). This relies on BlockDevice to properly return -1 in BlockDevice::get_erase_value().

…orrectly

SeppoTakalo · 2019-12-03T13:25:07Z

Cherry-picked two commits from #11986 which should also fix the failing test case here.

Rebased and started a test run.

mbed-ci · 2019-12-03T18:07:49Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

SeppoTakalo · 2019-12-04T09:05:45Z

Failed test NUCLEO_F429ZI IAR / tests-netsocket-tls – NUCLEO_F429ZI-IAR.tests-netsocket-tls looks unrelated.

0xc0170 · 2019-12-04T09:18:43Z

@SeppoTakalo You could have just restarted greentea test (exporters, test after build OK can be restarted separately), all restarted is also fine.

VeijoPesonen

Looks good to me

mbed-ci · 2019-12-04T11:21:36Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

SeppoTakalo · 2019-12-04T11:29:58Z

@VeliMattiLahtela Any idea why TCP tests on F429ZI are now failing?

0xc0170 · 2019-12-04T12:49:27Z

Looks like network problem as all failed on the first run, second run is all good

SeppoTakalo · 2019-12-04T12:51:56Z

Yes, verified from CI room that ethernet cable was unplugged.

Copy over the TDBStore.cpp|.h as they contain a few bug fixes which we seem to be needing (in some corner cases). The files are copied over from Mbed OS master at the tip of master as of today, hash pointing at 2f87d59. This brings in a number of changes, but primarily we are interested in this: ARMmbed#11987 Device 943 ended up in a state, where it would not boot up and in the debug image I was able to spot it died in the TDBStore.cpp, 00> ++ MbedOS Error Info ++ 00> Error Status: 0x80FF011B Code: 283 Module: 255 00> Error Message: TDBSTORE: Unable to build RAM table at init 00> Location: 0x71347 00> Error Value: 0x0 00> Current Thread: main Id: 0x20012C68 Entry: 0x7482F StackSize: 0x1400 StackMem: 0x200168E8 SP: 0x20017AEC 00> For more info, visit: https://mbed.com/s/error?error=0x80FF011B&tgt=EPEN3 This is due to corrupted QSPIF. This should prevent that from happening or at least recover from it in a more sensible way, as it will not "guess if it's been erased or not", but more deterministily just erase properly. Device was recoverable even with existing SW by forcing an erase in the error branch, patch below: --- start of patch: ``` diff --git i/features/storage/kvstore/tdbstore/TDBStore.cpp w/features/storage/kvstore/tdbstore/TDBStore.cpp index efeedf42c2..701ce7d044 100644 --- i/features/storage/kvstore/tdbstore/TDBStore.cpp +++ w/features/storage/kvstore/tdbstore/TDBStore.cpp @@ -1027,6 +1027,7 @@ int TDBStore::init() } #endif + printf("_bd.type == %s\n", _bd->get_type()); _max_keys = initial_max_keys; @@ -1097,6 +1098,7 @@ int TDBStore::init() // In case we have two empty areas, arbitrarily use area 0 as the active one. if ((area_state[0] == TDBSTORE_AREA_STATE_EMPTY) && (area_state[1] == TDBSTORE_AREA_STATE_EMPTY)) { + printf("active area = 0\n"); _active_area = 0; _active_area_version = 1; ret = write_master_record(_active_area, _active_area_version, _free_space_offset); @@ -1115,6 +1117,7 @@ int TDBStore::init() } else { _active_area = 1; } + printf("active area = %d\n", _active_area); _active_area_version = versions[_active_area]; ret = reset_area(1 - _active_area); if (ret) { @@ -1128,6 +1131,11 @@ int TDBStore::init() ret = build_ram_table(); if ((ret != MBED_SUCCESS) && (ret != MBED_ERROR_INVALID_DATA_DETECTED)) { + printf("TDBSTORE: Unable to build RAM table at init - try erase area!"); + ret = reset_area(0); + printf("reset_area(0))=%d\n", ret); + ret = reset_area(1); + printf("reset_area(1))=%d\n", ret); MBED_ERROR(ret, "TDBSTORE: Unable to build RAM table at init"); } @@ -1159,6 +1167,7 @@ int TDBStore::init() } end: + printf("TDBStore init() done for %s\n", _bd->get_type()); _is_initialized = true; _mutex.unlock(); return ret; ``` --- end of patch

Since ARMmbed/mbed-os#11987 was merged to Mbed OS, a flash device or a simulated flash device is no longer a requirement.

JanneKiiskila · 2022-01-21T08:22:14Z

@0xc0170 @adbridge - could this patch be ported to a 5.15.x release?

0xc0170 · 2022-01-24T10:43:36Z

@0xc0170 @adbridge - could this patch be ported to a 5.15.x release?

We are not able to port this feature request to 5.15 branch. If it's required (not certain which parts are, as this pull request includes multiple changes), it needs to be contributed.

JanneKiiskila · 2022-01-24T15:12:04Z

This is not just a feature request, this is actually a bug fix in many respects.

VeijoPesonen approved these changes Nov 29, 2019

View reviewed changes

ciarmcom requested review from VeijoPesonen and a team November 29, 2019 12:00

ciarmcom added the needs: review label Nov 29, 2019

Seppo Takalo added 8 commits December 3, 2019 14:38

Fix Greentea handler typecasting.

b9eaac4

Remove flaky error_inject_test

2ddd406

TDBStore: erase one program unit more, when cleaning an area

3dc6c48

TDBStore: Move Assert to init(), so Block parameter are initialised c…

fe3c355

…orrectly

TDBStore: Don't copy more data than what we can hold

ce7b196

SeppoTakalo force-pushed the feature_tdbstore_refactoring branch from e79de64 to ce7b196 Compare December 3, 2019 13:22

0xc0170 added needs: CI release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: review labels Dec 3, 2019

VeijoPesonen approved these changes Dec 4, 2019

View reviewed changes

0xc0170 approved these changes Dec 4, 2019

View reviewed changes

0xc0170 removed the needs: CI label Dec 4, 2019

0xc0170 merged commit 7a085b4 into master Dec 4, 2019

SeppoTakalo deleted the feature_tdbstore_refactoring branch December 4, 2019 12:52

LDong-Arm mentioned this pull request Dec 7, 2020

Clean-up: TDBStore no longer requires BlockDevice to have flash behaviour #14007

Merged

LDong-Arm added a commit to LDong-Arm/mbed-os-5-docs that referenced this pull request Dec 7, 2020

TDBStore no longer requires flash behaviour

6417894

Since ARMmbed/mbed-os#11987 was merged to Mbed OS, a flash device or a simulated flash device is no longer a requirement.

LDong-Arm mentioned this pull request Dec 7, 2020

TDBStore no longer requires flash behaviour ARMmbed/mbed-os-5-docs#1386

Merged

LDong-Arm mentioned this pull request Mar 29, 2021

Improve HeapBlockDevice, TDBStore and tests #14483

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDBStore refactoring #11987

TDBStore refactoring #11987

SeppoTakalo commented Nov 29, 2019

VeijoPesonen left a comment

mbed-ci commented Nov 29, 2019

ciarmcom commented Nov 29, 2019

SeppoTakalo commented Dec 3, 2019

mbed-ci commented Dec 3, 2019

SeppoTakalo commented Dec 4, 2019

0xc0170 commented Dec 4, 2019

VeijoPesonen left a comment

mbed-ci commented Dec 4, 2019

SeppoTakalo commented Dec 4, 2019

0xc0170 commented Dec 4, 2019

SeppoTakalo commented Dec 4, 2019

JanneKiiskila commented Jan 21, 2022

0xc0170 commented Jan 24, 2022

JanneKiiskila commented Jan 24, 2022

TDBStore refactoring #11987

TDBStore refactoring #11987

Conversation

SeppoTakalo commented Nov 29, 2019

Description

Summary of change

Documentation

Pull request type

Test results

Reviewers

Release Notes

Summary of changes

Impact of changes

Migration actions required

VeijoPesonen left a comment

Choose a reason for hiding this comment

mbed-ci commented Nov 29, 2019

Test run: FAILED

ciarmcom commented Nov 29, 2019

SeppoTakalo commented Dec 3, 2019

mbed-ci commented Dec 3, 2019

Test run: FAILED

SeppoTakalo commented Dec 4, 2019

0xc0170 commented Dec 4, 2019

VeijoPesonen left a comment

Choose a reason for hiding this comment

mbed-ci commented Dec 4, 2019

Test run: FAILED

SeppoTakalo commented Dec 4, 2019

0xc0170 commented Dec 4, 2019

SeppoTakalo commented Dec 4, 2019

JanneKiiskila commented Jan 21, 2022

0xc0170 commented Jan 24, 2022

JanneKiiskila commented Jan 24, 2022