[DRAFT] [Time/Alarm] Split interface and relay logic

[williampMSFT]

No description provided.

[Copilot]

Pull request overview

This PR refactors the Time/Alarm subsystem by separating the public service interface/types from the MCTP relay message/serialization logic, turning the time-alarm service into a pure implementation of the interface while introducing a dedicated relay crate.

Changes:

• Introduce time-alarm-service-interface (public types + TimeAlarmService trait) and time-alarm-service-relay (MCTP relay request/response + handler wrapper).
• Update time-alarm-service to depend on the new interface crate and remove its embedded relay handler implementation.
• Relax Send requirements on relay handler futures in embedded-services, and remove UART service’s temporary dependencies on service message crates.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
uart-service/Cargo.toml	Removes temporary *-service-messages deps and related feature flags.
time-alarm-service/src/lib.rs	Switches to implementing TimeAlarmService and drops in-crate relay handler impl.
time-alarm-service/Cargo.toml	Replaces messages dependency with time-alarm-service-interface.
time-alarm-service-relay/src/lib.rs	New relay message types + serialization + relay handler wrapper.
time-alarm-service-relay/Cargo.toml	New crate manifest for relay layer.
time-alarm-service-interface/src/lib.rs	New interface crate defining shared types + TimeAlarmService trait.
time-alarm-service-interface/src/acpi_timestamp.rs	Moves ACPI timestamp parsing/encoding into interface crate.
time-alarm-service-interface/Cargo.toml	Renames messages crate into interface crate; trims deps/features.
embedded-service/src/relay/mod.rs	Removes Send bound from relay handler future return types.
Cargo.toml	Adds new workspace members and updates workspace dependency paths.
Cargo.lock	Reflects crate rename/additions and dependency removals.

Comments suppressed due to low confidence (1)

time-alarm-service-relay/src/lib.rs:314

• TimeAlarmServiceRelayHandler stores the service by value (service: T), which forces consumers to move the control handle into the relay handler even if they also need to keep using it elsewhere. Consider storing a reference (e.g. &'a T / &'a dyn TimeAlarmService) and/or adding a blanket impl<T: TimeAlarmService + ?Sized> TimeAlarmService for &T so T = &Service can satisfy the bound cleanly.

---

💡 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.

[kurtjd]

I really like this, cool idea to decouple the relay interface from the service interface. Should definitely help with composability. I don't really have any additional suggestions; this seems good to me and I'd approve once the checks pass.

[williampMSFT]

I really like this, cool idea to decouple the relay interface from the service interface. Should definitely help with composability. I don't really have any additional suggestions; this seems good to me and I'd approve once the checks pass.

thanks! as currently written, I think it introduces some ergonomics issues with the impl_odp_relay_handler!() macro (you have to declare a bunch of extra staticcells/mutexes) so I'm working through those now, but once I have that solved I'll get this published

[williampMSFT]

Still thinking through this bit - by taking the service by value, I think we implicitly impose the requirement that either we take sole ownership of the service, or the service's control handle must be cloneable.

In the case of the time-alarm service I think it's fine for the control handle to be cloneable, and that's in line with what e.g. embassy_net::Stack does, but that may not generalize well to other services. I think it carries the implication that the service's internal reference must be non-mut, which is true of the time-alarm service but may not be for other services that want external synchronization.

Maybe the answer here is that for services that want to be relayable and also have methods that take &mut self, their relay handler implementations need to be generic over a Lockable or something? Would be interested in others' thoughts on this one

[kurtjd]

So, I'm guessing the issue with consuming the service by value and taking ownership is the possibility we may want multiple wrappers similar to the relay handler wrapper in the future, wrapping the same service simultaneously?

For interfaces that only take &self, another option is we could just blanket impl TimeAlarmService for &T so at least callers have the option if they want the relay service to own T or share it with others.

[kurtjd]

So far, I think relayable services at least only use methods that take &self in their implementations of RelayServiceHandler. But I agree I think it makes sense to let the service decide how it wants to handle this, whether with a generic Lockable or whatever, if it needs &mut self for its RelayServiceHandler implementation.

[williampMSFT]

Yeah, more generally it's that we may want to share access to a service between multiple clients, of which a relay handler is one.

In the case of the time-alarm service, you need a handle to the service to be able to query the RTC for wall-clock time, so any service that wants to be able to do that needs to also get a handle to it somehow. For a system-wide service like the time-alarm service, this is probably fine; most time APIs I'm aware of work this way (where you don't need an exclusive handle to the clock to read the time). That implies that the service is using interior mutability via a mutex or something, and in those cases, we can just make the handle cloneable and give everyone a handle. That's what I've done here, and it's in line with what embassy_net::Stack does.

