Skip to content

Change stack sizes of threads used in tests. GR_LYCHEE failing CI #11417

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 1 commit into from
Sep 9, 2019

Conversation

Tharazi97
Copy link
Contributor

Change stack sizes in test stats_cpu and mbedmicro-rtos-mbed-threads. The value 384 was declared to make this test pass on STM32F070RB, but its main stack value has been changed to 3KB so now it passes this test with 512 stack size. The change was needed to make GR_LYCHEE pass this test on ARM toolchain.

Description

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Reviewers

@jamesbeyond @maciejbocianski

Release Notes

@ciarmcom
Copy link
Member

ciarmcom commented Sep 5, 2019

@Tharazi97, thank you for your changes.
@maciejbocianski @jamesbeyond @ARMmbed/mbed-os-core @ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@maciejbocianski maciejbocianski left a comment

Choose a reason for hiding this comment

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

LGTM

@0xc0170 0xc0170 removed the request for review from a team September 5, 2019 11:33
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 5, 2019

Fixing one target but what about the rest? Smaller targets - will this test pass there?

#elif defined(TARGET_CY8CKIT_062_WIFI_BT_PSA)
#define PARALLEL_THREAD_STACK_SIZE 512
#else
#define PARALLEL_THREAD_STACK_SIZE 384
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we get PARALLEL_THREAD_STACK_SIZE fixed to 512? I think in some case some small target would not fit final compiled the images. GR_LYCHEE is an A9 target, maybe we need to make the macro defines as well in the stats_cpu tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but i tried to remove too many #if statements as it was working on STM32F070RB and the size of the stack was reduced to make it pass on this target.

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we get PARALLEL_THREAD_STACK_SIZE fixed to 512? I think in some case some small target would not fit final compiled the images. GR_LYCHEE is an A9 target, maybe we need to make the macro defines as well in the stats_cpu tests

Such targets should be optimized in the way as it was done here:
f24ac50
37bfc9e

@maciejbocianski
Copy link
Contributor

As I remember the problem was only on NUCLEO_F070RB (16kB RAM) where on GCC its malloc implementation was wasting some heap memory due to 4kB alignment. Then main stack was reduced for this target to 3kB what gave more heap and the test were passed.

From the other side I thought that it was agreed long time ago that we does not support running tests on targets with less than 32kB RAM (or at least less than 20kB like NUCLEO_L073RZ)

So assuming above, this change should be fine

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 5, 2019

cc @ARMmbed/team-st-mcd

@maciejbocianski
Copy link
Contributor

These PRs can shed some light on the problem :
#5386
#5360
#4890
#5285

@Tharazi97
Copy link
Contributor Author

For now we can implement it like this and discuss other possibilities.

@@ -29,7 +29,12 @@ using namespace utest::v1;

DigitalOut led1(LED1);

#if defined(__CORTEX_A9) || defined(__CORTEX_M23) || defined(__CORTEX_M33) || defined(__CORTEX_M7)
#define MAX_THREAD_STACK 512
#else
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the link between core and memory 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.

Targets with these cores have their RAM enough size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment above stating this assumption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Change stack sizes in test stats_cpu and mbedmicro-rtos-mbed-threads.
The value 384 was declared to make this test pass on STM32F070RB,
but its main stack value has been changed to 3KB so now it passes
this test with 512 stack size. The change was needed to make GR_LYCHEE
pass this test on ARM compiler.
Copy link
Contributor

@jamesbeyond jamesbeyond left a comment

Choose a reason for hiding this comment

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

Looks good

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 9, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 9, 2019

Test run: SUCCESS

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

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.

7 participants