Skip to content
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

core: define default flags #2761

Closed
miri64 opened this issue Apr 3, 2015 · 18 comments
Closed

core: define default flags #2761

miri64 opened this issue Apr 3, 2015 · 18 comments
Assignees
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! State: archived State: The PR has been archived for possible future re-adaptation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Comments

@miri64
Copy link
Member

miri64 commented Apr 3, 2015

#2711 (comment) revealed that it can be problematic starting a thread without CREATE_STACKTEST when trying to debug. #2758 (comment) suggested to create a macro for default flags (if there are more than CREATE_STACKTEST these can be added to).

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: core Area: RIOT kernel. Handle PRs marked with this with care! labels Apr 3, 2015
@kaspar030
Copy link
Contributor

For CREATE_STACKTEST, fix ps to show edit that the thread doesn't have that information.

All other flags have intrinsic defaults, e.g., CREATE_STACKTEST and CREATE_SLEEPING are opt-in, CREATE_WITHOUT_YIELD is opt-out. No need to waste code & flash for another place to store defaults.

@OlegHahm
Copy link
Member

OlegHahm commented Apr 8, 2015

I don't get the proposal. @authmillenon, can you rephrase, please?

@miri64
Copy link
Member Author

miri64 commented Apr 8, 2015

It was more or less @haukepetersen's idea and this was what I gathered from him. Maybe he can do that?

@kaspar030
Copy link
Contributor

If I understand correctly, @haukepetersen wondered why it's pktpuf thread always showed the full stacksize when watched with ps. The problem was that the thread was not created with the CREATE_STACKTEST flag set, which he fixed by setting that flag.

Now @authmillenon proposed to introduce default thread creation flags, so that i.e., CREATE_STACKTEST can be switched on globally.

@LudwigKnuepfer
Copy link
Member

I think this would be beneficial because one could save the cost of CREATE_STACKTEST in production environments.

@OlegHahm
Copy link
Member

OlegHahm commented Apr 8, 2015

Sounds good.

@LudwigKnuepfer
Copy link
Member

Maybe we can remove this from the flags altogether and just ifdef the section of the thread creation function? I can't really think of a situation where one would want to turn this off for individual threads only.

@LudwigKnuepfer
Copy link
Member

This -> CREATESTACKTEST

@Kijewski
Copy link
Contributor

Kijewski commented Apr 8, 2015

Maybe we can remove this from the flags altogether and just ifdef the section of the thread creation function?

👍

@OlegHahm
Copy link
Member

OlegHahm commented Apr 9, 2015

👍 @kaspar030, I guess you've initially implemented this. Was there a rationale behind that?

@kaspar030
Copy link
Contributor

Well, CREATE_STACKTEST makes the thread creation write something to the entire stack. As that is time-consuming, there was no "ps" and it's O(n), I made it optional.

Also there might be (future) use-cases where this is not needed / too time-consuming:

  • Using CREATE_STACKTEST for stacks created one at boot is fine, when creating them dynamically, it feels like way too much overhead. Imagine implementing signals as one-shot threads using a shared stack. No need to CREATE_STACKTEST
  • Remember the discussion whether it should be possible to create a thread in ISR context? CREATE_STACKTEST as default is a no-go there.

Mind that this is only used by ps. But we could turn the flag around (CREATE_NOSTACKTEST) for the special use cases, and #ifdef out the code for "production" builds.

@OlegHahm OlegHahm modified the milestone: Release 2015.06 May 28, 2015
@OlegHahm
Copy link
Member

OlegHahm commented Sep 2, 2015

The rationale provided by @kaspar030 seems sound to me. However,

For CREATE_STACKTEST, fix ps to show edit that the thread doesn't have that information.

is also not possible without storing the information somewhere in the tcb which is also definitely not desirable. Hence, I would tend to close this as wontfix. Any objections?

@kaspar030
Copy link
Contributor

no :)

@OlegHahm OlegHahm modified the milestones: Release 2015.09, Release NEXT MAJOR Oct 22, 2015
@OlegHahm OlegHahm modified the milestone: Release 2015.12 Dec 2, 2015
@LudwigKnuepfer
Copy link
Member

Not going to work on this any time soon.

@kaspar030
Copy link
Contributor

Ok to close this (maybe with memo flag)?

@miri64
Copy link
Member Author

miri64 commented Mar 22, 2016

I'm fine. IIRC @haukepetersen was the one originally proposing the idea. What has he to say?

@kYc0o
Copy link
Contributor

kYc0o commented Aug 4, 2016

Is this still a known issue?

@miri64
Copy link
Member Author

miri64 commented Oct 13, 2016

Why was it in the known issues anyway? This is a feature proposal, not a bug...

@miri64 miri64 closed this as completed Oct 13, 2016
@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Oct 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! State: archived State: The PR has been archived for possible future re-adaptation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

No branches or pull requests

6 participants