IOTSTOR-978: Bugfixes to TDBStore and SecureStore #11918
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SeppoTakalo
force-pushed
the
IOTSTOR-978
branch
from
ddbf13e
to
8bbc4c3
Compare
Test run: FAILEDSummary: 4 of 4 test jobs failed Failed test jobs:
|
|
@SeppoTakalo, thank you for your changes. |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
SeppoTakalo
force-pushed
the
IOTSTOR-978
branch
from
8bbc4c3
to
d855e90
Compare
|
Rebased on top of master to fix unittest failure |
|
@AnttiKauppila Can you review the |
0xc0170
requested changes
Nov 22, 2019
features/storage/internal/utils.h
Outdated
|
|
||
| #include <stdint.h> | ||
|
|
||
| uint32_t align_up(uint64_t val, uint64_t size); |
There was a problem hiding this comment.
these should at least contain short doxygen docs eventhough its internal
Test run: FAILEDSummary: 1 of 11 test jobs failed Failed test jobs:
|
SeppoTakalo
force-pushed
the
IOTSTOR-978
branch
2 times, most recently
from
74ed7e6
to
a02db39
Compare
AnttiKauppila
approved these changes
Nov 22, 2019
Test run: FAILEDSummary: 1 of 11 test jobs failed Failed test jobs:
|
|
Test failures look related, please review Is this targeting 5.15 ? We should label and needs to pass CI within this week. |
|
I believe I have managed to find a bug in F411 FlashIAP implementation by introducing new test case. I'll remove the testcase from this PR and submit as a new bugreport. |
|
Can we get it in 5.15? |
SeppoTakalo
force-pushed
the
IOTSTOR-978
branch
from
a02db39
to
11589ab
Compare
Test run: FAILEDSummary: 3 of 4 test jobs failed Failed test jobs:
|
|
CI was restarted |
|
After adding ac99126 commit to the branch, there's a conflict with master |
SeppoTakalo
force-pushed
the
IOTSTOR-978
branch
2 times, most recently
from
9eef874
to
1be11f8
Compare
|
CI started |
Test run: FAILEDSummary: 2 of 4 test jobs failed Failed test jobs:
|
|
The unittest failures might be related to the crc that was reverted yesterday and somehow its still on this branch or? |
|
Ah wait, this PR is adding crc functionality to unittests, is it based on what was on master (and was reverted for 5.15) ? Please review failures |
In case our are contains data from previous reset() or reset_area(), we might end up in the situation where free space contains valid key headers, but we have not erased that area yet. This can cause failures if the deinit() and init() because new scan of that area would continue as long as keys are found. This causes keys on the not-yet-erased area to be included in the new instance of TDBStore. To prevent this failure, check after each key-write that our free space does not contain valid key headers. Also make sure that we erase one program unit sector over the master record. If we erased just the master record,first key might is still there, causing next init() to find it. Extend erase area by one program unit, so that build_ram_table() won't find any keys.
At least with LPC55S69's default TDBStore configuration it's impossible to run storage Greentea tests without exhausting the memory reserved for storing keys. Fixes an issue where number of keys were removed based on number of threads which didn't have anything to do with the test case. Fixes an issue where number of keys were assumed to be constant but variable number was used for configuration.
|
Wait.. I'll rework this PR into smaller chunks because it has grown too big to review. |
Previously Greentea tests was not initialising its storage before asking for bd->get_program_size(), causing FlashBlockDevice to return zero. This caused both TDBStorage's to use zero for both parameter to SlicingBlockDevice(bd, 0, 0), effetivaly both then used same addresses for slice. This caused SecureStore tests to fail, because writes to internal RBP storage overwrote keys from external storage. Fine-tune TDBStore sizes, so that all tests can fit into storage.
SeppoTakalo
force-pushed
the
IOTSTOR-978
branch
from
1be11f8
to
abbb248
Compare
SeppoTakalo
changed the title
Test run: FAILEDSummary: 4 of 4 test jobs failed Failed test jobs:
|
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
|
@0xc0170 This is now ready. Should be able to merge into 5.15.. can go to patch release. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Summary of change
This is list of changes leading up to the bug fix. Many of the tests were done when exploring the issue #11862, so they logically deserve to be in this same PR.
Fixes #11862
Documentation
Need to update the documentation to remove limitation from TDBStore. It no longer requires Flash storage.
Pull request type
Test results
Reviewers
@VeijoPesonen
Release Notes
Summary of changes
Impact of changes
Migration actions required