Skip to content

Move event thread flag #20868

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+14 −8
Draft

Move event thread flag #20868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76c9059
core/thread_flags: add missing whitespace
Enoch247 Sep 20, 2024
d958512
sys/event: add event thread flag to THREAD_FLAG_PREDEFINED_MASK
Enoch247 Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion core/include/thread_flags.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,21 @@
* }
*/
#define THREAD_FLAG_MSG_WAITING (1u << 15)

/**
* @brief Set by @ref xtimer_set_timeout_flag() when the timer expires
*
* @see xtimer_set_timeout_flag
*/
#define THREAD_FLAG_TIMEOUT (1u << 14)

/**
* @brief Thread flag use to notify available events in an event queue
*
* This flag is used by the `event` module.
*/
#define THREAD_FLAG_EVENT (1u << 13)
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the value from 0x1 to 1 << 13 might break applications that relied on the fact that the latter was unused before.

Copy link
Contributor Author

@Enoch247 Enoch247 Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I considered that, but figured that it boiled down to too cases; applications that rely on the flag 1 << 13 being free, and applications that rely on THREAD_FLAG_EVENT being equal to 0x1.

In the first case, applications should rely in the macro THREAD_FLAG_PREDEFINED_MASK to tell them if a flag is free and should be prepared to deal with once free flags becoming no longer free in later versions of RIOT.

In the second case, applications should use THREAD_FLAG_EVENT rather than the value of 0x1 directly, and should not be affected.

One artifact of changing the value is that thread_flags_wait_one() does state that the flags are serviced in a specific order according to their value. However, again the thread flag's value was re-definable. This means that no part of RIOT itself should have ever relied on this ordering. I suppose you could make an argument that an application could have relied on this, but seems unlikely.

I am fine with it being set to either value. What's you preference?


/**
* @brief Comprehensive set of all predefined flags
*
Expand All @@ -113,9 +121,14 @@
* When using custom flags, asserting that they are not in this set can help
* avoid conflict with future additions to the predefined flags.
*/
#define THREAD_FLAG_PREDEFINED_MASK (THREAD_FLAG_MSG_WAITING | THREAD_FLAG_TIMEOUT)
#define THREAD_FLAG_PREDEFINED_MASK (\
THREAD_FLAG_EVENT |\
THREAD_FLAG_MSG_WAITING |\
THREAD_FLAG_TIMEOUT\
)

/** @} */

Check warning on line 131 in core/include/thread_flags.h

View workflow job for this annotation

GitHub Actions / static-tests 

Uncrustify proposes the following patch:

--- a/core/include/thread_flags.h
+++ b/core/include/thread_flags.h
@@ -121,11 +121,11 @@ extern "C" {
  * When using custom flags, asserting that they are not in this set can help
  * avoid conflict with future additions to the predefined flags.
  */
-#define THREAD_FLAG_PREDEFINED_MASK (\
-    THREAD_FLAG_EVENT |\
-    THREAD_FLAG_MSG_WAITING |\
-    THREAD_FLAG_TIMEOUT\
-    )
+#define THREAD_FLAG_PREDEFINED_MASK ( \
+        THREAD_FLAG_EVENT | \
+        THREAD_FLAG_MSG_WAITING | \
+        THREAD_FLAG_TIMEOUT \
+        )
 
 /** @} */
/**
* @brief Type definition of thread_flags_t
*/
Expand Down
7 changes: 0 additions & 7 deletions sys/include/event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,6 @@
extern "C" {
#endif

#ifndef THREAD_FLAG_EVENT
/**
* @brief Thread flag use to notify available events in an event queue
*/
#define THREAD_FLAG_EVENT (0x1)
#endif

/**
* @brief event_queue_t static initializer
*/
Expand Down
Loading