Skip to content
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.

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

NVStore: fix area calculation function #9445

Merged
merged 1 commit into from Jan 31, 2019

Conversation

Projects
None yet
6 participants
@davidsaada
Copy link
Contributor

davidsaada commented Jan 21, 2019

Description

NVStore includes a function (calc_validate_area_params) that either calculates or validates the addresses and sizes of both areas. This function allocates a sector map array used in both calculation/validation of areas. While this generally worked, it had a few problems: First of all, it allocated and deallocated the sector map inside the function, while a more efficient code wouldn't need it. Second, it had a potential bug - number of of entries in the array was calculated according to the first sector size, but in case of a variant sector size, with a large first sector, the array size could have been smaller than required (this probably will never happen, but it's still a bug).
This PR removes the allocation and usage of sector map array. Instead it traverses the flash sectors on the fly (forwards in the validation case, backwards in the calculation case).
In addition, fix tests to get over failures in low end boards.
Tested with K64F (both default and user configured), K82F, K66F, NUCLEO_F429ZI and UBLOX_EVK_ODIN_W2.

Resolves #8513.

Pull request type

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

Reviewers

@jeromecoutant

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 21, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Jan 21, 2019

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

sector_map[num_sectors++] = flash_addr;
if (user_config) {
flash_addr = flash_start_addr;
while (left_size) {

This comment has been minimized.

@dannybenor

dannybenor Jan 21, 2019

while (left_size) does not seems safe. Maybe int left_size = flash_size; and while (left_size>0)?

This comment has been minimized.

@davidsaada

davidsaada Jan 21, 2019

Author Contributor

Can do, though:

  • Will probably need a few casts as all the rest of the variables are of size_t types.
  • The scenario you're afraid of is pretty erroneous: It can only happen if the flash driver exposes a flash size which is not a sum of its own sectors.
@jeromecoutant

This comment has been minimized.

Copy link
Contributor

jeromecoutant commented Jan 22, 2019

Hi

(Probably) resolves #8513.

No, I'm sorry...

Target Toolchain TestSuite TestCase #9445 mbed-os-5.11.2
NUCLEO_F070RB ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality OK OK
NUCLEO_F070RB ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test ERROR ERROR
NUCLEO_F070RB ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F070RB GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality OK OK
NUCLEO_F070RB GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test OK OK
NUCLEO_F070RB GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F070RB IAR features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality FAIL OK
NUCLEO_F070RB IAR features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test OK OK
NUCLEO_F070RB IAR features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F072RB ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality OK OK
NUCLEO_F072RB ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test ERROR ERROR
NUCLEO_F072RB ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F072RB GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality OK OK
NUCLEO_F072RB GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test OK OK
NUCLEO_F072RB GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F072RB IAR features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality FAIL OK
NUCLEO_F072RB IAR features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test OK OK
NUCLEO_F072RB IAR features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F091RC ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality OK OK
NUCLEO_F091RC ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test OK OK
NUCLEO_F091RC ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F091RC GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality OK OK
NUCLEO_F091RC GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Multiple thread test OK OK
NUCLEO_F091RC GCC_ARM features-storage-nvstore-tests-nvstore-functionality NVStore: Race test OK OK
NUCLEO_F091RC IAR features-storage-nvstore-tests-nvstore-functionality NVStore: Basic functionality OK OK

@0xc0170 0xc0170 requested a review from jeromecoutant Jan 22, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

davidsaada commented Jan 22, 2019

Hi

(Probably) resolves #8513.

No, I'm sorry...

Ugh... Will work with you offline on this.
PR should proceed regardless of this failure though (as resolving it wasn't its only purpose).

@adbridge adbridge requested a review from bulislaw Jan 23, 2019

@davidsaada davidsaada force-pushed the davidsaada:david_nvstore_fix_area_calc branch Jan 23, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

davidsaada commented Jan 23, 2019

Pushed a fix to the tests, as it seems that this change worsened the state of test in low memory boards.
@jeromecoutant I believe that at least few of the issues you had are addressed now. Please retest.
If this works, will update the commit message and PR description.

@jeromecoutant
Copy link
Contributor

jeromecoutant left a comment

All green!

Thx

@davidsaada davidsaada force-pushed the davidsaada:david_nvstore_fix_area_calc branch Jan 24, 2019

NVStore: fix area calculation function
Don't allocate the sector map array in this function,
as it was buggy and redundant. Separate user config vs. automatic allocation
cases instead (which was essentially the case anyway).
In addition, fix tests to get over failures in low end boards

@davidsaada davidsaada force-pushed the davidsaada:david_nvstore_fix_area_calc branch to 765b483 Jan 24, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

davidsaada commented Jan 24, 2019

Thanks @jeromecoutant for verifying that. Description and commit message modified.

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

jeromecoutant commented Jan 30, 2019

Maybe we can start CI ? :-)

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Jan 30, 2019

CI started

@cmonr

cmonr approved these changes Jan 30, 2019

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Jan 31, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Jan 31, 2019

@cmonr cmonr merged commit 66792b4 into ARMmbed:master Jan 31, 2019

24 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(-24 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9200 cycles (-49 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 31, 2019

@davidsaada davidsaada deleted the davidsaada:david_nvstore_fix_area_calc branch Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.