Event loop with mbed-events #2860

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

Description

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

Status

Ready for PR, more documentation to follow.

Related PRs

List related PRs against other branches:

branch	PR
master	#2828

Todos

Tests
Documentation

bogdanm · 2016-09-29T09:47:40Z

More documentation willl follow, but the basic usage is this:

#include "mbed.h"
#include "mbed_events.h"

using events::EventQueue;

DigitalOut led1(LED1);
InterruptIn sw(SW2);
EventQueue queue(1024);
Thread t;

void handler(void) {
    printf("Context: %p\r\n", Thread::gettid());
    led1 = !led1;
}

int main() {
    t.start(callback(&queue, &EventQueue::dispatch_forever));
    printf("Starting in context %p\r\n", Thread::gettid());
    sw.rise(handler); // handler will run in interrupt context
    sw.fall(queue.event(handler)); // handler will run in the context of 't'
    Thread::wait(osWaitForever);
}

bogdanm · 2016-09-29T09:47:55Z

@sg- @geky

bogdanm · 2016-09-29T10:08:04Z

Also, @pan-

bridadan · 2016-09-29T14:40:08Z

@bogdanm I don't want to get too annoying with naming (seeing how I already asked you to move everything 😄), but should we remove the mbed- prefix from the mbed-events directory? The hal and rtos folders don't have this prefix either.

geky

This looks great! Excited to see this get in!

Added mbed-events from https://github.com/ARMMbed/mbed-events. Changes from upstream: - the whole code is licensed under the Apache license. Sources and headers were updates with this information. - removed the porting layers for Windows and FreeRTOS and the references to these porting layers in equeue_platform.h. - moved the TESTS directory in mbed-events to the TESTS directory of mbed-os.

bogdanm · 2016-09-29T15:44:51Z

I forced-push a new version that just changed the name of the mbed-events folder to events.

bridadan · 2016-09-29T15:51:38Z

Awesome!

/morph test

bogdanm · 2016-09-29T16:48:14Z

I also added some documentation about how to use the event loop in mbed-os. It's not much (yet), but it should be enough to get people started, I think.

geky · 2016-09-29T16:56:20Z

docs/events.md

+#include "mbed.h"
+#include "mbed_events.h"
+
+using events::EventQueue;


Should we provide the events library through the mbed.h header similarly to the rtos?
https://github.com/ARMmbed/mbed-os/blob/master/hal/api/mbed.h#L21-L23

Another note is that this using statement is not needed. The mbed_events.h entry point un-namespaces the library to be consistent with the rtos and nsapi (for better or worse):
https://github.com/ARMmbed/mbed-os/pull/2860/files#diff-de8f685be747e42feaafbdfaabed4d4fR28

Should we provide the events library through the mbed.h header similarly to the rtos?

I'm inclined to say "no" for two reasons:

it is a completely optional component at the moment.

mbed classic builds compatibility. We'd have to add a configuration to events/ and add a conditional inclued in mbed.h. Not a big deal, I'm just not sure it's worth it.

it is a completely optional component at the moment.

This is also true of the nsapi, and that library is also exposed through mbed.h (for better or worse). It's even under the features directory.

If it's a conditional inclusion (such as rtos and nsapi) than users can opt-out by overriding the MBED_CONF_EVENTS_PRESENT config option.

I don't have a strong feeling if you think we should keep the mbed-events as opt-in, just concerned about consistency.

👍 for consistency. To the API guidelines for clarification...

geky · 2016-09-29T16:58:16Z

docs/events.md

+    sw.rise(rise_handler);
+    // The 'fall' handler will execute in the context of thread 't'
+    sw.fall(queue.event(fall_handler));
+    Thread::wait(osWaitForever);


It should be fine to let main exit, so I'm not sure this line is necessary.

geky · 2016-09-29T17:07:11Z

That's a very nice write up for introducing people to interrupt/queue contexts : )

mbed-bot · 2016-09-29T17:17:03Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1003

Build failed!

bridadan · 2016-09-29T17:43:52Z

Welp, looks like our friend KL25Z is the only target that failed to build for IAR. It only has 16KB of RAM :/

Maybe this test needs to be broken apart a bit?

geky · 2016-09-29T18:50:13Z

TESTS/events/queue/main.cpp

+    queue.dispatch(N*100);
+}
+
+struct big { char data[1024]; } big;


It looks like the test issue is with this allocation. It looks like IAR's linker can't find a contiguous region of space?

We may want to omit this test (allocate_failure_test1) for now. It has had other problems such as overflowing stacks, and allocation failure is also tested by allocate_failure_test2.

c1728p9

Was there any discussion around having a class that has no scheduling and executes enqueued tasks immediately? In many use cases, the scheduling functionality is not needed. An execute-only queue has a simpler user API and implementation. It also has a fixed time enqueue and dequeue. Using the existing Ticker class with an execute-only queue would provide nearly the same functionality as a scheduling queue. If a scheduling queue is needed, then it could inherit from the basic queue and be a separate class.

