[RFC] Uniform task spawning (trait version)

embedded-service/src/lib.rs

@@ -23,6 +23,7 @@ pub mod ipc;
 pub mod keyboard;
 pub mod power;
 pub mod relay;
+pub mod service;
 pub mod sync;
 pub mod type_c;
 

embedded-service/src/service.rs

@@ -0,0 +1,87 @@
+//! This module contains helper traits and functions for services that run on the EC.
+
+/// A trait for a service that can be run on the EC.
+/// Implementations of RunnableService 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 RunnableService<'hw>: Sized {
+    /// A token type used to restrict users from spawning more than one service runner.  Services will generally
+    /// define this as a zero-sized type and only provide a constructor for it that is private to the service module,
+    /// which prevents users from constructing their own tokens and spawning multiple runners.
+    /// Most services should consider using the `impl_runner_creation_token!` macro to do this automatically.
+    type Runner: ServiceRunner<'hw>;
+
+    /// 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 in the provided OnceLock and returns a reference to the service and
+    /// a runner that can be used to run the service.
+    fn init(
+        storage: &'hw embassy_sync::once_lock::OnceLock<Self>, // TODO could be resources?
+        params: Self::InitParams,
+    ) -> impl core::future::Future<Output = Result<(&'hw Self, Self::Runner), Self::ErrorType>>;
+}
+
+/// A trait for a run handle used to execute a service's event loop.  This is returned by RunnableService::init()
+/// and the user is expected to call its run() method in an embassy task (or similar parallel execution context
+/// on other async runtimes).
+pub trait ServiceRunner<'hw> {
+    /// Run the service event loop. This future never completes.
+    fn run(self) -> impl core::future::Future<Output = crate::Never> + 'hw;
+    // TODO: Do we want to take &mut self instead of consuming self? I think the difference is that it allows for the possibility of
+    //       the user select!()ing over the ServiceRunner and something else, then having that other thing complete and bailing
+    //       out of execution. In the consume-self version, the user can't restart afterward, but in the &mut self version they could
+    //       potentially restart the runner.  It's not clear to me if we have any use cases for the 'restartable runner' version, and if
+    //       we don't then the consume-self version more clearly telegraphs the fact that the runner is not meant to be restarted or
+    //       reused after it's started and lets the implementor care less about drop safety on the future
+}
+
+/// 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
+///
+/// Returns a Result<reference-to-service, Error> where Error is the error type of $service_ty::init().
+///
+/// Arguments
+///
+/// - spawner:    An embassy_executor::Spawner.
+/// - service_ty: The service type that implements RunnableService that you want to create and run.
+/// - init_arg:   The init argument type to pass to `Service::init()`
+///
+/// Example:
+///
+/// ```ignore
+/// let time_service = embedded_services::spawn_service!(
+///     spawner,
+///     time_alarm_service::Service<'static>,
+///     time_alarm_service::ServiceInitParams { dt_clock, tz, ac_expiration, ac_policy, dc_expiration, dc_policy }
+/// ).expect("failed to initialize time_alarm service");
+/// ```
+#[macro_export]
+macro_rules! spawn_service {
+    ($spawner:expr, $service_ty:ty, $init_arg:expr) => {{
+        use $crate::service::RunnableService;
+        use $crate::service::ServiceRunner;
+        static SERVICE: embassy_sync::once_lock::OnceLock<$service_ty> = embassy_sync::once_lock::OnceLock::new();
+        match <$service_ty>::init(&SERVICE, $init_arg).await {
+            Ok((service_ref, runner)) => {
+                #[embassy_executor::task]
+                async fn service_task_fn(runner: <$service_ty as $crate::service::RunnableService<'static>>::Runner) {
+                    runner.run().await;
+                }
+
+                $spawner.must_spawn(service_task_fn(runner));
+                Ok(service_ref)
+            }
+            Err(e) => Err(e),
+        }
+    }};
+}
+
+pub use spawn_service;

espi-service/src/espi_service.rs

