type-c-service/wrapper: Disable sink path during voltage transition (#861)

The existing behavior can cause overcurrent/overvoltage conditions
during the renegotiation. Disconnect power during the transition and
introduce flags to help users of this code detect this condition.

Author: RobertZ2011

battery-service/src/context.rs

@@ -527,7 +527,7 @@ impl Context {
         let psu_state = guard.deref_mut();
 
         match power_info {
-            embedded_services::power::policy::CommsData::ConsumerDisconnected(_) => {
+            embedded_services::power::policy::CommsData::ConsumerDisconnected(..) => {
                 *psu_state = PsuState {
                     psu_connected: false,
                     power_capability: None,

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

@@ -1,6 +1,6 @@
 //! Device state machine actions
 use super::*;
-use crate::power::policy::{ConsumerPowerCapability, Error, ProviderPowerCapability, device, policy};
+use crate::power::policy::{ConsumerPowerCapability, Error, ProviderPowerCapability, device, flags, policy};
 use crate::{info, trace};
 
 /// Device state machine control
@@ -55,12 +55,12 @@ impl<'a, S: Kind> Device<'a, S> {
     }
 
     /// Disconnect this device
-    async fn disconnect_internal(&self) -> Result<(), Error> {
+    async fn disconnect_internal(&self, flags: flags::ConsumerDisconnect) -> Result<(), Error> {
         info!("Device {} disconnecting", self.device.id().0);
         self.device.update_consumer_capability(None).await;
         self.device.update_requested_provider_capability(None).await;
         self.device.set_state(device::State::Idle).await;
-        policy::send_request(self.device.id(), policy::RequestData::NotifyDisconnect)
+        policy::send_request(self.device.id(), policy::RequestData::NotifyDisconnect(flags))
             .await?
             .complete_or_err()
     }
@@ -136,8 +136,8 @@ impl Device<'_, Idle> {
 
 impl<'a> Device<'a, ConnectedConsumer> {
     /// Disconnect this device
-    pub async fn disconnect(self) -> Result<Device<'a, Idle>, Error> {
-        self.disconnect_internal().await?;
+    pub async fn disconnect(self, flags: flags::ConsumerDisconnect) -> Result<Device<'a, Idle>, Error> {
+        self.disconnect_internal(flags).await?;
         Ok(Device::new(self.device))
     }
 
@@ -152,8 +152,8 @@ impl<'a> Device<'a, ConnectedConsumer> {
 
 impl<'a> Device<'a, ConnectedProvider> {
     /// Disconnect this device
-    pub async fn disconnect(self) -> Result<Device<'a, Idle>, Error> {
-        self.disconnect_internal().await?;
+    pub async fn disconnect(self, flags: flags::ConsumerDisconnect) -> Result<Device<'a, Idle>, Error> {
+        self.disconnect_internal(flags).await?;
         Ok(Device::new(self.device))
     }
 

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

@@ -127,6 +127,72 @@ impl Provider {
     }
 }
 
+bitfield! {
+    /// Flags for disconnect events
+    #[derive(Copy, Clone, PartialEq, Eq)]
+    #[cfg_attr(feature = "defmt", derive(defmt::Format))]
+    struct ConsumerDisconnectRaw(u32);
+    impl Debug;
+    /// Renegotiation
+    ///
+    /// When set this flag indicates that the current consumer is attempting to negotiate a new power capability.
+    pub bool, renegotiation, set_renegotiation: 0;
+    /// Switching
+    ///
+    /// When set this flag indicates that the service is switching to a different PSU.
+    pub bool, switching, set_switching: 1;
+}
+
+/// Type safe wrapper for consumer disconnect flags
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub struct ConsumerDisconnect(ConsumerDisconnectRaw);
+
+impl ConsumerDisconnect {
+    /// Create new consumer disconnect flags with no flags set
+    pub const fn none() -> Self {
+        Self(ConsumerDisconnectRaw(0))
+    }
+
+    /// Builder method to set the renegotiation flag
+    pub fn with_renegotiation(mut self, value: bool) -> Self {
+        self.set_renegotiation(value);
+        self
+    }
+
+    /// Set the value of the renegotiation flag
+    pub fn set_renegotiation(&mut self, value: bool) {
+        self.0.set_renegotiation(value);
+    }
+
+    /// Get the value of the renegotiation flag
+    pub fn renegotiation(&self) -> bool {
+        self.0.renegotiation()
+    }
+
+    /// Builder method to set the switching flag
+    pub fn with_switching(mut self, value: bool) -> Self {
+        self.set_switching(value);
+        self
+    }
+
+    /// Set the value of the switching flag
+    pub fn set_switching(&mut self, value: bool) {
+        self.0.set_switching(value);
+    }
+
+    /// Get the value of the switching flag
+    pub fn switching(&self) -> bool {
+        self.0.switching()
+    }
+}
+
+impl Default for ConsumerDisconnect {
+    fn default() -> Self {
+        Self::none()
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -180,4 +246,20 @@ mod tests {
         provider.set_psu_type(PsuType::Unknown);
         assert_eq!(provider.0.0, 0x0);
     }
+
+    #[test]
+    fn test_consumer_disconnect_renegotiation() {
+        let mut disconnect = ConsumerDisconnect::none().with_renegotiation(true);
+        assert_eq!(disconnect.0.0, 0x1);
+        disconnect.set_renegotiation(false);
+        assert_eq!(disconnect.0.0, 0x0);
+    }
+
+    #[test]
+    fn test_consumer_disconnect_switching() {
+        let mut disconnect = ConsumerDisconnect::none().with_switching(true);
+        assert_eq!(disconnect.0.0, 0x2);
+        disconnect.set_switching(false);
+        assert_eq!(disconnect.0.0, 0x0);
+    }
 }

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

@@ -142,7 +142,7 @@ impl UnconstrainedState {
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub enum CommsData {
     /// Consumer disconnected
-    ConsumerDisconnected(DeviceId),
+    ConsumerDisconnected(DeviceId, flags::ConsumerDisconnect),
     /// Consumer connected
     ConsumerConnected(DeviceId, ConsumerPowerCapability),
     /// Provider disconnected

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

@@ -3,7 +3,7 @@ use core::sync::atomic::{AtomicBool, Ordering};
 
 use crate::GlobalRawMutex;
 use crate::broadcaster::immediate as broadcaster;
-use crate::power::policy::{CommsMessage, ConsumerPowerCapability, ProviderPowerCapability};
+use crate::power::policy::{CommsMessage, ConsumerPowerCapability, ProviderPowerCapability, flags};
 use embassy_sync::channel::Channel;
 
 use super::charger::ChargerResponse;
@@ -26,7 +26,7 @@ pub enum RequestData {
     /// Request the given amount of power to provider
     RequestProviderCapability(ProviderPowerCapability),
     /// Notify that a device cannot consume or provide power anymore
-    NotifyDisconnect,
+    NotifyDisconnect(flags::ConsumerDisconnect),
     /// Notify that a device has detached
     NotifyDetached,
 }

power-policy-service/src/consumer.rs

@@ -218,7 +218,10 @@ impl PowerPolicy {
             self.disconnect_chargers().await?;
 
             self.comms_notify(CommsMessage {
-                data: CommsData::ConsumerDisconnected(current_consumer.device_id),
+                data: CommsData::ConsumerDisconnected(
+                    current_consumer.device_id,
+                    flags::ConsumerDisconnect::default().with_switching(true),
+                ),
             })
             .await;
 
@@ -268,7 +271,10 @@ impl PowerPolicy {
             if let Some(consumer_state) = state.current_consumer_state {
                 self.disconnect_chargers().await?;
                 self.comms_notify(CommsMessage {
-                    data: CommsData::ConsumerDisconnected(consumer_state.device_id),
+                    data: CommsData::ConsumerDisconnected(
+                        consumer_state.device_id,
+                        flags::ConsumerDisconnect::default(),
+                    ),
                 })
                 .await;
             }

power-policy-service/src/lib.rs

@@ -77,7 +77,11 @@ impl PowerPolicy {
         Ok(())
     }
 
-    async fn process_notify_disconnect(&self, device: &device::Device) -> Result<(), Error> {
+    async fn process_notify_disconnect(
+        &self,
+        device: &device::Device,
+        flags: flags::ConsumerDisconnect,
+    ) -> Result<(), Error> {
         self.context.send_response(Ok(policy::ResponseData::Complete)).await;
 
         self.remove_connected_provider(device.id()).await;
@@ -93,7 +97,7 @@ impl PowerPolicy {
             self.disconnect_chargers().await?;
 
             self.comms_notify(CommsMessage {
-                data: CommsData::ConsumerDisconnected(consumer.device_id),
+                data: CommsData::ConsumerDisconnected(consumer.device_id, flags),
             })
             .await;
 
@@ -175,9 +179,9 @@ impl PowerPolicy {
                 );
                 self.process_request_provider_power_capabilities(device.id()).await
             }
-            policy::RequestData::NotifyDisconnect => {
+            policy::RequestData::NotifyDisconnect(flags) => {
                 info!("Received notify disconnect from device {}", device.id().0);
-                self.process_notify_disconnect(device).await
+                self.process_notify_disconnect(device, flags).await
             }
         }
     }

type-c-service/src/service/power.rs

@@ -16,7 +16,7 @@ impl<'a> Service<'a> {
                     power_policy::CommsData::Unconstrained(state) => {
                         return Event::PowerPolicy(PowerPolicyEvent::Unconstrained(state));
                     }
-                    power_policy::CommsData::ConsumerDisconnected(_) => {
+                    power_policy::CommsData::ConsumerDisconnected(..) => {
                         return Event::PowerPolicy(PowerPolicyEvent::ConsumerDisconnected);
                     }
                     power_policy::CommsData::ConsumerConnected(_, _) => {

type-c-service/src/wrapper/mod.rs

@@ -29,7 +29,7 @@ use embassy_time::Instant;
 use embedded_cfu_protocol::protocol_definitions::{FwUpdateOffer, FwUpdateOfferResponse, FwVersion};
 use embedded_services::GlobalRawMutex;
 use embedded_services::power::policy::device::StateKind;
-use embedded_services::power::policy::{self, action};
+use embedded_services::power::policy::{self, action, flags};
 use embedded_services::sync::Lockable;
 use embedded_services::type_c::controller::{self, Controller, PortStatus};
 use embedded_services::type_c::event::{PortEvent, PortNotificationSingle, PortPending, PortStatusChanged};
@@ -267,15 +267,21 @@ where
                     ),
                     _ => {}
                 }
-                if let Err(e) = connected_consumer.disconnect().await {
+                if let Err(e) = connected_consumer
+                    .disconnect(flags::ConsumerDisconnect::default())
+                    .await
+                {
                     error!(
                         "Port{}: Error disconnecting from ConnectedConsumer after PD hard reset: {:#?}",
                         global_port_id.0, e
                     );
                 }
             } else if let Ok(connected_provider) = power.try_device_action::<action::ConnectedProvider>().await {
                 info!("Port{}: Disconnecting provider after hard reset", global_port_id.0);
-                if let Err(e) = connected_provider.disconnect().await {
+                if let Err(e) = connected_provider
+                    .disconnect(flags::ConsumerDisconnect::default())
+                    .await
+                {
                     error!(
                         "Port{}: Error disconnecting from ConnectedProvider after PD hard reset: {:#?}",
                         global_port_id.0, e

type-c-service/src/wrapper/pd.rs

@@ -144,20 +144,31 @@ where
         debug!("Port{}: Current state: {:#?}", local_port.0, state);
         if let Ok(connected_consumer) = power_device.try_device_action::<action::ConnectedConsumer>().await {
             debug!("Port{}: Set max sink voltage, connected consumer found", local_port.0);
-            if voltage_mv.is_some()
-                && voltage_mv
-                    < power_device
+
+            // If the new max voltage is None (remove max voltage limit) or different from the current consumer capability then
+            // we need to disconnect because a change in max voltage requires renegotiation
+            if voltage_mv.is_none()
+                || voltage_mv
+                    != power_device
                         .consumer_capability()
                         .await
                         .map(|c| c.capability.voltage_mv)
             {
-                // New max voltage is lower than current consumer capability which will trigger a renegociation
-                // So disconnect first
                 debug!(
                     "Port{}: Disconnecting consumer before setting max sink voltage",
                     local_port.0
                 );
-                let _ = connected_consumer.disconnect().await;
+
+                match controller.enable_sink_path(local_port, false).await {
+                    Ok(()) => Ok(controller::PortResponseData::Complete),
+                    Err(e) => match e {
+                        Error::Bus(_) => Err(PdError::Failed),
+                        Error::Pd(e) => Err(e),
+                    },
+                }?;
+                let _ = connected_consumer
+                    .disconnect(flags::ConsumerDisconnect::default().with_renegotiation(true))
+                    .await;
             }
         }