Skip to content

NANO130: Fix OOM with packing algorithm at IAR linking #12047

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

Merged
merged 2 commits into from
Dec 9, 2019

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 commented Dec 9, 2019

Summary of changes

This PR tries to address #12033 (comment) for NUMAKER_PFM_NANO130 target:

  1. At IAR linking, the default method of initialize by copy is auto, which will estimate different packing algorithms, including complex lz77, for smallest memory footprint. But the algorithm itself can require some SRAM overhead and cause OOM at linking stage for small SRAM target like NANO130, which just has 16KiB SRAM. To avoid this error on NANO130, always choose none packing algorithm.
  2. On IAR, heap configuration is hard-coded and cannot be adjusted according to static SRAM usage. Reduce heap configuration to spare more SRAM for static allocation.

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

@mprse @0xc0170

At IAR linking, the default method of 'initialize by copy' is 'auto', which will estimate
different packing algorithms, including complex 'lz77', for smallest memory footprint. But
the algorithm itself can consume some SRAM and cause OOM at linking time for NANO130, which
just has 16KiB SRAM. To avoid this error, always choose 'none' packing algorithm.
@ciarmcom
Copy link
Member

ciarmcom commented Dec 9, 2019

@ccli8, thank you for your changes.
@mprse @0xc0170 @Ronny-Liu @ARMmbed/mbed-os-maintainers please review.

This adjustment is to pass compile on IAR on which heap configuration is hard-coded and cannot be adjusted according to staic SRAM usage.
@ccli8 ccli8 force-pushed the nuvoton_nano130_fix_iar branch from 989888f to 3e4e062 Compare December 9, 2019 10:58
Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

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

I can confirm that this solves the problem found in PR #12033.

@ccli8 Thanks for help!

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

On IAR, heap configuration is hard-coded and cannot be adjusted according to static SRAM usage. Reduce heap configuration to spare more SRAM for static allocation.

We haven't yet enabled dynamic, have we ? 👀 cc @mprse

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

As there is a dependancy on this one for 5.15rc2, this one was set to the same version 5.15rc2

cc @adbridge

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

@ccli8 see https://github.com/ARMmbed/mbed-os/pull/10938/files , can this target one as well?

Change line 24:

define block HEAP with alignment = 8, size = __ICFEDIT_size_heap__ { };
, to use dynamic heap.

@@ -24,7 +24,7 @@ define block CSTACK with alignment = 8, size = __ICFEDIT_size_cstack__ { };
define block HEAP with alignment = 8, size = __ICFEDIT_size_heap__ { };
Copy link
Contributor

@0xc0170 0xc0170 Dec 9, 2019

Choose a reason for hiding this comment

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

Suggested change
define block HEAP with alignment = 8, size = __ICFEDIT_size_heap__ { };
define block HEAP with expanding size, alignment = 8, minimum size = __ICFEDIT_size_heap__ { };

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't commit to the branch, this should do the trick

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

Dynamic heap change can be done separately, the PR as it is should fix the issue (heap will be set as minimum once we get dynamic heap there) and unblock other PRs . Starting CI now

@mbed-ci
Copy link

mbed-ci commented Dec 9, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 removed the needs: CI label Dec 9, 2019
@0xc0170 0xc0170 merged commit 66eb594 into ARMmbed:master Dec 9, 2019
@ccli8 ccli8 deleted the nuvoton_nano130_fix_iar branch December 10, 2019 03:04
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.

5 participants