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

Fix check of stack size in debug #1244

Closed
wants to merge 1 commit into from
Closed

Fix check of stack size in debug #1244

wants to merge 1 commit into from

Conversation

Jentsch
Copy link

@Jentsch Jentsch commented May 27, 2014

This behavior should be more intuitive. Otherwise Threads need to have KERNEL_CONF_STACKSIZE_PRINTF + 1 to run properly.

@thomaseichinger
Copy link
Member

Seems nobody used DEVELHELP recently. The comment is somewhat confusing too, will open a PR for this.

ACK

@Jentsch
Copy link
Author

Jentsch commented May 27, 2014

We used it for our network scanner.

@Jentsch
Copy link
Author

Jentsch commented May 30, 2014

Are you going to accept or deny this PR?

@mehlis
Copy link
Contributor

mehlis commented May 30, 2014

I think the size KERNEL_CONF_STACKSIZE_PRINTF is the memory printf will use, but for a stack you need to have a stacksize which can handle your code and printf.

I think using KERNEL_CONF_STACKSIZE_PRINTF for debugging isn't right, use KERNEL_CONF_STACKSIZE_MAIN in your code in case of debugging.

I will clarify this on Monday!

@Jentsch
Copy link
Author

Jentsch commented May 30, 2014

Ok, thanks

@OlegHahm
Copy link
Member

OlegHahm commented Jun 2, 2014

@Jentsch, apparently the name of stack size constants is still confusing. I tried to document the idea in the Wiki but it seems that there is still room for improvement. Do you have any suggestion how to improve the documentation or the naming (see #732)?

@OlegHahm OlegHahm assigned OlegHahm and unassigned Kijewski Jun 2, 2014
@mehlis
Copy link
Contributor

mehlis commented Jun 2, 2014

@OlegHahm thanks for giving the hint to the wiki!

to cite: "KERNEL_CONF_STACKSIZE_PRINTF defines additional stack space needed if the thread needs to call printf (which requires additional memory when using newlib. KERNEL_CONF_STACKSIZE_MAIN is the stack size for the main thread and probably a good size for your application."

@OlegHahm so I conclude that this PR is invalid, right?

@Jentsch
Copy link
Author

Jentsch commented Jun 2, 2014

May we rename the constant, but the comparison operator should stay >= in my opinion.

@Kijewski
Copy link
Contributor

Kijewski commented Jun 2, 2014

Yep, using >= is correct.

@Jentsch
Copy link
Author

Jentsch commented Jun 2, 2014

So we could merge this commit?

@OlegHahm
Copy link
Member

OlegHahm commented Jun 2, 2014

Actually, I would say neither is correct.

Correct would be

sched_active_thread->stack_size >= KERNEL_CONF_STACKSIZE_PRINTF + MINIMUM_STACK_SIZE

@Jentsch
Copy link
Author

Jentsch commented Jun 2, 2014

So we all agree with >= ?
May we contiue our nameing discussion in #732?

@Kijewski
Copy link
Contributor

ACK.

@OlegHahm OlegHahm assigned LudwigKnuepfer and unassigned OlegHahm Jun 20, 2014
@OlegHahm
Copy link
Member

See #1244 (comment)

@Kijewski
Copy link
Contributor

Throughout all examples, tests and sys modules KERNEL_CONF_STACKSIZE_PRINTF is used like it was the whole needed stack size. Maybe it was not intended this way, but this is how it is used.

@OlegHahm
Copy link
Member

That's simply wrong:

`--> git grep -l STACKSIZE_MAIN examples/ tests | wc -l 
9
`--> git grep -l STACKSIZE_PRINTF examples/ tests | wc -l
11

I would rather fix these examples.

@Kijewski
Copy link
Contributor

Works for me, but first I'll get some sleep. :)

@OlegHahm OlegHahm assigned Kijewski and unassigned LudwigKnuepfer Jun 22, 2014
@mehlis
Copy link
Contributor

mehlis commented Jul 10, 2014

@OlegHahm 's fix is merged: #1370

this PR is invalid so far, but thanks for spotting the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants