Event loop #2801

bogdanm · 2016-09-23T19:15:32Z

Description

Adds the Event type and an event loop (EventLoop) for event-based programming.

Status

Main code ready. More tests and more documentation to follow.

Migrations

API changes are backward compatible.

This commit introduces another Mail class, called DynamicMail. The only difference between them is that DynamicMail allocates memory dynamically for the mail-related data structures, while Mail keeps the data structures as part of the instance itself (like it did before). The common implementation of the two classes was factored out into a generic implementation class (GenericMail) that can't be instantiated directly (since it doesn't know anything about the mail-related data structures storage). Also updated the Mail test to test both a Mail and a DynamicMail instance.

Event is a callback that executes either immediately, like a regular callback, or is posted in an event queue for defered execution. EventLoop is a queue of events, built on top of the DynamicMail object. It runs in its own thread and dispatches events as they arrive in the mail.

In the core classes, the callback event type (used for 'attach'-like functions) was 'Callback<void()>'. This commit changes the type to Event, which allows the callbacks to be used both like regular callbacks, or enqueued in an event queue for deferred execution. The commit preserves backward compatibility (the default behaviour is still to run the callbacks immediately (like 'Callback' does), not defer them). Event and Callback have similar constructors and attach() functions, so it was enough to replace one type with the other, without other code changes. Future similar changes to the API should be able to follow the same pattern.

geky · 2016-09-23T21:53:45Z

hal/api/Event.h

+     *  executed immediately (EVENT_CALL_IMMEDIATELY) or enqueued in the
+     *  system queue (EVENT_CALL_ENQUEUE)
+     */
+    void call() const;


Should we also provide an operator() member function for for the Event?

This may be feature creep, but a thunk member function would be useful for C callback APIs:
https://github.com/ARMmbed/mbed-os/blob/master/hal/api/Callback.h#L440

For example:

lwip_network_callback(void (*cb)(void *), void *data); Event ev; lwip_network_callback(&Event::thunk, &ev);

Just missed that you took care of this already 👍

geky · 2016-09-23T21:58:03Z

hal/api/Event.h

+ *  @param queue Pointer to the event queue (NULL to call the event immediately)
+ *  @return     Callback with infered type
+ */
+Event event(void (*func)(void) = 0, EventQueue *queue = 0);


If we allow a null queue here, we've locked ourselves out into ticker.attach(event(doit)) being in irq context.

Currently irq context is possible (and backwards compatible) via the Event overloads:

ticker.attach(callback(doit)); // or just ticker.attach(doit);

It we don't allow a null queue for irq context, this opens up the option of slipping in a system queue in the future:

ticker.attach(callback(doit)); // irq context ticker.attach(event(doit)); // user context on system queue ticker.attach(event(doit, &queue)); // user context on user provided queue

Nice taking care of this already 👍

geky · 2016-09-23T22:07:06Z

hal/api/EventQueue.h

+
+    /** Starts the event queue thread
+     */
+    void start();


I'm not sure about this composition of an EventQueue and Thread. Although marginally more convenient for user created event queues, providing a public dispatch function would be sufficient:

EventQueue queue(32); Thread thread(2048); thread.start(&queue, &EventQueue::dispatch);

This would additionally allow the user to configure the Thread object and take advantage of set_priority, signal_set, and other thread functions, as well as pass the specific thread around to function built on the thread type.

Additionally, a public dispatch function would be more flexible, and resolve the issues with rtos dependencies.

If we aren't planning on exposing a public dispatch member, I would push to rename this EventQueue to EventLoop or something similar, to give us the option of introducing a thread-less EventQueue base class in the future.

Nice taking care of this already 👍

In the current implementation, the event queue doesn't actually work without a Thread. In that sense, I believe the composition is reasonable, since it makes this contract explicit. We could start it separately, yes, but I personally think that the added flexibility adds to the level of confusion of the class user.

geky · 2016-09-23T22:39:46Z

hal/api/Event.h

+ *  @param queue Pointer to the event queue
+ *  @return     Callback with infered type
+ */
+Event event(void (*func)(void), EventLoop *queue);


This may be subjective, but we could provide these convenience functions as member functions on the EventLoop class:

ticker.attach(queue->event(func)); // vs ticker.attach(event(func, &queue));

This avoids ambiguity on if queue effects the event or is just the argument to func.

bogdanm · 2016-09-23T22:42:49Z

Some of the above comments were already addressed. Sorry, push -f :) Figured I start from a clean slate before asking people to CR. But that's out of the window now anyway, so: @sg- @geky please CR :)

