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

yossi2le · 2018-10-03T15:16:15Z

…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

0xc0170 · 2018-10-04T15:32:09Z

components/storage/blockdevice/COMPONENT_FLASHIAP/FlashIAPBlockDevice.h

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


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

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.

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

geky · 2018-10-09T15:15:34Z

features/storage/system_storage/SystemStorage.cpp

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


Does this work if NVStore is not present?

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

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.

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.

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.

Thanks for that 👍

geky

Looks good to me

geky

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

Revert deprecation of FlashIAPBlockDevice 2 argument constructor has this was a breaking change. This follows a similar change in the external flashiap-driver repo.

…external repo

geky

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 · 2018-10-11T08:26:08Z

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 · 2018-10-11T15:35:57Z

/morph build

mbed-ci · 2018-10-11T16:24:28Z

Build : FAILURE

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

cmonr · 2018-10-12T18:46:14Z

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

mbed-ci · 2018-10-12T19:36:46Z

Build : FAILURE

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

NirSonnenschein · 2018-10-14T09:25:35Z

/morph build

mbed-ci · 2018-10-14T10:28:55Z

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 · 2018-10-14T12:06:01Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-10-14T14:48:06Z

Test : SUCCESS

Build number : 3155
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8317/3155

yossi2le changed the title ~~Add flashiap bd as default~~ Add flashiap block device as default Oct 4, 2018

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

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

0xc0170 requested a review from a team October 4, 2018 09:35

0xc0170 added the needs: review label Oct 4, 2018

dannybenor approved these changes Oct 4, 2018

View reviewed changes

0xc0170 requested changes Oct 4, 2018

View reviewed changes

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

yossi2le force-pushed the add-flashiap-bd-as-default branch 2 times, most recently from 2d6d256 to bb79a63 Compare October 9, 2018 15:09

geky reviewed Oct 9, 2018

View reviewed changes

geky approved these changes Oct 9, 2018

View reviewed changes

0xc0170 approved these changes Oct 9, 2018

View reviewed changes

0xc0170 added needs: CI release-version: 5.10.2 and removed needs: work labels Oct 9, 2018

geky suggested changes Oct 9, 2018

View reviewed changes

Fixing the order of component if more than one is included for a target.

473f8d3

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 added needs: work and removed needs: CI labels Oct 10, 2018

Removing nvstore. adding back the option for configuration as in the …

b84f377

…external repo

yossi2le force-pushed the add-flashiap-bd-as-default branch from bb79a63 to b84f377 Compare October 10, 2018 14:17

geky approved these changes Oct 11, 2018

View reviewed changes

adbridge added the priority: high label Oct 12, 2018

cmonr added the needs: CI label Oct 12, 2018

cmonr removed the needs: work label Oct 12, 2018

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

Fixing unicode error in IAR compilation

196fbe5

NirSonnenschein added needs: CI and removed needs: work labels Oct 14, 2018

NirSonnenschein added ready for merge and removed needs: CI labels Oct 14, 2018

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

cmonr removed the ready for merge label Oct 15, 2018

dannybenor mentioned this pull request Jan 23, 2019

Update example to use Mbed OS 5.11.2 ARMmbed/mbed-os-example-mbed-crypto#12

Merged

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)

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)

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

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

Uh oh!

Conversation

yossi2le commented Oct 3, 2018

Description

Pull request type

Uh oh!

0xc0170 Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

yossi2le commented Oct 11, 2018

Uh oh!

cmonr commented Oct 11, 2018

Uh oh!

mbed-ci commented Oct 11, 2018

Uh oh!

cmonr commented Oct 12, 2018

Uh oh!

mbed-ci commented Oct 12, 2018

Uh oh!

NirSonnenschein commented Oct 14, 2018

Uh oh!

mbed-ci commented Oct 14, 2018

Triggering tests

Uh oh!

mbed-ci commented Oct 14, 2018

Uh oh!

mbed-ci commented Oct 14, 2018

Uh oh!

Uh oh!

0xc0170 Oct 4, 2018 •

edited

Loading

geky Oct 9, 2018 •

edited

Loading