-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
event_periodic_callback: add convenience wrapper for periodic callbacks #18598
Conversation
146d3d1
to
3aac88a
Compare
3aac88a
to
ba1de18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Code looks good. I trust you have run the test application?
Sure and it will be run by CI too. |
ifneq (,$(filter event_periodic_callback,$(USEMODULE))) | ||
USEMODULE += event_callback | ||
USEMODULE += event_periodic | ||
USEMODULE += event_thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this module needed for? From the code I don't see any obvious reason for this dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which of those modules do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
event_thread
. The module that's in the line I commented on...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need an event thread to execute the callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that's application code (you provide the queue to the init function). An application (or using module) might as well just use its own runner thread (which should remain an option anyways IMHO).
|
||
#include "event/callback.h" | ||
#include "event/periodic.h" | ||
#include "event/thread.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for this include btw. I am not sure what of event/thread.h
is used here.
event_periodic_callback_init(&a, ZTIMER_MSEC, EVENT_PRIO_MEDIUM, _event_cb, "event A"); | ||
event_periodic_callback_init(&b, ZTIMER_MSEC, EVENT_PRIO_MEDIUM, _event_cb, "event B"); | ||
event_periodic_callback_init(&c, ZTIMER_MSEC, EVENT_PRIO_MEDIUM, _event_cb, "event C"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use the queues of event_thread
here, but this is application code, so maybe you saw the link error and thought it was a missing dependency?
Mhhh... I darn, forgot to disable auto-merge 😅 |
Contribution description
We have
event_periodic
to periodically trigger an event (callback in interrupt context) and we haveevent_callback
to execute an event callback by the event thread.Putting both together always requires some boilerplate and is not easy to discover - let's add a
event_periodic_callback
convenience module that ties it all together nicely.Testing procedure
A new test case is provided with
tests/event_periodic_callback
.Event callbacks are executed in thread context.
Issues/PRs references