Skip to content

stack_size_unification test: set expected stack sizes from config #12979

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.

Sign up for GitHub

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
Jun 4, 2020
Merged

stack_size_unification test: set expected stack sizes from config #12979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ecd99f
stack_size_unification test: set expected stack sizes from config
LDong-Arm May 15, 2020
0ac17ea
Define MBED_CONF_RTOS_MAIN_THREAD_STACK_SIZE properly in rtos mbed_li…
LDong-Arm May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions TESTS/mbed_hal/stack_size_unification/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,11 @@ extern osThreadAttr_t _main_thread_attr;
#endif
extern uint32_t mbed_stack_isr_size;

#if !defined(MBED_CONF_RTOS_PRESENT)
#define EXPECTED_ISR_STACK_SIZE (4096)
#else
#define EXPECTED_ISR_STACK_SIZE (1024)
#endif
#define EXPECTED_ISR_STACK_SIZE (MBED_CONF_TARGET_BOOT_STACK_SIZE)

#if defined(TARGET_NUCLEO_F070RB) || defined(TARGET_STM32F072RB) || defined(TARGET_TMPM46B) || defined(TARGET_TMPM066)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

#define EXPECTED_MAIN_THREAD_STACK_SIZE (3072)
#else
#define EXPECTED_MAIN_THREAD_STACK_SIZE (4096)
#endif
#define EXPECTED_MAIN_THREAD_STACK_SIZE (MBED_CONF_RTOS_MAIN_THREAD_STACK_SIZE)

#define EXPECTED_USER_THREAD_DEFAULT_STACK_SIZE (4096)
#define EXPECTED_USER_THREAD_DEFAULT_STACK_SIZE (MBED_CONF_RTOS_THREAD_STACK_SIZE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


#if ((MBED_RAM_SIZE - MBED_BOOT_STACK_SIZE) <= (EXPECTED_MAIN_THREAD_STACK_SIZE + EXPECTED_ISR_STACK_SIZE))
#error [NOT_SUPPORTED] Insufficient stack for staci_size_unification tests
Expand Down
3 changes: 3 additions & 0 deletions rtos/source/TARGET_CORTEX/mbed_lib.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@
"STM32F072RB": {
"main-thread-stack-size": 3072
},
"TMPM46B": {
"main-thread-stack-size": 3072
},
Copy link

@MarceloSalazar MarceloSalazar May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@LDong-Arm LDong-Arm May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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/...

Copy link
Contributor Author

@LDong-Arm LDong-Arm May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@LDong-Arm LDong-Arm May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

"NUVOTON": {
"idle-thread-stack-size-debug-extra": 512
}
Expand Down
4 changes: 0 additions & 4 deletions targets/TARGET_TOSHIBA/mbed_rtx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@
#ifndef INITIAL_SP
#define INITIAL_SP (0x20080000UL)
#endif
#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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


#endif

Expand Down