diff --git a/type-c-service/src/controller/event_receiver.rs b/type-c-service/src/controller/event_receiver.rs index 53983c78..69533a69 100644 --- a/type-c-service/src/controller/event_receiver.rs +++ b/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)) } } diff --git a/type-c-service/src/controller/max_sink_voltage.rs b/type-c-service/src/controller/max_sink_voltage.rs index 567e633a..6e343af8 100644 --- a/type-c-service/src/controller/max_sink_voltage.rs +++ b/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. diff --git a/type-c-service/src/controller/power.rs b/type-c-service/src/controller/power.rs index 6b9d117d..15a20f18 100644 --- a/type-c-service/src/controller/power.rs +++ b/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(()) } diff --git a/type-c-service/src/controller/state.rs b/type-c-service/src/controller/state.rs index 1bf716bd..c4d8a7eb 100644 --- a/type-c-service/src/controller/state.rs +++ b/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, + pub(crate) sink_ready_deadline: Option, } 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 { - self.sink_ready_timeout + pub fn sink_ready_deadline(&self) -> Option { + self.sink_ready_deadline } } diff --git a/type-c-service/tests/power.rs b/type-c-service/tests/power.rs index 6edc2466..d305d1bb 100644 --- a/type-c-service/tests/power.rs +++ b/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(