Skip to content

Remove main stack customizations #2984

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

Closed
wants to merge 2 commits into from

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Oct 11, 2016

Set all platforms to have a main stack twice as big as the default.

Set all platforms to have a main stack twice as big as the default.
@c1728p9
Copy link
Contributor Author

c1728p9 commented Oct 11, 2016

/morph test

@bridadan
Copy link
Contributor

So much more room for activities! 😄

@mbed-bot
Copy link

Result: FAILURE

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

/morph test

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 11, 2016

The targets should change the default stack size and not to edit sources, shouldn't they? Just to confirm that this change is to unify one place to edit the default main stack size?

@c1728p9
Copy link
Contributor Author

c1728p9 commented Oct 11, 2016

The intention is to have the default stack the same across all platforms. That way code written for one device will not overflow if used on another.

Set the interrupt stack on the NUCLO_F070RB and NUCLO_F072RB to 1K so
it matches ARM and GCC_ARM. This also frees up enough space to allow
tests to build.
@c1728p9
Copy link
Contributor Author

c1728p9 commented Oct 11, 2016

/morph test-nightly

@mbed-bot
Copy link

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 0

Test failed!

@bridadan
Copy link
Contributor

Fixed the TLS failures! Awesome!

@c1728p9
Copy link
Contributor Author

c1728p9 commented Oct 11, 2016

@pan- This doubles the size of the nrf51 stacks. Do you expect this to break any of the BLE examples?

@pan-
Copy link
Member

pan- commented Oct 12, 2016

I'm sorry but I don't get why the main stack should depends on DEFAULT_STACK_SIZE which itself depends on WORDS_STACK_SIZE which is not even configurable by target.

Looking at the configuration:

//   <o>Main Thread stack size [bytes] <64-32768:8><#/4>
#ifndef OS_MAINSTKSIZE
 #error "no target defined"
#endif

There is a define for the main stack size and every targets defines it.
What was the motivation behind this change ?
I would also add that OS_MAINSTKSIZE is still used to compute OS_STACK_SZ which at the end defines the size os_stack_mem. So, we're wasting space in this area by reserving space for the main stack twice.

@c1728p9 It won't break anything if it remains configurable like it was before (5.1.1).
This behavior was introduced with #2402 but I can't find any justification for this change. Static allocation is good but forcing a fixed size, non configurable, for the main stack isn't, especially when it is defined by every platform.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 12, 2016

@c1728p9 It won't break anything if it remains configurable like it was before (5.1).
This behavior was introduced with #2402 but I can't find any justification for this change. Static allocation is good but forcing a fixed size, non configurable, for the main stack isn't, especially when it is defined by every platform.

@c1728p9 was there intention to make it non-configurable? This PR needs much more details ! Please amend the commit messages: at least to add - explaining why we are changing and what are the consequences. Because my question above was mainly about that one, same from @pan- .

@sg-
Copy link
Contributor

sg- commented Nov 3, 2016

@c1728p9 Going to close this. When RTX 5 lands we'll unify the stack size. This allows OOB to determine more run time failures if we shrink the main stack and document the new kernel and modifications and how to migrate. If you and @pan- can otherwise make an agreement please reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants