Skip to content

core/msg: Set THREAD_FLAG_MSG_WAITING when queueing messages#7533

Merged
haukepetersen merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/core-msg-threadflag
Aug 30, 2017
Merged

core/msg: Set THREAD_FLAG_MSG_WAITING when queueing messages#7533
haukepetersen merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/core-msg-threadflag

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Aug 29, 2017

The define THREAD_FLAG_MSG_WAITING exists in thread_flags.h since forever, but it was never used by the IPC implementation. This PR fixes that.

@jnohlgard jnohlgard added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 29, 2017
@jnohlgard jnohlgard added this to the Release 2017.10 milestone Aug 29, 2017
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 29, 2017

Nice! The flag should also be set before going send-blocked.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 29, 2017

@kaspar030 could you point me to which line you mean? I assume it's the exact same thread_flags_set call that should be used there as well

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 29, 2017

@kaspar030 could you point me to which line you mean? I assume it's the exact same thread_flags_set call that should be used there as well

Just before the irq_restore() (line 140 in current master).
Also you'll need to use

#ifdef MODULE_CORE_THREAD_FLAGS
        target->flags |= THREAD_FLAG_MSG_WAITING;
        thread_flags_wake(target);
#endif

in both cases. thread_flags_set() does extra irq enable/disable and also invokes thread_yield(), which we can skip here.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 30, 2017

@kaspar030 is this what you meant?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 30, 2017

Yes, please squash!

@kaspar030 kaspar030 mentioned this pull request Aug 30, 2017
Merged
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 30, 2017

#7536 adds some documentation on how to use this flag.

@jnohlgard jnohlgard force-pushed the pr/core-msg-threadflag branch from 51d8558 to ef01efc Compare August 30, 2017 10:04
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 30, 2017

Thanks for improving the docs!
@kaspar030 squashed

@jnohlgard jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 30, 2017
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 30, 2017

Do we need a second ACK?

kaspar030
kaspar030 approved these changes Aug 30, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 30, 2017

Do we need a second ACK?

Can't hurt... @haukepetersen @PeterKietzmann @aabadie care to take a look?

haukepetersen
haukepetersen approved these changes Aug 30, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

Did take a look: looks good to me -> ACK

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Aug 30, 2017

2nd ACK is there, all green -> go

@haukepetersen haukepetersen merged commit 9cb6aee into RIOT-OS:master Aug 30, 2017
@jnohlgard jnohlgard mentioned this pull request Aug 30, 2017
Closed
@jnohlgard jnohlgard deleted the pr/core-msg-threadflag branch August 30, 2017 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haukepetersen haukepetersen haukepetersen approved these changes

@kaspar030 kaspar030 kaspar030 approved these changes

Assignees

@kaspar030 kaspar030

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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2017.10

Development

Successfully merging this pull request may close these issues.

3 participants

@jnohlgard @kaspar030 @haukepetersen