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

Fix: Allow target size restriction for LPC55S69 #10792

Merged
merged 1 commit into from Jun 19, 2019

Conversation

@hugueskamba
Copy link
Contributor

commented Jun 7, 2019

Description

The build tool uses the sector size found in the CMSIS Pack to determine if
the size that can be specified by target.restrict_size is enough to fit
all the parts of a given binary. See target.restrict_size documentation
in the Mbed OS manual for more information.

The sector size found in the CMSIS Pack is overriden to allow the build
tool to accurately make the decision.

The target's sectors in the CMSIS Pack are defined in 32KB pages.
However, you can erase pages at the 512 byte level.

This commit changes defined sector erase size to 512 bytes instead of
32 kilobytes.

Pull request type

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

Reviewers

@bridadan @theotherjimmy

Fixes internal issue IOTCORE-1149

The build tool uses the sector size found in the CMSIS Pack to determine if
the size that can be specified by `target.restrict_size` is enough to fit
all the parts of a given binary. See `target.restrict_size` documentation
in the Mbed OS manual for more information.

The sector size found in the CMSIS Pack is overriden to allow the build
tool to accurately make the decision.

The target's sectors in the CMSIS Pack are defined in 32KB pages.
However, you can erase pages at the 512 byte level.

This commit changes defined sector erase size to 512 bytes instead of
32 Kilobytes.
@ciarmcom ciarmcom requested review from bridadan, theotherjimmy and ARMmbed/mbed-os-maintainers Jun 7, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@hugueskamba, thank you for your changes.
@bridadan @theotherjimmy @ARMmbed/mbed-os-maintainers please review.

@@ -2058,7 +2058,8 @@
"detect_code": ["0236"],
"device_name": "LPC55S69JBD100",
"release_versions": ["5"],
"program_cycle_s": 10
"program_cycle_s": 10,
"sectors": [[0,512]]

This comment has been minimized.

Copy link
@evedon

evedon Jun 10, 2019

Contributor

Page erase is defined at 256 byte in
https://www.nxp.com/docs/en/user-guide/UM11126.pdf (page 3)
So 512 will work but we could optimise further.

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Jun 17, 2019

Author Contributor

I agree with you. However, @offirko left the following comment on the internal ticket:

Jimmy Brisson - please note that while LPC55S69 has 32KB sector size, it supports 512-bytes Page Erase command (subsectors).

When target's erase sector size is queried it returns Page Erase size of 512-bytes. (this is the value eventually used by FlashIAP block device,..etc)

uint32_t flash_get_sector_size(const flash_t *obj, uint32_t address)

Could we change the sector size defined at index.json to 512-bytes ?

I would prefer to leave it as 512 bytes to avoid introducing potential bugs. This is unless we also want to change the define for the flash page size. @evedon Please advise.

This comment has been minimized.

Copy link
@evedon

evedon Jun 18, 2019

Contributor

Let's not do this change now.

This comment has been minimized.

Copy link
@mmahadevan108

mmahadevan108 Jun 18, 2019

Contributor

Page erase is defined at 256 byte in
https://www.nxp.com/docs/en/user-guide/UM11126.pdf (page 3)
So 512 will work but we could optimise further.

This is a documentation error. Program and erase size is 512, thank you for finding this. We have raised an issue with the documentation team to correct this.

@0xc0170 0xc0170 requested a review from mmahadevan108 Jun 11, 2019
Copy link
Contributor

left a comment

Change looks fine to me, nice work!

@adbridge adbridge added this to Needs review in Test Jun 13, 2019
@evedon
evedon approved these changes Jun 18, 2019
Test automation moved this from Needs review to Needs CI Jun 18, 2019
@adbridge adbridge removed the request for review from theotherjimmy Jun 18, 2019
@adbridge adbridge added needs: CI and removed needs: review labels Jun 18, 2019
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

CI started.

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 19, 2019

Test run: SUCCESS

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

@adbridge adbridge merged commit 32c8bf5 into ARMmbed:master Jun 19, 2019
26 checks passed
26 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-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+4488 bytes) RAM(+17104 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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8566 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@hugueskamba hugueskamba deleted the hugueskamba:fix-lpc55s69-sector-size branch Jun 26, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

Postponing this fix to 5.13.2 as it is causing 5.13.1 RC not building for client. This fix needs further investigation - implications are bigger than it seems ( as I understood there's connection to bootloader in the internal ticket IOTSTOR-880 and we cant' get anyone from storage to help us this week).

@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Postponing this fix to 5.13.2 as it is causing 5.13.1 RC not building for client. This fix needs further investigation - implications are bigger than it seems ( as I understood there's connection to bootloader in the internal ticket IOTSTOR-880 and we cant' get anyone from storage to help us this week).

Can you share the error(s)/warning(s) displayed when it fails to build? Is there a ticket?

@evedon

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

@hugueskamba will investigate why this is causing build error for client
See following PRs:
ARMmbed/mbed-cloud-client-example-internal#918
ARMmbed/mbed-client-testapp#1363

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

As we were told, this has dependency on bootloader + client update, moving to 5.14

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