The above approach would be simpler and use more of the existing code with which users are familiar but would have the disadvantage of being less powerful. I'm not necessarily advocating for this, but I want to make sure that we have not overlooked anything simpler and more user-friendly. The last thing I want to see is an API that we are stuck with that has been designed hastily and is more complicated than it needs to be.

CC: @sg- @pan- @geky @bogdanm

c1728p9 · 2016-09-29T17:39:42Z

events/equeue/equeue_platform.h

+// Platform mutex type
+//
+// The equeue library requires at minimum a non-recursive mutex that is
+// safe in interrupt contexts. The mutex section is help for a bounded


typo - I think you meant held rather than help. Also, this is a bit confusing since mutexes are never safe in interrupt context.

The definition of a mutex does not disclude irq-safety:
https://en.wikipedia.org/wiki/Mutual_exclusion

In retrospect, the name may have not been the best choice, given the connotation of the name "mutex". However it's not unreasonable given that the user-space implementation is often the normal blocking mutex.

In mbed, this is implemented as a critical section, and the next line details that disabling interrupts is an acceptable implementation.

c1728p9 · 2016-09-29T18:18:24Z

events/equeue/equeue.h

+// and acts as a handle to the underlying event. If there is not enough memory
+// to allocate the event, equeue_alloc returns null.
+void *equeue_alloc(equeue_t *queue, size_t size);
+void equeue_dealloc(equeue_t *queue, void *event);


You mention that fragmentation and runtime "grows linearly as the quantity of different sized allocations increases." As an alternative to this, could the memory management be decoupled from the queue in at least the C layer? The equeue_event struct has a "dtor" member which could be used to allow a user supplied free callback. This would allow the native rtos slab allocators to be used with true constant time (even for different sized chunks). It would also allow the standard library new and malloc to be used if the object is created on a thread.

The equeue library has some interesting properties for fixed-size allocations:

Constant time allocation

Constant time deallocation

Constant time enqueueing

Zero fragmentation

There is no asymptotic benefit to using a slab allocator, and the biggest downside of the rtos's allocators is that they can't handle different sized chunks.

If the user wants to use a custom allocator, they can reserve a single word of space for the pointer required, and the resulting usage will have the same runtime complexity as a redesigned equeue without memory management. This is actually provided already as the convenient equeue_call function.

However, a memory-less C api is useless in the context of the C++ api, where the size of the callback itself is unbounded.

I don't see a benefit to separating the memory management component from the rest of the event queue, other than complicating the C api.

geky · 2016-09-29T20:35:14Z

@c1728p9, long story short: yes

You are describing @bogdanm's initial proposal: #2801

