Add relayable service event plumbing#866
Conversation
Assisted-by: GitHub Copilot:claude-opus-4.6 Signed-off-by: Kurtis Dinelle <kdinelle@microsoft.com>
There was a problem hiding this comment.
Pull request overview
This PR adds foundational “relayable service event” plumbing across the relay framework and begins integrating it into the UART relay task so it can react to both host requests and service-originated events.
Changes:
- Extend
RelayServiceHandlerTypeswith anEventTypeassociated type and add a macro-generatedServiceEventwrapper plusevent_mux()constructor for muxing relayable service event receivers. - Introduce new generic event receiver adapters (
MapReceiver,FilterReceiver,MuxReceiver,NeverReceiver) to support mapping/filtering/multiplexing event streams. - Update
uart-servicetoselect()between host requests and a “notifiable event” receiver, and refactor UART read handling to be cancel-safe via persistentReadState.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
uart-service/src/task.rs |
Selects between host requests and incoming notifiable events; adds placeholder notification handling. |
uart-service/src/lib.rs |
Refactors request reading into a cancel-safe ReadState + wait_for_request/process_request split. |
uart-service/Cargo.toml |
Adds embassy-futures dependency for select(). |
embedded-service/src/relay/mod.rs |
Adds EventType to relay handler types; macro now generates ServiceEvent and event_mux(). |
embedded-service/src/event.rs |
Adds receiver utilities: map/filter/mux plus a never-producing receiver for ergonomic macro construction. |
thermal-service-interface/src/lib.rs |
Introduces thermal_service_interface::Event wrapping sensor/fan events. |
thermal-service-relay/src/lib.rs |
Hooks relay handler EventType to the new thermal event type. |
battery-service-relay/src/lib.rs |
Defines relay EventType as () (placeholder). |
time-alarm-service-relay/src/lib.rs |
Defines relay EventType as () (placeholder). |
debug-service/src/debug_service.rs |
Defines relay EventType as () (placeholder). |
Cargo.lock |
Records new embassy-futures dependency for uart-service. |
| /// `Receiver`, a `DynamicReceiver`, or a custom impl). | ||
| /// | ||
| /// The caller will then need to further filter this event mux to whatever | ||
| /// events they consider worth notifying the host about. |
There was a problem hiding this comment.
I think all this filtering stuff belongs in the relay handlers?
There was a problem hiding this comment.
Thought about that, but I was trying to maximize flexibility in that I'm not sure if we should assume that all users will agree on what events are considered "notifiable". Maybe some users have platforms where they want to notify the host in case of sensor error where others don't.
I think the relay handlers need to take something that emits an event at construction time (or have a way to interrogate the service they're passed for a pubsubclient but I think that's incompatible with the DIY broadcast thing we're doing) Refers to: time-alarm-service-relay/src/lib.rs:17 in 4651f80. [](commit_id = 4651f80, deletion_comment = False) |
|
|
||
| // Allows this generated relay type to be publicly re-exported | ||
| pub use [< _odp_impl_ $relay_type_name:snake >]::$relay_type_name; | ||
| pub use [< _odp_impl_ $relay_type_name:snake >]::ServiceEvent; |
There was a problem hiding this comment.
Hm, though with the current approach: the caller has to filter and map the MUXed event receiver, the caller needs to know about the ServiceEvent type no?
Understood, but the challenge was coming up with a solution for this while still supporting the design we are heading toward: services only take a list of senders and don't own any backing channel. So I haven't been able to come up with a way to merry both these approaches together unfortuantely. |
|
sorry, to clarify, I'm asserting that the relay handler should own a backing channel. The services can continue to do the DIY-broadcast thing if they want, but a relay handler has exactly one client and the shape of the messages that it emits are different than the shape of the messages that the services emit. It can emit messages to its channel based on one of the DIY channels from the underlying service, and it'll need a handle to that at construction time In reply to: 4566389330 |
I can play around with this but that means we now need a task for the relay service that is dedicated to listening to all the receivers for each service, then sending them onto a new channel, which the relay serivce then also needs to listen on (as opposed to the current approach I have where there is no middle man like that). Not exactly sure what that will look like but can experiment. |
It might not need to be a literal embassy |
I think I get what you're saying... I originally wanted to not return a separate |
As discussed, a grand unified notification handling system is not really feasible, so this PR does a few things.
First, it adds some plumbing though that should make it easier for all relay services to handle notifications to host:
Second, updated the uart service to turn events into notifications. I had to make the
wait_for_requestmethod cancel-safe, so split it up and added an external struct for storing read state in case it is cancelled. This now allows us to select over requests from the host and events from the services.The uart service now takes a "notifiable event" receiver. This is the part I'm not entirely happy with and can't think of a very clean approach. The problem is a relay service like the uart service doesn't know what services ahead of time will be declared relayable by the relay macro, and additionally it doesn't know which events the caller wants to consider notifiable.
And the way the MCTP uart service might choose to handle notifications over the wire is different from say HID I3C. So, I added an additional FilterReceiver type with the idea that the caller first needs to take the MUXed receiver produced by the macro, filter it for events they consider notifiable on their platform, then finally map them to a single u8 which the uart service will TX to the host (with the assumption that the caller ensures host agrees on what these u8s mean). Right now that part is TODO because we have to get the plumbing all the way through otherwise this can cause issues.
The reason I'm not too happy with this is because the caller has to handle creating a channel for each relayable service, ensuring the receiver passed to the
event_muxfunction belongs to the same channel that the sender which was passed to the service belongs to, and then they have to set up the filter+map of events to something the relay service understands. But again, a clean solution to this is not coming to me that still allows flexibility for all kinds of relay serivces... You can see an example of how it's setup in this branch on odp-ec: https://github.com/kurtjd/odp-embedded-controller/blob/baa55f1bc6af1839f27e092d9602e27dbd8a1aa3/platform/dev-qemu/src/main.rs#L37Resolves #733
Assisted-by: GitHub Copilot:claude-opus-4.6