[DRAFT] [Time/Alarm] Split interface and relay logic#748
[DRAFT] [Time/Alarm] Split interface and relay logic#748williampMSFT wants to merge 4 commits intoOpenDevicePartnership:v0.2.0from
Conversation
There was a problem hiding this comment.
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 +TimeAlarmServicetrait) andtime-alarm-service-relay(MCTP relay request/response + handler wrapper). - Update
time-alarm-serviceto depend on the new interface crate and remove its embedded relay handler implementation. - Relax
Sendrequirements on relay handler futures inembedded-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
TimeAlarmServiceRelayHandlerstores 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 blanketimpl<T: TimeAlarmService + ?Sized> TimeAlarmService for &TsoT = &Servicecan 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.
| impl<'hw> TimeAlarmService for Service<'hw> { | ||
| fn get_capabilities(&self) -> TimeAlarmDeviceCapabilities { | ||
| self.inner.get_capabilities() | ||
| } | ||
|
|
| embedded-mcu-hal.workspace = true | ||
| embedded-services.workspace = true | ||
| odp-service-common.workspace = true | ||
| time-alarm-service-messages.workspace = true | ||
| time-alarm-service-interface.workspace = true | ||
| zerocopy.workspace = true |
| time-alarm-service-interface.workspace = true | ||
|
|
||
| [features] | ||
| defmt = ["dep:defmt", "embedded-mcu-hal/defmt"] |
|
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 |
…control handle must be cloneable, which may not be appropriate for all services? might be able to get by with requiring a mutex for services for which it isn't? TBD, need to proof-of-concept some of the other services
| } | ||
|
|
||
| impl<T: TimeAlarmService> TimeAlarmServiceRelayHandler<T> { | ||
| pub fn new(service: T) -> Self { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 +TimeAlarmServicetrait) andtime-alarm-service-relay(MCTP message serialization + relay handler wrapper). - Update
time-alarm-serviceand 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
Sendfrom 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.
| time-alarm-service-interface.workspace = true | ||
|
|
||
| [features] | ||
| defmt = ["dep:defmt", "embedded-mcu-hal/defmt"] |
| /// | ||
| /// impl_odp_mctp_relay_handler!( | ||
| /// MyRelayHanderType; | ||
| /// Battery, 0x9, battery_service::Service<'static>; | ||
| /// Battery, 0x9, battery_service::Service<'static>; // TODO update example to reflect new expectation for service handler | ||
| /// TimeAlarm, 0xB, time_alarm_service::Service<'static>; | ||
| /// ); |
| @@ -196,7 +196,7 @@ pub mod mctp { | |||
| /// | |||
| /// impl_odp_mctp_relay_handler!( | |||
| /// MyRelayHanderType; | |||
No description provided.