stack_size_unification test: set expected stack sizes from config #12979

LDong-Arm · 2020-05-15T14:02:36Z

Summary of changes

Use macros defined by the build system to set expected stack sizes in stack_size_unification test so that

we do not need to maintain hardcoded values
ISR stack size is determined in the same way for bare metal and RTOS
there is no target-dependency.

Credits to @jeromecoutant for raising this.

Impact of changes

The test passes even if a target (i.e. one that has low memory) overrides the default stack sizes.

Migration actions required

Documentation

None.

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)
[x] Tests / results supplied as part of this PR

Tested on

NRF52840_DK with target.boot-stack-size manually changed, with both the full profile and the bare metal profile.
LPC11U24 (low-memory target) with the bare metal profile. (This target is unsupported now but I copied its config + drivers from an older Mbed OS).

Reviewers

@evedon @ARMmbed/mbed-os-core @mprse @MarceloSalazar

…b.json

LDong-Arm · 2020-05-15T14:22:59Z

targets/TARGET_TOSHIBA/mbed_rtx.h

-#ifdef MBED_CONF_RTOS_MAIN_THREAD_STACK_SIZE
-#undef MBED_CONF_RTOS_MAIN_THREAD_STACK_SIZE
-#endif
-#define MBED_CONF_RTOS_MAIN_THREAD_STACK_SIZE 3072


Note: This affects only TMPM46B - see a few lines above.
We remove this macro-redefinition and override it properly in rtos/source/TARGET_CORTEX/mbed_lib.json.

jeromecoutant

| NUCLEO_F303K8-ARMC6 | NUCLEO_F303K8 | tests-mbed_hal-stack_size_unification | Stack size unification test | 1 | 0 | OK | 0.06 |

LDong-Arm · 2020-05-15T14:24:33Z

TESTS/mbed_hal/stack_size_unification/main.cpp


-#if defined(TARGET_NUCLEO_F070RB) || defined(TARGET_STM32F072RB) || defined(TARGET_TMPM46B) || defined(TARGET_TMPM066)


All of NUCLEO_F070RB, STM32F072RB and TMPM46B have thread stack sizes overriden in rtos/source/TARGET_CORTEX/mbed_lib.json, so we don't need target-dependent code here. As for TARGET_TMPM066, it's not supported anymore).

LDong-Arm · 2020-05-15T14:25:32Z

| NUCLEO_F303K8-ARMC6 | NUCLEO_F303K8 | tests-mbed_hal-stack_size_unification | Stack size unification test | 1 | 0 | OK | 0.06 |

Thanks for testing!

ciarmcom · 2020-05-15T15:00:41Z

@LDong-Arm, thank you for your changes.
@evedon @MarceloSalazar @mprse @ARMmbed/mbed-os-core @ARMmbed/mbed-os-test @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

MarceloSalazar · 2020-05-15T15:47:31Z

rtos/source/TARGET_CORTEX/mbed_lib.json

@@ -76,6 +76,9 @@
        "STM32F072RB": {
            "main-thread-stack-size": 3072
        },
+        "TMPM46B": {
+            "main-thread-stack-size": 3072
+        },


Do we really need to specify this per target?
Or can we set the stack-size depending on the total RAM? (which would be target independent)
EDIT: and of course let the user override it in mbed_app.json if they want

Checking RAM size is ideal, and we do have target.mbed_ram_size for this purpose.

The trouble is, we only have it defined for a few targets. For most other targets, the RAM and ROM sizes are hardcoded in the linker scripts (e.g. scatter files) - only the linker knows the RAM, not the application/libraries/...

Well, 17 targets have it defined: https://github.com/ARMmbed/mbed-os/blob/master/targets/targets.json
Okay it's more than a few, but still far from a good coverage.

Agree, thanks for clarifying it
This PR is great anyway as it helps us with some clean-up. More could be done in the future.

Personally I'd like to see the sizes defined - it can help us to disable certain Greentea test cases for ultra-constrained targets for which we even struggle to compile some very basic tests... @evedon

The best would be to generate the linker script (having set per toolchain and that is it) so having the size defined is one way forward there :)

evedon

That's great @LDong-Arm. Thanks for the PR.

0xc0170 · 2020-05-19T09:11:10Z

CI started

LDong-Arm · 2020-05-19T10:52:09Z

CI failed with

tools.utils.NotSupportedException: Target NU_PFM_M2351_NPSA_S is not supported by toolchain GCC_ARM

Looks unrelated to this PR?

0xc0170 · 2020-05-25T10:20:31Z

CI restarted

It was a bug on master.

LDong-Arm · 2020-05-25T11:17:25Z

@0xc0170 Thanks but could you please keep this on hold? There's a slight issue I'll fix tomorrow

mbed-ci · 2020-05-25T12:04:36Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

LDong-Arm · 2020-05-26T12:54:07Z

TESTS/mbed_hal/stack_size_unification/main.cpp


-#define EXPECTED_USER_THREAD_DEFAULT_STACK_SIZE  (4096)
+#define EXPECTED_USER_THREAD_DEFAULT_STACK_SIZE  (MBED_CONF_RTOS_THREAD_STACK_SIZE)


