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

FlashIAP & NVStore tests: Skip test if overwriting code in flash #7212

Merged
merged 2 commits into from Aug 21, 2018

Conversation

Projects
None yet
7 participants
@davidsaada
Contributor

davidsaada commented Jun 13, 2018

Description

The FlashIAP test erases the last two sectors in some of its test cases. NVStore test does the same. However, with boards that have small flash components (and large last sectors), this test may overwrite the code section. This fix checks that scenario and skips the test if it is indeed the case.
Resolves #7149 and #7388.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jun 13, 2018

@jeromecoutant Would be glad if you could test this fix on your board, as I don't have it. If you can do it on all supported three toolchains (GCC_ARM, ARM, IAR), it would be best. Note that if it works, you should see a print telling the test was skipped.

@cmonr cmonr added the needs: review label Jun 13, 2018

@cmonr cmonr requested a review from jeromecoutant Jun 13, 2018

#endif
// This macro returns if flash test overwrites the code in flash.
#define RETURN_IF_TEST_OVERWRITES_CODE(ADDRESS) \

This comment has been minimized.

@0xc0170

0xc0170 Jun 14, 2018

Member

this would be nicer if its just a function, no need for a macro?

This comment has been minimized.

@davidsaada

davidsaada Jun 14, 2018

Contributor

I'm not a big macro fan myself. However, in this case, macro seems like the better solution, as it includes a return inside. Turning this into a function would make the code a bit more bloated, as it would make all three tests check the return value, and return if it indicates that the code is getting overwritten. No biggie, can change if required.

This comment has been minimized.

@0xc0170

0xc0170 Jun 14, 2018

Member

👍 Fine as it is

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jun 15, 2018

Hi
It looks OK with F410 and GCC,

But I couldn't compile for L475VG and ARM...

[DEBUG] Errors: Error: L6218E: Undefined symbol Load$$LR$$LR_m_text$$Limit (referred from BUILD/tests/DISCO_L475VG_IOT01A/ARM/TESTS/mbed_drivers/flashiap/main.o).

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jun 15, 2018

@jeromecoutant Yes I am able to reproduce this problem with the ARM compiler (the other toolchains seem to behave properly). Will probably need more time to investigate.

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_test_small_flash branch from 5900707 to a7657df Jun 18, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jun 18, 2018

@jeromecoutant Please retest with the ARM compiler. Made a change that depends on another open PR, so even if it works, it'll be some time until it gets merged. You only need to pull TESTS/mbed_drivers/flashiap/main.cpp (other files aren't relevant for your target).

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_test_small_flash branch from a7657df to 3097d66 Jun 18, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jun 19, 2018

Hi

  • ARM: OK
  • GCC: OK
  • IAR: test crashes (it doesn't detect the end of code section ?)

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jun 20, 2018

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_test_small_flash branch 2 times, most recently from 908711d to dc5147f Jun 20, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jun 20, 2018

IAR: test crashes (it doesn't detect the end of code section ?)

OK, I think I understand the problem (has to do with the beginning of the text section). Pushed a fix for it now. Keeps working on the boards I have.
@jeromecoutant Would appreciate retesting. I added a debug print in the beginning of the init test. Will be used if the test doesn't pass and removed if it does.

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_test_small_flash branch from dc5147f to 102eeaa Jun 20, 2018

@jeromecoutant

As discussed by mail, test seems OK with ARM, GCC and IAR now

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_test_small_flash branch from 102eeaa to 0e7f7f4 Jun 21, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jun 21, 2018

Thanks @jeromecoutant. Just pushed a commit without the debug print.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jun 21, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 21, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 21, 2018

@cmonr cmonr added needs: work and removed needs: CI labels Jun 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

@davidsaada Please review the build linker issues.

@offirko

Looks great to me

@mbed-ci

This comment has been minimized.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 16, 2018

Test needs to be rerun. Failure is on a timeout after flashing the image. Tried myself with the same board and it works OK.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 16, 2018

/morph test

davidsaada added some commits Jun 13, 2018

Fix TMPM64B IAR linker file
Remove unnecessary manual inclusion of tmpm64b_fc object file in linker script
@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

Welp, at least now the test that caused this PR to fail is not longer in the repo.

/morph test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

/morph test

@davidsaada davidsaada force-pushed the davidsaada:david_flashiap_test_small_flash branch from 7fddba1 to 876b5f7 Aug 17, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 17, 2018

@cmonr Rebased following merge of #7670 (as both modify the NVstore tests). Please rerun morph build when done.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 17, 2018

Build : SUCCESS

Build number : 2821
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7212/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Aug 19, 2018

@cmonr Don't know what this failure is about, probably not related to this PR. Regardless, please note that this PR is depending on #7791. As both PRs are on the verge of merging (after uVisor test failure gets sorted out), I don't think we should rebase this one and go through the entire morph build process again. Please just take this into consideration if and when both PRs get merged.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 19, 2018

It's possible that a commit that was merged caused the uVisor test to break in a strange way. We've been keeping an eye on it this weekend since it's blocking a whole slew of PRs from being merged :(

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Aug 21, 2018

/morph uvisor-test

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 21, 2018

@cmonr cmonr merged commit c10ad7f into ARMmbed:master Aug 21, 2018

15 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 584 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9138 cycles (-765 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@davidsaada davidsaada deleted the davidsaada:david_flashiap_test_small_flash branch Aug 22, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7212 from davidsaada/david_flashiap_test_s…
…mall_flash

FlashIAP & NVStore tests: Skip test if overwriting code in flash
@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 23, 2018

Retagging for 5.10 since FLASHIAP_ROM_END is not in a patch release.

@jeromecoutant jeromecoutant referenced this pull request Oct 26, 2018

Closed

NVSTORE test issue #8555

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment