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

Add FlashIAP block device as default block device for WISE 1570 #8317

Merged
merged 3 commits into from Oct 15, 2018

Conversation

Projects
None yet
9 participants
@yossi2le
Contributor

yossi2le commented Oct 3, 2018

…AP as default block device for wise 1570

Description

This PR is adding FlashIAP block device component as the default block device for Wise 1570 board.
To accomplish this the deprecated constructor was reverted and used by calculating the start address at the first sector after the code address zone and the end address at the last sector before nvstore address zone.
This way the FlashIAP default block device does not overwrite any code or nvstore data.

Fix both issues MBEDOSTEST-209 and MBEDOSTEST-208 as they have been found to be related.

Pull request type

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

@yossi2le yossi2le changed the title from Add flashiap bd as default to Add flashiap block device as default Oct 4, 2018

@yossi2le yossi2le changed the title from Add flashiap block device as default to Add flashiap block device as default block device for WISE 1570 Oct 4, 2018

@yossi2le yossi2le changed the title from Add flashiap block device as default block device for WISE 1570 to Add FlashIAP block device as default block device for WISE 1570 Oct 4, 2018

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-storage Oct 4, 2018

*
* @address Physical address where the block device start
* @size The block device size
*/
FlashIAPBlockDevice(uint32_t address, uint32_t size = 0);

This comment has been minimized.

@0xc0170

0xc0170 Oct 4, 2018

Member

Why ? can you add details to the commit message why this is not deprecated anymore? And also changes in FlashIAPBlockDevice ctor - part of the revert or?

This will need to get into release notes if that is the case

This comment has been minimized.

@davidsaada

davidsaada Oct 4, 2018

Contributor

We originally deprecated this constructor in the external repo, as this would have required the user to either keep a list of absolute addresses or to use FlashIAP driver to query the flash parameters (which would have been "ugly" if we already use FlashIAPBlockDevice). However, it turned out that this had actually broke those who used this block device and finally reverted that change in the external repo. In addition, there was no escape from using FlashIAP to query flash parameters before instantiating FlashIAPBlockDevice, as we needed to query sector sizes in order to calculate the partial file system location in flash.

This comment has been minimized.

@0xc0170

0xc0170 Oct 9, 2018

Member

Can you add some of these details to the commit msg?

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 4, 2018

@yossi2le yossi2le force-pushed the yossi2le:add-flashiap-bd-as-default branch 2 times, most recently from 2d6d256 to bb79a63 Oct 9, 2018

size_t area_size;
FlashIAP flash;
NVStore &nvstore = NVStore::get_instance();
nvstore.get_area_params(0, top_address, area_size); //Find where nvstore begins

This comment has been minimized.

@geky

geky Oct 9, 2018

Member

Does this work if NVStore is not present?

This comment has been minimized.

@yossi2le

yossi2le Oct 9, 2018

Contributor

No. However, if there is flash so there is also NVStroe.

This comment has been minimized.

@geky

geky Oct 9, 2018

Member

Is that true? I thought you need to call NVStore::init to create NVStore on disk.

Actually looking at get_area_params, by calling this function we are indirectly formatting the disk with NVStore. Which means we also have to pull in the NVStore logic.

This comment has been minimized.

@geky

geky Oct 9, 2018

Member

With call to nvstore.get_area_params():

Module .text .data .bss
[fill] 131(+0) 4(+0) 17(+0)
[lib] 20671(+0) 2488(+0) 117(+0)
components 902(+0) 0(+0) 0(+0)
drivers 764(+0) 4(+0) 140(+0)
features 2530(+0) 0(+0) 128(+0)
hal 1460(+0) 4(+0) 68(+0)
main.o 10(+0) 0(+0) 0(+0)
platform 1624(+0) 256(+0) 133(+0)
rtos 8293(+0) 168(+0) 6053(+0)
targets 11687(+0) 8(+0) 912(+0)
Subtotals 48072(+0) 2932(+0) 7568(+0)

Without call to nvstore.get_area_params():

Module .text .data .bss
[fill] 123(-8) 4(+0) 21(+4)
[lib] 20671(+0) 2488(+0) 117(+0)
components 902(+0) 0(+0) 0(+0)
drivers 764(+0) 4(+0) 140(+0)
features 140(-2390) 0(+0) 44(-84)
hal 1460(+0) 4(+0) 68(+0)
main.o 10(+0) 0(+0) 0(+0)
platform 1620(-4) 256(+0) 133(+0)
rtos 8055(-238) 168(+0) 6053(+0)
targets 11687(+0) 8(+0) 912(+0)
Subtotals 45432(-2640) 2932(+0) 7488(-80)

That's an extra 2.6KiB on platforms that just want internal flash not NVStore. Could we instead use config options specify the flash region? That's what we're currently doing for SPIF and SD pins.

This comment has been minimized.

@yossi2le

yossi2le Oct 10, 2018

Contributor

I have removed nvstore and added back the configuration option.
If no configuration supplied and still FlashIAP block device is enabled the block device will take the address of the first sector after code section till the end of the flash.

This comment has been minimized.

@geky

geky Oct 11, 2018

Member

Thanks for that 👍

@geky

geky approved these changes Oct 9, 2018

Looks good to me

@0xc0170

0xc0170 approved these changes Oct 9, 2018

@geky

See above comment, this shouldn't be pulling in the NVStore codebase

Fixing the order of component if more than one is included for a target.
Revert deprecation of FlashIAPBlockDevice 2 argument constructor has this was a breaking change. This follows a similar change in the external flashiap-driver repo.

@cmonr cmonr added needs: work and removed needs: CI labels Oct 10, 2018

@yossi2le yossi2le force-pushed the yossi2le:add-flashiap-bd-as-default branch from bb79a63 to b84f377 Oct 10, 2018

@geky

geky approved these changes Oct 11, 2018

Thanks for the fix 👍

Looks good to me. Though the use of the ROM_END makes me wonder what applications should do if they want to update (and grow in size) in the future. Should they be expected to specify a flash region with the config?

@yossi2le

This comment has been minimized.

Contributor

yossi2le commented Oct 11, 2018

Looks good to me. Though the use of the ROM_END makes me wonder what applications should do if they want to update (and grow in size) in the future. Should they be expected to specify a flash region with the config?

This is a known limitation for now and I guess if they want to be able to update they should specify the config parameters.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 11, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 12, 2018

Huh. Gonna see if this is was just a glitch.
/morph build

@cmonr cmonr added the needs: CI label Oct 12, 2018

@cmonr cmonr removed the needs: work label Oct 12, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 12, 2018

@cmonr cmonr added needs: work and removed needs: CI labels Oct 12, 2018

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Oct 14, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 14, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit a6651b8 into ARMmbed:master Oct 15, 2018

15 checks passed

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 bytes) RAM(+0 bytes)
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-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 660 files (+2 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9161 cycles (-777 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

@cmonr cmonr removed the ready for merge label Oct 15, 2018

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