bogdanm · 2016-09-23T22:49:09Z

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=GCC_ARM
TARGETS=K64F,NRF51_DK,NRF51_MICROBIT,NUCLEO_F411RE

bogdanm · 2016-09-23T22:49:16Z

/morph test

bogdanm · 2016-09-23T22:49:51Z

Signing out now, will resume looking at this tomorrow.

mbed-bot · 2016-09-23T23:31:00Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: null

Examples Build failed!

mbed-bot · 2016-09-24T00:18:19Z

[Build 975]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

mbed-bot · 2016-09-24T00:18:21Z

[Build ${MBED_BUILD_ID}]
FAILURE: Something went wrong when building and testing.

geky · 2016-09-24T00:20:57Z

hal/common/EventLoop.cpp

+}
+
+void EventLoop::start() {
+    _thread.start(callback(this, &EventLoop::dispatch));


Should this propogate errors from the thread start function?

I will change this, thanks.

geky · 2016-09-24T00:23:44Z

hal/common/EventLoop.cpp

+}
+
+void EventLoop::stop() {
+    _thread.terminate();


Wouldn't _thread.join() be a better way to stop the event queue? This would work well if you had a flag in the actual dispatch loop.

Why would it be better? Is terminate problematic? I couldn't find any references to this in the RTX docs, or our docs.

@bogdanm Unlike terminate, join can allows the user to do a proper cleanup.
Maybe the behavior can be selected with an input parameter:

TERMINATE: terminate right away, current activity is stopped; might lead to resources leak.

STOP_AFTER_CURRENT_EVENT: Stop the event loop once the current event has been processed. The event loop doesn't accept events after this point.

STOP_WHEN_QUEUE_EMPTY: The event loop doesn't accept events after this point. Stop when the queue is empty.

REQUEST_STOP: The event loop accept events after this point. Stop when the queue is empty. This might never stop

I see what you mean, thanks. It seems a bit overkill to provide all those termination modes. It looks like the most useful one is STOP_WHEN_QUEUE_EMPTY (and possibly TERMINATE for immediate and unconditional termination, but even that doesn't feel right after your explanation above).

I added a commit that implements this behaviour (empty the event queue before stopping), please let me know what you think.

pan- · 2016-09-26T11:51:35Z

@bogdanm I'm a bit confused about this PR. While it sounds reasonable for the HAL APIs, I don't see myself use it for the following reasons:

It is tied to a different/specific thread: As a consequence, the callback will not run in the thread that registered it if the registration didn't occur in the event loop thread. It becomes complicated to think about where things happens and how to avoid race condition. Users might harden themselves by putting mutexes everywhere but this increase the surface of deadlocks and lock contentions.

Another issue with this model is the memory wasted in the process:

int main() {
    queue.start();
    sw.rise(event(handler, &queue));
    Thread::wait(osWaitForever);
}

In that scenario, the stack of the main thread will never be used. On a platforms like the NRF51_DK, it is
10% of the memory available to the user (20K) which is wasted. That's huge.

No builtin scheduling: It is a nice idea to defer the recurring events and timed events to the Timeout and Ticker class but I'm not sure it is a practical one: Timeout and Ticker are RAII classes; when an instance is destroyed, the callback registered is detached. In other words, the Ticker and Timeout instances used by this use case have to outlive the scope of the registration.

void bar();

void foo(EventLoop* el) { 
  // Invalid, timeout will be destroyed at the end of the function!
  Timeout timeout;
  timeout.attach(event(bar, el), 1000.0);
}

One solution would be to use a global variable but this doesn't scale well in term of memory usage and it is difficult to use: One global variable have to be declared every time the user code needs a recurring event or a deferred event and the programmer has to makes sure that it is wrapped inside a SingletonPtr or a static inside a function to avoid this variable to be pulled in the final binary if it is not used.

It is not possible to bind any data in a callback; this might be a big blocker for some use cases. Of course it is possible to create structure which pack the argument and a function which unpack them and do the process but it is not very convenient and error prone:

class Foo { 
   process_bar(int a , int b);
}

Foo foo;

EventLoop evl;


//////////////////////////////////////////////////////////////////////
// Current version: 

class BarEvent { 
  friend Callback<void()> make_BarEvent(int a, int b, Foo& f);

   bar_event(int a, int b, Foo& f) : 
      _a(a), _b(b), _f(f) { 
  }

  void execute() { 
    f.process_bar(_a, _f);
    delete this;
  }

   int _a;
   int _b;
   Foo& _f;
}

Callback<void()> make_BarEvent(int a, int b, Foo& f) { 
  bar_event* ev = new BarEvent(a, b, f);
  return Callback<void()>(ev, &BarEvent::execute);
}

void bar() { 
   int a = get_a();
   int b = get_b();
   evl.post(make_BarEvent(a, b, foo));
}

with a binding facility, this code would be much more simpler:

void bar() { 
   int a = get_a();
   int b = get_b();
   evl.post(bind(&Foo::process_bar, &foo, a, b));
}

The Event class would became unnecessary and Callback<void()> would become the universal type for the event loop and general callbacks.

// equivalent to the `event` function
Callback<void()> defer(const Callback<void()>& cb, EventLoop& event_loop)  { 
  return bind(&EventLoop::post, event_loop, cb);
}

With such facility, the intention become more explicit in my opinion (execute directly or defer to an event loop):

EventLoop event_loop;
InterruptIn foo;

void handle_foo_rise();

// direct call
foo.rise(handle_foo_rise);

// deferred  via event_loop
foo.rise(defer(handle_foo_rise, event_loop));

bogdanm · 2016-09-26T12:19:53Z

Hi @pan-. I definitely wasn't aware of some of your requirements for the event loop. To respond to some of your concerns:

initially I tried to "attach" the event loop to the RTX timer thread, but that proved to be impossible due to some RTX limitations. In the end, the only reasonable solution that I could find is to implement a separate thread for the event queue.
while the event loop is indeed tied to a different thread, all the events are serialized. If the user doesn't call the event function manually from somewhere else, and if we assume that there's a single event queue that handles the event function, I don't see a lot of concerns related to synchronization; on the contrary, because events are serialized, synchronization should be easier.
you can use "wait" in the body of the event function if you need to have your event executed later than needed, for example:

void callback() {
    Thread::wait(1000);
    // actual code here
}

Again, not sure if this covers your use cases or not.

binding would be very nice indeed, but we don't have bind() implemented currently, and implementing it is likely a non-trivial effort. We could of course make Event a template and allow it to have different number of parameters (like Callback), but bind is a much nicer solution. But again, we don't have currently a working bind implementation (note that we're not using FunctionPointerBind and its companions in mbed OS 5).

pan- · 2016-09-26T13:05:08Z

while the event loop is indeed tied to a different thread, all the events are serialized. If the user doesn't call the event function manually from somewhere else, and if we assume that there's a single event queue that handles the event function, I don't see a lot of concerns related to synchronization; on the contrary, because events are serialized, synchronization should be easier.

My main concern with that approach is the overhead induced by the use of another thread. About the synchronization issue: all the events are serialized. That's true but they can be serialized in another thread if they are post from a thread different than the one use by the event loop.
Would it be desirable to have an EventLoop class which just embed post and dispatch without dealing with threading at all and another class: EventDispatchThread which embed a thread and an EventLoop ? I think users can benefits from this decomposition.

you can use "wait" in the body of the event function if you need to have your event executed later than needed, for example:

I'm not sure to understand, wait will block the dispatch thread. Maybe other events are waiting in the queue.

binding would be very nice indeed, but we don't have bind() implemented currently, and implementing it is likely a non-trivial effort. We could of course make Event a template and allow it to have different number of parameters (like Callback), but bind is a much nicer solution. But again, we don't have currently a working bind implementation (note that we're not using FunctionPointerBind and its companions in mbed OS 5).

I know bind does require a non-trivial effort and it is certainly out of the scope of this PR. But still, it would be interesting to have such feature in the future.

bogdanm · 2016-09-26T13:18:29Z

Would it be desirable to have an EventLoop class which just embed post and dispatch without dealing with threading at all and another class: EventDispatchThread which embed a thread and an EventLoop.

Yes, this would be possible. I'll implement that.

I'm not sure to understand, wait will block the dispatch thread. Maybe other events are waiting in the queue.

In the current implementation, the event queue doesn't make any guarantees related to timing. Also, the events will always execute in order. So, if an event executes wait, it'll prevent all the other events from running. If there's a practical scenario that requires multiple events to be executed after different timeouts, this will need a different implementation.

pan- · 2016-09-26T14:23:03Z

So, if an event executes wait, it'll prevent all the other events from running. If there's a practical scenario that requires multiple events to be executed after different timeouts, this will need a different implementation.

Just doing a recurrent task (blinking, logging, whatever...) every x ms while doing other processing is, in my opinion, a valid scenario. It is possible to use Ticker or Timeout but as I pointed out earlier, it is neither user friendly or scalable. Using wait in a callback invoked by the event queue will just block other pending processing and might lead very quickly to an overflow of the queue. I think, usage of wait should be avoided.

While I understand it is not possible to have everything solved and sorted out with this PR, I think it would be nice to have some documentation or examples explaining how to currently:

Post recurring events with the help of a Ticker instance.
Post timeout events with the help of a Timeout instance.
Write a callback which Bind and unpack data; manually.

I can help of these items if you want and if you feel that it is reasonable.

geky · 2016-09-26T15:38:00Z

hal/common/EventLoop.cpp

 }

 bool EventLoop::post(const queue_event_t& evt) {
+    if (_stop_request) {
+        // An event loop that's stopping doesn't accept more events.
+        return false;


One concern, this does limit resuming event queues that are acting as state machines after stopping.

EventQueue queue; queue.start(); // run for a while wait_for_important_thing(); queue.stop(); // pause event loop for important thing that may need jitter low do_important_thing(); queue.start(); // resume running, unfortunately events may have been dropped during important thing

An alternative solution would be to poison the EventLoop with a dummy event (pardon my bad pseudocode):

void EventLoop::dispatch(void) { while (true) { e = _queue.get(); if (e.value.p == EVENT_POISON) { break; } } } void EventLoop::stop(void) { post(EVENT_POISON); thread.join(); }

geky · 2016-09-26T15:41:16Z

hal/common/EventLoop.cpp

-void EventLoop::start() {
-    _thread.start(callback(this, &EventLoop::dispatch));
+bool EventLoop::start() {
+    return _thread.start(callback(this, &EventLoop::dispatch)) == osOK;


Hmm, should we actually propagate the osStatus? Or do you think this binds too closesly to the rtos?

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

Replaced by #2828

c1728p9 · 2016-09-29T21:55:04Z

I think there might still be some useful discussion left in this PR.

@pan-, you mentioned having an example showing the use of Ticker and Timeout from the event queue. This is a great idea. It would give me much more confidence in the implementation if one or more of the existing drivers could be updated to use this as a proof of concept.

The way I would envision Ticker and Timeout being used in a driver is as resources that are part of a class. This would both make their use apparent and allocate space for them. Any information that needs to be moved out of an ISR could be done by pushing data onto a circular buffer.

class SomeConnectionHandler {
    ...
    SomeConnectionHandler(EventLoop* event_loop) {
        loop = event_loop;
    }
    ...
    EventLoop * loop;
    Timeout tx_timeout;
    Timeout rx_timeout;
    Ticker state_check;
    CircularBuffer<uint8_t, 1024> data_buffer;
};

bogdanm force-pushed the event_loop branch 2 times, most recently from 38902de to de9b495 Compare September 23, 2016 19:58

Bogdan Marinescu added 2 commits September 24, 2016 01:09

bogdanm force-pushed the event_loop branch from de9b495 to 3df5eca Compare September 23, 2016 22:28

geky reviewed Sep 23, 2016

View reviewed changes

geky approved these changes Sep 23, 2016

View reviewed changes

geky reviewed Sep 24, 2016

View reviewed changes

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

Bogdan Marinescu added 2 commits September 26, 2016 11:26

Added return status to EventLoop::start

25483e2

Added graceful termination for EventLoop::stop()

ab8a107

geky reviewed Sep 26, 2016

View reviewed changes

bogdanm mentioned this pull request Sep 27, 2016

Event loop with mbed-events #2828

Closed

2 tasks

bogdanm closed this Sep 27, 2016

geky mentioned this pull request Sep 29, 2016

Event loop with mbed-events #2860

Merged

2 tasks

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

Event loop #2801

Event loop #2801

Uh oh!

Conversation

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

Uh oh!

Description

Status

Migrations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Sep 23, 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!

geky Sep 23, 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!

geky Sep 23, 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!

bogdanm commented Sep 23, 2016

Uh oh!

bogdanm commented Sep 23, 2016

Uh oh!

bogdanm commented Sep 23, 2016

Uh oh!

bogdanm commented Sep 23, 2016

Uh oh!

mbed-bot commented Sep 23, 2016

Output

Uh oh!

mbed-bot commented Sep 24, 2016

Uh oh!

mbed-bot commented Sep 24, 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!

pan- commented Sep 26, 2016

Uh oh!

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

Uh oh!

Uh oh!

pan- commented Sep 26, 2016

Uh oh!

bogdanm commented Sep 26, 2016

Uh oh!

pan- commented Sep 26, 2016

Uh oh!

geky Sep 26, 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!

bogdanm commented Sep 27, 2016

Uh oh!

bogdanm commented Sep 23, 2016 •

edited

Loading

geky Sep 23, 2016 •

edited

Loading

geky Sep 23, 2016 •

edited

Loading

geky Sep 23, 2016 •

edited

Loading

bogdanm commented Sep 26, 2016 •

edited

Loading

geky Sep 26, 2016 •

edited

Loading