This falls apart if the interface has any &mut methods on it, though, which I think is more likely on things that we may not want to share / want to use exterior mutability on. The cyw43 driver is a good example for this - you need to use a &mut reference to e.g. join a wifi network. I know Surface has expressed interest in doing this on some classes of services (USB-C comes to mind), so I want to make sure we have a path to enabling those use cases too. I think it might involve making the relay agent generic over a Lockable<ServiceHandle> or something?

I think you're right that all the currently relayable services are only taking &self rather than &mut self, but I think in some cases that may be vestigial from the comms system, where locking had to be the responsibility of the service because the comms system itself needed a reference to the service to be able to deliver messages. Do you know if there are any methods on relayable services that we may want to move to taking &mut self? If not, maybe we punt on demonstrating the lockable thing for now - I'm pretty sure it'd work, it's just a bit verbose (because in addition to putting the Resources object in a static / StaticCell, you also have to put the control handle in a static StaticCell<Mutex<ControlHandle>> or something. That may be unavoidable if we want &mut methods on the interface trait, though?

@asasine @JamesHuard @RobertZ2011 thoughts on this approach?

[kurtjd]

Could you clarify which part you mean should be generic over a Lockable? Maybe I'm a bit confused where exactly you see that (in RelayHandler? RelayServiceHandler?)

[williampMSFT]

Oh, yeah, the RelayServiceHandler would have to be generic, e.g. if the FooService trait needed to have a method that took &mut self, then that implies that the FooServiceRelayHandler would have to be generic over an impl Lockable<impl FooService> instead of just impl FooService (syntax is probably a bit off but you get the idea). It'd then have to take a &'hw impl Lockable<impl FooService> at construction time, which means the caller would probably have to set all that up inside a StaticCell or similar. It's doable, but a lot of boilerplate. I don't see a clean way around that, though

[Copilot]

Pull request overview

This PR splits the Time/Alarm functionality into a hardware-facing interface crate and a separate relay/serialization crate, and updates the service + example usage accordingly. It also adjusts the embedded relay traits/macro to make relay handlers owned values rather than borrowed references.

Changes:

• Introduce time-alarm-service-interface (types + TimeAlarmService trait) and time-alarm-service-relay (MCTP message serialization + relay handler wrapper).
• Update time-alarm-service and its tests/examples to use the new interface trait and move relay handling out of the service crate.
• Update embedded relay traits/macro to drop Send from returned futures and make generated relay handlers own their service handlers.

Reviewed changes

Copilot reviewed 12 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
uart-service/Cargo.toml	Removes temporary message-crate dependencies and their feature wiring.
time-alarm-service/tests/tad_test.rs	Updates tests to use the new interface crate/types and import TimeAlarmService.
time-alarm-service/src/lib.rs	Switches to interface types/trait; removes in-crate MCTP relay handler impl.
time-alarm-service/Cargo.toml	Swaps dependency from messages crate to interface crate; feature wiring updated.
time-alarm-service-relay/src/serialization.rs	Moves Time/Alarm request/response serialization into relay crate and reuses interface types.
time-alarm-service-relay/src/lib.rs	Adds TimeAlarmServiceRelayHandler<T> implementing relay traits by delegating to TimeAlarmService.
time-alarm-service-relay/Cargo.toml	New crate manifest for relay wrapper/serialization.
time-alarm-service-interface/src/lib.rs	New interface crate: ACPI types + TimeAlarmService trait.
time-alarm-service-interface/src/acpi_timestamp.rs	New ACPI timestamp layout/serialization implementation using zerocopy.
time-alarm-service-interface/Cargo.toml	Renames/repurposes the old messages crate into an interface crate.
examples/rt685s-evk/src/bin/time_alarm.rs	Updates example to use interface + new relay handler wrapper.
examples/rt685s-evk/Cargo.toml	Updates example deps to include the new interface + relay crates.
examples/rt685s-evk/Cargo.lock	Lockfile updates for new crates.
embedded-service/src/relay/mod.rs	Removes Send from relay futures; updates macro to store handler types by value.
Cargo.toml	Adds new crates to workspace members and workspace dependencies.
Cargo.lock	Lockfile updates for crate renames/additions and removed deps.

---

💡 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.