Event loop with mbed-events #2828

bogdanm · 2016-09-27T14:52:41Z

Description

mbed event loop implementatio using the mbed-events library. Replaces #2801.

Status

Ready for PR, more documentation to follow.

Related PRs

List related PRs against other branches:

branch	PR
master	#2801

Todos

Tests
Documentation

Origin: https://github.com/ARMmbed/mbed-events A function without arguments (called 'dispatch_forever') was added to the original implementation of EventQueue. With that function, a thread for the event queue can be started simply like this: t.start(&queue, &EventQueue::dispatch_forever);

EventAdapter holds either a Callback<void()> or an Event<void()>. It is useful for attach()-like functions, so that their aguments can run either in user context or an interrupt context. See EventAdapter.h for more details.

The core classes that used attach()-like functions (Ticker, CAN, SerialBase, InterruptIn) were modified to use EventAdapter instead of Callback as their callback type. This gives them the ability to run callbacks (IRQ context) or events (user context).

bogdanm · 2016-09-27T14:56:17Z

Main differences from #2801:

uses the mbed-events library now. It was pulled into the mbed-os tree in the features/frameworks/mbed-events directory. I'm open to discussion about this location.
introduces an EventAdapter class that's mainly used to 'upgrade' the existing code to use events with minimal changes (see 85b996b).
EventLoop was removed. I don't think there's an use case for it anymore, but I can add it back if you think it's a good idea.

bogdanm · 2016-09-27T14:57:02Z

@geky @pan- @sg- please review

Added a configuration parameter for mbed-events that can be used to detect the presence of the library (useful for legacy builds). When mbed-events is not present, EventAdapter becomes an alias for Callback<void()>.

geky · 2016-09-27T15:45:54Z

features/frameworks/mbed-events/EventQueue.h

+     *                  (default to -1)
+     */
+    void dispatch(int ms);
+    void dispatch_forever() { dispatch(-1); }


Previously dispatch defaulted to dispatching forever. Was there a reason to change the name instead of relying on the overloaded function?

Yes:

Thread t; EventQueue e(1024); t.start(callback(&e, &EventQueue::dispatch));

doesn't work, since callback() expects a void(void) function. Not a big deal, just seemed to make things a bit more convenient than t.start(callback(&queue, &EventQueue::dispatch, -1)); (hope I got the right syntax).

If you just provide an overload, c++ correctly casts to void (EventQueue::*)(void). It's c++ being a bit clunky, but valuable for the threaded use case:
https://github.com/ARMmbed/mbed-events/blob/master/EventQueue.h#L79

The default just gives users one less function to worry about, and matches the default pattern we use in other places:
https://github.com/ARMmbed/mbed-os/blob/master/rtos/rtos/Mutex.h#L42
https://github.com/ARMmbed/mbed-os/blob/master/rtos/rtos/Queue.h#L68

Looked into it again, looks like you're right, the inference in the callback function is causing an ambiguity with the overloads. Looks like overloaded functions may have some issues with the callback class.

You can force the overload resolution to the expected callback type explicitly as a general solution:

t.start(Callback<void()>(&e, &EventQueue::dispatch));

Or your dispatch_forever member function provides an ok workaround:

t.start(callback(&e, &EventQueue::dispatch_forever));

I'll let you decide what to go with, I put a pr up for dispatch_forever to keep these in synch if you think this is cleaner (ARMmbed/mbed-events#20)

It would be nice to keep the dispatch(int ms=-1) default argument.

geky · 2016-09-27T15:52:47Z

hal/api/EventAdapter.h