@pan- had a good write-up of his concerns based directly on his experience integrating the events library with BLE (#2801 (comment)):

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.

I agree, a simple rtos::Mailbox<Callback<void()>> would provide an elegant solution if we only want to defer callbacks from interrupt context. But it's simply insufficient for asynchronous applications (ie network protocols that need to handle multiple external events/timeouts) and adds the same necessary user overhead (how many events do I need to support). The missing functionality becomes especially noticable in the context of libraries migrating from mbed 3.

c1728p9 · 2016-09-29T21:53:18Z

LWIP and other drivers which support asynchronous operation are able to operate on a queue which provides no scheduling so I'm not convinced the simpler approach is insufficient. What would really give me confidence in one of the solutions is to compare how they both look when hooked up to a more complicated driver that will/is using it. Something like BLE, Nanostack/Client or Atmel RF driver among other libraries. I have a couple of days off so if I'm not lazy I might try to implement this on a driver. What is your timeframe for merging this PR?

geky · 2016-09-29T22:17:51Z

LWIP is an excellent example where a full scheduler was needed to handle async operations:
https://github.com/ARMmbed/lwip/blob/34682facd1f3c6d0c8c167df100e7fb63e90b37e/src/core/timeouts.c#L189-L205

c1728p9 · 2016-09-29T23:06:30Z

@geky, lwip depends on a mailbox implementation in the porting layer (sys_arch_mbox_fetch), which does not have any scheduling functionality. LWIP provides its own scheduling internally. For libraries which don't provide scheduling, Ticker and Timeout could be used.

sg- · 2016-09-29T23:27:09Z

What is your timeframe for merging this PR?

EoD Friday 30th Sept.

bogdanm · 2016-09-30T07:32:38Z

I'm unsure on why are we going over this again? I thought we already discussed the disadvantages of the initial approach and went against it?

@geky

This tests doesn't run on some MCUs with low RAM. According to @geky: "We may want to omit this test (allocate_failure_test1) for now. It has had other problems such as overflowing stacks, and allocation failure is also tested by allocate_failure_test2."

The inclusion is conditioned by the presence of the events library (`MBED_CONF_EVENTS_PRESENT`). This ensures backward compatibility with SDK builds.

geky · 2016-09-30T16:44:33Z

/morph test

mbed-bot · 2016-09-30T17:17:37Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: null

Examples Build failed!

bogdanm · 2016-09-30T17:39:51Z

The CI failures may have something to do with #2869. @geky

bridadan · 2016-09-30T18:05:50Z

@bogdanm The issue actually is that the BLE examples currently pull in the event loop as a separate library (mbed-events), so there's redefinition errors 😄

My suggestion is to remove these lines from the testing data here: https://github.com/ARMmbed/mbed-os/blob/master/tools/test/examples/examples.json#L3-L6

That will stop the current BLE examples from being tested. Those examples will then need to be updated to use the next release (probably test em manually), then we'll add them back to the testing data so they get tested on future PRs.

As mbed-events is being brought into the mbed-os tree, the examples using the external mbed-events library will fail due to duplicate definitions. The tests for BLE examples will be readded after the examples are updated to only use the mbed-events library in the mbed-os tree.

geky · 2016-09-30T18:44:38Z

/morph test

mbed-bot · 2016-09-30T20:18:44Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1015

All builds and test passed!

bogdanm mentioned this pull request Sep 29, 2016

Event loop with mbed-events #2828

Closed

2 tasks

geky approved these changes Sep 29, 2016

View reviewed changes

bogdanm force-pushed the event_loop_mbed_events_new branch from 93484ff to e7abc11 Compare September 29, 2016 15:44

Added initial documentation for the event loop

8545bf1

geky reviewed Sep 29, 2016

View reviewed changes

A few improvements to the events documentation

0abd141

geky reviewed Sep 29, 2016

View reviewed changes

c1728p9 reviewed Sep 29, 2016

View reviewed changes

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

Bogdan Marinescu added 2 commits September 30, 2016 17:01

Include events library headers in mbed.h

53218f9

The inclusion is conditioned by the presence of the events library (`MBED_CONF_EVENTS_PRESENT`). This ensures backward compatibility with SDK builds.

sg- added ready for merge needs: CI and removed needs: review ready for merge labels Sep 30, 2016

bridadan added ready for merge and removed needs: CI labels Sep 30, 2016

sg- merged commit 20756cb into master Sep 30, 2016

sg- added release-version: 5.2.0 and removed ready for merge labels Sep 30, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event loop with mbed-events #2860

Event loop with mbed-events #2860

bogdanm commented Sep 29, 2016

bogdanm commented Sep 29, 2016

bogdanm commented Sep 29, 2016

bogdanm commented Sep 29, 2016

bridadan commented Sep 29, 2016 •

edited

Loading

geky left a comment

bogdanm commented Sep 29, 2016

bridadan commented Sep 29, 2016

bogdanm commented Sep 29, 2016

geky Sep 29, 2016

bogdanm Sep 29, 2016

geky Sep 29, 2016

sg- Sep 29, 2016 •

edited

Loading

geky Sep 29, 2016

geky commented Sep 29, 2016 •

edited

Loading

mbed-bot commented Sep 29, 2016

bridadan commented Sep 29, 2016

geky Sep 29, 2016 •

edited

Loading

c1728p9 left a comment

c1728p9 Sep 29, 2016

geky Sep 29, 2016

c1728p9 Sep 29, 2016

geky Sep 29, 2016

geky commented Sep 29, 2016 •

edited

Loading

c1728p9 commented Sep 29, 2016

geky commented Sep 29, 2016

c1728p9 commented Sep 29, 2016

sg- commented Sep 29, 2016

bogdanm commented Sep 30, 2016

geky commented Sep 30, 2016

mbed-bot commented Sep 30, 2016

bogdanm commented Sep 30, 2016

bridadan commented Sep 30, 2016

geky commented Sep 30, 2016

mbed-bot commented Sep 30, 2016

Event loop with mbed-events #2860

Event loop with mbed-events #2860

Conversation

bogdanm commented Sep 29, 2016

Description

Status

Related PRs

Todos

bogdanm commented Sep 29, 2016

bogdanm commented Sep 29, 2016

bogdanm commented Sep 29, 2016

bridadan commented Sep 29, 2016 • edited Loading

geky left a comment

Choose a reason for hiding this comment

bogdanm commented Sep 29, 2016

bridadan commented Sep 29, 2016

bogdanm commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg- Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky commented Sep 29, 2016 • edited Loading

mbed-bot commented Sep 29, 2016

Output

bridadan commented Sep 29, 2016

geky Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

c1728p9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky commented Sep 29, 2016 • edited Loading

c1728p9 commented Sep 29, 2016

geky commented Sep 29, 2016

c1728p9 commented Sep 29, 2016

sg- commented Sep 29, 2016

bogdanm commented Sep 30, 2016

geky commented Sep 30, 2016

mbed-bot commented Sep 30, 2016

Output

bogdanm commented Sep 30, 2016

bridadan commented Sep 30, 2016

geky commented Sep 30, 2016

mbed-bot commented Sep 30, 2016

Output

bridadan commented Sep 29, 2016 •

edited

Loading

sg- Sep 29, 2016 •

edited

Loading

geky commented Sep 29, 2016 •

edited

Loading

geky Sep 29, 2016 •

edited

Loading

geky commented Sep 29, 2016 •

edited

Loading