@@ -75,32 +75,68 @@ pub struct Service<RelayHandler: embedded_services::relay::mctp::RelayHandler> {
 //
 
 ///////// COMMON FUNCTIONS ///////////
-impl<RelayHandler: embedded_services::relay::mctp::RelayHandler> Service<RelayHandler> {
+
+pub struct Runner<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> {
+    service: &'hw Service<RelayHandler>,
+}
+
+impl<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> embedded_services::service::ServiceRunner<'hw>
+    for Runner<'hw, RelayHandler>
+{
+    async fn run(self) -> embedded_services::Never {
+        self.service.run().await
+    }
+}
+
+pub struct ServiceInitParams<RelayHandler: embedded_services::relay::mctp::RelayHandler> {
+    pub espi: espi::Espi<'static>,
+    pub relay_handler: RelayHandler,
+}
+
+impl<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler + 'hw>
+    embedded_services::service::RunnableService<'hw> for Service<RelayHandler>
+where
+    'hw: 'static, // TODO we should be able to relax this constraint when we remove the dependency on the comms service
+{
     // TODO a lot of the input lifetimes here have to be static because we have a dependency on the comms system, which requires
     //      that everything that talks over it is 'static. Once we eliminate that dependency, we should be able to relax these lifetimes.
-    pub async fn init(
-        service_storage: &'static OnceLock<Self>,
-        mut espi: espi::Espi<'static>,
-        relay_handler: RelayHandler,
-    ) -> &'static Self {
-        espi.wait_for_plat_reset().await;
-
-        let result = service_storage.get_or_init(|| Service {
+    async fn init(
+        service_storage: &'hw OnceLock<Self>,
+        mut params: Self::InitParams,
+    ) -> Result<(&'hw Self, Runner<'hw, RelayHandler>), core::convert::Infallible> {
+        params.espi.wait_for_plat_reset().await;
+
+        let service = service_storage.get_or_init(|| Service {
             endpoint: DEPRECATED_comms::Endpoint::uninit(DEPRECATED_comms::EndpointID::External(
                 DEPRECATED_comms::External::Host,
             )),
-            espi: Mutex::new(espi),
+            espi: Mutex::new(params.espi),
             host_tx_queue: Channel::new(),
-            relay_handler,
+            relay_handler: params.relay_handler,
         });
 
-        DEPRECATED_comms::register_endpoint(result, &result.endpoint)
+        DEPRECATED_comms::register_endpoint(service, &service.endpoint)
             .await
             .unwrap();
-        result
+
+        Ok((service, Runner { service }))
     }
 
-    pub(crate) async fn run_service(&self) -> ! {
+    type ErrorType = core::convert::Infallible;
+    type InitParams = ServiceInitParams<RelayHandler>;
+    type Runner = Runner<'hw, RelayHandler>;
+}
+
+impl<RelayHandler: embedded_services::relay::mctp::RelayHandler> Service<RelayHandler> {
+    // TODO The notification system was not actually used, so this is currently dead code.
+    //      We need to implement some interface for triggering notifications from other subsystems, and it may do something like this:
+    //
+    // async fn process_notification_to_host(&self, espi: &mut espi::Espi<'_>, notification: &NotificationMsg) {
+    //     espi.irq_push(notification.offset).await;
+    //     info!("espi: Notification id {} sent to Host!", notification.offset);
+    // }
+
+    async fn run(&self) -> embedded_services::Never {
         let mut espi = self.espi.lock().await;
         loop {
             let event = select(espi.wait_for_event(), self.host_tx_queue.receive()).await;
@@ -120,14 +156,6 @@ impl<RelayHandler: embedded_services::relay::mctp::RelayHandler> Service<RelayHa
         }
     }
 
-    // TODO The notification system was not actually used, so this is currently dead code.
-    //      We need to implement some interface for triggering notifications from other subsystems, and it may do something like this:
-    //
-    // async fn process_notification_to_host(&self, espi: &mut espi::Espi<'_>, notification: &NotificationMsg) {
-    //     espi.irq_push(notification.offset).await;
-    //     info!("espi: Notification id {} sent to Host!", notification.offset);
-    // }
-
     fn write_to_hw(&self, espi: &mut espi::Espi<'static>, packet: &[u8]) -> Result<(), embassy_imxrt::espi::Error> {
         // Send packet via your transport medium
         // SAFETY: Safe as the access to espi is protected by a mut reference.

espi-service/src/lib.rs

@@ -25,8 +25,5 @@ mod espi_service;
 #[cfg(not(test))]
 mod mctp;
 
-#[cfg(not(test))]
-pub mod task;
-
 #[cfg(not(test))]
 pub use espi_service::*;

examples/rt685s-evk/src/bin/time_alarm.rs

@@ -1,7 +1,6 @@
 #![no_std]
 #![no_main]
 
-use embassy_sync::once_lock::OnceLock;
 use embedded_mcu_hal::{
     Nvram,
     time::{Datetime, Month, UncheckedDatetime},
@@ -24,25 +23,19 @@ async fn main(spawner: embassy_executor::Spawner) {
     embedded_services::init().await;
     info!("services initialized");
 
-    static TIME_SERVICE: OnceLock<time_alarm_service::Service> = OnceLock::new();
-    let time_service = time_alarm_service::Service::init(
-        &TIME_SERVICE,
-        dt_clock,
-        tz,
-        ac_expiration,
-        ac_policy,
-        dc_expiration,
-        dc_policy,
+    let time_service = embedded_services::spawn_service!(
+        spawner,
+        time_alarm_service::Service<'static>,
+        time_alarm_service::ServiceInitParams {
+            backing_clock: dt_clock,
+            tz_storage: tz,
+            ac_expiration_storage: ac_expiration,
+            ac_policy_storage: ac_policy,
+            dc_expiration_storage: dc_expiration,
+            dc_policy_storage: dc_policy
+        }
     )
-    .await
-    .expect("Failed to initialize time-alarm service");
-
-    #[embassy_executor::task]
-    async fn time_alarm_task(service: &'static time_alarm_service::Service<'static>) {
-        time_alarm_service::task::run_service(service).await
-    }
-
-    spawner.must_spawn(time_alarm_task(time_service));
+    .expect("Failed to spawn time alarm service");
 
     use embedded_services::relay::mctp::impl_odp_mctp_relay_handler;
     impl_odp_mctp_relay_handler!(