In the test case below, we use this macro to verify OS_STACK_SIZE:

TEST_ASSERT_EQUAL(EXPECTED_USER_THREAD_DEFAULT_STACK_SIZE, OS_STACK_SIZE);

where OS_STACK_SIZE is conditional

mbed-os/rtos/source/TARGET_CORTEX/mbed_rtx_conf.h

Lines 31 to 35 in 9a8c9e2

#if defined(MBED_CONF_APP_THREAD_STACK_SIZE)

#define OS_STACK_SIZE MBED_CONF_APP_THREAD_STACK_SIZE

#else

#define OS_STACK_SIZE MBED_CONF_RTOS_THREAD_STACK_SIZE

#endif

The trouble is, to get the assertion to pass in all scenarios, our expectation needs to be conditional in the same way. But isn't it a duplication? I'm not sure there's any value in comparing two macros in a Greentea test to begin with...

In my opinion, we should remove this assertion from the test case - which is out of this PR's initial scope (i.e. fixing line 34). But still this line of diff doesn't make things worse than before (i.e. hardcoded 4096).

@0xc0170 If this PR blocks any release, maybe we can get it in as it is?

LDong-Arm · 2020-05-26T16:08:27Z

@adbridge This Greentea test fix is required for all targets with a non-default stack size, particularly bare-metal-only targets with small memories. Is it critical for the release?

adbridge · 2020-05-26T16:32:08Z

Patch updates can wait until the next release

mergify · 2020-06-04T12:40:51Z

This PR does not contain release version label after merging.

LDong-Arm added 2 commits May 15, 2020 14:42

stack_size_unification test: set expected stack sizes from config

4ecd99f

Define MBED_CONF_RTOS_MAIN_THREAD_STACK_SIZE properly in rtos mbed_li…

0ac17ea

…b.json

LDong-Arm force-pushed the stack_size_expectation_from_config branch from 73ab9a8 to 0ac17ea Compare May 15, 2020 14:16

LDong-Arm commented May 15, 2020

View reviewed changes

jeromecoutant approved these changes May 15, 2020

View reviewed changes

LDong-Arm commented May 15, 2020

View reviewed changes

ciarmcom requested review from evedon, MarceloSalazar, mprse and a team May 15, 2020 15:00

ciarmcom added the needs: review label May 15, 2020

MarceloSalazar reviewed May 15, 2020

View reviewed changes

evedon approved these changes May 18, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels May 18, 2020

0xc0170 approved these changes May 18, 2020

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels May 25, 2020

LDong-Arm commented May 26, 2020

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Jun 2, 2020

mergify bot added ready for merge and removed needs: CI labels Jun 2, 2020

0xc0170 added the release-type: patch Indentifies a PR as containing just a patch label Jun 4, 2020

0xc0170 merged commit ab43962 into ARMmbed:master Jun 4, 2020

mergify bot removed the ready for merge label Jun 4, 2020

mergify bot added the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label Jun 4, 2020

adbridge added release-version: 6.1.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 24, 2020

0xc0170 removed the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label Aug 23, 2021


		#if defined(TARGET_NUCLEO_F070RB) \|\| defined(TARGET_STM32F072RB) \|\| defined(TARGET_TMPM46B) \|\| defined(TARGET_TMPM066)


		#define EXPECTED_USER_THREAD_DEFAULT_STACK_SIZE (4096)
		#define EXPECTED_USER_THREAD_DEFAULT_STACK_SIZE (MBED_CONF_RTOS_THREAD_STACK_SIZE)

	#if defined(MBED_CONF_APP_THREAD_STACK_SIZE)
	#define OS_STACK_SIZE MBED_CONF_APP_THREAD_STACK_SIZE
	#else
	#define OS_STACK_SIZE MBED_CONF_RTOS_THREAD_STACK_SIZE
	#endif

stack_size_unification test: set expected stack sizes from config #12979

stack_size_unification test: set expected stack sizes from config #12979

Uh oh!

Conversation

LDong-Arm commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

LDong-Arm May 15, 2020

Choose a reason for hiding this comment

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

LDong-Arm May 15, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm commented May 15, 2020

Uh oh!

ciarmcom commented May 15, 2020

Uh oh!

MarceloSalazar May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarceloSalazar May 15, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 May 18, 2020

Choose a reason for hiding this comment

Uh oh!

evedon left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented May 19, 2020

Uh oh!

LDong-Arm commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented May 25, 2020

Uh oh!

LDong-Arm commented May 25, 2020

Uh oh!

mbed-ci commented May 25, 2020

Test run: SUCCESS

Uh oh!

LDong-Arm May 26, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm May 26, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm commented May 26, 2020

Uh oh!

adbridge commented May 26, 2020

Uh oh!

mergify bot commented Jun 4, 2020

Uh oh!

Uh oh!

LDong-Arm commented May 15, 2020 •

edited

Loading

MarceloSalazar May 15, 2020 •

edited

Loading

LDong-Arm May 15, 2020 •

edited

Loading

LDong-Arm May 15, 2020 •

edited

Loading

LDong-Arm May 15, 2020 •

edited

Loading

LDong-Arm commented May 19, 2020 •

edited

Loading