Skip to content

Conversation

hugueskamba
Copy link
Collaborator

Summary of changes

The heap size was incorrectly calculated.
This fixes it by subtracting the Stack size, any memory chunks allocated
before the start of the application (for vectors and/or crash report), and
finally the size of the application from the total RAM size.

Impact of changes

Migration actions required

Documentation


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


The heap size was incorrectly calculated.
This fixes it by subtracting the Stack size, any memory chunks allocated
before the start of the application (for vectors and/or crash report), and
finally the size of the application from the total RAM size.
@hugueskamba hugueskamba requested review from jeromecoutant and a team January 8, 2021 18:43
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jan 8, 2021
@ciarmcom ciarmcom requested a review from a team January 8, 2021 19:00
@ciarmcom
Copy link
Member

ciarmcom commented Jan 8, 2021

@hugueskamba, thank you for your changes.
@ARMmbed/team-st-mcd @ARMmbed/mbed-os-maintainers please review.

@mergify mergify bot added needs: CI and removed needs: review labels Jan 11, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 11, 2021

Ci started

@mbed-ci
Copy link

mbed-ci commented Jan 11, 2021

Jenkins CI Test : ✔️ SUCCESS

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-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit 331cebe into ARMmbed:master Jan 11, 2021
@mergify mergify bot removed the ready for merge label Jan 11, 2021
@hugueskamba hugueskamba deleted the hk_heap_size_fix_stm branch January 11, 2021 16:32
Copy link
Contributor

@LDong-Arm LDong-Arm left a comment

Choose a reason for hiding this comment

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

@hugueskamba
Sorry for the late review. As per my comments, two targets have specially memory layouts, and from my understanding their calculations were correct before this PR.

}

ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_RAM_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) {
ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_IRAM2_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This target has a very different memory layout - in fact, it has two RAMs at two distinct locations (see all lines above):

  • RAM1 (address: MBED_RAM_START, size: MBED_RAM_SIZE):
    • some part of static memory (see comment at line 88)
    • heap (what we are calculating here)
    • stack (== RAM_FIXED_SIZE, defined at line 86)
  • RAM2 (address: MBED_RAM2_START, size: MBED_RAM2_SIZE):
    • vector
    • crash report
    • remaining part of static memory (that didn't fit into RAM1), starting at MBED_IRAM2_START (they call that...)

So the old calculation was correct for this particular target.

Suggested change
ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_IRAM2_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) {
ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_RAM_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) {

}

ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_RAM_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) { ; Heap growing up
ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_IRAM2_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) { ; Heap growing up
Copy link
Contributor

@LDong-Arm LDong-Arm Jan 12, 2021

Choose a reason for hiding this comment

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

Similarly, this targets has two RAMs at two distinct locations. Some of the macros are defined in https://github.com/ARMmbed/mbed-os/blob/mbed-os-6.6.0/targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L452xE/cmsis_nvic.h

See my other comment. I think the old calculation is correct here.

Suggested change
ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_IRAM2_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) { ; Heap growing up
ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY (MBED_RAM_SIZE-RAM_FIXED_SIZE+MBED_RAM_START-AlignExpr(ImageLimit(RW_IRAM1), 16)) { ; Heap growing up

@mbedmain mbedmain added release-version: 6.7.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants