[RFC] Notification/Standard broadcasting v2 by kurtjd · Pull Request #745 · OpenDevicePartnership/embedded-services

kurtjd · 2026-03-11T23:04:04Z

This RFC is another attempt at designing a notification system (see #741) that's hopefully a bit more closely aligned to what everyone's thinking. Additionally, tried to attempt a form of standardization for service message broadcasting based on some discussions I had offline with Billy.

At a high level, basically what I'm thinking is:

Have a wrapper around a PubSubChannel I call SinglePublisherChannel that only allows a single DynPublisher (intended to be consumed by a single service) but SUBS number of DynSubscribers. The dyn variants were used since the cost of dynamic dispatch seems minimal and this helps prevent needing CAPS and SUBS generics to bleed into everything using this broadcasting system.
In the service-messages crates, add a new ServiceMessage enum type (e.g. TimeAlarmServiceMessage) that is used by the service's broadcasting channel.
During service setup on a platform, we create a new SinglePublisherChannel for every service that wants to broadcast messages. Those services will have a field in their structs for a DynPublisher that we pass into their constructor. Services that want to subscribe to another service will then need to update their structs to hold a field for a DynPublisher of each service they want to subscribe to (e.g. the power policy service might want to hold a DynSubscriber<TimeAlarmMessage> to receive timer expired messages).

Then for notifications:

We piggy back off the broadcasting system by similarly holding a DynSubscriber for each Service provided in the relay macro in the generated relay struct.
We then add a wait_notification method to the Relay trait which the macro implements by waiting for a message to be received on any of the DynSubscribers it holds.
To determine if any of these messages are a notification, the RelayServiceTrait now has a is_notification method that each service needs to implement to match a message variant to a notification.
This is just a bool since the relay service will discard the payload (if any). Instead, the macro now requires an additional notification id associated with each service. Basically, if the relay handler receives a notification, it discards the payload and returns the associated notification id for the service the notification id belongs to, and the relay service can then directly use this id without needing to know anything about which service produced it.

I've updated the time-alarm-service and espi-service to show how these might be used and also updated the time-alarm example to show how it would all be integrated together.

Depends on #740 so there are a lot of changes shown here that are actually just from #740 so it might be clearer to review the details of this PR after that is merged.

@RobertZ2011 and @williampMSFT curious to see if this is closer to how you were envisioning message broadcasting. I've looked at the existing broadcaster/immediate.rs to see if it could be reworked with intrusive list removed but to be honest, I don't know the context behind its design and it's not clear to me yet exactly what it is trying to solve over a raw PubSubChannel broadcaster. But I'm open to seeing if we can make use of that instead.

…ner-service pattern

Copilot

Pull request overview

This RFC introduces a v2 approach for service message broadcasting and relay notifications by standardizing on a single-publisher pubsub channel per service, adding per-service message types, and extending the relay handler macro/traits to wait for notifications across subscribed services.

Changes:

Add SinglePublisherChannel (wrapper over embassy_sync::PubSubChannel) and new per-service *Message types for broadcasting.
Introduce odp-service-common with a (control_handle, Runner) service pattern and spawn_service! helper macro; migrate time-alarm and eSPI services accordingly.
Extend relay traits + impl_odp_mctp_relay_handler! to accept per-service subscribers and implement wait_for_notification() using is_notification().

Reviewed changes

Copilot reviewed 25 out of 29 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
time-alarm-service/tests/tad_test.rs	Updates tests to new `(Service, Runner)` init/run pattern (currently missing `message_publisher` init param).
time-alarm-service/src/task.rs	Removes legacy “run the service task” wrapper.
time-alarm-service/src/lib.rs	Refactors into `Resources`/`Runner`/control-handle + adds `DynPublisher` for broadcasting `TimeAlarmMessage`.
time-alarm-service/Cargo.toml	Adds dependency on `odp-service-common`.
time-alarm-service-messages/src/lib.rs	Adds `TimeAlarmMessage` enum for broadcast/notification classification.
thermal-service/src/lib.rs	Adds `ThermalMessage` + implements `is_notification`/`MessageType`.
thermal-service-messages/src/lib.rs	Adds `ThermalMessage` type.
odp-service-common/src/runnable_service.rs	Introduces `Service`/`ServiceRunner` traits and `spawn_service!` macro.
odp-service-common/src/lib.rs	New crate root exporting `runnable_service`.
odp-service-common/Cargo.toml	New crate manifest for shared service infrastructure.
examples/rt685s-evk/src/bin/time_alarm.rs	Updates example to create `SinglePublisherChannel`, spawn service via `spawn_service!`, and pass subscriber into relay handler.
examples/rt685s-evk/Cargo.toml	Adds `odp-service-common` dependency.
examples/rt685s-evk/Cargo.lock	Lockfile update reflecting new crate + dependency revisions.
examples/rt633/Cargo.lock	Lockfile update reflecting dependency revisions.
espi-service/src/task.rs	Legacy task helper signature adjusted but now stale vs new runner-based design.
espi-service/src/lib.rs	Stops exporting task module; re-exports new implementation.
espi-service/src/espi_service.rs	Migrates to `Resources`/`Runner` pattern and integrates `wait_for_notification()` into main select loop.
espi-service/Cargo.toml	Replaces message-crate dependencies with `odp-service-common`.
embedded-service/src/relay/mod.rs	Extends relay traits/macro: adds `MessageType`, `is_notification`, and `wait_for_notification` with per-service subscribers + notification ids.
embedded-service/src/lib.rs	Re-exports `embassy_sync` in `_macro_internal` for macro expansion.
embedded-service/src/broadcaster/single_publisher.rs	Adds `SinglePublisherChannel` wrapper and unit tests.
embedded-service/src/broadcaster/mod.rs	Exposes new `single_publisher` broadcaster module.
docs/api-guidelines.md	Adds repository-wide API guidelines documenting external resources + runner-based concurrency patterns.
debug-service/src/debug_service.rs	Adds `DebugMessage` and relay `MessageType`/`is_notification` implementation.
debug-service-messages/src/lib.rs	Adds `DebugMessage` type.
battery-service/src/lib.rs	Adds `BatteryMessage` and relay `MessageType`/`is_notification` implementation.
battery-service-messages/src/lib.rs	Adds `BatteryMessage` type.
Cargo.toml	Adds `odp-service-common` to workspace members and workspace deps.
Cargo.lock	Lockfile update reflecting new crate + dependency adjustments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T23:09:27Z

time-alarm-service/tests/tad_test.rs

+                ac_expiration_storage: &mut ac_exp_storage,
+                ac_policy_storage: &mut ac_pol_storage,
+                dc_expiration_storage: &mut dc_exp_storage,
+                dc_policy_storage: &mut dc_pol_storage,
+            },


time_alarm_service::InitParams now includes a required message_publisher: DynPublisher<TimeAlarmMessage> field, but this test constructs InitParams without it, so it won't compile. Consider creating a local SinglePublisherChannel<TimeAlarmMessage, ..> (or PubSubChannel) in the test, obtaining a publisher, and passing it in (or make the publisher optional in InitParams if tests/services should be able to run without broadcasting).

Copilot · 2026-03-11T23:09:27Z

embedded-service/src/relay/mod.rs

    /// ```ignore
    ///
    ///     impl_odp_mctp_relay_handler!(
    ///         MyRelayHanderType;


Typo in the macro docs: MyRelayHanderType should be MyRelayHandlerType to match the later usage and avoid copy/paste confusion.

Suggested change

/// MyRelayHanderType;

/// MyRelayHandlerType;

Copilot · 2026-03-11T23:09:27Z

embedded-service/src/relay/mod.rs

+                                                    $crate::_macro_internal::embassy_sync::pubsub::WaitResult::Lagged(count) => {
+                                                        // Revisit: This can only happen if other services use a `publish_immediate` on their channel, which can result in older messages getting discarded.
+                                                        // We really don't want notifications potentially getting lost, so we could change `SinglePublisherChannel` to not allow immediate publishing,
+                                                        // or we could just keep the burden on services so they have the fleixibility to use `publish_immediate` if they want to at the risk of their own notifications being lost.


Typo in comment: fleixibility should be flexibility.

Suggested change

// or we could just keep the burden on services so they have the fleixibility to use `publish_immediate` if they want to at the risk of their own notifications being lost.

// or we could just keep the burden on services so they have the flexibility to use `publish_immediate` if they want to at the risk of their own notifications being lost.

Copilot · 2026-03-11T23:09:28Z

odp-service-common/src/runnable_service.rs

+/// 3. Defining an embassy_executor::task to run the service
+/// 4. Spawning the task on the provided executor
+///
+/// Returns a Result<&Service, Error> where Error is the error type of $service_ty::new().


The macro docs claim spawn_service! returns Result<&Service, Error>, but the macro actually returns the control_handle value returned by Service::new(...) (not necessarily a reference). Updating this docstring would prevent misleading API consumers.

Suggested change

/// Returns a Result<&Service, Error> where Error is the error type of $service_ty::new().

/// Returns a `Result<ControlHandle, Error>` where `ControlHandle` is the control handle type

/// returned by `$service_ty::new()` and `Error` is the error type of `$service_ty::new()`.

Copilot · 2026-03-11T23:09:28Z

odp-service-common/src/runnable_service.rs

+/// let time_service = odp_service_common::runnable_service::spawn_service!(
+///     spawner,
+///     time_alarm_service::Service<'static>,
+///     time_alarm_service::ServiceInitParams { dt_clock, tz, ac_expiration, ac_policy, dc_expiration, dc_policy }


The spawn_service! example uses time_alarm_service::ServiceInitParams, but the current API in this PR uses time_alarm_service::InitParams. Align the example with the actual type name to keep the docs copy/pasteable.

Suggested change

/// time_alarm_service::ServiceInitParams { dt_clock, tz, ac_expiration, ac_policy, dc_expiration, dc_policy }

/// time_alarm_service::InitParams { dt_clock, tz, ac_expiration, ac_policy, dc_expiration, dc_policy }

Copilot · 2026-03-11T23:09:28Z

espi-service/src/task.rs

 pub async fn espi_service<'hw, R: embedded_services::relay::mctp::RelayHandler>(
-    espi_service: &'hw Service<'hw, R>,
+    espi_service: &'hw mut Service<'hw, R>,
 ) -> Result<embedded_services::Never, crate::espi_service::Error> {
    espi_service.run_service().await
 }


