Rework spawn_service macro to relax constraints on constructors#884
Rework spawn_service macro to relax constraints on constructors#884williampMSFT wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reworks the odp_service_common::runnable_service::Service<'hw> pattern so that spawn_service! no longer depends on a single trait-mandated new(storage, InitParams) constructor shape. Instead, spawn_service! takes a caller-provided initialization closure that receives &'static mut Resources and returns an async Result<(ServiceHandle, Runner), Error>. This preserves the ergonomic “static storage + task spawning” workflow while enabling services to expose multiple constructors and/or constructors that accept temporary borrows without forcing those borrows into an InitParams associated type. The PR also updates several services, examples, and documentation to match the new construction approach.
Changes:
- Removed
ErrorType,InitParams, and the requiredService::new(...)from theService<'hw>trait; constructors are now inherent methods on the service types. - Updated
spawn_service!to accept an init closure and to allocate onlyResourcesstatically (then spawn the runner task). - Updated affected services/tests/examples/docs to use the new constructor +
spawn_service!closure calling convention.
Summary of changes (reviewer-focused)
The core change is shifting constructor flexibility from a trait-associated InitParams into an explicit init closure passed to spawn_service!, while keeping the same runtime model (resources in a StaticCell, runner spawned onto an Embassy executor). Services now implement Service<'hw> purely to declare Resources and Runner, and expose one or more inherent new(...) functions as appropriate. Call sites move from spawn_service!(..., ServiceType, init_params) to spawn_service!(..., ServiceType, |resources| ServiceType::new(resources, ...)). This addresses the “single constructor / non-generic params” limitation described in the PR without changing the expected service runtime behavior.
Step-by-step review guide
-
Trait/API shape change (constructor removed from
Service<'hw>)Service<'hw>becomes a “type-shape” trait: it only specifiesResources: DefaultandRunner: ServiceRunner<'hw>.- Why it matters: callers (and the macro) no longer rely on an associated
InitParamstype, so service authors can define constructors that are generic or accept temporary borrows without encoding them into an associated type.
-
spawn_service!contract change (init closure instead of init params)- The macro now allocates
Resourcesin aStaticCell, then calls the init closure with&'static mut Resources, awaits it, and if successful spawns the runner task. - Non-obvious detail: by awaiting the init closure inside the macro before spawning, you can capture and use non-
'staticborrows during initialization (as long as they don’t escape into the returned handle/runner), which was a key limitation of the previousInitParams-driven pattern.
- The macro now allocates
-
Service updates (constructors moved to inherent
new)- Services now implement
Service<'hw>only forResources/Runner, and place initialization onimpl ServiceType { pub async fn new(...) -> Result<(Self, Runner), E> }. - Why it matters: constructor signatures can now be tailored per service. Example:
time-alarm-servicedrops itsInitParamswrapper and takes the required references directly, while thermal services keep theirInitParamsstruct but no longer bind it via the trait.
- Services now implement
-
Call-site updates (tests/examples/docs)
- Tests and examples are updated to call constructors directly and to adapt
spawn_service!invocations to the closure form. - The developer guidance in
.github/copilot-instructions.mdis updated to reflect the new pattern.
- Tests and examples are updated to call constructors directly and to adapt
Potential issues
No concrete issues found in the reviewed diffs.
| # | Severity | File | Description | Code |
|---|---|---|---|---|
| — | — | — | No issues identified. | — |
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
odp-service-common/src/runnable_service.rs |
Removes constructor requirements from the Service trait and updates spawn_service! to accept an init closure returning (handle, runner). |
time-alarm-service/src/lib.rs |
Removes InitParams and provides an inherent Service::new(...) that takes direct references needed during init. |
time-alarm-service/tests/tad_test.rs |
Updates tests to call the new inherent constructor signature. |
thermal-service/src/sensor.rs |
Moves new(...) from the trait impl into an inherent constructor while keeping InitParams. |
thermal-service/src/fan.rs |
Same constructor relocation as sensor service; keeps InitParams. |
battery-service/src/lib.rs |
Moves new(...) into an inherent impl (preserving the 'hw: 'static constraint there) and simplifies the trait impl. |
espi-service/src/espi_service.rs |
Relocates new(...) to an inherent method and simplifies Service<'hw> impl to type declarations only. |
examples/std/src/bin/thermal.rs |
Updates example usage of spawn_service! to the init-closure calling convention. |
examples/std/src/bin/battery.rs |
Updates example usage of spawn_service! to pass an init closure. |
examples/rt685s-evk/src/bin/time_alarm.rs |
Updates board example to construct the time alarm service via the init-closure form. |
.github/copilot-instructions.md |
Updates repository instructions to reflect the new service construction and spawn_service! usage pattern. |
|
apparently pushed to the wrong repo |
The
spawn_servicemacro / RunnableService trait were intended to standardize the way we launch services that need worker tasks in order to eliminate the repetitiveness required to do all the setup associated with this pattern (declaring static storage, declaring a worker task, etc).However, as part of that, it also imposed the constraint that there's only one way to construct an instance of the service - using an
InitParamstype as an associated type on theRunnableServiceimplementation. This has the unfortunate consequence of meaning that you can't have multiple constructors and your constructors can't be generic over anything that the struct itself is not generic over, which is particularly annoying when you want to take a parameter by reference but only need it for the duration of the constructor.This change reworks the RunnableService trait to not require a new()/InitParams function, and instead take as an argument a closure that yields a Result<(RunnableService, Runner)>. This opens up a lot more flexibility around the shape of constructors while still getting the ergonomics properties of automatic storage declarations and automatic service starting.