[RFC] Uniform service spawning with external 'resources'#740
Conversation
…ner-service pattern
There was a problem hiding this comment.
Pull request overview
Introduces an RFC service initialization/spawning pattern in embedded-services based on explicit Resources storage and a Runner handle, and applies it to the time-alarm and eSPI services (plus updating an example and tests) to reduce per-service task boilerplate and make service startup uniform.
Changes:
- Add
embedded_services::service::{Service, ServiceResources, ServiceRunner}traits and aspawn_service!()macro to standardize service construction and task spawning. - Refactor
time-alarm-serviceandespi-serviceto use(Service, Runner)returned fromService::new(...)with externalResources, removing the oldtaskmodules. - Update the RT685 example and time-alarm tokio tests to use the new APIs.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
embedded-service/src/service.rs |
Adds the new service traits and spawn_service!() macro for uniform initialization/spawning. |
embedded-service/src/lib.rs |
Exposes the new service module publicly. |
time-alarm-service/src/lib.rs |
Refactors time-alarm into Resources + Runner + control-handle Service implementing the new trait. |
time-alarm-service/src/task.rs |
Removes the old dedicated task wrapper API. |
time-alarm-service/tests/tad_test.rs |
Updates tokio tests to construct resources and drive the new Runner. |
examples/rt685s-evk/src/bin/time_alarm.rs |
Switches the example to spawn_service!() and updates relay handler wiring accordingly. |
espi-service/src/espi_service.rs |
Refactors eSPI service into Resources + Runner + control-handle Service implementing the new trait. |
espi-service/src/task.rs |
Removes the old task wrapper API. |
espi-service/src/lib.rs |
Stops exporting the removed task module. |
Comments suppressed due to low confidence (4)
embedded-service/src/service.rs:27
- Docs here still refer to an
init()API, but the trait method is namednew(...). Please update the wording so it matches the current public API and avoids sending users to a non-existentinit()method.
/// A trait for a service that can be run on the EC.
/// Implementations of Service should have an init() function to construct the service that
/// returns a Runner, which the user is expected to spawn a task for.
pub trait Service<'hw>: Sized {
/// A type that can be used to run the service. This is returned by the init() function and the user is
/// expected to call its run() method in an embassy task (or similar parallel execution context on other
/// async runtimes).
type Runner: ServiceRunner<'hw>;
/// Any memory resources that your service needs. This is typically an opaque types that is only used by the service
/// and is not interacted with by users of the service. Must be default-constructible for spawn_service!() to work.
type Resources: ServiceResources;
/// The error type that your `init` function can return on failure.
type ErrorType;
/// Any initialization parameters that your service needs to run.
type InitParams;
/// Initializes an instance of the service using the provided storage and returns a control handle for the service and
/// a runner that can be used to run the service.
fn new(
storage: &'hw mut Self::Resources,
params: Self::InitParams,
) -> impl core::future::Future<Output = Result<(Self, Self::Runner), Self::ErrorType>>;
embedded-service/src/service.rs:51
spawn_service!docs mention creating aOnceLockand callinginit(), but the macro body usesStaticCell+Service::new(...). Please update this doc block to reflect the actual storage mechanism and call flow.
/// Initializes a service, creates an embassy task to run it, and spawns that task.
///
/// This macro handles the boilerplate of:
/// 1. Creating a `static` [`OnceLock`](embassy_sync::once_lock::OnceLock) to hold the service
/// 2. Calling the service's `init()` method
/// 3. Defining an embassy_executor::task to run the service
/// 4. Spawning the task on the provided executor
time-alarm-service/src/lib.rs:300
Resources::new()is only provided via theServiceResourcestrait impl, so callers must import that trait just to construct resources. Consider adding an inherentpub fn new()(orDefault) and have the trait impl forward to it, to make manual instantiation ergonomic.
impl<'hw> embedded_services::service::ServiceResources for Resources<'hw> {
/// Allocate storage for the service's resources.
fn new() -> Self {
Resources { inner: None }
espi-service/src/espi_service.rs:45
Resources::new()is only available via theServiceResourcestrait method, so callers need that trait in scope (or UFCS) to construct resources. If you expect manual instantiation, consider adding an inherentpub fn new()/Defaultand forwarding the trait method to it.
{
fn new() -> Self {
Self { inner: None }
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@asasine Robert mentioned you might have an interest in this - would love to get your feedback :) |
|
Hey @williampMSFT, he's right, this is interesting. I think it has a lot of merit and we've been experimenting with a similar idea internally. We ultimately walked back on something as predefined like We found that the perceived rigidity of a The main benefit we did discover was adding new traits for the behavior, which effectively makes the It's possible that a consolidated |
This is a proof-of-concept attempt to create a common pattern for initializing and starting the services in embedded-services. It demonstrates this pattern on the time-alarm and espi service.
Currently, starting the various services in embedded-services is pretty complicated. Each service has a different set of 'worker tasks' that users have to declare and start, each of which has varying numbers of parameters of varying types. It's very easy to miss one, and whenever a change comes in that changes what state a given task needs to function, that's a breaking change that, in the best case, causes a build failure, and in the worst case causes the service to silently misbehave.
This attempts to solve this disparity with a few tricks:
select!()over a list of worker tasks. This works because worker 'green threads' were already intended to never return, so the select never returns either. This lets us execute them all against a single embassy task.Service,ServiceRunner) tuple. TheServiceRunnerimplementation which has a single method that takes no arguments and never returns. This has a few benefits:ServiceRunnerobject back when the instantiate the service, and if they don't use it the compiler will complain about unused variables. They can bypass the warning with a leading_, but that's an explicit action the programmer has to take. I haven't found a way to outright prevent this class of error, unfortunately.&'hw mut Resourcesas an argument at service creation time. This Resources object is the 'real' service, and by taking it as a mutable borrow rather than a reference-to-oncelock or similar, we avoid prescribing any particular way of storing the memory required to run the service.spawn_service!()macro that does all the boilerplate of declaring storage/tasks and spawning the tasks on Embassy for a given service. In product use cases, this makes it outright impossible to forget to start the runner, and reduces the ~10 lines of somewhat verbose boilerplate required for each service down to zero.This approach aligns with a what a few other embassy services are doing - see embassy_net::new for a good example.
Some notes on testing -
The
spawn_service!()macro isn't useful in test scenarios where you need to run on tokio, but in those situations you can manually callnew()andrun()with stack variables thanks to (3). In order to have the long-running task future terminate at the end of the test, you can simplyselect()overrunner.run()and some test code; when the test code returns, the worker 'green thread' stops and the test exits. This technique is demonstrated in tad_test.rs.