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

Update devices to have minimum 2K RAM and heap, also added test #5285

Merged
merged 4 commits into from Oct 12, 2018

Conversation

@c1728p9
Contributor

c1728p9 commented Oct 9, 2017

Update the linker scripts (memory map) of targets to have atleast 2K free ram and 2K free heap.
Add a test to ensure the same, this test should be the first test that fails due to running out of ram or heap.

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 9, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 10, 2017

Looks good. Are the numbers 'random' or did you pick them based on something?

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 12, 2017

Looks good. Are the numbers 'random' or did you pick them based on something?

👍 Documented anywhere? should be ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 18, 2017

The latest build shows devices that fail this test

KL25Z/
NUCLEO_F070RB/
NUCLEO_F072RB/
SARA_NBIOT_EVK/
TMPM066/

Any other feedback for this test and requirement? Please review!

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 18, 2017

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 24, 2017

@c1728p9 @bulislaw @0xc0170
This test likely eliminates all devices with RAM <= 16kB (at least for IAR and GCC_ARM compilers)
Below output for NUCLEO_F070RB

NUCLEO_F070RB - ARM: pass
NUCLEO_F070RB - GCC_ARM: fail

...
[1508831066.26][CONN][RXD]
[1508831066.26][CONN][INF] found KV pair in stream: {{__testcase_name;Minimum data test}}, queued...
[1508831066.30][CONN][RXD] >>> Running case #1: 'Minimum heap test'...
[1508831066.33][CONN][INF] found KV pair in stream: {{__testcase_start;Minimum heap test}}, queued...
[1508831066.36][CONN][RXD] :33::FAIL: Expected Not-Equal
[1508831066.41][CONN][INF] found KV pair in stream: {{__testcase_finish;Minimum heap test;0;1}}, queued...
[1508831066.49][CONN][RXD] >>> 'Minimum heap test': 0 passed, 1 failed with reason 'Assertion Failed'
[1508831066.49][CONN][RXD]
[1508831066.57][CONN][RXD] >>> Test cases: 0 passed, 1 failed with reason 'Assertion Failed'
[1508831066.58][CONN][RXD] >>> TESTS FAILED!
...

NUCLEO_F070RB - IAR: fail

...
Link: minimum_requirements
[DEBUG] Link: C:\Program Files (x86)\IAR Systems\Embedded Workbench 7.5\arm\bin\ilinkarm -f C:\Users\MacBoc01\work\mbed\dev\mbed_os_my_fork\BUILD\tests\nucleo_f070rb\IAR\.\TESTS\mbed_hal\minimum_requirements\.link_files.txt
[DEBUG] Return: 2
[DEBUG] Output:
[DEBUG] Output:    IAR ELF Linker V7.80.4.12462/W32 for ARM
[DEBUG] Output:    Copyright 2007-2017 IAR Systems AB.
[DEBUG] Output:
[DEBUG] Output:   37 144 bytes of readonly  code memory
[DEBUG] Output:    1 643 bytes of readonly  data memory
[DEBUG] Output:   16 543 bytes of readwrite data memory
[DEBUG] Output:
[DEBUG] Output: Errors: 2
[DEBUG] Output: Warnings: none
[DEBUG] Output:
[DEBUG] Output: Link time:   0.28 (CPU)   0.28 (elapsed)
[DEBUG] Errors: Error[Lp011]: section placement failed
[DEBUG] Errors:             unable to allocate space for sections/blocks with a total estimated
[DEBUG] Errors:                       minimum size of 0x40a8 bytes (max align 0x8) in
[DEBUG] Errors:                       <[0x200000c0-0x20003fff]> (total uncommitted space
[DEBUG] Errors:                       0x3f40).
[DEBUG] Errors: Error[Lp021]: the destination for compressed initializer batch "P2-1" is placed
[DEBUG] Errors:           at an address that is dependent on the size of the batch, which is
[DEBUG] Errors:           not allowed when using packbits compression. Consider using
[DEBUG] Errors:           "initialize by copy with packing = zeros" (or none) instead.
...
@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 24, 2017

Maybe we should require 1k on heap?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 24, 2017

Maybe we should require 1k on heap?

We see some failures with IAR, updaring IAR to use dynamic heap for this upcoming minor release will resolve this.

1k heap + 2k RAM

I am wondering if a device with 16k does not have 4k RAM left, if it is a target that should be supported? What others think?

@jeromecoutant Where is all RAM for NUCLEO_F070RB that GCC fails this test?

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 30, 2017

Hi
Maybe we can link #5386 with this PR ?

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 30, 2017

If #5386 is a real bug and we will get at least 1kB of heap back then most of memory problems with basic tests on devices with small RAM will disappear

@c1728p9 c1728p9 force-pushed the c1728p9:minimum_requirements_test branch to e6faa68 Oct 30, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 30, 2017

I rebased this PR and added some comments to the PR to indicate the purpose of this test.

Looks good. Are the numbers 'random' or did you pick them based on something?

I tried to pick numbers that were on the higher end on most tests. These numbers aren't based on any hard requirement or anything though. If they need to be increased or decreased its not a problem.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 30, 2017

/morph build

1 similar comment
@studavekar

This comment has been minimized.

Collaborator

studavekar commented Oct 31, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 31, 2017

@theotherjimmy

theotherjimmy approved these changes Nov 6, 2017 edited

Approved by @bulislaw

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 8, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 8, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 9, 2017

Failures are for few targets, only IAR toolchain. I am wondering if updating to IAR 8.x,dynamic heap resolves this to get some space back?

From the log, these are offending boards:

KL25Z/
NUCLEO_F070RB/
NUCLEO_F072RB/
NUMAKER_PFM_NANO130/
SARA_NBIOT_EVK/
TMPM066/

I checked few, failing to allocate around 1k in readwrite region.

@theotherjimmy theotherjimmy added needs: work and removed needs: CI labels Nov 13, 2017

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Dec 5, 2017

Hi

these are offending boards:
NUCLEO_F070RB/
NUCLEO_F072RB/

What are next steps:

  • correct #5386 ?
  • remove OS5 capability of these boards ?
  • change IAR version ?
@cmonr

This comment has been minimized.

Contributor

cmonr commented Dec 7, 2017

@c1728p9 ^^^

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Dec 12, 2017

This may need to wait for a new version of IAR. I can close this for now if you would like, or I can leave this PR up as a reminder.

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 8, 2018

NUCLEO_F070RB test:

| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbedmicro-rtos-mbed-heap_and_stack | FAIL | 18.81 | default |

Test was OK with 5.10.0

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Oct 8, 2018