This helper still calls Service::run_service(), but the service was refactored to the (control_handle, Runner) pattern and run_service no longer exists. Since lib.rs no longer declares/exports this module, consider deleting this file or updating it to accept a Runner and call runner.run() to avoid leaving broken, stale code behind.

Copilot · 2026-03-11T23:09:29Z

time-alarm-service/tests/tad_test.rs

+                ac_expiration_storage: &mut ac_exp_storage,
+                ac_policy_storage: &mut ac_pol_storage,
+                dc_expiration_storage: &mut dc_exp_storage,
+                dc_policy_storage: &mut dc_pol_storage,
+            },


time_alarm_service::InitParams requires message_publisher, but this test's InitParams { ... } literal omits it, which should be a compile error. Add a publisher sourced from a local SinglePublisherChannel<TimeAlarmMessage, ..> / PubSubChannel and pass it via message_publisher: ....

Copilot · 2026-03-11T23:09:29Z

embedded-service/src/relay/mod.rs

-        ) -> impl core::future::Future<Output = Self::ResultEnumType> + Send + 'a;
+        ) -> impl core::future::Future<Output = Self::ResultEnumType> + 'a;
+
+        /// Wait for a notification from any service and return the associated service ID.


The wait_for_notification docstring says it returns the associated service ID, but the implementation returns the per-service notification id ($service_notification_id). Update the comment to avoid confusing callers about what the returned u8 represents.

Suggested change

/// Wait for a notification from any service and return the associated service ID.

/// Wait for a notification from any service and return the per-service notification ID (`u8`).

williampMSFT and others added 18 commits March 10, 2026 13:10

Add support for uniform spawning of services using the resources + in…

70ce52d

…ner-service pattern

replace unnecessary trait with default

3392cb1

derive default

c19c1c2

Move common helper code to new crate

f39a32f

readme

afe558e

api guidelines

fe8e408

ci fixes

9bd110d

doc

eb2587f

example cargo.lock

a1ecc5d

sp

23cc098

doc

af8d6e4

doc

5fa35be

update example code to disambiguate "my"

642c9ff

update docs

eabbbe8

comment

b16157c

update docs on interface patterns

7657af8

docs

5d3d0c9

Notification v2

30fbc76

kurtjd self-assigned this Mar 11, 2026

Copilot AI review requested due to automatic review settings March 11, 2026 23:04

Copilot started reviewing on behalf of kurtjd March 11, 2026 23:04 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Notification/Standard broadcasting v2#745

[RFC] Notification/Standard broadcasting v2#745
kurtjd wants to merge 18 commits intoOpenDevicePartnership:v0.2.0from
kurtjd:notification-v2

kurtjd commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// or we could just keep the burden on services so they have the fleixibility to use `publish_immediate` if they want to at the risk of their own notifications being lost.
	// or we could just keep the burden on services so they have the flexibility to use `publish_immediate` if they want to at the risk of their own notifications being lost.

	/// Returns a Result<&Service, Error> where Error is the error type of $service_ty::new().
	/// Returns a `Result<ControlHandle, Error>` where `ControlHandle` is the control handle type
	/// returned by `$service_ty::new()` and `Error` is the error type of `$service_ty::new()`.

	/// time_alarm_service::ServiceInitParams { dt_clock, tz, ac_expiration, ac_policy, dc_expiration, dc_policy }
	/// time_alarm_service::InitParams { dt_clock, tz, ac_expiration, ac_policy, dc_expiration, dc_policy }

	/// Wait for a notification from any service and return the associated service ID.
	/// Wait for a notification from any service and return the per-service notification ID (`u8`).

Conversation

kurtjd commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants