sys: add asynchronous event system by kaspar030 · Pull Request #6000 · RIOT-OS/RIOT

kaspar030 · 2016-10-26T20:15:54Z

I've been playing around with asynchronous event (callbacks). This is my current code, which I think might make it possible to write more predictable asynchronous applications, compared to using message queues.

~~PR'ing it now so others can take an early look.~~

miri64 · 2016-10-26T20:57:59Z

PR #6000 🎉

miri64 · 2016-10-26T20:58:44Z

Are there still plans on your side to merge this with the event timer in #5999?

kaspar030 · 2016-10-26T21:38:29Z

yes, but I wanted to get this PR'ed for numerological reasons. ;)

-----Original Message-----
From: Martine Lenders [email protected]
To: RIOT-OS/RIOT [email protected]
Cc: Kaspar Schleiser [email protected], Author [email protected]
Sent: Mi., 26 Okt. 2016 22:57
Subject: Re: [RIOT-OS/RIOT] sys: add asynchronous event system (#6000)

Are there still plans on your side to merge this with the event timer in #5999?

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#6000 (comment)

haukepetersen · 2016-12-19T09:20:53Z

As talked offline, this PR would be very useful for integrating events into SAUL!

@kaspar030: would you mind to

add documentation
add some kind of test or unittest for both testing and demonstration purposes
thanks!

miri64 · 2017-01-24T15:19:39Z

Postponed due to feature freeze.

miri64 · 2017-10-02T09:37:26Z

Any plans to further work on this?

kaspar030 · 2017-10-02T10:39:23Z

Yes! I've been using it for the latest jerryscript work. I'll update this PR soon.

kaspar030 · 2017-10-22T11:56:57Z

This is ready for review.

kaspar030 · 2017-10-22T12:37:33Z

Are there still plans on your side to merge this with the event timer in #5999?

This PR contains some code for timing events, but for now they're xtimer based. If we see it would make sense, we can extend or modify later.

kaspar030 · 2017-10-22T12:38:04Z

add documentation

Done.

add some kind of test or unittest for both testing and demonstration purposes

Done.

miri64 · 2017-10-22T18:01:32Z

sys/include/event/timeout.h

+
+/**
+ * @ingroup     sys_event
+ * @brief       Provides functionality to trigger events after timeout


Question for clarification: Is this to replace evtimer?

Nope. This is so that users of the event module can also have timed events. It would probably be a good idea to implement event_timer_t on top of evtimer. The similarity of the names is unfortunately confusing...

Well this PR provides an event queue implementation, evtimer is a timed event queue. Shouldn't there be some way to merge them? What is event_timer_t?

There is, maybe, but not a trivial one... Also, the use-cases are quite different. evtimer basically runs thread-independent, with it's event handlers being run in ISR context.
This event queue is much more under user control.

What is event_timer_t?

I meant event_timeout_t, the method of haveing timed events within event queues.

miri64

Some preliminary review.

miri64 · 2017-10-23T15:41:14Z

sys/include/event.h

+ *    one thread, e.g., in order to create a state-machine like process flow.
+ *    This is not (easily) possible using msg queues, as they might fill up.
+ * 4. an event can only be queued in one event queue at the same time.
+ *    Notifying many queues using only one event object is impossible.


Wording: I doubt the impossibility of this. I might however be convinced that it is not possible with this implementation.

miri64 · 2017-10-23T15:43:24Z

sys/include/event.h

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


Params? Also, not sure how flag colisions are handled in thread_flags in general.

I thought about it... making this configurable per queue adds 33% to the queue structure's size...
I hoped that we can get by with one queue per thread, in which case a globally configurable constant might be ok...

OK to leave as is and gather experience?

Params?

That was a typo... what I meant was "parans" ;-). As such I think you misunderstood the second half: I'm actually not sure if other modules using thread_flags won't collide with the flag you define here, so can you put my mind here at ease?

parans

fixed

I'm actually not sure if other modules using thread_flags won't collide with the flag you define here, so can you put my mind here at ease?

Maybe half. :) Users of thread_flags might actually collide by using the same thread flag. I don't see that as a huge problem (unlike crashing msg ids), as thread flags are supposed to be used in a "notifying" way, meaning "there might be something you might want to look at".

As said, I'd like to gather some experience before deciding whether the flag number needs to be changed and/or made configurable.

miri64 · 2017-10-23T15:44:34Z

sys/include/event.h

+ *
+ * This will set the calling thread as owner of @p queue.
+ *
+ * @param[in,out]   queue   event queue object to initialize


I would argue it is just an out parameter. Or are there any fields required to be set before initialization.

miri64 · 2017-10-23T15:45:50Z

sys/include/event.h

+ *
+ * Examples:
+ *
+ *     // simple event handler


Guard with

~~~~~~~~~~~~~~~~~~~~~~~~~~~ {.c} ... ~~~~~~~~~~~~~~~~~~~~~~~~~~~

for maximum syntax flavor ;-)

miri64 · 2017-10-23T15:46:20Z

sys/include/event.h

+ *
+ * It is pretty much defined as:
+ *
+ *     while((event = event_wait(queue))) {


Use C-fences?

~~~~~~~~~~~~~~~~~~~~~~~~~~~ {.c} ... ~~~~~~~~~~~~~~~~~~~~~~~~~~~

miri64 · 2017-10-23T15:48:02Z

tests/events/main.c

@@ -0,0 +1,80 @@
+/*


Please add a readme and/or a pexpect script.

kaspar030 · 2017-11-06T11:44:09Z

(I had a rebase mishap, that's fixed now)

fixed ~~~~~~~ {.c}
added parens

miri64

Final review. Apart from a few nits in style and wording:

I'm not happy about the usage of implicit sub-modules, better use explicit ones.
event_timeout work on a microsecond scale, so why are their tests on a second scale. Wastes time during testing ;-).

miri64 · 2017-11-06T12:25:08Z

sys/event/Makefile

@@ -0,0 +1,5 @@
+SRC := event.c
+
+SUBMODULES = 1


I don't think that this is managed pretty transparent here. For core I was understanding of that, but in sys the sub-folders for sub-modules is already established (also you don't need pseudo-modules then ;-)).

I don't understand transparent.
Using submodules here saves two folders and two makefiles with 2 lines each.

But makes it harder for newcomers to understand what to include ... (that's what I meant with transparent)

miri64 · 2017-11-06T12:25:47Z

sys/event/callback.c

+    event_callback->callback(event_callback->arg);
+}
+
+void event_callback_init(event_callback_t *event_callback, void (callback)(void*),  void *arg)


extra space before void *arg

miri64 · 2017-11-06T12:26:25Z

sys/event/callback.c

+    event_callback->callback(event_callback->arg);
+}
+
+void event_callback_init(event_callback_t *event_callback, void (callback)(void*),  void *arg)


Missing space in-between void* in the callback parameter.

miri64 · 2017-11-06T12:26:58Z

sys/event/timeout.c

+
+#include "event/timeout.h"
+
+static void _event_timeout_callback(void* arg)


s/\* / \*/

miri64 · 2017-11-06T12:28:21Z

sys/include/event.h

+ * @ingroup     sys
+ * @brief       Provides an Event loop
+ *
+ * This module offers an event queue framework not unlike libevent or libuev.


Maybe make it a little easier on non-native speakers and remove the double negative.

miri64 · 2017-11-06T12:34:08Z

sys/include/event/callback.h

+ */
+typedef struct {
+    event_t super;              /**< event_t structure that gets extended   */
+    void (*callback)(void*);    /**< callback function                      */


s/void*/void */

Not applied ;-)

miri64 · 2017-11-06T12:34:20Z

sys/include/event/callback.h

+ * @param[in]   callback        callback to set up
+ * @param[in]   arg             callback argument to set up
+ */
+void event_callback_init(event_callback_t *event_callback, void (*callback)(void*),  void *arg);


s/void*/void */

miri64 · 2017-11-06T12:34:38Z

sys/include/event/callback.h

+ * @param[in]   callback        callback to set up
+ * @param[in]   arg             callback argument to set up
+ */
+void event_callback_init(event_callback_t *event_callback, void (*callback)(void*),  void *arg);


Remove one space before void *arg

miri64 · 2017-11-06T12:35:07Z

sys/include/event/callback.h

+    { \
+        .super.handler=_event_callback_handler, \
+        .callback=_cb, \
+        .arg=(void*)_arg \


s/void\*/void \*/

miri64 · 2017-11-06T12:39:36Z

tests/events/main.c

+    printf("posting timed callback with timeout 1sec\n");
+    event_timeout_init(&event_timeout, &queue, (event_t*)&event_callback);
+    before = xtimer_now_usec();
+    event_timeout_set(&event_timeout, 1000000);


I think this can be smaller by at least one magnitude (maybe 100000).

miri64

Running uncrustify over your code revealed some more minor coding-style related issues.

miri64 · 2017-11-06T12:44:40Z

sys/include/event/callback.h

+    { \
+        .super.handler=_event_callback_handler, \
+        .callback=_cb, \
+        .arg=(void*)_arg \


Please add spaces before and after the =s

miri64 · 2017-11-06T12:45:18Z

sys/event/event.c

+void event_loop(event_queue_t *queue)
+{
+    event_t *event;
+    while((event = event_wait(queue))) {


Space between while and (

miri64 · 2017-11-06T12:45:59Z

tests/events/main.c

+
+
+static event_t event = { .handler=callback };
+static event_t event2 = { .handler=callback };


spaces missing around inner assignments.

miri64 · 2017-11-06T12:46:30Z

tests/events/main.c

+{
+    puts("[START] event test application.\n");
+
+    event_queue_t queue = { .waiter=(thread_t*)sched_active_thread };


Spaces missing around inner assignment

s/thread_t\*/thread_t \*/

miri64 · 2017-11-06T12:47:11Z

tests/events/main.c

+    const char *text;
+} custom_event_t;
+
+static custom_event_t custom_event = { .super.handler=custom_callback, .text="CUSTOM CALLBACK" };


Here as well.

miri64 · 2017-11-06T12:47:34Z

tests/events/main.c

+    event_timeout_t event_timeout;
+
+    printf("posting timed callback with timeout 1sec\n");
+    event_timeout_init(&event_timeout, &queue, (event_t*)&event_callback);


s/event_t\*/event_t \*/

miri64

Another test another nit ;-)

miri64 · 2017-11-06T12:59:17Z

sys/include/event.h

+ *
+ * This will remove a queued event from an event queue.
+ *
+ * @note: due to the underlying list implementation, this will run in O(n).


remove the : please. It parses weirdly in Doxygen

miri64

I think this will be my last comment ^^"

miri64 · 2017-11-06T13:00:52Z

sys/include/event.h

+ *     printf("triggered custom event with text: \"%s\"\n", custom_event->text);
+ * }
+ *
+ * static custom_event_t custom_event = { .super.callback=custom_handler, .text="CUSTOM EVENT" };


Here as well.

miri64 · 2017-11-06T13:00:54Z

sys/include/event.h

+ *    printf("triggered 0x%08x\n", (unsigned)event);
+ * }
+ *
+ * static event_t event = { .handler=handler };


Sorry, only noticed this during reading the parsed doc: spaces around inner =

kaspar030 · 2017-11-06T13:02:53Z

@miri64 sorry for all the style errors.

I'm not happy about the usage of implicit sub-modules, better use explicit ones.

Hm. Using submodules saves two folders and two Makefiles. Can we merge as is and discuss whether submodules should be used out of this PR?

event_timeout work on a microsecond scale, so why are their tests on a second scale. Wastes time during testing ;-).

Good idea. Done.

miri64 · 2017-11-06T13:14:40Z

Can we merge as is and discuss whether submodules should be used out of this PR?

Begrudgingly...

miri64 · 2017-11-06T13:16:27Z

Please squash

miri64 · 2017-11-06T13:18:35Z

PR #6000 now has 666 additions. That alone is a reason against the explicit submodules, for now ^^

miri64

ACK

miri64 · 2017-11-06T13:25:15Z

and go

kaspar030 · 2017-11-06T13:26:22Z

Thanks for reviewing!

miri64 self-assigned this Nov 10, 2016

miri64 added this to the Release 2017.01 milestone Nov 10, 2016

miri64 mentioned this pull request Jan 4, 2017

sys: add evtimer WIP #5999

Closed

miri64 modified the milestones: Release 2017.04, Release 2017.01 Jan 24, 2017

kaspar030 modified the milestones: Release 2017.04, Release 2017.07 Apr 21, 2017

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

kaspar030 force-pushed the add_event_system branch from e4351af to 3c1d721 Compare October 22, 2017 11:56

kaspar030 requested review from haukepetersen and miri64 October 22, 2017 11:57

miri64 reviewed Oct 22, 2017

View reviewed changes

kaspar030 mentioned this pull request Oct 23, 2017

sys/js: initial support of RIOT <-> javascript API mapping #7796

Closed

miri64 requested changes Oct 23, 2017

View reviewed changes

kaspar030 force-pushed the add_event_system branch from 3c1d721 to 65c5cee Compare October 26, 2017 15:36

kaspar030 force-pushed the add_event_system branch from 9b50069 to 572bdc4 Compare November 6, 2017 11:43

miri64 requested changes Nov 6, 2017

View reviewed changes

miri64 reviewed Nov 6, 2017

View reviewed changes

miri64 requested changes Nov 6, 2017

View reviewed changes

kaspar030 added 2 commits November 6, 2017 14:19

sys/event: initial commit of handler-based event system

0e09213

tests/event: initial commit of sys/event test application

27cdd54

kaspar030 force-pushed the add_event_system branch from bcfa7e3 to 27cdd54 Compare November 6, 2017 13:19

miri64 approved these changes Nov 6, 2017

View reviewed changes

miri64 merged commit 0954dba into RIOT-OS:master Nov 6, 2017

kaspar030 deleted the add_event_system branch November 6, 2017 13:26

miri64 mentioned this pull request Nov 24, 2017

gcoap: can't build with network stacks other than GNRC #8130

Closed


		#include "event/timeout.h"

		static void _event_timeout_callback(void* arg)



		static event_t event = { .handler=callback };
		static event_t event2 = { .handler=callback };

Conversation

kaspar030 commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Oct 26, 2016

Uh oh!

miri64 commented Oct 26, 2016

Uh oh!

kaspar030 commented Oct 26, 2016

Uh oh!

haukepetersen commented Dec 19, 2016

Uh oh!

miri64 commented Jan 24, 2017

Uh oh!

miri64 commented Oct 2, 2017

Uh oh!

kaspar030 commented Oct 2, 2017

Uh oh!

kaspar030 commented Oct 22, 2017

Uh oh!

kaspar030 commented Oct 22, 2017

Uh oh!

kaspar030 commented Oct 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Nov 6, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Oct 26, 2016 •

edited

Loading