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

Adjust Stack size Allocation (IAR, LPC176x) #4924

Merged
merged 1 commit into from Sep 5, 2017

Conversation

Projects
None yet
8 participants
@hasnainvirk
Contributor

hasnainvirk commented Aug 17, 2017

Since mbed-os 5.4.3, something increased foot print of mbed-os and the applications
that were barely fitting in started to spill.

IAR toolchain for LPC176x target family is set to use 2 RAM regions (32K each). RAM region
2 is being used for ETH/USB and 1 is being used for vector table, stack/heap/static data.

In this commit we have decreased heap size allocation from 8K to 7K so that the is more room for static data.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:iar_fix_c027 branch 6 times, most recently Aug 17, 2017

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:iar_fix_c027 branch 3 times, most recently Aug 17, 2017

targets/TARGET_NXP/TARGET_LPC176X/device/TOOLCHAIN_IAR/LPC17xx.icf Outdated
/*Heap 1/4 of ram and stack 1/8*/
define symbol __ICFEDIT_size_cstack__ = 0x1000;
define symbol __ICFEDIT_size_heap__ = 0x2000;
/*Heap 1/4 of ram and stack 1/4*/

This comment has been minimized.

@0xc0170

0xc0170 Aug 17, 2017

Member

is this comment true now with these update? Do we really need almost 2x bigger ISR stack (that is cstack as I recall) ?

I am still hoping IAR to add the feature for dynamic sized heap, checked the last time this week release notes, have not found it yet there 🙏

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:iar_fix_c027 branch 2 times, most recently Aug 17, 2017

@hasnainvirk hasnainvirk changed the title from Adjusting Stack/Heap Allocation (IAR, LPC176x) to Adjusting Stack size Allocation (IAR, LPC176x) Aug 17, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 21, 2017

@0xc0170 who is going to review this ? Its a blocker for mbed-os-example-cellular. I don't have permission to add reviewers.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

It looks fine to me, just wondering if it is really required to increase ISR STACK for iar by 0x800? Does celullar use that much?

cc @c1728p9

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 21, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1057

Example Build failed!

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 21, 2017

@0xc0170 Isn't that a fashion now to put stuff on stack rather than heap so that we can track memory allocations better ?
The example was barely fitting in (we have 48K available on C027 not the complete 64) previously. Something in mbed-os increased the utilisation of stack and it blew apart. I want to keep some slack now.

@theotherjimmy theotherjimmy changed the title from Adjusting Stack size Allocation (IAR, LPC176x) to Adjust Stack size Allocation (IAR, LPC176x) Aug 21, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 21, 2017

Hi @hasnainvirk, most targets have an interrupt stack of 1K. In this PR you are increasing the interrupt stack from 4k to 6k. If that much stack is actually being used, then the application you are using will crash on other devices. What makes you think the interrupt stack is too small? Are you seeing a crash?

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 22, 2017

@c1728p9 I don't think it is interrupt stack.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 22, 2017

@0xc0170 morph test show a failure for DISCO_F769NI for blinky and socket examples.
I tried on my local machine, they work. Below is the snippet from sockets example.
Can you please check what happened in morph test ?

Link: mbed-os-example-sockets
Elf2Bin: mbed-os-example-sockets
+----------------------+-------+-------+-------+
| Module               | .text | .data |  .bss |
+----------------------+-------+-------+-------+
| [lib]/dl7M_tlf.a     | 16180 |   620 |   716 |
| [lib]/dlpp7M_tl_fc.a |    40 |     0 |     0 |
| [lib]/m7M_tlv.a      |  1482 |     0 |     0 |
| [lib]/rt7M_tl.a      |  1170 |     0 |     0 |
| [misc]               |   268 |     0 |     0 |
| main.o               |   524 |     0 |    56 |
| mbed-os/drivers      |    72 |     0 |   100 |
| mbed-os/features     | 48468 |   334 | 46786 |
| mbed-os/hal          |   524 |     0 |    24 |
| mbed-os/platform     |  1302 |     0 |   189 |
| mbed-os/rtos         | 11766 |   180 |  6120 |
| mbed-os/targets      | 11640 |     4 |  1120 |
| Subtotals            | 93436 |  1138 | 55111 |
+----------------------+-------+-------+-------+
Total Static RAM memory (data + bss): 56249 bytes
Total Flash memory (text + data): 94574 bytes

Here is the build result for blinky example:

