power-policy-service: Clean up recovery logic and add more documentation

* Recovery flow now explicity sets recovery power instead of relying on
  `attempt_provider_recovery`
* Remove `Result` return types from functions only using it when
  checking for invalid data in the devices list
* Update documentation in docs and in the power-policy-service module

Author: RobertZ2011

docs/power-policy.md

@@ -27,6 +27,10 @@ stateDiagram-v2
     Provider --> Detached : NotifyDetach
     Idle --> Detached : NotifyDetach
 ```
+
+#### Device recovery
+Requests to a device can fail for any number of reasons (bus communication issues, deadlock in driver code, etc). In most cases this won't result in any issues as most failed requests result in the system being in a safe state. E.g. a failed transition from `Idle` to `ConnectedConsummer` will result in the system just not drawing power. However, a failed request in the `ConnectedProvider` state can leave the system providing more power than intended. The device state isn't enough to capture this situation as the device must be assumed to still be the in the `ConnectedProvider` state. The `Device` struct contains a `recovery` member to track this situation. The specifics of the recovery process are implementation defined.
+
 ### Policy Messages
 These messages are sent from a device to the power policy.
 

embedded-service/src/power/policy/action/policy.rs

@@ -108,11 +108,7 @@ impl<'a> Policy<'a, ConnectedProvider> {
     /// Disconnect this device
     pub async fn disconnect(self) -> Result<Policy<'a, Idle>, Error> {
         if let Err(e) = self.disconnect_internal().await {
-            error!(
-                "Error disconnecting device {}: {:?}, entering recovery mode",
-                self.device.id().0,
-                e
-            );
+            error!("Error disconnecting device {}: {:?}", self.device.id().0, e);
             self.device.enter_recovery().await;
             return Err(e);
         }

examples/std/src/bin/power_policy.rs

@@ -171,16 +171,40 @@ async fn run(spawner: Spawner) {
     let device0 = device0.attach().await.unwrap();
     device0.request_provider_power_capability(LOW_POWER).await.unwrap();
     Timer::after_millis(250).await;
-    // First rejected request is connect request
-    // Second rejected request is disconnect request
-    // Third rejected request is recovery disconnect
-    info!("Rejecting next 3 requests");
-    device0_mock.reject_next_requests(3);
+    // Requests we're rejecting:
+    // Connect request
+    // Disconnect request after failing connect request
+    // Request to connect at recovery limit
+    // Disconnect request after failing recovery limit
+    // First recovery disconnect from `attempt_provider_recovery`
+    // Next recovery disconnect completes
+    info!("Rejecting next 5 requests");
+    device0_mock.reject_next_requests(5);
 
     // Attach device 1, device 0 will fail provider connect request and trigger recovery flow
     let device1 = device1.attach().await.unwrap();
     device1.request_provider_power_capability(LOW_POWER).await.unwrap();
-    Timer::after_millis(5000).await;
+
+    // Wait for the recovery flow to start
+    while !device0_mock.device.is_in_recovery().await {
+        info!("Waiting for recovery flow to start");
+        Timer::after_millis(100).await;
+    }
+
+    // Wait for the recovery flow to complete
+    while device0_mock.device.is_in_recovery().await {
+        info!("Waiting for recovery flow to complete");
+        Timer::after_millis(100).await;
+    }
+
+    // Reconnect device 0
+    info!("Reconnecting device 0 as provider");
+    device0.request_provider_power_capability(LOW_POWER).await.unwrap();
+    // Wait for device 0 to reconnect
+    while !device0_mock.device.is_provider().await {
+        info!("Waiting for device 0 to reconnect");
+        Timer::after_millis(1000).await;
+    }
 
     // Disconnect device 1
     info!("Disconnecting device 1");

power-policy-service/src/lib.rs

@@ -67,7 +67,7 @@ impl PowerPolicy {
     async fn process_notify_detach(&self) -> Result<(), Error> {
         self.context.send_response(Ok(policy::ResponseData::Complete)).await;
         self.update_current_consumer().await?;
-        self.update_providers(None).await?;
+        self.update_providers(None).await;
         Ok(())
     }
 
@@ -79,14 +79,15 @@ impl PowerPolicy {
 
     async fn process_request_provider_power_capabilities(&self, device: DeviceId) -> Result<(), Error> {
         self.context.send_response(Ok(policy::ResponseData::Complete)).await;
-        self.update_providers(Some(device)).await?;
+        self.update_providers(Some(device)).await;
         Ok(())
     }
 
     async fn process_notify_disconnect(&self) -> Result<(), Error> {
         self.context.send_response(Ok(policy::ResponseData::Complete)).await;
         self.update_current_consumer().await?;
-        self.update_providers(None).await
+        self.update_providers(None).await;
+        Ok(())
     }
 
     /// Send a notification with the comms service

power-policy-service/src/provider.rs

@@ -3,7 +3,13 @@
 //! the system is in unlimited power state. In this mode [provider_unlimited](super::Config::provider_unlimited)
 //! is provided to each device. Above this threshold, the system is in limited power state.
 //! In this mode [provider_limited](super::Config::provider_limited) is provided to each device
-use embedded_services::{debug, trace};
+//! Lastly, the system can be in recovery mode. This mode is only entered when a connected provider fails to
+//! connect at a new power level. In this mode, all connected providers are set to
+//! [provider_recovery](super::Config::provider_recovery). While in this mode
+//! [attempt_provider_recovery](PowerPolicy::attempt_provider_recovery) is called periodically
+//! which attempts to disconnect all providers in recovery mode. If this succeeds, the system will
+//! return to normal operating mode.
+use embedded_services::{debug, trace, warn};
 
 use super::*;
 
@@ -29,16 +35,22 @@ pub(super) struct State {
 
 impl PowerPolicy {
     /// Computes the total requested power considering all current providers
-    async fn compute_total_provider_power(&self, new_request: bool) -> Result<PowerState, Error> {
+    async fn compute_total_provider_power(&self, new_request: bool) -> PowerState {
         let mut num_providers = if new_request { 1 } else { 0 };
 
         for device in self.context.devices().await {
-            let device = device.data::<device::Device>().ok_or(Error::InvalidDevice)?;
+            let device = device.data::<device::Device>();
+            if device.is_none() {
+                // A non-power device somehow got into the list of devices, note it and move on
+                warn!("Found non-power device in devices list");
+                continue;
+            }
 
+            let device = device.unwrap();
             if device.is_in_recovery().await {
                 // If any device is in recovery mode, we need to recover
                 info!("Device {}: In recovery mode", device.id().0);
-                return Ok(PowerState::Recovery);
+                return PowerState::Recovery;
             }
 
             if device.is_provider().await {
@@ -48,19 +60,25 @@ impl PowerPolicy {
 
         let total_provided_power = num_providers * self.config.provider_unlimited.max_power_mw();
         if total_provided_power > self.config.limited_power_threshold_mw {
-            Ok(PowerState::Limited)
+            PowerState::Limited
         } else {
-            Ok(PowerState::Unlimited)
+            PowerState::Unlimited
         }
     }
 
     /// Update the power capability of all connected providers
     /// Returns true if we need to enter recovery mode
-    async fn update_provider_capability(&self, target_power: PowerCapability) -> Result<bool, Error> {
+    async fn update_provider_capability(&self, target_power: PowerCapability, exit_on_recovery: bool) -> bool {
         let mut recovery = false;
         for device in self.context.devices().await {
-            let device = device.data::<device::Device>().ok_or(Error::InvalidDevice)?;
+            let device = device.data::<device::Device>();
+            if device.is_none() {
+                // A non-power device somehow got into the list of devices, note it and move on
+                warn!("Found non-power device in devices list");
+                continue;
+            }
 
+            let device = device.unwrap();
             if let Ok(action) = self
                 .context
                 .try_policy_action::<action::ConnectedProvider>(device.id())
@@ -76,27 +94,34 @@ impl PowerPolicy {
                         );
 
                         if let Err(_) = action.disconnect().await {
-                            error!(
-                                "Device{}: Failed to disconnect provider, entering recovery mode",
-                                device.id().0
-                            );
+                            error!("Device{}: Failed to disconnect provider", device.id().0);
+
+                            // Early exit if that's what we want
+                            // This is used to avoid excessively switching power capabilities in the recovery flow
+                            if exit_on_recovery {
+                                return true;
+                            }
+
                             recovery = true;
                         }
                     }
                 }
             }
         }
-        Ok(recovery)
+
+        return recovery;
     }
 
     /// Update the provider state of currently connected providers
-    pub(super) async fn update_providers(&self, new_provider: Option<DeviceId>) -> Result<(), Error> {
+    pub(super) async fn update_providers(&self, new_provider: Option<DeviceId>) {
         trace!("Updating providers");
         let mut state = self.state.lock().await;
+        let mut already_in_recovery = true;
 
         if state.current_provider_state.state != PowerState::Recovery {
             // Only update the power state if we're not in recovery mode
-            state.current_provider_state.state = self.compute_total_provider_power(new_provider.is_some()).await?;
+            already_in_recovery = false;
+            state.current_provider_state.state = self.compute_total_provider_power(new_provider.is_some()).await;
         }
         debug!("New power state: {:?}", state.current_provider_state.state);
 
@@ -106,24 +131,39 @@ impl PowerPolicy {
             PowerState::Limited => self.config.provider_limited,
         };
 
-        let recovery = self.update_provider_capability(target_power).await?;
+        let recovery = self.update_provider_capability(target_power, true).await;
         if let Some(new_provider) = new_provider {
             info!("Connecting new provider");
-            if let Ok(action) = self.context.try_policy_action::<action::Idle>(new_provider).await {
-                action.connect_provider(target_power).await?;
+            let connected = if let Ok(action) = self.context.try_policy_action::<action::Idle>(new_provider).await {
+                let target_power = if recovery {
+                    // We entered recovery mode so attempt to connect at the recovery power
+                    self.config.provider_recovery
+                } else {
+                    target_power
+                };
+                action.connect_provider(target_power).await.is_ok()
             } else {
-                // Don't enter recovery mode if we can't connect to the new provider
-                // Since it's a new provider that hasn't been connected then it's
-                // not drawing power as far as we're concerned
+                false
+            };
+
+            // Don't enter recovery mode if we can't connect the new provider.
+            // Since it's a new provider that hasn't been connected then it's
+            // not drawing power as far as we're concerned
+            if !connected {
                 error!("Device {}: Failed to connect provider", new_provider.0);
             }
         }
 
-        if recovery {
+        if recovery && !already_in_recovery {
+            // Entering recovery, set power capability on all responding providers to recovery limit
+            // Don't check return value of update_provider_capability here, if we've spontaneously recovered
+            // then it'll get caught by the next call of attempt_provider_recovery
+            info!("Entering recovery mode");
+            let _ = self
+                .update_provider_capability(self.config.provider_recovery, false)
+                .await;
             state.current_provider_state.state = PowerState::Recovery;
         }
-
-        Ok(())
     }
 
     pub(super) async fn attempt_provider_recovery(&self) {
@@ -134,10 +174,13 @@ impl PowerPolicy {
         }
 
         info!("Attempting provider recovery");
+        let mut recovered = true;
         // Attempt to by disconnecting all providers in recovery
         for device in self.context.devices().await {
             let device = device.data::<device::Device>().ok_or(Error::InvalidDevice);
             if device.is_err() {
+                // A non-power device somehow got into the list of devices, note it and move on
+                warn!("Found non-power device in devices list");
                 continue;
             }
 
@@ -150,25 +193,25 @@ impl PowerPolicy {
                 {
                     if let Err(_) = action.disconnect().await {
                         error!("Device {}: Failed to recover", device.id().0);
+                        recovered = false;
                     }
                 }
             }
         }
 
-        // Attempt to restart in the limited power state
-        self.state.lock().await.current_provider_state.state = PowerState::Limited;
-        if self.update_providers(None).await.is_err() {
-            // Failed to update providers, stay in recovery mode
-            info!("Failed to update providers, staying in recovery mode");
+        if !recovered {
+            info!("Failed to recover all providers, staying in recovery mode");
             return;
         }
 
-        if self.state.lock().await.current_provider_state.state != PowerState::Recovery {
-            // Successfully recovered
-            info!("Successfully recovered from provider recovery mode");
-        } else {
-            // Still in recovery mode
-            info!("Still in provider recovery mode");
+        // Attempt to restart in the unlimited power state
+        self.state.lock().await.current_provider_state.state = PowerState::Unlimited;
+        self.update_providers(None).await;
+        if self.state.lock().await.current_provider_state.state == PowerState::Recovery {
+            info!("Failed to update providers, staying in recovery mode");
+            return;
         }
+
+        info!("Successfully recovered from provider recovery mode");
     }
 }