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.

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

Jump to bottom

Clarify and unify naming of thread-related constants (stacksize and priority) #2881

Merged
merged 1 commit into from
May 21, 2015
Merged

Clarify and unify naming of thread-related constants (stacksize and priority) #2881

merged 1 commit into from
May 21, 2015

Conversation

x3ro
Copy link
Contributor

@x3ro x3ro commented Apr 28, 2015

Changes as discussed in #2725

  1. For base stacksizes: drop the KERNEL_CONF prefix in favor of THREAD.
    • KERNEL_CONF_STACKSIZE_DEFAULT becomes THREAD_STACKSIZE_DEFAULT.
  2. For supplemental stacksizes: drop the KERNEL_CONF prefix in favor of EXTRA.
    • KERNEL_CONF_STACKSIZE_PRINTF becomes THREAD_EXTRA_STACKSIZE_PRINTF.

In addition:

  1. Thread priorities previously called PRIORITY_... are now prefixed with THREAD_
  2. MINIMUM_STACK_SIZE adapted to the naming scheme of the other stacksizes, thus becoming THREAD_STACKSIZE_MINIMUM

@x3ro x3ro added the Area: core Area: RIOT kernel. Handle PRs marked with this with care! label Apr 28, 2015
@x3ro x3ro mentioned this pull request Apr 28, 2015
Closed
OlegHahm
OlegHahm reviewed Apr 28, 2015
View reviewed changes
boards/qemu-i386/include/cpu-conf.h
#define KERNEL_CONF_STACKSIZE_PRINTF_FLOAT (8192)
#define THREAD_STACKSIZE_DEFAULT (8192)
#define THREAD_STACKSIZE_IDLE (8192)
#define THREAD_EXTRA_STACKSIZE_PRINTF (8192)
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off.

@OlegHahm
Copy link
Member

You're planning to squash? Otherwise the commit messages are missing a proper prefix.

@x3ro
Copy link
Contributor Author

x3ro commented Apr 28, 2015

Yep, planning to squash. Or would you say the commits should stay this way? Then I'd rename

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Apr 28, 2015
haukepetersen
haukepetersen reviewed Apr 28, 2015
View reviewed changes
core/include/thread.h
* @def THREAD_PRIORITY_IDLE
* @brief Priority of the idle thread
*/
#define THREAD_PRIORITY_IDLE THREAD_PRIORITY_MIN
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to put parenthesis around this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though it just maps to THREAD_PRIORITY_MIN which is in parentheses?

@haukepetersen
Copy link
Contributor

ACK when comment above is addressed

@OlegHahm
Copy link
Member

ACK

@BytesGalore
Copy link
Member

looks good please squash, then lets see what travis says

@OlegHahm OlegHahm modified the milestone: Release 2015.06 Apr 29, 2015
@x3ro x3ro force-pushed the clarify-stacksize-constants branch from adeb101 to eed76f2 Compare April 30, 2015 09:15
@miri64
Copy link
Member

miri64 commented May 9, 2015

Please rebase

@x3ro
Copy link
Contributor Author

x3ro commented May 9, 2015

@authmillenon Will do. Travis was unhappy about smth and I didn't manage to fix that yet, but I'll give my best to get it merged asap.

@OlegHahm
Copy link
Member

Any news?

@x3ro x3ro force-pushed the clarify-stacksize-constants branch from eed76f2 to abf650f Compare May 17, 2015 17:11
@x3ro
Copy link
Contributor Author

x3ro commented May 17, 2015

Since I've touched a lot of files in this PR, I also have to deal with a lot of doxygen warnings that I have no clue how to fix. How do we handle this?

@OlegHahm
Copy link
Member

Travis should only complain if the PR introduces doxygen warnings.

@x3ro
Copy link
Contributor Author

x3ro commented May 17, 2015

Does it not complain about all warnings in files that have been changed in a PR? That was my impression so far.

@OlegHahm
Copy link
Member

See https://github.com/RIOT-OS/RIOT/blob/master/dist/tools/doccheck/check.sh#L12:

 ERR_DIFFFILTER="--diff-filter=AC"
 WARN_DIFFFILTER="--diff-filter=MR"

Hence, only created and copied files should create errors, the rest should output only warnings.

