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

core: move scheduler defines #15481

Merged
merged 11 commits into from
Nov 23, 2020
Merged

core: move scheduler defines #15481

merged 11 commits into from
Nov 23, 2020

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Nov 20, 2020 •
edited
Loading

Contribution description

This PR moves some defines that clearly belong to scheduler/threading into from kernel_types.h to sched.h.

Testing procedure

Only moved code, successful CI run should be sufficient.

Issues/PRs references

@kaspar030 kaspar030 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 20, 2020
@kaspar030 kaspar030 requested review from benpicco and maribu November 20, 2020 10:54
@benpicco
Copy link
Contributor

Looks like kernel_types.h is empty now safe for those SSIZE_MAX shenanigans.
But SSIZE_MAX is not used anywhere in the code, so we might as well delete that too.

@kaspar030
Copy link
Contributor Author

Looks like kernel_types.h is empty now safe for those SSIZE_MAX shenanigans.
But SSIZE_MAX is not used anywhere in the code, so we might as well delete that too.

hah, but kernel_types.h is included by 53 files. Merge this one and I open another PR for the removal of kernel_types.h?

@benpicco
Copy link
Contributor

benpicco commented Nov 20, 2020 •
edited
Loading

sure, but you might as well drop SSIZE_MAX while you are touching this file.

@kaspar030
Copy link
Contributor Author

kaspar030 commented Nov 20, 2020 •
edited
Loading

sure, but you might as well drop SSIZE_MAX while you are touching this file.

well, IMO one thing at a time makes things easier to handle... do you mind?

I pushed a commit adding an include of inttypes.h to sched.h.

@kaspar030 kaspar030 mentioned this pull request Nov 20, 2020
Merged
benpicco
benpicco previously approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Sure, just thought this would be a trivial removal either way.
But if CI is happy, this should be good.

@kaspar030 kaspar030 force-pushed the refactor_thread_defines branch from 0316474 to 83ad55b Compare November 20, 2020 12:57
@kaspar030 kaspar030 requested a review from gschorcht as a code owner November 23, 2020 10:14
@kaspar030 kaspar030 mentioned this pull request Nov 23, 2020
Merged
@benpicco benpicco dismissed their stale review November 23, 2020 13:53

some commits are already split out to separate PRs

@kaspar030 kaspar030 force-pushed the refactor_thread_defines branch from 00377db to 4cd9138 Compare November 23, 2020 15:36
@kaspar030
Copy link
Contributor Author

I fixed the remaining warts, some files needed to replace a "kernel_types.h" with "sched.h", some were missing "limits.h" now.

@benpicco
Copy link
Contributor

Just squash that single fixup so Murdock doesn't have to run twice

@kaspar030 kaspar030 force-pushed the refactor_thread_defines branch from 4cd9138 to d6b6c0e Compare November 23, 2020 15:56
benpicco
benpicco reviewed Nov 23, 2020
View reviewed changes
pkg/ndn-riot/patches/0003-adapt-to-moved-kernel_pid_t-location.patch
ndn.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/face-table.h b/face-table.h
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 do a PR to https://github.com/named-data-iot/ndn-riot as a follow-up?
It feels a bit silly to carry downstream patches to a project that targets RIOT exclusively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup!

named-data-iot/ndn-riot#14

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably do the same for the other two patches in pkg/ndn-riot/patches, those also stem from previous API changes in RIOT

benpicco
benpicco reviewed Nov 23, 2020
View reviewed changes
core/include/sched.h
/**
* Unique process identifier
*/
typedef int16_t kernel_pid_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

What meaning do negative pids have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's a KERNEL_PID_UNDEF that's -1? Anyhow, this is all copied and otherwise unchanged.

Copy link
Contributor

Choose a reason for hiding this comment

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

You just moved KERNEL_PID_UNDEF 😉

It's 0 - so that's why I'm wondering what negative PIDs mean.

benpicco
benpicco approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

simple cleanup, if Murdock is happy this should be good.

@benpicco
Copy link
Contributor

uncrustify is not happy :(

@kaspar030
Copy link
Contributor Author

kaspar030 commented Nov 23, 2020 •
edited
Loading

uncrustify is not happy

I think this was it:

file does not have a C++ compatible header: 'core/include/kernel_types.h

@benpicco
Copy link
Contributor

Huh I thought you removed kernel_types.h - is it still used by anything after this?

@kaspar030
Copy link
Contributor Author

Huh I thought you removed kernel_types.h - is it still used by anything after this?

Yeah, I'm removing the remaining uses in #15482.

@benpicco benpicco merged commit b3b07e4 into RIOT-OS:master Nov 23, 2020
@kaspar030 kaspar030 deleted the refactor_thread_defines branch November 25, 2020 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@maribu maribu Awaiting requested review from maribu

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@smlng smlng Awaiting requested review from smlng

@gschorcht gschorcht Awaiting requested review from gschorcht gschorcht is a code owner

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

Assignees
No one assigned
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kaspar030 @benpicco