Skip to content

Add relayable service event plumbing#866

Open
kurtjd wants to merge 2 commits into
OpenDevicePartnership:mainfrom
kurtjd:relay-event-support
Open

Add relayable service event plumbing#866
kurtjd wants to merge 2 commits into
OpenDevicePartnership:mainfrom
kurtjd:relay-event-support

Conversation

@kurtjd
Copy link
Copy Markdown
Member

@kurtjd kurtjd commented May 27, 2026

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:

  • Introduces several new generic Receiver types: A MapReceiver which is self-explanatory and a MuxReceiver which allows an arbitrary number of heterogeneous Receiver types to be combined into a single Receiver. The relay handler macro for MCTP relay services uses these to construct a mapped and muxed receiver of all relayable service events which relay service tasks can make use of. Also added a FilterReceiver for reasons I'll discuss in next paragraph.
  • Adds associated types to service relay traits for specifying the event that relayable service emits (added a ThermalEvent which just wraps Sensor and Fan events, and left the other services unit type for now until we know what they want to emit).

Second, updated the uart service to turn events into notifications. I had to make the wait_for_request method 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_mux function 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#L37

Resolves #733

Assisted-by: GitHub Copilot:claude-opus-4.6

Assisted-by: GitHub Copilot:claude-opus-4.6
Signed-off-by: Kurtis Dinelle <kdinelle@microsoft.com>
@kurtjd kurtjd self-assigned this May 27, 2026
Copilot AI review requested due to automatic review settings May 27, 2026 20:20
@kurtjd kurtjd requested a review from a team as a code owner May 27, 2026 20:20
@github-project-automation github-project-automation Bot moved this to In progress in ODP v0.2 May 27, 2026
@github-project-automation github-project-automation Bot moved this to Backlog in ODP Backlog May 27, 2026
@kurtjd kurtjd moved this from Backlog to In review in ODP Backlog May 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 RelayServiceHandlerTypes with an EventType associated type and add a macro-generated ServiceEvent wrapper plus event_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-service to select() between host requests and a “notifiable event” receiver, and refactor UART read handling to be cancel-safe via persistent ReadState.

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.

Comment thread embedded-service/src/event.rs Outdated
Comment thread uart-service/src/task.rs
Comment thread uart-service/src/task.rs Outdated
RobertZ2011
RobertZ2011 previously approved these changes May 27, 2026
/// `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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think all this filtering stuff belongs in the relay handlers?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@williampMSFT
Copy link
Copy Markdown
Collaborator

}

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;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ServiceEvent

I don't think we want to emit a hardcoded name into the surrounding namespace - we may want this to be an associated type on the RelayHandler trait to avoid this problem

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

@kurtjd
Copy link
Copy Markdown
Member Author

kurtjd commented May 28, 2026

}

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)

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.

@williampMSFT
Copy link
Copy Markdown
Collaborator

williampMSFT commented May 28, 2026

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

@kurtjd
Copy link
Copy Markdown
Member Author

kurtjd commented May 28, 2026

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.

@williampMSFT
Copy link
Copy Markdown
Collaborator

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 Channel - maybe just a 'next_notification(&self)` function on the relayhandler that blocks until a notification is ready, and that can be implemented by waiting on the DIY channel or something like that? I don't think you need an extra task for this, just a select over "host wants me to do something" and "notification came in"

@kurtjd
Copy link
Copy Markdown
Member Author

kurtjd commented May 28, 2026

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 Channel - maybe just a 'next_notification(&self)` function on the relayhandler that blocks until a notification is ready, and that can be implemented by waiting on the DIY channel or something like that? I don't think you need an extra task for this, just a select over "host wants me to do something" and "notification came in"

I think I get what you're saying... I originally wanted to not return a separate event_mux function that the user had to call and just tie it directly to the relay service handler like you described, but it gets tricky because the Receiver trait takes &mut which makes it hard to select over two futures which both borrow self. Would have to come up with some interior mutability approach I guess using mutexes. I'll try to think of a good way around this some more but nothing immediately came to mind when I thought about it originally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review
Status: In progress

Development

Successfully merging this pull request may close these issues.

embedded-services: Add support for notifications

4 participants