+    /** Create an EventAdapter that holds a callback
+     *  @param cb The callback
+     */
+    EventAdapter(const adapter_callback_t& cb) {


@0xc0170, should we use the same spacing style for pointers and references? I don't think this has been clarified in the style guide, but I would err on consistency:

EventAdapter(const adapter_callback_t& cb); // vs EventAdapter(const adapter_callback_t &cb);

Good catch.

geky · 2016-09-27T15:54:00Z

hal/api/EventAdapter.h

+class EventAdapter {
+
+    typedef Callback<void()> adapter_callback_t;
+    typedef events::Event<void()> adapter_event_t;


I'm concerned these typedefs don't help much and actually hide information about the type (such as the arguments). They made more sense with the convoluded function-pointer syntax, but with these classes:

EventAdapter(const adapter_callback_t& cb); // vs EventAdapter(const Callback<void()> &cb);

They are internal typedefs, used only in the definition and the implementation of the class., they're not supposed to be user by outside code. I can remove them, no problem.

Ah, I was just worried for users who go to the function declaration looking for help (via ctags or similar)

geky · 2016-09-27T15:57:17Z

hal/api/EventAdapter.h

+    }
+
+    const adapter_event_t *get_event() const {
+        return reinterpret_cast<const adapter_event_t*>(&_evt_data);


It's a small thing, but these functions look like they really want an assert on _is_cb : )

Yup, can do.

geky · 2016-09-27T16:02:47Z

hal/api/EventAdapter.h

+ *      sw.rise(handler); // shortcut for the form above (run in interrupt context)
+ *  }
+ */
+class EventAdapter {


Should this actually be EventAdapter<void()> to give us room to expand in the future?

I thought about that, but given the recent discussions about binding, I'd consider this to be a temporary solution and bind to be the actual solution that we're going to end up implementing. But I guess it doesn't hurt to make it EventAdapter<void()> anyway.

mazimkhan · 2016-09-27T16:26:14Z

retest uvisor

The main change is that EventAdapter is now a template, which leaves room for future improvement if needed. Unfortunately, this also means that `typedef Callback<void()> EventAdapter` doesn't work anymore, so the implementation had to be changed to take into account the cases when `mbed-events` is not present (legacy builds). Other changes: - added MBED_ASSERT for safety - a few fixed to coding style

bogdanm · 2016-09-27T17:21:33Z

Changed EventAdapter to a template and implemented some other CR suggestions. I kept the typedefs though, code looks cleaner with them.

sg-

Mostly looks good. I'm a bit confused about the multiple equeue implementations. which equeue is used? Can we remove the rest? Same goes for prof and tests that are not compatible with greentea. I'd expect this code is developed in the mbed OS tree. Also the macro EVENTS PRESENT. Given these run in a user defined thread why is the macro needed to determine if present? I'd expect this is always present and optionally used.

sg- · 2016-09-28T01:59:01Z

hal/api/EventAdapter.h

+#define MBED_EVENT_ADAPTER_H
+
+#include "Callback.h"
+#if MBED_CONF_MBED_EVENTS_PRESENT


Is this used to exclude from mbed OS 2 releases?

The EventAdapter class is acting as a replacement for Callback, which means it's used by the hal even in the mbed 2 release.

The MBED_CONF_MBED_EVENTS_PRESENT is used for EventAdapter similarly to the MBED_CONF_MBED_RTOS_PRESENT for mutexes.

In other words, yes, it is used to exclude from mbed OS 2 releases.

sg- · 2016-09-28T02:03:48Z

hal/api/EventAdapter.h

+
+    void destroy() {
+        if (_is_cb) {
+            get_callback()->~adapter_callback_t();


style: Can we overload the -> operator to clean this up?

Not sure what you mean by that. That's a manual destructor call, needed because of a previous call to the placement new operator. I don't think that overloading -> helps you in this situation.

sg- · 2016-09-28T02:12:43Z

mbed-events/EventQueue.h

+}
+
+
+// Include event class here to workaround cyclic dependencies


is this valid or can be deleted?

Removed this on mbed-events along with a few other changes, @bogdanm is planning to update:
ARMmbed/mbed-events@v0.6...master

There are a few other changes requested in this PR that are related to mbed-events, so it makes sense to also address them upstream. After we agree that all changes to mbed-events are in, I'll update this PR.

sg- · 2016-09-28T02:14:05Z

mbed-events/README.md

@@ -0,0 +1,153 @@
+## The mbed-events library ##


This should move to the handbook and get a light dusting to reflect the integration?

sg- · 2016-09-28T02:16:46Z

mbed-events/equeue/README.md

@@ -0,0 +1,210 @@
+## The equeue library ##


Seems this should be in the handbook

sg- · 2016-09-28T02:17:30Z

mbed-events/equeue/LICENSE

@@ -0,0 +1,19 @@
+Copyright (c) 2016 Christopher Haster


Why is this not Apache? Is it not original work?

Seems there are multiple licenses in mbed-events: Apache (https://github.com/ARMmbed/mbed-events/blob/master/LICENSE) and MIT (https://github.com/ARMmbed/mbed-events/blob/master/equeue/LICENSE). I don't think either is an issue, although I don't understand which are the Apache parts and which are the MIT parts. About the copyright line, I don't know the full history of the mbed-events library. That's something that needs to be sorted with @geky.

This comes from mbed-events being built on the little equeue library that I put together outside of mbed. I'm not opposed to Apache 2.0 if the upstream project is unimpacted.

sg- · 2016-09-28T02:19:26Z

mbed-events/equeue/equeue_freertos.c

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


Don't think this need to be part of the integration

It's all a question of how we want to use mbed-events:

integrate it completely into mbed-os (which means forget about the non-mbed-os related parts).

keep it as an external library, mirror it partially into mbed-os (without the parts related to other operating systems) and update the mbed-os mirror from time to time.

Again, this is something that needs to be sorted with @geky.

This is a bit unrelated to mbed-events. These ports come from the little equeue library and the lazy manner I pulled it into mbed-events.

I don't see a reason to not integrate mbed-events completely and cull the unrelated files.

geky · 2016-09-28T06:34:54Z

This mostly comes from mbed-events being built on the little equeue library that I put together outside of mbed. It was under the MIT license which I assumed was acceptable in the mbed-os codebase?

The non-mbed files should probably be culled.

There may also need to be some effort in consolidating the docs/tests in the context of mbed:

The mbed-events/README.md covers most of the C++ interface, mbed-events/equeue/README.md is probably irrelevant.
The mbed-events/equeue/tests are very useful for preventing low-level regressions if we're expecting local changes to the equeue library. @bridadan, does greentea have some form of local tests these could be ported to?
We should probably add integration tests with the mbed API, the mbed-events/TESTS are mostly just smoke/regression tests on the C++ interface.

pan-

It is a been sad that we have to live with EventAdapter, @geky proposed a nice solution to get rid of it but it is too late now.

What is the plan regarding backward compatibility for mbed OS version 5.3 ? Should we warn users that the EventAdapter class might be deprecated ?

I have a concern, about the unit used to model ms. Is it reasonable to use int ? In my opinion, uint64_t is the best candidate to avoid overflows. What do you think ?

pan- · 2016-09-28T08:59:53Z

mbed-events/EventQueue.h

+     *  @param buffer   Pointer to buffer to use for events
+     *                  (default to NULL)
+     */
+    EventQueue(unsigned size=EVENTS_QUEUE_SIZE, unsigned char *buffer=NULL);


It would be nice to have two different constructors:

One for dynamic memory allocation which only takes the size of the buffer to allocate in input

EventQueue(unsigned size=EVENTS_QUEUE_SIZE);

The other which takes a reference to an array of uint64_t, this would also help with alignment.

template<size_t EVENTS_QUEUE_SIZE> EventQueue(uint64_t (&events_buffer)[EVENTS_QUEUE_SIZE]);

From an offline discussion, we'll keep the current API to match the Thread class. The array type may be valuable to change later, but should probably be consistent between EventQueue and Thread.

pan- · 2016-09-28T09:00:38Z

mbed-events/EventQueue.h

+     *  If ms is negative, the dispatch function will dispatch events
+     *  indefinitely or until break_dispatch is called on this queue.
+     *
+     *  When called with a finite timeout, the dispatch function is garunteed


Little typo garunteed

I can never spell this word right...

Dude me neither. Every time lol

pan- · 2016-09-28T09:04:14Z

mbed-events/EventQueue.h

+     *  should be dispatched. A negative timeout will be passed to the update
+     *  function when the time is no longer needed.
+     *
+     *  Passing a null update function disables the existing timre.


Typo: timre

pan- · 2016-09-28T09:10:29Z

mbed-events/EventQueue.h

+     */
+    void cancel(int id);
+
+    /** Background an event queue onto a single-shot timer


It is a bit hard to understand what this function does.
Maybe it would be interesting to use a verb as a function name.

I'll look into added better documentation. The background function has very specific behaviour related to attaching the queue to background timers and I don't have a much better name.

pan- · 2016-09-28T09:11:06Z

mbed-events/EventQueue.h

+     *
+     *  A null queue as the target will unchain the existing queue.
+     *
+     *  The chain function allows multiple event queuest to be composed,


Typo: queuest

pan- · 2016-09-28T09:19:25Z

mbed-events/EventQueue.h

+        return call(context50<F, A0, A1, A2, A3, A4>(f, a0, a1, a2, a3, a4));
+    }
+
+    /** Post an event to the queue after a specified delay


Same inconsistency as before: post vs call.
The documentation is also incorrect, the event will be post immediately but execution will happens after x ms.

pan- · 2016-09-28T09:20:10Z

mbed-events/EventQueue.h

+    }
+
+    template <typename F, typename A0>
+    int call_in(int ms, F f, A0 a0) {


Please add a documentation to the overloads.

pan- · 2016-09-28T09:21:00Z

mbed-events/EventQueue.h

+        return call_in(ms, context50<F, A0, A1, A2, A3, A4>(f, a0, a1, a2, a3, a4));
+    }
+
+    /** Post an event to the queue periodically


Again, post vs call

pan- · 2016-09-28T09:21:17Z

mbed-events/EventQueue.h

+        return equeue_post(&_equeue, &local::call, e);
+    }
+
+    template <typename F, typename A0>


Please add a documentation block to the overloads.

pan- · 2016-09-28T09:22:20Z

mbed-events/EventQueue.h

+    Event<void()> event(F f);
+
+    template <typename F, typename A0>
+    Event<void()> event(F f, A0 a0);


Please add a documentation block to the overloads.

bridadan · 2016-09-28T18:38:41Z

@geky

The mbed-events/equeue/tests are very useful for preventing low-level regressions if we're expecting local changes to the equeue library. @bridadan, does greentea have some form of local tests these could be ported to?

Not sure exactly what you mean by "local tests". Could you clarify a bit? If you're talking about tests that run on the host PC, then no. Greentea is used specifically for running tests on mbeds.

It'd be good to try and reduce some of the unused code in the mbed-os tree if at all possible instead of using .mbedignore files as it adds more clutter, but I totally understand the maintenance overhead that incurs when mirroring repos. I suppose we just have to strike a reasonable balance there.

We should probably add integration tests with the mbed API, the mbed-events/TESTS are mostly just smoke/regression tests on the C++ interface.

The more tests the better 😄

bogdanm · 2016-09-29T09:46:32Z

Replaced by #2860.

Bogdan Marinescu added 3 commits September 27, 2016 17:44

Added EventAdapter

5adc699

EventAdapter holds either a Callback<void()> or an Event<void()>. It is useful for attach()-like functions, so that their aguments can run either in user context or an interrupt context. See EventAdapter.h for more details.

bogdanm mentioned this pull request Sep 27, 2016

Event loop #2801

Closed

Detect the presence of the mbed-events library

281b967

Added a configuration parameter for mbed-events that can be used to detect the presence of the library (useful for legacy builds). When mbed-events is not present, EventAdapter becomes an alias for Callback<void()>.

geky reviewed Sep 27, 2016

View reviewed changes

Move mbed-events library at the root of mbed-os

30e68a3

sg- suggested changes Sep 28, 2016

View reviewed changes

sg- added the needs: review label Sep 28, 2016

pan- requested changes Sep 28, 2016

View reviewed changes

bogdanm mentioned this pull request Sep 29, 2016

Event loop with mbed-events #2860

Merged

2 tasks

bogdanm closed this Sep 29, 2016

bogdanm deleted the event_loop_mbed_events branch October 18, 2016 13:53

		}


		// Include event class here to workaround cyclic dependencies

Event loop with mbed-events #2828

Event loop with mbed-events #2828

Uh oh!

Conversation

bogdanm commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Related PRs

Todos

Uh oh!

bogdanm commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogdanm commented Sep 27, 2016

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!

geky Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mazimkhan commented Sep 27, 2016

Uh oh!

bogdanm commented Sep 27, 2016

Uh oh!

sg- 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!

geky Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sg- Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

bogdanm commented Sep 27, 2016 •

edited

Loading

bogdanm commented Sep 27, 2016 •

edited

Loading

geky Sep 27, 2016 •

edited

Loading

geky Sep 28, 2016 •

edited

Loading

sg- Sep 28, 2016 •

edited

Loading

geky commented Sep 28, 2016 •

edited

Loading