Skip to content

Use boot stack size from config system #13452

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 10, 2020
Merged

Use boot stack size from config system #13452

merged 3 commits into from
Sep 10, 2020

Conversation

Patater
Copy link
Contributor

@Patater Patater commented Aug 18, 2020 •
edited
Loading

Summary of changes

To allow overriding of the boot stack size from the Mbed configuration
system, consistently use MBED_CONF_TARGET_BOOT_STACK_SIZE rather than
MBED_BOOT_STACK_SIZE.

Fixes #10319

Impact of changes

Migration actions required

Documentation

Porting guide needs updating. PR available at ARMmbed/mbed-os-5-docs#1362

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 18, 2020
@ciarmcom ciarmcom requested review from ashok-rao, bentcooke, maclobdell and a team August 18, 2020 11:30
@ciarmcom
Copy link
Member

@Patater, thank you for your changes.
@ashok-rao @maclobdell @bentcooke @ARMmbed/mbed-os-core @ARMmbed/mbed-os-test @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

hugueskamba
hugueskamba previously approved these changes Aug 18, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels Aug 18, 2020
@hugueskamba hugueskamba requested a review from evedon August 18, 2020 11:59
@hugueskamba
Copy link
Collaborator

@Patater Should the present PR be merged after this one?

@Patater
Copy link
Contributor Author

Patater commented Aug 18, 2020

@Patater Should the present PR be merged after this one?

Either order is OK. #13455 needs updating to use the CONF define.

@hugueskamba
Copy link
Collaborator

@Patater Should the present PR be merged after this one?

Either order is OK. #13455 needs updating to use the CONF define.

Done.

This was referenced Aug 18, 2020
Merged
Closed
@hugueskamba
Copy link
Collaborator

Fixes ARMmbed/mbed-tools#55

@0xc0170 0xc0170 requested a review from jeromecoutant August 19, 2020 08:19
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 19, 2020

Fixes #10319

@jeromecoutant Please review, you found this inconsistency in the tree previously.

0xc0170
0xc0170 previously approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall any detail why we added another config which led to this confusion. It looks fine to me as it is now.

@LDong-Arm
Copy link
Contributor

Thanks, it makes things a lot more consistent.

It'd be good to have a similar PR to use target.mbed_rom_size and target.mbed_ram_size for all targets instead of hardcoding memory sizes in linker scripts, but that could be a significant amount of work.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 19, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Aug 19, 2020

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-ARM ❌

@mergify mergify bot added needs: work and removed needs: CI labels Aug 19, 2020
@mergify mergify bot dismissed stale reviews from hugueskamba and 0xc0170 August 20, 2020 11:34

Pull request has been modified.

@mergify mergify bot removed the needs: CI label Aug 26, 2020
@mbed-ci
Copy link

mbed-ci commented Aug 26, 2020

Jenkins CI Test : ❌ FAILED

Build Number: 6 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ❌
jenkins-ci/mbed-os-ci_build-ARM ✔️

@Patater
Copy link
Contributor Author

Patater commented Aug 27, 2020

CI failure is:

Build failures:
  * NUCLEO_G031K8::GCC_ARM::TESTS-MBED_HAL-MINIMUM_REQUIREMENTS
        Building project minimum_requirements (NUCLEO_G031K8, GCC_ARM)
        Scan: GCC_ARM
        Scan: minimum_requirements
        Compile [100.0%]: main.cpp
        Link: minimum_requirements
        /usr/local/gcc-arm-none-eabi-9-2019-q4-major/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld:./BUILD/tests/NUCLEO_G031K8/GCC_ARM/./TESTS/mbed_hal/minimum_requirements/.link_script.ld:90 cannot move location counter backwards (from 00000000200010b8 to 0000000020001000)
        collect2: error: ld returned 1 exit status

Looks like that target ran out of RAM.

@jeromecoutant
Copy link
Collaborator

Looks like that target ran out of RAM.

Yes, see #13498

@0xc0170 0xc0170 mentioned this pull request Aug 31, 2020
Closed
@mergify
Copy link

mergify bot commented Sep 2, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@Patater Patater mentioned this pull request Sep 10, 2020
Merged
Patater and others added 3 commits September 10, 2020 10:08
@Patater
Use boot stack size from config system
39e69d3 
To allow overriding of the boot stack size from the Mbed configuration
system, consistently use MBED_CONF_TARGET_BOOT_STACK_SIZE rather than
MBED_BOOT_STACK_SIZE.

Fixes ARMmbed#10319
@hugueskamba @Patater
NRF52: Fix failure if boot stack size is not provided
2ed7403 
The same default value is provided in the GCC_ARM linker file.
@Patater
stack: armc: Workaround config passing bug
612b148 
Workaround a bug where the boot stack size configuration option is not
passed on to armlink, the Arm Compiler's linker. Prefer
MBED_CONF_TARGET_BOOT_STACK_SIZE if present, as this is what the
configuration system should provide. Fall back to MBED_BOOT_STACK_SIZE
if MBED_CONF_TARGET_BOOT_STACK_SIZE is not defined, as in the case of
buggy tools. If both MBED_CONF_TARGET_BOOT_STACK_SIZE and
MBED_BOOT_STACK_SIZE are not defined, then we fall back to a hard-coded
value provided by the linkerscript. See
ARMmbed#13474 for more information.
@Patater Patater force-pushed the conf-boot-stack-size branch from 17f55ef to 612b148 Compare September 10, 2020 09:16
@Patater
Copy link
Contributor Author

Patater commented Sep 10, 2020

Rebased on latest master branch. A few more linkerscripts were added on master which I've also updated here in the same spirit as other changes.

@Patater
Copy link
Contributor Author

Patater commented Sep 10, 2020

CI Started

@mbed-ci
Copy link

mbed-ci commented Sep 10, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 7 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️
jenkins-ci/mbed-os-ci_wisun-mesh-test ✔️

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 10, 2020

Already approved by @ARMmbed/mbed-os-core , I'll merge now

@0xc0170 0xc0170 merged commit e207719 into ARMmbed:master Sep 10, 2020
@mergify mergify bot removed the ready for merge label Sep 10, 2020
@0xc0170 0xc0170 mentioned this pull request Sep 10, 2020
Closed
@mbedmain mbedmain added release-version: 6.3.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Sep 14, 2020
@jeromecoutant jeromecoutant mentioned this pull request Sep 18, 2020
Merged
@jeromecoutant jeromecoutant mentioned this pull request Sep 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromecoutant jeromecoutant jeromecoutant left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@ashok-rao ashok-rao Awaiting requested review from ashok-rao

@bentcooke bentcooke Awaiting requested review from bentcooke

@maclobdell maclobdell Awaiting requested review from maclobdell

@evedon evedon Awaiting requested review from evedon

@hugueskamba hugueskamba Awaiting requested review from hugueskamba

Assignees
No one assigned
Labels
priority: high release-version: 6.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Question on MBED_BOOT_STACK_SIZE
8 participants
@Patater @ciarmcom @hugueskamba @0xc0170 @LDong-Arm @mbed-ci @jeromecoutant @mbedmain