@x3ro x3ro force-pushed the clarify-stacksize-constants branch 2 times, most recently from 5c20fec to 1183747 Compare May 17, 2015 21:53
@x3ro x3ro removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 18, 2015
@x3ro x3ro force-pushed the clarify-stacksize-constants branch from 1183747 to e5bc5a4 Compare May 18, 2015 10:59
@x3ro
Copy link
Contributor Author

x3ro commented May 18, 2015

@OlegHahm The last Travis build passed but there was a minor merge conflict :( Can I merge this as soon as Travis comes back with a green light? It's ACKed otherwise, right?

@OlegHahm
Copy link
Member

Yes, you can.

@x3ro x3ro force-pushed the clarify-stacksize-constants branch 2 times, most recently from f7cb1c7 to cf10a66 Compare May 18, 2015 21:22
@x3ro x3ro force-pushed the clarify-stacksize-constants branch 3 times, most recently from a0f4b7a to f0bf928 Compare May 20, 2015 12:39
@x3ro
Improve naming of thread stacksize/priority constants
426170b 
As discussed in RIOT-OS#2725, this commit renames a number of stacksize constants to
better convey their intended usage. In addition, constants for thread priority
are given a `THREAD_` prefix. Changes are:

* KERNEL_CONF_STACKSIZE_PRINTF renamed to THREAD_EXTRA_STACKSIZE_PRINTF
* KERNEL_CONF_STACKSIZE_DEFAULT renamed to THREAD_STACKSIZE_DEFAULT
* KERNEL_CONF_STACKSIZE_IDLE renamed to THREAD_STACKSIZE_IDLE
* KERNEL_CONF_STACKSIZE_MAIN renamed to THREAD_STACKSIZE_MAIN
* Move thread stacksizes from kernel.h to thread.h, since the prefix changed
* PRIORITY_MIN renamed to THREAD_PRIORITY_MIN
* PRIORITY_IDLE renamed to THREAD_PRIORITY_IDLE
* PRIORITY_MAIN renamed to THREAD_PRIORITY_MAIN
* Move thread priorities from kernel.h to thread.h since the prefix has changed
* MINIMUM_STACK_SIZE renamed to THREAD_STACKSIZE_MINIMUM for consistency
@OlegHahm
Copy link
Member

tests/driver_kw2xrf needs an update. 😩

@x3ro x3ro force-pushed the clarify-stacksize-constants branch from f0bf928 to 426170b Compare May 21, 2015 13:00
@x3ro
Copy link
Contributor Author

x3ro commented May 21, 2015

Build passed over at my fork's Travis. Is it okay if I merge?

@OlegHahm
Copy link
Member

Works for me.

@x3ro
Copy link
Contributor Author

x3ro commented May 21, 2015

Then brace for impact 💥

x3ro added a commit that referenced this pull request May 21, 2015
@x3ro
Merge pull request #2881 from x3ro/clarify-stacksize-constants
742c39e 
Clarify and unify naming of thread-related constants (stacksize and priority)
@x3ro x3ro merged commit 742c39e into RIOT-OS:master May 21, 2015
@jonas-rem
Copy link
Contributor

Sry, this is too late now and maybe it´s a dump question. Why are the old auto-init apps introduced again? And why in this PR?

  • eg. tests/driver_at86rf2xx/auto_init_ng_netif/netif_app.c

tests/driver_kw2xrf needs an update. 😩

What exactly needs to be updated there?

@x3ro
Copy link
Contributor Author

x3ro commented May 21, 2015

If they should not be there, then I probably messed up one of the many rebases :(

@jonas-rem
Copy link
Contributor

Ah ok, yes they should not be there. The PR that changed the auto-init process is #2901. Maybe this was merged before your last rebase or so.

But this doesn´t break anything, so the files could just be deleted by any follow-up or so. Just asked because this could affect the driver implementation for kw2xrf.

@miri64 miri64 mentioned this pull request May 21, 2015
Merged
@PeterKietzmann PeterKietzmann mentioned this pull request May 27, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@OlegHahm OlegHahm

Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care!
Projects
None yet
Milestone
Release 2015.09
Development

Successfully merging this pull request may close these issues.
6 participants
@x3ro @OlegHahm @haukepetersen @BytesGalore @miri64 @jonas-rem