Skip to content

Rework spawn_service macro to relax constraints on constructors#884

Closed
williampMSFT wants to merge 1 commit into
mainfrom
user/williamp/spawn-cleanup
Closed

Rework spawn_service macro to relax constraints on constructors#884
williampMSFT wants to merge 1 commit into
mainfrom
user/williamp/spawn-cleanup

Conversation

@williampMSFT

Copy link
Copy Markdown
Collaborator

The spawn_service macro / 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 InitParams type as an associated type on the RunnableService implementation. 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.

Copilot AI review requested due to automatic review settings June 8, 2026 20:47
@williampMSFT williampMSFT requested review from a team as code owners June 8, 2026 20:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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 required Service::new(...) from the Service<'hw> trait; constructors are now inherent methods on the service types.
  • Updated spawn_service! to accept an init closure and to allocate only Resources statically (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

  1. Trait/API shape change (constructor removed from Service<'hw>)

    • Service<'hw> becomes a “type-shape” trait: it only specifies Resources: Default and Runner: ServiceRunner<'hw>.
    • Why it matters: callers (and the macro) no longer rely on an associated InitParams type, so service authors can define constructors that are generic or accept temporary borrows without encoding them into an associated type.
  2. spawn_service! contract change (init closure instead of init params)

    • The macro now allocates Resources in a StaticCell, 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-'static borrows during initialization (as long as they don’t escape into the returned handle/runner), which was a key limitation of the previous InitParams-driven pattern.
  3. Service updates (constructors moved to inherent new)

    • Services now implement Service<'hw> only for Resources/Runner, and place initialization on impl ServiceType { pub async fn new(...) -> Result<(Self, Runner), E> }.
    • Why it matters: constructor signatures can now be tailored per service. Example: time-alarm-service drops its InitParams wrapper and takes the required references directly, while thermal services keep their InitParams struct but no longer bind it via the trait.
  4. 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.md is updated to reflect the new pattern.

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.

@williampMSFT

Copy link
Copy Markdown
Collaborator Author

apparently pushed to the wrong repo

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants