Skip to content

core: thread_flags: improve documentation#7536

Merged
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
kaspar030:improve_thread_flags_docs
Aug 30, 2017
Merged

core: thread_flags: improve documentation#7536
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
kaspar030:improve_thread_flags_docs

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

Previously, due to borked doxygen, the thread flags docs where half missing and half hidden in the thread module docs.

This PR creates a doxygen group for thread flags.
It also adds documentation for the THREAD_FLAG_MESSAGE_WAITING flag as implemented in #7533.

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: doc Area: Documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances 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 Aug 30, 2017
@kaspar030 kaspar030 requested a review from jnohlgard August 30, 2017 08:36
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Looks good, apart from the minor comment on the #ifdef

* thread_flags_t flags = thread_flags_wait_any(0xFFFF);
* if (flags & THREAD_FLAG_MSG_WAITING) {
* msg_t msg;
* while (msg_try_receive(&msg) == 1) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than the mask and using > 0 instead of == 1, I used this exact same loop for my event loop after writing #7533

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hopefully we'll see this pattern in more places throughout the code base. ;)

*
* It is only reset through thread_flags_wait_*(), not by msg_receive().
* Care must be taken to a) not lose message notifications and b) not block for
* messages but not thread flags.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

difficult to understand what you mean by "not block messages but not thread flags."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does "not block the thread for messages without blocking for flags" describe what you meant?
(bl rx vs. bl anyfl)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've completely reworded the not, is it clearer like this?

* @{
*/

#if defined(MODULE_CORE_THREAD_FLAGS) || defined(DOXYGEN)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should not be necessary to check for MODULE_CORE_THREAD_FLAGS since we are in the thread_flags.h header.
If you want to add additional checks you could do

#ifndev MODULE_CORE_THREAD_FLAGS
#error Missing USEMODULE += core_thread_flags
#endif

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Didn't mean to approve until the ifdef and wording was addressed

@kaspar030
Copy link
Copy Markdown
Contributor Author

Hm, seems hidden in "outdated". I've changed the explanation you commented on, is it clearer like this?

Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

ACK, please squash

@kaspar030 kaspar030 force-pushed the improve_thread_flags_docs branch from 6165b5b to ee94597 Compare August 30, 2017 11:43
@jnohlgard jnohlgard merged commit 5dd38c6 into RIOT-OS:master Aug 30, 2017
@kaspar030 kaspar030 deleted the improve_thread_flags_docs branch August 30, 2017 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants