Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

sys/event: add event sources #18758

Merged
merged 2 commits into from
Feb 10, 2023
Merged

sys/event: add event sources #18758

merged 2 commits into from
Feb 10, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Oct 17, 2022

Contribution description

The idea is similar to the message bus, but instead of registering threads to which we send messages, instead we have a list of event handlers that can all be triggered at once.

The advantage is of course that as opposed to messages, events can't be lost.

Testing procedure

A very simple test application has been added to tests/event_bus.

Issues/PRs references

@github-actions github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework labels Oct 17, 2022
@benpicco benpicco force-pushed the event/bus branch 2 times, most recently from db23b3f to 60929de Compare October 17, 2022 17:25
@benpicco benpicco requested a review from kaspar030 November 29, 2022 14:50
@kaspar030
Copy link
Contributor

I like this! And had something similar in mind for a while.
The implementation seems nice and lean.

Just one thing: The queue that events are posted to are part of the event bus, instead of the event_bus_entry.
That saves memory if the events should be triggered event_thread style in some handler thread.
It costs some flexibility, like, if a thread would like to have the event posted to a different (e.g., its own vs some system wide predefined one) queue.
I think I'd prefer to have the target queue as part of the entry struct, I think it allows some nifty patterns, allowing to access thread local state in a synchronized (serialized) manner, ...

Also the queue-ptr-in-bus behavior can be trivially implemented with the queue-ptr-in-entry variant (albeit at the extra cost of the ptr per entry vs ptr per bus).

What do you think?

And bike shedding - naming wise I'm not sure I'm happy with "event bus". What do you think about "event_source", or "event hooks", ...?

@kaspar030
Copy link
Contributor

Did you consider using nested structs vs. pointers? events are already using them.

struct event_bus_entry_t {
  list_node *next;
  event_t event[];  // dynamic struct member
}

struct my_event_entry {
   event_bus_entry_t bus_entry;
   event_callback_t whatever_event; // all event types have an event_t as first member, so this is legal wrt. C's aliasing rules
}

my_event_entry foo;
event_bus_add(&bus, &foo.bus_entry); // it is a bit more clumsy to use

event_t* event_bus_entry_get_event(event_bus_entry_t *entry) { entry->event[0] }; // event is an array (of size 1)

@benpicco
Copy link
Contributor Author

Did you consider using nested structs vs. pointers? events are already using them.

Yes, that's why I use pointers here. This way we can use the bus with events that are already extended (e.g. event_callback_t in the test application)

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 13, 2022
@riot-ci
Copy link

riot-ci commented Dec 13, 2022

Murdock results

✔️ PASSED

3b38b29 tests/event_source: add test for event sources

Success Failures Total Runtime
6808 0 6808 11m:11s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@bergzand
Copy link
Member

And bike shedding - naming wise I'm not sure I'm happy with "event bus". What do you think about "event_source", or "event hooks", ...?

I agree here, to me this reads as an event fan out construct and less as a bus. I would go with something along the lines of "event_hook" or "event_subscription". What do you think?

@bergzand
Copy link
Member

Please squash btw :)

@benpicco
Copy link
Contributor Author

I'm struggling a bit to find consistent naming with non-bus alternatives.

@kaspar030
Copy link
Contributor

kaspar030 commented Jan 17, 2023

How about:

event_bus_t -> event_source_t,
event_bus_entry_t -> event_subscriber_t,
event_bus_add/remove/post() -> event_source_attach/detach/trigger().

event_source_subscriber_t seems awefully long. I think we can get away with a name that doesn't match the prefix because we might have a use for event_subscriber_t elsewhere. :)

@bergzand
Copy link
Member

Sounds good to me.

event_source_subscriber_t

I prefer event_source_subscription_t. Personally I don't mind the slightly longer name.

@kaspar030
Copy link
Contributor

I prefer event_source_subscription_t. Personally I don't mind the slightly longer name.

Would be fine with me, and consistent.

@benpicco
Copy link
Contributor Author

Like so?

@kaspar030 kaspar030 changed the title sys/event: add event bus sys/event: add event sources Jan 19, 2023
@kaspar030
Copy link
Contributor

I think this is good to go, please squash and maybe fix bus->"event sources" (or similar) in the commit messages.

Down the line:

  1. we should provide static inline initializer functions. Easier to reason about, easier to auto-generate Rust bindings for.
  2. we should discuss if clist wouldn't be a better choice, as it provides O(1) append on both ends. That would then keep the event execution order in event insertion order. Not if sure that's beneficial.

This adds an event bus where multiple events can be triggered at once.
@benpicco
Copy link
Contributor Author

Squashed.

@benpicco benpicco added the State: waiting for maintainer State: Action by a maintainer is required label Feb 2, 2023
@benpicco
Copy link
Contributor Author

Still interested in this one?

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

@kaspar030
Copy link
Contributor

I'm curious what we'll build with this!

bors merge

@benpicco benpicco removed the State: waiting for maintainer State: Action by a maintainer is required label Feb 10, 2023
@bors
Copy link
Contributor

bors bot commented Feb 10, 2023

Build succeeded:

@bors bors bot merged commit ea300c3 into RIOT-OS:master Feb 10, 2023
@benpicco benpicco deleted the event/bus branch February 10, 2023 19:01
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants