diff --git a/embedded-service/src/lib.rs b/embedded-service/src/lib.rs index f20d6e89..c5d15a58 100644 --- a/embedded-service/src/lib.rs +++ b/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; diff --git a/embedded-service/src/service.rs b/embedded-service/src/service.rs new file mode 100644 index 00000000..3b9b6178 --- /dev/null +++ b/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 + '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 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 +/// 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; diff --git a/espi-service/src/espi_service.rs b/espi-service/src/espi_service.rs index 479059e9..872816a2 100644 --- a/espi-service/src/espi_service.rs +++ b/espi-service/src/espi_service.rs @@ -75,6 +75,34 @@ pub struct Service { // ///////// COMMON FUNCTIONS /////////// + +embedded_services::impl_runner_creation_token!(RunnerCreationToken); + +impl<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> embedded_services::service::RunnableService<'hw> + for Service +{ + 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 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. @@ -82,10 +110,11 @@ impl Service, 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 Service ! { - 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. diff --git a/espi-service/src/lib.rs b/espi-service/src/lib.rs index e8233bc9..c89b89f5 100644 --- a/espi-service/src/lib.rs +++ b/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::*; diff --git a/espi-service/src/task.rs b/espi-service/src/task.rs deleted file mode 100644 index ff7f7fe8..00000000 --- a/espi-service/src/task.rs +++ /dev/null @@ -1,12 +0,0 @@ -use crate::Service; - -// TODO: We currently require that the service has lifetime 'static because we still communicate with -// some services over the legacy comms system, which requires that things that interact with it -// have lifetime 'static. Once we've fully transitioned to the direct async call method of interfacing -// between services, we should be able to relax this requirement to just require that the service has -// the same lifetime as the services it's communicating with. -pub async fn espi_service( - espi_service: &'static Service, -) -> Result { - espi_service.run_service().await -} diff --git a/examples/rt685s-evk/src/bin/time_alarm.rs b/examples/rt685s-evk/src/bin/time_alarm.rs index 9b1a8182..80c50cdd 100644 --- a/examples/rt685s-evk/src/bin/time_alarm.rs +++ b/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 = 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!( diff --git a/time-alarm-service/src/lib.rs b/time-alarm-service/src/lib.rs index 333420ae..ee1c0e9d 100644 --- a/time-alarm-service/src/lib.rs +++ b/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; diff --git a/time-alarm-service/src/task.rs b/time-alarm-service/src/task.rs deleted file mode 100644 index a45fdf62..00000000 --- a/time-alarm-service/src/task.rs +++ /dev/null @@ -1,9 +0,0 @@ -use crate::Service; -use embedded_services::info; - -/// Call this from a dedicated async task. Must be called exactly once on each service instance. -/// Note that on-device, 'hw must be 'static. We're generic over 'hw to enable some test scenarios leveraging tokio and mocks. -pub async fn run_service<'hw>(service: &'hw Service<'hw>) -> ! { - info!("Starting time-alarm service task"); - service.run_service().await -} diff --git a/time-alarm-service/tests/tad_test.rs b/time-alarm-service/tests/tad_test.rs index dc3d43d8..6b3b71e0 100644 --- a/time-alarm-service/tests/tad_test.rs +++ b/time-alarm-service/tests/tad_test.rs @@ -25,7 +25,7 @@ mod test { let mut clock = MockDatetimeClock::new_running(); let storage = OnceLock::new(); - let service = time_alarm_service::Service::init( + let (service, runner) = time_alarm_service::Service::init( &storage, &mut clock, &mut tz_storage, @@ -45,7 +45,7 @@ mod test { // return !, so we should go until the test arm completes and then shut down. // tokio::select! { - _ = time_alarm_service::task::run_service(service) => unreachable!("time alarm service task finished unexpectedly"), + _ = runner.run() => unreachable!("time alarm service task finished unexpectedly"), _ = async { let delay_secs = 2; let begin = service.get_real_time().unwrap(); @@ -74,7 +74,7 @@ mod test { .unwrap(); let storage = OnceLock::new(); - let service = time_alarm_service::Service::init( + let (service, runner) = time_alarm_service::Service::init( &storage, &mut clock, &mut tz_storage, @@ -87,7 +87,7 @@ mod test { .unwrap(); tokio::select! { - _ = time_alarm_service::task::run_service(service) => unreachable!("time alarm service task finished unexpectedly"), + _ = runner.run() => unreachable!("time alarm service task finished unexpectedly"), _ = async { // Clock is paused, so time shouldn't advance unless we set it. let begin = service.get_real_time().unwrap();