Rework spawn_service macro to relax constraints on constructors

[williampMSFT]

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]

Pull request overview

This PR refactors the odp_service_common::runnable_service service-launching pattern to remove the enforced InitParams/Service::new(...) shape from the Service<'hw> trait. Instead, spawn_service! now accepts a caller-provided initialization closure that constructs (Service, Runner) using the allocated Resources, enabling more flexible constructors (including ones that take temporary borrows). The change updates multiple services, examples, and docs to the new pattern while keeping the overall “Resources + Runner + spawn macro” architecture intact.

Changes:

• Simplified Service<'hw> to only require Runner and Resources, moving constructors to inherent impl blocks.
• Updated spawn_service! to accept an initialization closure (FnOnce(&'static mut Resources) -> Future<Output = Result<(Service, Runner), E>>).
• Adjusted services/tests/examples/docs to use the closure-based spawning and/or inherent constructors.

Step-by-step review guide (conceptual)

1. Trait shape change (Service<'hw>)

   • The trait no longer dictates constructor signature or error/params associated types; it’s now a lightweight “service metadata” trait (Runner/Resources).
   • Reviewers should confirm this still supports the intended standardization goals and doesn’t reduce discoverability too much (constructors are now purely inherent APIs).

2. Macro rework (spawn_service!)

   • The macro now allocates Resources in a StaticCell, calls the provided closure to construct (control_handle, runner), and then spawns a task to run the runner.
   • This is the most important piece to scrutinize for correctness because it centralizes spawning behavior.

3. Service-level constructor migrations

   • Several services moved from trait Service::new(...) to impl Service { pub async fn new(...) ... }.
   • Review the updated constructors for signature ergonomics and consistency with existing patterns (many services still use an InitParams struct even though it’s no longer required).

Potential issues

#	Severity	File	Description	Code
1	High	odp-service-common/src/runnable_service.rs:81	spawn_service! calls .expect(...) on service_task_fn(runner) instead of on the spawn(...) result (API misuse).	$spawner.spawn(service_task_fn(runner).expect("Failed to spawn service task"));
2	Medium	time-alarm-service/src/lib.rs:379-386	Service::new now takes many positional parameters, increasing the risk of argument mis-ordering; most other services keep an InitParams struct for named fields.	pub async fn new(... backing_clock, tz_storage, ac_expiration_storage, ac_policy_storage, dc_expiration_storage, dc_policy_storage, ...)

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
time-alarm-service/tests/tad_test.rs	Updates tests to call the new inherent Service::new(...) signature (no InitParams).
time-alarm-service/src/lib.rs	Removes InitParams and migrates construction to an inherent Service::new(...) with explicit parameters.
thermal-service/src/sensor.rs	Moves constructor from trait method to inherent Service::new(...) while keeping InitParams.
thermal-service/src/fan.rs	Moves constructor from trait method to inherent Service::new(...) while keeping InitParams.
odp-service-common/src/runnable_service.rs	Refactors Service trait and rewrites spawn_service! to accept an init closure.
examples/std/src/bin/thermal.rs	Updates example usage to pass an init closure to spawn_service!.
examples/std/src/bin/battery.rs	Updates example usage to pass an init closure to spawn_service!.
examples/rt685s-evk/src/bin/time_alarm.rs	Updates example usage to pass an init closure to spawn_service! and call the new constructor signature.
espi-service/src/espi_service.rs	Moves constructor from trait method to inherent Service::new(...) while keeping InitParams.
battery-service/src/lib.rs	Moves constructor from trait method to inherent Service::new(...) while keeping InitParams.
.github/copilot-instructions.md	Updates repository documentation to reflect the new closure-based spawn_service! pattern and inherent constructors.

Comments suppressed due to low confidence (1)

odp-service-common/src/runnable_service.rs:82

• The spawn_service! macro is calling .expect(...) on service_task_fn(runner) and passing that into Spawner::spawn(...), instead of calling .expect(...) on the spawn(...) result. This is an API misuse and prevents proper error handling when spawning the task.

            .await
            .map(|(control_handle, runner)| {
                $spawner.spawn(service_task_fn(runner).expect("Failed to spawn service task"));
                control_handle

[kurtjd]

Looks good and makes sense to me.

[jerrysxie]

In the end, there is always closure.