$ mbed compile -m DISCO_F769NI -t IAR
Building project mbed-os-example-blinky (DISCO_F769NI, IAR)
Scan: .
Scan: mbed
Scan: env
Scan: FEATURE_LWIP
+----------------------+-------+-------+-------+
| Module               | .text | .data |  .bss |
+----------------------+-------+-------+-------+
| [lib]/dl7M_tlf.a     |  4316 |   364 |   216 |
| [lib]/dlpp7M_tl_fc.a |    40 |     0 |     0 |
| [lib]/m7M_tlv.a      |   580 |     0 |     0 |
| [lib]/rt7M_tl.a      |   868 |     0 |     0 |
| [misc]               |   189 |     0 |     0 |
| main.o               |    68 |     0 |    28 |
| mbed-os/drivers      |    72 |     0 |   100 |
| mbed-os/features     |    44 |     0 | 12604 |
| mbed-os/hal          |   588 |     0 |    24 |
| mbed-os/platform     |   736 |     0 |   133 |
| mbed-os/rtos         |  9130 |   180 |  6120 |
| mbed-os/targets      |  8804 |     4 |  1120 |
| Subtotals            | 25435 |   548 | 20345 |
+----------------------+-------+-------+-------+
Total Static RAM memory (data + bss): 20893 bytes
Total Flash memory (text + data): 25983 bytes

My IAR version:

$ iccarm --version
IAR ANSI C/C++ Compiler V7.80.1.11864/W32 for ARM
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2017

@c1728p9 I don't think it is interrupt stack.

It is for IAR, therefore I asked as well. The default value ~1k should be sufficient.

For IAR:

/* Interrupt stack and heap always defined for IAR
 * Main thread defined here
 */
#if defined(__ICCARM__)
    #pragma section="CSTACK"
    #pragma section="HEAP"
    #define HEAP_START          ((unsigned char*)__section_begin("HEAP"))
    #define HEAP_SIZE           ((uint32_t)__section_size("HEAP"))
    #define ISR_STACK_START     ((unsigned char*)__section_begin("CSTACK"))
    #define ISR_STACK_SIZE      ((uint32_t)__section_size("CSTACK"))
#endif
@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 22, 2017

@0xc0170 Interesting. Then why was it set to 4K at the first place ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2017

@0xc0170 Interesting. Then why was it set to 4K at the first place ?

This target has not changed for some time. For mbed 2, this number defines actually a stack.

We are not certain from the above description what is happening with the cellular ? And how 6K ISR stack fixes it

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 22, 2017

@0xc0170 It needs half a K for serial buffers , then it uses LWIP ..
Anyway, I think I can reduce 1 K from heap and that will do ..

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:iar_fix_c027 branch Aug 22, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 22, 2017

@0xc0170 @c1728p9 Please review again.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Aug 24, 2017

@0xc0170 are you happy with the review changes ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2017

Should be fine. This however will be replaced soon (IAR 8.x update and then IAR dynamic heap patch).

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 31, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 31, 2017

@0xc0170 Yeah I heard about the patch. However, the thing it's a blocker for mbed-os-example-cellular. This particular example cannot be tagged to current mbed-os release as it doesn't build for IAR.
My be this can be merged and after IAR patch, all target linker files will be updated anyway to use dynamic heap and stack, and then everyone will live happily ever after :)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 1, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 1, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1157

Test failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 2, 2017

Failure for ARCH_PRO looks legit. Re-triggering

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 2, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1162

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@hasnainvirk Seems like ARCH PRO is affected, and threads test fails for it due to this change, please review the results above

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Sep 4, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 4, 2017

:) The test creates 3 threads with children in parallel(each with 768 B of stack size) and they are dynamically allocated and total heap allocation was 3K. I will send a patch through shortly.

Adjusting Stack size Allocation (IAR, LPC176x)
Since mbed-os 5.4.3, something increased foot print of mbed-os and the applications that were barely fitting in started to spill.

IAR toolchain for LPC176x target family is set to use 2 RAM regions (32K each). RAM region
2 is being used for ETH/USB and 1 is being used for vector table, stack/heap/static data.

In this commit we have decreased heap size allocation from 8K to 7K so that the is more room for stack and static data.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:iar_fix_c027 branch to fce2ca2 Sep 4, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 4, 2017

@0xc0170 A typo caused this issue. The idea was to take 1K off from the heap. Originally heap was 0x2000, i.e., 8K
Instead of 0x1C00, I typed 0xC00 which eventually took 5Ks off :D. Fun part is that I wrote in comment that I took 1K off i.e., from 4 to 3K :) silly me

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 4, 2017

@studavekar Can you please kick the morph test again ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Sep 4, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 5, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1211

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 5, 2017

@theotherjimmy theotherjimmy merged commit 2f2da77 into ARMmbed:master Sep 5, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 6, 2017

@hasnainvirk This relatively small decrease in heap results in lwip two tests failing to allocate memory for ARCH PRO. IAR will get soon the dynamic heap support so this PR will become obsolete. I'll talk to you now now, to resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment