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..704573fb --- /dev/null +++ b/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, // TODO could be resources? + params: Self::InitParams, + ) -> impl core::future::Future>; +} + +/// 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 + '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 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; diff --git a/espi-service/src/espi_service.rs b/espi-service/src/espi_service.rs index 479059e9..e6bbea7c 100644 --- a/espi-service/src/espi_service.rs +++ b/espi-service/src/espi_service.rs @@ -75,32 +75,68 @@ pub struct Service { // ///////// COMMON FUNCTIONS /////////// -impl Service { + +pub struct Runner<'hw, RelayHandler: embedded_services::relay::mctp::RelayHandler> { + service: &'hw Service, +} + +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 { + 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 +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, - 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, + 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; + type Runner = Runner<'hw, RelayHandler>; +} + +impl Service { + // 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 Service, 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. 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..161017f1 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,19 @@ 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, - 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!( diff --git a/time-alarm-service/src/lib.rs b/time-alarm-service/src/lib.rs index 333420ae..e8af5de1 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; @@ -116,53 +115,6 @@ pub struct Service<'hw> { } impl<'hw> Service<'hw> { - pub async fn init( - service_storage: &'hw OnceLock>, - backing_clock: &'hw mut impl DatetimeClock, - tz_storage: &'hw mut dyn NvramStorage<'hw, u32>, - ac_expiration_storage: &'hw mut dyn NvramStorage<'hw, u32>, - 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> { - info!("Starting time-alarm service task"); - - let service = service_storage.get_or_init(|| Service { - clock_state: Mutex::new(RefCell::new(ClockState { - datetime_clock: backing_clock, - tz_data: TimeZoneData::new(tz_storage), - })), - power_source_signal: Signal::new(), - timers: Timers::new( - ac_expiration_storage, - ac_policy_storage, - dc_expiration_storage, - dc_policy_storage, - ), - capabilities: { - // TODO [CONFIG] We could consider making some of these user-configurable, e.g. if we want to support devices that don't have a battery - let mut caps = TimeAlarmDeviceCapabilities(0); - caps.set_ac_wake_implemented(true); - caps.set_dc_wake_implemented(true); - caps.set_realtime_implemented(true); - caps.set_realtime_accuracy_in_milliseconds(false); - caps.set_get_wake_status_supported(true); - caps.set_ac_s4_wake_supported(true); - caps.set_ac_s5_wake_supported(true); - caps.set_dc_s4_wake_supported(true); - caps.set_dc_s5_wake_supported(true); - caps - }, - }); - - // TODO [POWER_SOURCE] we need to subscribe to messages that tell us if we're on AC or DC power so we can decide which alarms to trigger, but those notifications are not yet implemented - revisit when they are. - // TODO [POWER_SOURCE] if it's possible to learn which power source is active at init time, we should set that one active rather than defaulting to the AC timer. - service.timers.ac_timer.start(&service.clock_state, true)?; - service.timers.dc_timer.start(&service.clock_state, false)?; - - Ok(service) - } - /// Query clock capabilities. Analogous to ACPI TAD's _GRT method. pub fn get_capabilities(&self) -> TimeAlarmDeviceCapabilities { self.capabilities @@ -263,17 +215,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 +252,82 @@ impl<'hw> Service<'hw> { } } +pub struct Runner<'hw> { + service: &'hw Service<'hw>, +} + +impl<'hw> embedded_services::service::ServiceRunner<'hw> for Runner<'hw> { + fn run(self) -> impl core::future::Future + 'hw { + async move { + loop { + embassy_futures::select::select3( + self.service.handle_power_source_updates(), + self.service.handle_timer(AcpiTimerId::AcPower), + self.service.handle_timer(AcpiTimerId::DcPower), + ) + .await; + } + } + } +} + +pub struct ServiceInitParams<'hw> { + pub backing_clock: &'hw mut dyn DatetimeClock, + pub tz_storage: &'hw mut dyn NvramStorage<'hw, u32>, + pub ac_expiration_storage: &'hw mut dyn NvramStorage<'hw, u32>, + pub ac_policy_storage: &'hw mut dyn NvramStorage<'hw, u32>, + pub dc_expiration_storage: &'hw mut dyn NvramStorage<'hw, u32>, + pub dc_policy_storage: &'hw mut dyn NvramStorage<'hw, u32>, +} + +impl<'hw> embedded_services::service::RunnableService<'hw> for Service<'hw> { + type Runner = Runner<'hw>; + type ErrorType = DatetimeClockError; + type InitParams = ServiceInitParams<'hw>; + + async fn init( + service_storage: &'hw OnceLock>, + init_params: Self::InitParams, + ) -> Result<(&'hw Self, Runner<'hw>), DatetimeClockError> { + info!("Starting time-alarm service task"); + + let service = service_storage.get_or_init(|| Service { + clock_state: Mutex::new(RefCell::new(ClockState { + datetime_clock: init_params.backing_clock, + tz_data: TimeZoneData::new(init_params.tz_storage), + })), + power_source_signal: Signal::new(), + timers: Timers::new( + init_params.ac_expiration_storage, + init_params.ac_policy_storage, + init_params.dc_expiration_storage, + init_params.dc_policy_storage, + ), + capabilities: { + // TODO [CONFIG] We could consider making some of these user-configurable, e.g. if we want to support devices that don't have a battery + let mut caps = TimeAlarmDeviceCapabilities(0); + caps.set_ac_wake_implemented(true); + caps.set_dc_wake_implemented(true); + caps.set_realtime_implemented(true); + caps.set_realtime_accuracy_in_milliseconds(false); + caps.set_get_wake_status_supported(true); + caps.set_ac_s4_wake_supported(true); + caps.set_ac_s5_wake_supported(true); + caps.set_dc_s4_wake_supported(true); + caps.set_dc_s5_wake_supported(true); + caps + }, + }); + + // TODO [POWER_SOURCE] we need to subscribe to messages that tell us if we're on AC or DC power so we can decide which alarms to trigger, but those notifications are not yet implemented - revisit when they are. + // TODO [POWER_SOURCE] if it's possible to learn which power source is active at init time, we should set that one active rather than defaulting to the AC timer. + service.timers.ac_timer.start(&service.clock_state, true)?; + service.timers.dc_timer.start(&service.clock_state, false)?; + + Ok((service, Runner { service })) + } +} + 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..14cfea16 100644 --- a/time-alarm-service/tests/tad_test.rs +++ b/time-alarm-service/tests/tad_test.rs @@ -9,6 +9,7 @@ mod test { use embassy_sync::once_lock::OnceLock; use embassy_time::Timer; use embedded_mcu_hal::time::{Datetime, DatetimeClock}; + use embedded_services::service::{RunnableService, ServiceRunner}; use time_alarm_service_messages as msg; @@ -23,16 +24,18 @@ mod test { let mut dc_pol_storage = MockNvramStorage::new(0); 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, - &mut ac_exp_storage, - &mut ac_pol_storage, - &mut dc_exp_storage, - &mut dc_pol_storage, + time_alarm_service::ServiceInitParams { + backing_clock: &mut clock, + tz_storage: &mut tz_storage, + ac_expiration_storage: &mut ac_exp_storage, + ac_policy_storage: &mut ac_pol_storage, + dc_expiration_storage: &mut dc_exp_storage, + dc_policy_storage: &mut dc_pol_storage, + }, ) .await .unwrap(); @@ -45,7 +48,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,20 +77,23 @@ 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, - &mut ac_exp_storage, - &mut ac_pol_storage, - &mut dc_exp_storage, - &mut dc_pol_storage, + time_alarm_service::ServiceInitParams { + backing_clock: &mut clock, + tz_storage: &mut tz_storage, + ac_expiration_storage: &mut ac_exp_storage, + ac_policy_storage: &mut ac_pol_storage, + dc_expiration_storage: &mut dc_exp_storage, + dc_policy_storage: &mut dc_pol_storage, + }, ) .await .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();