@jeromecoutant - Please share the test log. I dont have NUCLEO_F070RB :-( , but have NUCLEO_F072RB.

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 8, 2018

Same result with NUCLEO_F072RB, test is failing here :

TEST_ASSERT_TRUE_MESSAGE(result, "Interrupt stack in wrong location");

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 8, 2018

@jeromecoutant Would toy mind providing steps to reproduce?

For myself, running the following all succeeded without issues:

git clone https://github.com/ARMmbed/mbed-os
cd mbed-os
git fetch origin refs/pull/5285/head:pr5285
git checkout pr5285
mbed test --compile -t GCC_ARM -m NUCLEO_F072RB
@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 9, 2018

@cmonr , maybe I was not explicit enough.
With this fix, every tests are OK,
including the new test from this PR,
but not tests-mbedmicro-rtos-mbed-heap_and_stack

So I would appreciate to check this regression before merging PR

Thx

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 9, 2018

Did some additional tests with NUCLEO_F070RB and tests-mbedmicro-rtos-mbed-heap_and_stack with the below code. With pure master and master + the changes from this PR

I can see following problems:

diff --git a/TESTS/mbedmicro-rtos-mbed/heap_and_stack/main.cpp b/TESTS/mbedmicro-rtos-mbed/heap_and_stack/main.cpp
index d4334ad6f..b72daec4a 100644
--- a/TESTS/mbedmicro-rtos-mbed/heap_and_stack/main.cpp
+++ b/TESTS/mbedmicro-rtos-mbed/heap_and_stack/main.cpp
@@ -54,8 +54,70 @@ struct linked_list {
     uint8_t data[MALLOC_TEST_SIZE];
 };

+#define MEM_LOG

+#ifdef MEM_LOG
+volatile uint32_t isr_variable1_addr = 0;
+volatile uint32_t isr_variable2_addr = 0;
+volatile uint32_t isr_variable3_addr = 0;

+void fun()
+{
+    uint8_t isr_variable1 = 1;
+    uint8_t isr_variable2 = 2;
+    uint8_t isr_variable3 = 3;
+    isr_variable1_addr = (uint32_t)&isr_variable1;
+    isr_variable2_addr = (uint32_t)&isr_variable2;
+    isr_variable3_addr = (uint32_t)&isr_variable3;
+
+}
+
+void test_log()
+{
+    mbed_rtos_storage_thread_t *thread = (mbed_rtos_storage_thread_t *) osThreadGetId();
+
+    uint32_t psp = __get_PSP();
+    uint8_t *stack_mem = (uint8_t *) thread->stack_mem;
+    uint32_t stack_size = thread->stack_size;
+    uint8_t main_stack_variable1 = 3;
+    uint8_t main_stack_variable2 = 2;
+    uint8_t main_stack_variable3 = 3;
+
+    utest_printf("mbed_main_stack_size: %X\r\n", stack_size);
+    utest_printf("%X: mbed_main_stack_start\r\n", (uint32_t)stack_mem);
+    utest_printf("%X: psp\r\n", psp);
+    utest_printf("%X: main_stack_variable1_addr\r\n", (uint32_t)&main_stack_variable1);
+    utest_printf("%X: main_stack_variable2_addr\r\n", (uint32_t)&main_stack_variable2);
+    utest_printf("%X: main_stack_variable3_addr\r\n", (uint32_t)&main_stack_variable3);
+    utest_printf("%X: mbed_main_stack_end\r\n", (uint32_t)stack_mem + stack_size);
+
+    utest_printf("mbed_heap_size: %X\r\n", mbed_heap_size);
+    utest_printf("%X: mbed_heap_start\r\n", (uint32_t)mbed_heap_start);
+    uint8_t *heap_variable1_addr = (uint8_t*)malloc(1);
+    utest_printf("%X: heap_variable1_addr\r\n", (uint32_t)heap_variable1_addr);
+    uint8_t *heap_variable2_addr = (uint8_t*)malloc(1);
+    utest_printf("%X: heap_variable2_addr\r\n", (uint32_t)heap_variable2_addr);
+    uint8_t *heap_variable3_addr = (uint8_t*)malloc(1);
+    utest_printf("%X: heap_variable3_addr\r\n", (uint32_t)heap_variable3_addr);
+    utest_printf("%X: mbed_heap_end\r\n", (uint32_t)mbed_heap_start + mbed_heap_size);
+    free(heap_variable1_addr);
+    free(heap_variable2_addr);
+    free(heap_variable3_addr);
+
+    utest_printf("mbed_stack_isr_size: %X\r\n", mbed_stack_isr_size);
+    utest_printf("%X: mbed_stack_isr_start\r\n", mbed_stack_isr_start);
+
+    Timeout t;
+    t.attach_us(fun, 1);
+    while (isr_variable3_addr == 0);
+    utest_printf("%X: isr_variable1_addr\r\n", isr_variable1_addr);
+    utest_printf("%X: isr_variable2_addr\r\n", isr_variable2_addr);
+    utest_printf("%X: isr_variable3_addr\r\n", isr_variable3_addr);
+    uint32_t msp = __get_MSP();
+    utest_printf("%X: msp\r\n", msp);
+    utest_printf("%X: mbed_stack_isr_end\r\n", (uint32_t)mbed_stack_isr_start + mbed_stack_isr_size);
+
+
+}
+#endif
 /* TODO: add memory layout test.
  *
  * The test was skipped for now since not all devices seems to comply with Mbed OS memory.
@@ -234,6 +296,9 @@ void test_heap_allocation_free(void)

 // Test cases
 Case cases[] = {
+#ifdef MEM_LOG
+    Case("test_log", test_log),
+#endif
     Case("Test heap in range", test_heap_in_range),
     Case("Test main stack in range", test_main_stack_in_range),
     Case("Test isr stack in range", test_isr_stack_in_range),

Results:

no fix (pure master)

mbed_main_stack_size: 1000
20001B90: mbed_main_stack_start
20002A80: psp
20002A8D: main_stack_variable1_addr
20002A8E: main_stack_variable2_addr
20002A8F: main_stack_variable3_addr
20002B90: mbed_main_stack_end


mbed_heap_size: DF0
20002E10: mbed_heap_start
20002E20: heap_variable1_addr
20002E30: heap_variable2_addr
20002E40: heap_variable3_addr
20003C00: mbed_heap_end


mbed_stack_isr_size: 400
20003C00: mbed_stack_isr_start
20003F75: isr_variable1_addr
20003F76: isr_variable2_addr
20003F77: isr_variable3_addr
20003FC0: msp
20004000: mbed_stack_isr_end
with fix

mbed_main_stack_size: C00
20001F90: mbed_main_stack_start
20002A80: psp
20002A8D: main_stack_variable1_addr
20002A8E: main_stack_variable2_addr
20002A8F: main_stack_variable3_addr
20002B90: mbed_main_stack_end


mbed_heap_size: DF0
20002E10: mbed_heap_start
20002E20: heap_variable1_addr
20002E30: heap_variable2_addr
20002E40: heap_variable3_addr
20003C00: mbed_heap_end


mbed_stack_isr_size: 400
20003C00: mbed_stack_isr_start
20000FF5: isr_variable1_addr
20000FF6: isr_variable2_addr
20000FF7: isr_variable3_addr
20001040: msp
20004000: mbed_stack_isr_end
@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 9, 2018

Just a comment about tested fix: it is maybe now far from the original PR... "Add a minimum requirements HAL test"
Maybe this PR has to be split into STM32 fix and new test addition ?

Reposition heap at the end of RAM to be 4K aligned
HEAP memory should be 4K aligned for GCC newlib, with ISR stack at the end of
RAM memory we loose 3K of RAM memory. This fix is for device with <16K RAM to
use RAM entirely.

@deepikabhavnani deepikabhavnani force-pushed the c1728p9:minimum_requirements_test branch from 56004bd to 37bfc9e Oct 10, 2018

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Oct 10, 2018

Fixed tests-mbedmicro-rtos-mbed-heap_and_stack, defines ISR_STACK_START and ISR_STACK_SIZE were missed out, setting them to actual stack values instead defaults.

@jeromecoutant - Fix is not just for STM targets, but for all devices with low RAM/ROM memories hence will like to keep it in same PR as it is related and will help in tracking specific targets fixed for this requirement. Also commits are different, and I will update the header as well.

@deepikabhavnani deepikabhavnani changed the title from Add a minimum requirements HAL test to Update devices to have minimum 2K RAM and heap, also added test Oct 10, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 11, 2018

Hi
Is it possible to rebase now that #8013 has been merged ?
Thx

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 11, 2018

After last fix mbed-heap_and_stack test is OK

+-----------------------+---------------+------------------------------------------+-------------------------------+--------+--------+--------+--------------------+
| target                | platform_name | test suite                               | test case                     | passed | failed | result | elapsed_time (sec) |
+-----------------------+---------------+------------------------------------------+-------------------------------+--------+--------+--------+--------------------+
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbedmicro-rtos-mbed-heap_and_stack | Test heap allocation and free | 1      | 0      | OK     | 0.11               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbedmicro-rtos-mbed-heap_and_stack | Test heap in range            | 1      | 0      | OK     | 0.04               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbedmicro-rtos-mbed-heap_and_stack | Test isr stack in range       | 1      | 0      | OK     | 0.06               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbedmicro-rtos-mbed-heap_and_stack | Test main stack in range      | 1      | 0      | OK     | 0.05               |
| NUCLEO_F070RB-GCC_ARM | NUCLEO_F070RB | tests-mbedmicro-rtos-mbed-heap_and_stack | test_log                      | 1      | 0      | OK     | 0.64               |
+-----------------------+---------------+------------------------------------------+-------------------------------+--------+--------+--------+--------------------+

@cmonr cmonr added needs: CI and removed needs: work labels Oct 11, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 11, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2018

Build : SUCCESS

Build number : 3323
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5285/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@adbridge adbridge self-requested a review Oct 12, 2018

@adbridge adbridge merged commit 24857d0 into ARMmbed:master Oct 12, 2018

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 656 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9167 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment