type-c-service/max_sink_voltage: Port recovery logic from v1

type-c-service/src/controller/event_receiver.rs

@@ -119,7 +119,7 @@ impl<
     ///
     /// Returns the local port ID and the event bitfield.
     pub async fn wait_event(&mut self) -> Event {
-        let timeout = self.shared_state.lock().await.sink_ready_timeout;
+        let timeout = self.shared_state.lock().await.sink_ready_deadline;
         match select(self.port_event_receiver.wait_next(), async move {
             if let Some(timeout) = timeout {
                 Timer::at(timeout).await;
@@ -133,7 +133,7 @@ impl<
             Either::Second(_) => {
                 let mut status_event = PortStatusEventBitfield::none();
                 status_event.set_sink_ready(true);
-                self.shared_state.lock().await.sink_ready_timeout = None;
+                self.shared_state.lock().await.sink_ready_deadline = None;
                 Event::PortEvent(PortEvent::StatusChanged(status_event))
             }
         }

type-c-service/src/controller/max_sink_voltage.rs

@@ -1,4 +1,5 @@
 //! Max sink voltage port trait implementation
+use embassy_time::Instant;
 use embedded_services::{event::NonBlockingSender, sync::Lockable};
 use embedded_usb_pd::PdError;
 use power_policy_interface::capability::ConsumerDisconnect;
@@ -35,6 +36,18 @@ impl<
             debug!("({}): Disabling sink path before max sink voltage change", self.name);
             self.controller.lock().await.enable_sink_path(self.port, false).await?;
 
+            // In general it's not possible to know if setting the max sink voltage will trigger a renegotiation
+            // because the logic to select a particular contract is specific to the PD controller.
+            // Enable the sink ready timeout as a recovery mechanism. If there's no renegotiation, then the timeout
+            // will result in us broadcasting the existing contract back to the power policy.
+            {
+                let mut shared_state = self.shared_state.lock().await;
+                if shared_state.sink_ready_deadline.is_none() {
+                    shared_state.sink_ready_deadline =
+                        Some(Instant::now() + Self::check_sink_ready_timeout_duration(self.status.epr));
+                }
+            }
+
             // Move our local state out of the consumer state and notify the power policy so it stops
             // tracking us as the active consumer and broadcasts a ConsumerDisconnected event. The
             // renegotiation flag marks this as a temporary disconnect for a recontract.

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

@@ -123,6 +123,20 @@ impl<
         Ok(())
     }
 
+    /// Returns the timeout duration for the sink ready check.
+    pub(super) fn check_sink_ready_timeout_duration(is_epr: bool) -> Duration {
+        Duration::from_millis(
+            (if is_epr {
+                T_PS_TRANSITION_EPR_MS
+            } else {
+                T_PS_TRANSITION_SPR_MS
+            }
+            .maximum
+            .0 * 2)
+                .into(),
+        )
+    }
+
     /// Check the sink ready timeout
     ///
     /// After accepting a sink contract (new contract as consumer), the PD spec guarantees that the
@@ -136,32 +150,26 @@ impl<
     ) -> Result<(), PdError> {
         let contract_changed = self.status.available_sink_contract != new_status.available_sink_contract;
         let mut shared_state = self.shared_state.lock().await;
-        let timeout = &mut shared_state.sink_ready_timeout;
+        let deadline = &mut shared_state.sink_ready_deadline;
 
         // Don't start the timeout if the sink has signaled it's ready or if the contract didn't change.
         // The latter ensures that soft resets won't continually reset the ready timeout
         debug!(
             "({}): Check sink ready: new_contract={:?}, sink_ready={:?}, contract_changed={:?}, deadline={:?}",
-            self.name, new_contract, sink_ready, contract_changed, timeout,
+            self.name, new_contract, sink_ready, contract_changed, deadline,
         );
         if new_contract && !sink_ready && contract_changed {
             // Start the timeout
             // Double the spec maximum transition time to provide a safety margin for hardware/controller delays or out-of-spec controllers.
-            let timeout_ms = if new_status.epr {
-                T_PS_TRANSITION_EPR_MS
-            } else {
-                T_PS_TRANSITION_SPR_MS
-            }
-            .maximum
-            .0 * 2;
+            let timeout = Self::check_sink_ready_timeout_duration(new_status.epr);
 
-            debug!("({}): Sink ready timeout started for {}ms", self.name, timeout_ms);
-            *timeout = Some(Instant::now() + Duration::from_millis(timeout_ms as u64));
-        } else if timeout.is_some()
+            debug!("({}): Sink ready timeout started for {}ms", self.name, timeout);
+            *deadline = Some(Instant::now() + timeout);
+        } else if deadline.is_some()
             && (!new_status.is_connected() || new_status.available_sink_contract.is_none() || sink_ready)
         {
             debug!("({}): Sink ready timeout cleared", self.name);
-            *timeout = None;
+            *deadline = None;
         }
         Ok(())
     }

type-c-service/src/controller/state.rs

