[RFC] Uniform task spawning (structural-typed 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,133 @@
+//! 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> {
+    /// 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 RunnerCreationToken;
+
+    /// Run the service event loop. This future never completes.
+    fn run(
+        &'hw self,
+        _creation_token: Self::RunnerCreationToken,
+    ) -> impl core::future::Future<Output = crate::Never> + 'hw;
+}
+
+/// A handle that must be passed to a spawned task and `.run().await`'d to drive the service.
+/// Dropping this without calling `runner.run().await` means the service will not process events
+pub struct ServiceRunner<'hw, T: RunnableService<'hw>> {
+    service: &'hw T,
+    creation_token: T::RunnerCreationToken, // This token is used to ensure that only the service can create a runner for itself. It's probably a zero-sized type.
+}
+
+impl<'hw, T: RunnableService<'hw>> ServiceRunner<'hw, T> {
+    /// Runs the service event loop. This future never completes.
+    pub async fn run(self) -> crate::Never {
+        self.service.run(self.creation_token).await
+    }
+
+    /// Constructs a new service runner.  This is something the service will do in its init function; users of
+    /// the service should not need to call this directly.
+    pub fn new(service: &'hw T, token: T::RunnerCreationToken) -> Self {
+        Self {
+            service,
+            creation_token: token,
+        }
+    }
+}
+
+/// Generates a default implementation of a runner creation token for a service.  This token is used to ensure that
+/// only the service can create a runner for itself, and therefore it can control the number of tasks that a user is
+/// allowed to spawn to run the service (e.g. if the service is not designed to be run by multiple tasks, it can use
+/// this token to prevent that).
+///
+/// Most services will want to use this macro to generate a simple zero-sized token type - it needs to be a macro invoked
+/// in the service module rather than a generic type in this module because the constructor needs to be private to the
+/// service module to prevent users from constructing their own tokens and spawning multiple runners.
+///
+/// Arguments:
+///   - token_name: The name of the token type to generate.
+#[macro_export]
+macro_rules! impl_runner_creation_token {
+    ($token_name:ident) => {
+        /// A token type used to restrict users from spawning more than one service runner.
+        pub struct $token_name {
+            _private: (),
+        }
+
+        impl $token_name {
+            fn new() -> Self {
+                Self { _private: () }
+            }
+        }
+    };
+}
+
+pub use impl_runner_creation_token;
+
+/// 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().
+///
+/// Note that for a service to be supported, it must have the following properties: // TODO figure out if this should be a trait. Would require a single associated-type arg rather than letting each service define its own init list though...
+/// 1. Implements the RunnableService trait
+/// 2. Has an init() function with the following properties:
+///   i.  Takes as its first argument a &OnceLock<service_ty>
+///   ii. Returns a Result<(reference-to-service, service-runner), Error> where the service-runner
+///       is an instance of RunnableService.
+///
+/// Arguments
+///
+/// - spawner:    An [`embassy_executor::Spawner`].
+/// - service_ty: The service type, wrapped in brackets to allow generic arguments
+///   (e.g. `[my_crate::Service<'static>]`).
+/// - init_args:  The arguments to pass to `Service::init()`, excluding the `OnceLock` argument, which is codegenned.
+///
+/// Example:
+///
+/// ```ignore
+/// let time_service = embedded_services::spawn_service!(
+///     time_alarm_task,
+///     spawner,
+///     [time_alarm_service::Service<'static>],
+///     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:tt)* ], $($init_args:expr),* $(,)?) => {
+        {
+            static SERVICE: embassy_sync::once_lock::OnceLock<$($service_ty)*> = embassy_sync::once_lock::OnceLock::new();
+            match <$($service_ty)*>::init(
+                &SERVICE,
+                $($init_args),*
+            )
+            .await {
+                Ok((service_ref, runner)) => {
+                    #[embassy_executor::task]
+                    async fn service_task_fn(
+                        runner: $crate::service::ServiceRunner<'static, $($service_ty)*>,
+                    ) {
+                        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,17 +75,46 @@ pub struct Service<RelayHandler: embedded_services::relay::mctp::RelayHandler> {
 //
 
 ///////// COMMON FUNCTIONS ///////////
+
+embedded_services::impl_runner_creation_token!(RunnerCreationToken);
+
+impl<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> embedded_services::service::RunnableService<'hw>
+    for Service<RelayHandler>
+{
+    type RunnerCreationToken = RunnerCreationToken;
+    async fn run(&'hw self, _: Self::RunnerCreationToken) -> embedded_services::Never {
+        let mut espi = self.espi.lock().await;
+        loop {
+            let event = select(espi.wait_for_event(), self.host_tx_queue.receive()).await;
+
+            match event {
+                embassy_futures::select::Either::First(controller_event) => {
+                    self.process_controller_event(&mut espi, controller_event)
+                        .await
+                        .unwrap_or_else(|e| {
+                            error!("Critical error processing eSPI controller event: {:?}", e);
+                        });
+                }
+                embassy_futures::select::Either::Second(host_msg) => {
+                    self.process_response_to_host_routing(&mut espi, host_msg).await
+                }
+            }
+        }
+    }
+}
+
 impl<RelayHandler: embedded_services::relay::mctp::RelayHandler> Service<RelayHandler> {
     // 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 {
+    ) -> Result<(&'static Self, embedded_services::service::ServiceRunner<'static, Self>), core::convert::Infallible>
+    {
         espi.wait_for_plat_reset().await;
 
-        let result = service_storage.get_or_init(|| Service {
+        let service = service_storage.get_or_init(|| Service {
             endpoint: DEPRECATED_comms::Endpoint::uninit(DEPRECATED_comms::EndpointID::External(
                 DEPRECATED_comms::External::Host,
             )),
@@ -94,30 +123,14 @@ impl<RelayHandler: embedded_services::relay::mctp::RelayHandler> Service<RelayHa
             relay_handler,
         });
 
-        DEPRECATED_comms::register_endpoint(result, &result.endpoint)
+        DEPRECATED_comms::register_endpoint(service, &service.endpoint)
             .await
             .unwrap();
-        result
-    }
 
-    pub(crate) async fn run_service(&self) -> ! {
-        let mut espi = self.espi.lock().await;
-        loop {
-            let event = select(espi.wait_for_event(), self.host_tx_queue.receive()).await;
-
-            match event {
-                embassy_futures::select::Either::First(controller_event) => {
-                    self.process_controller_event(&mut espi, controller_event)
-                        .await
-                        .unwrap_or_else(|e| {
-                            error!("Critical error processing eSPI controller event: {:?}", e);
-                        });
-                }
-                embassy_futures::select::Either::Second(host_msg) => {
-                    self.process_response_to_host_routing(&mut espi, host_msg).await
-                }
-            }
-        }
+        Ok((
+            service,
+            embedded_services::service::ServiceRunner::new(service, RunnerCreationToken::new()),
+        ))
     }
 
     // TODO The notification system was not actually used, so this is currently dead code.

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,17 @@ 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,
+    let time_service = embedded_services::spawn_service!(
+        spawner,
+        [time_alarm_service::Service<'static>],
         dt_clock,
         tz,
         ac_expiration,
         ac_policy,
         dc_expiration,
-        dc_policy,
+        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!(

time-alarm-service/src/lib.rs

@@ -10,7 +10,6 @@ use embedded_services::GlobalRawMutex;
 use embedded_services::{info, warn};
 use time_alarm_service_messages::*;
 
-pub mod task;
 mod timer;
 use timer::Timer;
 
@@ -124,7 +123,7 @@ impl<'hw> Service<'hw> {
         ac_policy_storage: &'hw mut dyn NvramStorage<'hw, u32>,
         dc_expiration_storage: &'hw mut dyn NvramStorage<'hw, u32>,
         dc_policy_storage: &'hw mut dyn NvramStorage<'hw, u32>,
-    ) -> Result<&'hw Service<'hw>, DatetimeClockError> {
+    ) -> Result<(&'hw Self, embedded_services::service::ServiceRunner<'hw, Self>), DatetimeClockError> {
         info!("Starting time-alarm service task");
 
         let service = service_storage.get_or_init(|| Service {
@@ -160,7 +159,10 @@ impl<'hw> Service<'hw> {
         service.timers.ac_timer.start(&service.clock_state, true)?;
         service.timers.dc_timer.start(&service.clock_state, false)?;
 
-        Ok(service)
+        Ok((
+            service,
+            embedded_services::service::ServiceRunner::new(service, RunnerCreationToken::new()),
+        ))
     }
 
     /// Query clock capabilities.  Analogous to ACPI TAD's _GRT method.
@@ -263,17 +265,6 @@ impl<'hw> Service<'hw> {
         }
     }
 
-    pub(crate) async fn run_service(&'hw self) -> ! {
-        loop {
-            embassy_futures::select::select3(
-                self.handle_power_source_updates(),
-                self.handle_timer(AcpiTimerId::AcPower),
-                self.handle_timer(AcpiTimerId::DcPower),
-            )
-            .await;
-        }
-    }
-
     async fn handle_power_source_updates(&'hw self) -> ! {
         loop {
             let new_power_source = self.power_source_signal.wait().await;
@@ -311,6 +302,22 @@ impl<'hw> Service<'hw> {
     }
 }
 
+embedded_services::impl_runner_creation_token!(RunnerCreationToken);
+
+impl<'hw> embedded_services::service::RunnableService<'hw> for Service<'hw> {
+    type RunnerCreationToken = RunnerCreationToken;
+    async fn run(&'hw self, _: Self::RunnerCreationToken) -> embedded_services::Never {
+        loop {
+            embassy_futures::select::select3(
+                self.handle_power_source_updates(),
+                self.handle_timer(AcpiTimerId::AcPower),
+                self.handle_timer(AcpiTimerId::DcPower),
+            )
+            .await;
+        }
+    }
+}
+
 impl<'hw> embedded_services::relay::mctp::RelayServiceHandlerTypes for Service<'hw> {
     type RequestType = AcpiTimeAlarmRequest;
     type ResultType = AcpiTimeAlarmResult;