@@ -4,20 +4,20 @@ use embassy_time::Instant;
 #[derive(Copy, Clone)]
 pub struct SharedState {
     /// Sink ready timeout
-    pub(crate) sink_ready_timeout: Option<Instant>,
+    pub(crate) sink_ready_deadline: Option<Instant>,
 }
 
 impl SharedState {
     /// Create a new instance with default values
     pub fn new() -> Self {
         Self {
-            sink_ready_timeout: None,
+            sink_ready_deadline: None,
         }
     }
 
     /// Get the current sink ready timeout deadline, if one is pending
-    pub fn sink_ready_timeout(&self) -> Option<Instant> {
-        self.sink_ready_timeout
+    pub fn sink_ready_deadline(&self) -> Option<Instant> {
+        self.sink_ready_deadline
     }
 }
 

type-c-service/tests/power.rs

@@ -4,7 +4,11 @@ use std::ptr;
 
 use embassy_futures::join::join;
 use embassy_time::{Duration, Instant, TimeoutError, with_timeout};
-use embedded_usb_pd::{PowerRole, constants::T_PS_TRANSITION_SPR_MS, type_c::ConnectionState};
+use embedded_usb_pd::{
+    PowerRole,
+    constants::{T_PS_TRANSITION_EPR_MS, T_PS_TRANSITION_SPR_MS},
+    type_c::ConnectionState,
+};
 use power_policy_interface::{
     capability::{
         ConsumerDisconnect, ConsumerFlags, ConsumerPowerCapability, ProviderFlags, ProviderPowerCapability, PsuType,
@@ -310,7 +314,7 @@ impl Test for TestConsumerFlowTimerSinkReady {
 
         // Initially detached with no pending sink-ready timeout.
         assert_eq!(port.lock().await.state().psu_state, PsuState::Detached);
-        assert!(shared_state.lock().await.sink_ready_timeout().is_none());
+        assert!(shared_state.lock().await.sink_ready_deadline().is_none());
 
         let start = Instant::now();
 
@@ -328,7 +332,7 @@ impl Test for TestConsumerFlowTimerSinkReady {
         // The port is attached but not consuming yet, the sink-ready timeout is armed, and no
         // consumer connection has been broadcast to the power policy.
         assert_eq!(port.lock().await.state().psu_state, PsuState::Idle);
-        assert!(shared_state.lock().await.sink_ready_timeout().is_some());
+        assert!(shared_state.lock().await.sink_ready_deadline().is_some());
         assert!(power_policy_receiver.try_receive().is_err());
 
         // The next event is synthesized *inside* `wait_event` by a real timer; nothing in this test
@@ -348,7 +352,7 @@ impl Test for TestConsumerFlowTimerSinkReady {
         // The timer cleared the sink-ready timeout when it synthesized the sink-ready event. The
         // port is not a connected consumer yet: it has only forwarded the updated consumer
         // capability to the power policy, which still has to connect it.
-        assert!(shared_state.lock().await.sink_ready_timeout().is_none());
+        assert!(shared_state.lock().await.sink_ready_deadline().is_none());
 
         // The power policy should now broadcast a consumer connect event.
         match with_timeout(DEFAULT_PER_CALL_TIMEOUT, power_policy_receiver.receive()).await {
@@ -390,7 +394,7 @@ impl Test for TestConsumerFlowTimerSinkReady {
 
         // Back to detached with no pending sink-ready timeout.
         assert_eq!(port.lock().await.state().psu_state, PsuState::Detached);
-        assert!(shared_state.lock().await.sink_ready_timeout().is_none());
+        assert!(shared_state.lock().await.sink_ready_deadline().is_none());
     }
 }
 
@@ -513,6 +517,150 @@ impl Test for TestSinkDisableOnVoltageChange {
     }
 }
 
+/// It's not possible to know if setting the max sink voltage will trigger a renegotiation
+/// because the logic to select a particular contract is specific to the PD controller.
+/// This test ensures that the sink path is disabled and the power policy is notified regardless of whether a renegotiation occurs.
+struct TestSetMaxSinkVoltageRecovery;
+
+impl Test for TestSetMaxSinkVoltageRecovery {
+    async fn run<'port, 'ch>(
+        &mut self,
+        type_c_receiver: TypeCServiceReceiver<'port, 'ch>,
+        power_policy_receiver: PowerPolicyServiceReceiver<'port, 'ch>,
+        port0: TestPort<'port, 'ch>,
+        _port1: TestPort<'port, 'ch>,
+        _port2: TestPort<'port, 'ch>,
+    ) {
+        let TestPort {
+            port,
+            mock,
+            mut event_receiver,
+            ..
+        } = port0;
+
+        {
+            // Set up the mock to report a sink connection and allow enabling the sink path
+            let mut mock = mock.lock().await;
+
+            mock.next_result_get_port_status.push_back(Ok(PortStatus {
+                available_sink_contract: Some(POWER_CAPABILITY_5V_1A5),
+                connection_state: Some(ConnectionState::Attached),
+                power_role: PowerRole::Sink,
+                ..Default::default()
+            }));
+            mock.next_result_enable_sink_path.push_back(Ok(()));
+        }
+
+        // Simulate a plug event and a new consumer contract
+        let mut port_event = PortStatusEventBitfield::none();
+        port_event.set_plug_inserted_or_removed(true);
+        port_event.set_new_power_contract_as_consumer(true);
+        port_event.set_sink_ready(true);
+
+        port.lock()
+            .await
+            .process_event(Event::PortEvent(PortEvent::StatusChanged(port_event)))
+            .await
+            .unwrap();
+
+        let (type_c_result, power_policy_result) = join(
+            with_timeout(DEFAULT_PER_CALL_TIMEOUT, type_c_receiver.receive()),
+            with_timeout(DEFAULT_PER_CALL_TIMEOUT, power_policy_receiver.receive()),
+        )
+        .await;
+
+        // Power policy service should broadcast a consumer connected event
+        match power_policy_result {
+            Ok(PowerPolicyEvent::ConsumerConnected(psu, capability)) => {
+                assert_eq!(
+                    capability,
+                    ConsumerPowerCapability {
+                        capability: POWER_CAPABILITY_5V_1A5,
+                        flags: ConsumerFlags::none().with_psu_type(PsuType::TypeC),
+                    }
+                );
+                assert!(ptr::eq(psu, port0.port));
+            }
+            _ => panic!("Did not receive consumer connected event"),
+        }
+        // Shouldn't get any Type-C service events in this flow
+        assert_eq!(type_c_result.err(), Some(TimeoutError));
+
+        {
+            // Set up the mock to accept a max sink voltage change and disable the sink path
+            let mut mock = mock.lock().await;
+
+            mock.next_result_set_max_sink_voltage.push_back(Ok(()));
+            mock.next_result_enable_sink_path.push_back(Ok(()));
+        }
+
+        port.lock().await.set_max_sink_voltage(None).await.unwrap();
+
+        let (type_c_result, power_policy_result) = join(
+            with_timeout(DEFAULT_PER_CALL_TIMEOUT, type_c_receiver.receive()),
+            with_timeout(DEFAULT_PER_CALL_TIMEOUT, power_policy_receiver.receive()),
+        )
+        .await;
+
+        // Power policy service should broadcast a consumer disconnected event
+        match power_policy_result {
+            Ok(PowerPolicyEvent::ConsumerDisconnected(psu, flags)) => {
+                assert_eq!(flags, ConsumerDisconnect::none().with_renegotiation(true));
+                assert!(ptr::eq(psu, port0.port));
+            }
+            _ => panic!("Did not receive provider connected event"),
+        }
+        // Shouldn't get any Type-C service events in this flow
+        assert_eq!(type_c_result.err(), Some(TimeoutError));
+
+        {
+            // Setup the mock to return the existing contract and enable the sink path again
+            let mut mock = mock.lock().await;
+
+            mock.next_result_get_port_status.push_back(Ok(PortStatus {
+                available_sink_contract: Some(POWER_CAPABILITY_5V_1A5),
+                connection_state: Some(ConnectionState::Attached),
+                power_role: PowerRole::Sink,
+                ..Default::default()
+            }));
+            mock.next_result_enable_sink_path.push_back(Ok(()));
+        }
+
+        // Wait for sink ready recovery, x3 just to be safe and ensure the deadline has passed.
+        let event = with_timeout(
+            Duration::from_millis(T_PS_TRANSITION_EPR_MS.maximum.0 as u64 * 3),
+            event_receiver.wait_event(),
+        )
+        .await
+        .unwrap();
+
+        port.lock().await.process_event(event).await.unwrap();
+
+        let (type_c_result, power_policy_result) = join(
+            with_timeout(DEFAULT_PER_CALL_TIMEOUT, type_c_receiver.receive()),
+            with_timeout(DEFAULT_PER_CALL_TIMEOUT, power_policy_receiver.receive()),
+        )
+        .await;
+
+        // Power policy service should broadcast a consumer connected event
+        match power_policy_result {
+            Ok(PowerPolicyEvent::ConsumerConnected(psu, capability)) => {
+                assert_eq!(
+                    capability,
+                    ConsumerPowerCapability {
+                        capability: POWER_CAPABILITY_5V_1A5,
+                        flags: ConsumerFlags::none().with_psu_type(PsuType::TypeC),
+                    }
+                );
+                assert!(ptr::eq(psu, port0.port));
+            }
+            _ => panic!("Did not receive consumer connected event"),
+        }
+        // Shouldn't get any Type-C service events in this flow
+        assert_eq!(type_c_result.err(), Some(TimeoutError));
+    }
+}
+
 /// Test a power role swap from consumer to provider.
 ///
 /// Starting from a connected consumer, a power role swap turns the port into a provider. The port
@@ -851,6 +999,17 @@ async fn test_sink_disable_on_voltage_change() {
     .await;
 }
 
+#[tokio::test]
+async fn test_set_max_sink_voltage_recovery() {
+    common::run_test(
+        DEFAULT_TEST_DURATION,
+        Default::default(),
+        Default::default(),
+        TestSetMaxSinkVoltageRecovery,
+    )
+    .await;
+}
+
 #[tokio::test]
 async fn test_consumer_to_provider_role_swap() {
     common::run_test(