diff --git a/power-policy-interface/src/capability.rs b/power-policy-interface/src/capability.rs index d53698b1..b757e848 100644 --- a/power-policy-interface/src/capability.rs +++ b/power-policy-interface/src/capability.rs @@ -203,6 +203,72 @@ impl ProviderFlags { } } +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::*; @@ -256,4 +322,34 @@ 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); + assert!(disconnect.renegotiation()); + assert!(!disconnect.switching()); + disconnect.set_renegotiation(false); + assert_eq!(disconnect.0.0, 0x0); + assert!(!disconnect.renegotiation()); + } + + #[test] + fn test_consumer_disconnect_switching() { + let mut disconnect = ConsumerDisconnect::none().with_switching(true); + assert_eq!(disconnect.0.0, 0x2); + assert!(disconnect.switching()); + assert!(!disconnect.renegotiation()); + disconnect.set_switching(false); + assert_eq!(disconnect.0.0, 0x0); + assert!(!disconnect.switching()); + } + + #[test] + fn test_consumer_disconnect_default() { + let disconnect = ConsumerDisconnect::default(); + assert_eq!(disconnect.0.0, 0x0); + assert!(!disconnect.renegotiation()); + assert!(!disconnect.switching()); + } } diff --git a/power-policy-interface/src/psu/event.rs b/power-policy-interface/src/psu/event.rs index fb79b426..5824c731 100644 --- a/power-policy-interface/src/psu/event.rs +++ b/power-policy-interface/src/psu/event.rs @@ -2,7 +2,7 @@ use embedded_services::sync::Lockable; use crate::{ - capability::{ConsumerPowerCapability, ProviderPowerCapability}, + capability::{ConsumerDisconnect, ConsumerPowerCapability, ProviderPowerCapability}, psu, }; @@ -18,7 +18,7 @@ pub enum EventData { /// Request the given amount of power to provider RequestedProviderCapability(Option), /// Notify that a device cannot consume or provide power anymore - Disconnected, + Disconnected(ConsumerDisconnect), /// Notify that a device has detached Detached, } diff --git a/power-policy-interface/src/service/event.rs b/power-policy-interface/src/service/event.rs index 4414ac45..5b0e1c6a 100644 --- a/power-policy-interface/src/service/event.rs +++ b/power-policy-interface/src/service/event.rs @@ -1,7 +1,7 @@ use embedded_services::sync::Lockable; use crate::{ - capability::{ConsumerPowerCapability, ProviderPowerCapability}, + capability::{ConsumerDisconnect, ConsumerPowerCapability, ProviderPowerCapability}, psu::Psu, service::UnconstrainedState, }; @@ -16,7 +16,7 @@ use crate::{ #[non_exhaustive] pub enum EventData { /// Consumer disconnected - ConsumerDisconnected, + ConsumerDisconnected(ConsumerDisconnect), /// Consumer connected ConsumerConnected(ConsumerPowerCapability), /// Provider disconnected @@ -33,7 +33,7 @@ where { fn from(value: Event<'device, PSU>) -> Self { match value { - Event::ConsumerDisconnected(_) => EventData::ConsumerDisconnected, + Event::ConsumerDisconnected(_, flags) => EventData::ConsumerDisconnected(flags), Event::ConsumerConnected(_, capability) => EventData::ConsumerConnected(capability), Event::ProviderDisconnected(_) => EventData::ProviderDisconnected, Event::ProviderConnected(_, capability) => EventData::ProviderConnected(capability), @@ -51,7 +51,7 @@ where PSU::Inner: Psu, { /// Consumer disconnected - ConsumerDisconnected(&'device PSU), + ConsumerDisconnected(&'device PSU, ConsumerDisconnect), /// Consumer connected ConsumerConnected(&'device PSU, ConsumerPowerCapability), /// Provider disconnected diff --git a/power-policy-service/src/service/consumer.rs b/power-policy-service/src/service/consumer.rs index 2a2ea0ed..0ea79b8b 100644 --- a/power-policy-service/src/service/consumer.rs +++ b/power-policy-service/src/service/consumer.rs @@ -8,7 +8,10 @@ use super::*; use power_policy_interface::psu; use power_policy_interface::service::event::Event as ServiceEvent; -use power_policy_interface::{capability::ConsumerPowerCapability, psu::PsuState}; +use power_policy_interface::{ + capability::{ConsumerDisconnect, ConsumerPowerCapability}, + psu::PsuState, +}; /// State of the current consumer #[derive(Debug, PartialEq, Eq)] @@ -216,7 +219,15 @@ impl<'device, Reg: Registration<'device>, Customization: customization::Customiz // so just continue execution. self.disconnect_chargers().await?; - self.broadcast_event(ServiceEvent::ConsumerDisconnected(current_consumer.psu)); + // Indicate why the current consumer is being disconnected. If we are reconnecting + // the same device, it is renegotiating a new power capability. Otherwise, the service + // is switching to a different PSU. + let flags = if ptr::eq(current_consumer.psu, new_consumer.psu) { + ConsumerDisconnect::none().with_renegotiation(true) + } else { + ConsumerDisconnect::none().with_switching(true) + }; + self.broadcast_event(ServiceEvent::ConsumerDisconnected(current_consumer.psu, flags)); // Don't update the unconstrained here because this is a transitional state } @@ -238,7 +249,12 @@ impl<'device, Reg: Registration<'device>, Customization: customization::Customiz } /// Determines and connects the best external power - pub(super) async fn update_current_consumer(&mut self) -> Result<(), Error> { + /// + /// `disconnect_flags` describes the reason for a disconnect and is applied to the + /// [`ServiceEvent::ConsumerDisconnected`] event when the current consumer is removed and not + /// replaced by another one. When switching between consumers the flags are derived from the + /// switch itself (see [`Self::connect_new_consumer`]). + pub(super) async fn update_current_consumer(&mut self, disconnect_flags: ConsumerDisconnect) -> Result<(), Error> { let current_consumer_name = if let Some(current_consumer) = self.state.current_consumer_state { current_consumer.psu.lock().await.name() } else { @@ -262,7 +278,10 @@ impl<'device, Reg: Registration<'device>, Customization: customization::Customiz // Notify disconnect if recently detached consumer was previously attached. if let Some(current_consumer) = self.state.current_consumer_state { self.disconnect_chargers().await?; - self.broadcast_event(ServiceEvent::ConsumerDisconnected(current_consumer.psu)); + self.broadcast_event(ServiceEvent::ConsumerDisconnected( + current_consumer.psu, + disconnect_flags, + )); } // No new consumer available self.state.current_consumer_state = None; diff --git a/power-policy-service/src/service/mod.rs b/power-policy-service/src/service/mod.rs index 0edce4ba..2777257b 100644 --- a/power-policy-service/src/service/mod.rs +++ b/power-policy-service/src/service/mod.rs @@ -14,7 +14,7 @@ use embedded_services::{event::NonBlockingSender, info, sync::Lockable, trace}; use power_policy_interface::charger::{Charger, PsuState}; use power_policy_interface::{ - capability::{ConsumerPowerCapability, ProviderPowerCapability}, + capability::{ConsumerDisconnect, ConsumerPowerCapability, ProviderPowerCapability}, charger::{Event as ChargerEvent, EventData as ChargerEventData}, psu::{ Error, Psu, @@ -117,7 +117,7 @@ impl<'device, Reg: Registration<'device>, Customization: customization::Customiz async fn process_notify_detach(&mut self, device: &'device Reg::Psu) -> Result<(), Error> { info!("({}): Received notify detached", device.lock().await.name()); self.post_provider_removed(device).await; - self.update_current_consumer().await?; + self.update_current_consumer(ConsumerDisconnect::none()).await?; Ok(()) } @@ -132,7 +132,7 @@ impl<'device, Reg: Registration<'device>, Customization: customization::Customiz capability, ); - self.update_current_consumer().await + self.update_current_consumer(ConsumerDisconnect::none()).await } async fn process_request_provider_power_capabilities( @@ -149,10 +149,14 @@ impl<'device, Reg: Registration<'device>, Customization: customization::Customiz self.connect_provider(requester).await } - async fn process_notify_disconnect(&mut self, device: &'device Reg::Psu) -> Result<(), Error> { + async fn process_notify_disconnect( + &mut self, + device: &'device Reg::Psu, + flags: ConsumerDisconnect, + ) -> Result<(), Error> { info!("({}): Received notify disconnect", device.lock().await.name()); self.post_provider_removed(device).await; - self.update_current_consumer().await?; + self.update_current_consumer(flags).await?; Ok(()) } @@ -180,7 +184,7 @@ impl<'device, Reg: Registration<'device>, Customization: customization::Customiz self.process_request_provider_power_capabilities(device, capability) .await } - PsuEventData::Disconnected => self.process_notify_disconnect(device).await, + PsuEventData::Disconnected(flags) => self.process_notify_disconnect(device, flags).await, _ => { info!( "Received unknown PSU event from ({}): {:?}", diff --git a/power-policy-service/tests/common/mock.rs b/power-policy-service/tests/common/mock.rs index 04191064..82076260 100644 --- a/power-policy-service/tests/common/mock.rs +++ b/power-policy-service/tests/common/mock.rs @@ -4,7 +4,9 @@ use embassy_sync::{channel, mutex::Mutex, signal::Signal}; use embedded_batteries_async::charger::{MilliAmps, MilliVolts}; use embedded_services::{GlobalRawMutex, event::NonBlockingSender, info, named::Named}; use power_policy_interface::{ - capability::{ConsumerPowerCapability, PowerCapability, ProviderFlags, ProviderPowerCapability}, + capability::{ + ConsumerDisconnect, ConsumerPowerCapability, PowerCapability, ProviderFlags, ProviderPowerCapability, + }, charger, psu::{Error, Psu, State, event::EventData}, }; @@ -54,6 +56,14 @@ impl<'a, S: NonBlockingSender> Mock<'a, S> { .unwrap(); } + /// Simulate an already-attached consumer renegotiating a new power capability. + pub async fn simulate_update_consumer_power_capability(&mut self, capability: Option) { + self.state.update_consumer_power_capability(capability).unwrap(); + self.sender + .try_send(EventData::UpdatedConsumerCapability(capability)) + .unwrap(); + } + pub async fn simulate_detach(&mut self) { self.state.detach(); self.sender.try_send(EventData::Detached).unwrap(); @@ -77,7 +87,9 @@ impl<'a, S: NonBlockingSender> Mock<'a, S> { pub async fn simulate_disconnect(&mut self) { self.state.disconnect(true).unwrap(); - self.sender.try_send(EventData::Disconnected).unwrap(); + self.sender + .try_send(EventData::Disconnected(ConsumerDisconnect::none())) + .unwrap(); } pub async fn simulate_update_requested_provider_power_capability( diff --git a/power-policy-service/tests/common/mod.rs b/power-policy-service/tests/common/mod.rs index 8225b4a1..4c9de3a7 100644 --- a/power-policy-service/tests/common/mod.rs +++ b/power-policy-service/tests/common/mod.rs @@ -17,7 +17,7 @@ use embassy_time::{Duration, with_timeout}; use embedded_services::GlobalRawMutex; use power_policy_interface::psu::event::EventData; use power_policy_interface::{ - capability::{ConsumerPowerCapability, PowerCapability, ProviderPowerCapability}, + capability::{ConsumerDisconnect, ConsumerPowerCapability, PowerCapability, ProviderPowerCapability}, service::{UnconstrainedState, event::Event as ServiceEvent}, }; use power_policy_service::service::{Service, config::Config, customization}; @@ -168,12 +168,24 @@ pub async fn assert_consumer_disconnected<'a>( receiver: DynamicReceiver<'a, ServiceEvent<'a, DeviceType<'a>>>, expected_device: &DeviceType<'a>, ) { - let ServiceEvent::ConsumerDisconnected(device) = receiver.receive().await else { + let ServiceEvent::ConsumerDisconnected(device, _) = receiver.receive().await else { panic!("Expected ConsumerDisconnected event"); }; assert_eq!(device as *const _, expected_device as *const _); } +pub async fn assert_consumer_disconnected_with_flags<'a>( + receiver: DynamicReceiver<'a, ServiceEvent<'a, DeviceType<'a>>>, + expected_device: &DeviceType<'a>, + expected_flags: ConsumerDisconnect, +) { + let ServiceEvent::ConsumerDisconnected(device, flags) = receiver.receive().await else { + panic!("Expected ConsumerDisconnected event"); + }; + assert_eq!(device as *const _, expected_device as *const _); + assert_eq!(flags, expected_flags); +} + pub async fn assert_consumer_connected<'a>( receiver: DynamicReceiver<'a, ServiceEvent<'a, DeviceType<'a>>>, expected_device: &DeviceType<'a>, diff --git a/power-policy-service/tests/consumer.rs b/power-policy-service/tests/consumer.rs index 18cf97b7..e4638127 100644 --- a/power-policy-service/tests/consumer.rs +++ b/power-policy-service/tests/consumer.rs @@ -7,7 +7,7 @@ use embedded_services::info; use embedded_services::sync::Lockable; use power_policy_interface::capability::ProviderFlags; use power_policy_interface::capability::ProviderPowerCapability; -use power_policy_interface::capability::{ConsumerFlags, ConsumerPowerCapability}; +use power_policy_interface::capability::{ConsumerDisconnect, ConsumerFlags, ConsumerPowerCapability}; mod common; @@ -30,7 +30,8 @@ use crate::common::assert_no_event; use crate::common::assert_provider_connected; use crate::common::assert_provider_disconnected; use crate::common::{ - DEFAULT_TIMEOUT, HIGH_POWER, assert_consumer_connected, assert_consumer_disconnected, mock::FnCall, run_test, + DEFAULT_TIMEOUT, HIGH_POWER, assert_consumer_connected, assert_consumer_disconnected, + assert_consumer_disconnected_with_flags, mock::FnCall, run_test, }; const PER_CALL_TIMEOUT: Duration = Duration::from_millis(1000); @@ -791,6 +792,167 @@ impl Test for TestFindBestConsumerCustomization { } } +/// Test that disconnecting the current consumer to switch to a different PSU sets the +/// `switching` flag on the [`ServiceEvent::ConsumerDisconnected`] event. +struct TestConsumerDisconnectSwitchingFlag; + +impl Test for TestConsumerDisconnectSwitchingFlag { + type Customization = DefaultCustomization; + + async fn run<'a>( + &mut self, + _service: &ServiceMutex<'a, 'a, Self::Customization>, + service_receiver: DynamicReceiver<'a, ServiceEvent<'a, DeviceType<'a>>>, + device0: &DeviceType<'a>, + device0_signal: &Signal, + device1: &DeviceType<'a>, + device1_signal: &Signal, + ) { + info!("Running test_consumer_disconnect_switching_flag"); + // Connect device0 at low power. + device0 + .lock() + .await + .simulate_consumer_connection(LOW_POWER.into()) + .await; + assert_eq!( + with_timeout(PER_CALL_TIMEOUT, device0_signal.wait()).await.unwrap(), + ( + 1, + FnCall::ConnectConsumer(ConsumerPowerCapability { + capability: LOW_POWER, + flags: ConsumerFlags::none(), + }) + ) + ); + device0_signal.reset(); + assert_consumer_connected( + service_receiver, + device0, + ConsumerPowerCapability { + capability: LOW_POWER, + flags: ConsumerFlags::none(), + }, + ) + .await; + + // Connect device1 at high power, the service should switch to it. + device1 + .lock() + .await + .simulate_consumer_connection(HIGH_POWER.into()) + .await; + assert_eq!( + with_timeout(PER_CALL_TIMEOUT, device0_signal.wait()).await.unwrap(), + (1, FnCall::Disconnect) + ); + device0_signal.reset(); + assert_eq!( + with_timeout(PER_CALL_TIMEOUT, device1_signal.wait()).await.unwrap(), + ( + 1, + FnCall::ConnectConsumer(ConsumerPowerCapability { + capability: HIGH_POWER, + flags: ConsumerFlags::none(), + }) + ) + ); + device1_signal.reset(); + + // device0 should be disconnected with the switching flag set since we're switching to device1. + assert_consumer_disconnected_with_flags( + service_receiver, + device0, + ConsumerDisconnect::none().with_switching(true), + ) + .await; + assert_consumer_connected( + service_receiver, + device1, + ConsumerPowerCapability { + capability: HIGH_POWER, + flags: ConsumerFlags::none(), + }, + ) + .await; + + assert_no_event(service_receiver); + } +} + +/// Test that disconnecting the current consumer because it renegotiated a new power capability +/// sets the `renegotiation` flag on the [`ServiceEvent::ConsumerDisconnected`] event. +struct TestConsumerDisconnectRenegotiationFlag; + +impl Test for TestConsumerDisconnectRenegotiationFlag { + type Customization = DefaultCustomization; + + async fn run<'a>( + &mut self, + _service: &ServiceMutex<'a, 'a, Self::Customization>, + service_receiver: DynamicReceiver<'a, ServiceEvent<'a, DeviceType<'a>>>, + device0: &DeviceType<'a>, + device0_signal: &Signal, + _device1: &DeviceType<'a>, + _device1_signal: &Signal, + ) { + info!("Running test_consumer_disconnect_renegotiation_flag"); + // Connect device0 at low power. + device0 + .lock() + .await + .simulate_consumer_connection(LOW_POWER.into()) + .await; + assert_eq!( + with_timeout(PER_CALL_TIMEOUT, device0_signal.wait()).await.unwrap(), + ( + 1, + FnCall::ConnectConsumer(ConsumerPowerCapability { + capability: LOW_POWER, + flags: ConsumerFlags::none(), + }) + ) + ); + device0_signal.reset(); + assert_consumer_connected( + service_receiver, + device0, + ConsumerPowerCapability { + capability: LOW_POWER, + flags: ConsumerFlags::none(), + }, + ) + .await; + + // The same device renegotiates a new (higher) power capability. Since the best consumer is + // still the same device but with a different capability, the service disconnects and + // reconnects it. The disconnect event should carry the renegotiation flag. + device0 + .lock() + .await + .simulate_update_consumer_power_capability(Some(HIGH_POWER.into())) + .await; + + assert_consumer_disconnected_with_flags( + service_receiver, + device0, + ConsumerDisconnect::none().with_renegotiation(true), + ) + .await; + assert_consumer_connected( + service_receiver, + device0, + ConsumerPowerCapability { + capability: HIGH_POWER, + flags: ConsumerFlags::none(), + }, + ) + .await; + + assert_no_event(service_receiver); + } +} + #[tokio::test] async fn run_test_swap_higher() { run_test( @@ -863,3 +1025,25 @@ async fn run_test_find_best_consumer_hook() { ) .await; } + +#[tokio::test] +async fn run_test_consumer_disconnect_switching_flag() { + run_test( + DEFAULT_TIMEOUT, + TestConsumerDisconnectSwitchingFlag, + Default::default(), + DefaultCustomization, + ) + .await; +} + +#[tokio::test] +async fn run_test_consumer_disconnect_renegotiation_flag() { + run_test( + DEFAULT_TIMEOUT, + TestConsumerDisconnectRenegotiationFlag, + Default::default(), + DefaultCustomization, + ) + .await; +} diff --git a/type-c-interface-mocks/src/controller/max_sink_voltage.rs b/type-c-interface-mocks/src/controller/max_sink_voltage.rs new file mode 100644 index 00000000..ad9e4a01 --- /dev/null +++ b/type-c-interface-mocks/src/controller/max_sink_voltage.rs @@ -0,0 +1,25 @@ +//! Mock implementation of [`type_c_interface::controller::max_sink_voltage::MaxSinkVoltage`] + +use embedded_usb_pd::{LocalPortId, PdError}; +use type_c_interface::controller::max_sink_voltage::MaxSinkVoltage; + +use super::FnCall as ControllerFnCall; +use super::Mock; + +/// Contains a [`MaxSinkVoltage`] function call and its arguments +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum FnCall { + SetMaxSinkVoltage(LocalPortId, Option), +} + +impl MaxSinkVoltage for Mock { + async fn set_max_sink_voltage(&mut self, port: LocalPortId, voltage_mv: Option) -> Result<(), PdError> { + self.fn_calls + .push_back(ControllerFnCall::MaxSinkVoltage(FnCall::SetMaxSinkVoltage( + port, voltage_mv, + ))); + self.next_result_set_max_sink_voltage + .pop_front() + .expect("next_result_set_max_sink_voltage not set") + } +} diff --git a/type-c-interface-mocks/src/controller/mod.rs b/type-c-interface-mocks/src/controller/mod.rs index 0419195e..51941065 100644 --- a/type-c-interface-mocks/src/controller/mod.rs +++ b/type-c-interface-mocks/src/controller/mod.rs @@ -10,6 +10,7 @@ use type_c_interface::control::{ vdm::{AttnVdm, OtherVdm}, }; +pub mod max_sink_voltage; pub mod pd; pub mod ucsi; @@ -17,6 +18,7 @@ pub mod ucsi; pub enum FnCall { Pd(pd::FnCall), Ucsi(ucsi::FnCall), + MaxSinkVoltage(max_sink_voltage::FnCall), } /// Mock PD controller for use in tests @@ -30,6 +32,8 @@ pub struct Mock { pub next_result_clear_dead_battery_flag: VecDeque>, /// Next results to return for [`type_c_interface::controller::pd::Pd::enable_sink_path`] pub next_result_enable_sink_path: VecDeque>, + /// Next results to return for [`type_c_interface::controller::max_sink_voltage::MaxSinkVoltage::set_max_sink_voltage`] + pub next_result_set_max_sink_voltage: VecDeque>, /// Next results to return for [`type_c_interface::controller::pd::Pd::get_pd_alert`] pub next_result_get_pd_alert: VecDeque, PdError>>, /// Next results to return for [`type_c_interface::controller::pd::Pd::set_unconstrained_power`] @@ -74,6 +78,7 @@ impl Mock { next_result_get_port_status: VecDeque::new(), next_result_clear_dead_battery_flag: VecDeque::new(), next_result_enable_sink_path: VecDeque::new(), + next_result_set_max_sink_voltage: VecDeque::new(), next_result_get_pd_alert: VecDeque::new(), next_result_set_unconstrained_power: VecDeque::new(), next_result_get_other_vdm: VecDeque::new(), diff --git a/type-c-service/src/controller/max_sink_voltage.rs b/type-c-service/src/controller/max_sink_voltage.rs index d4e25923..567e633a 100644 --- a/type-c-service/src/controller/max_sink_voltage.rs +++ b/type-c-service/src/controller/max_sink_voltage.rs @@ -1,6 +1,7 @@ //! Max sink voltage port trait implementation use embedded_services::{event::NonBlockingSender, sync::Lockable}; use embedded_usb_pd::PdError; +use power_policy_interface::capability::ConsumerDisconnect; use type_c_interface::controller::max_sink_voltage::MaxSinkVoltage; use super::*; @@ -17,6 +18,40 @@ impl< for Port<'device, C, Shared, TypeCSender, PowerSender, LoopbackSender> { async fn set_max_sink_voltage(&mut self, voltage_mv: Option) -> Result<(), PdError> { + // A change in the maximum sink voltage can trigger a PD renegotiation. During that transition the + // source may briefly output a voltage that does not match the active contract, which can cause an + // overcurrent/overvoltage condition on the sink path. If we currently have a connected consumer and + // the limit is actually changing (or being removed), disable the sink path and notify the power + // policy that we have disconnected before applying the new limit. The power policy re-enables the + // sink path when it reconnects the consumer to the renegotiated contract. + let disable_sink_path = match self.psu_state.psu_state { + PsuState::ConnectedConsumer(capability) => { + voltage_mv.is_none() || voltage_mv != Some(capability.capability.voltage_mv) + } + _ => false, + }; + + if disable_sink_path { + debug!("({}): Disabling sink path before max sink voltage change", self.name); + self.controller.lock().await.enable_sink_path(self.port, false).await?; + + // 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. + if let Err(e) = self.psu_state.disconnect(true) { + error!("({}): Error updating PSU state on disconnect: {:?}", self.name, e); + } + if self + .power_policy_sender + .try_send(power_policy_interface::psu::event::EventData::Disconnected( + ConsumerDisconnect::none().with_renegotiation(true), + )) + .is_none() + { + error!("({}): Failed to notify power policy of disconnect", self.name); + } + } + self.controller .lock() .await diff --git a/type-c-service/src/service/power.rs b/type-c-service/src/service/power.rs index b585821c..5a1778db 100644 --- a/type-c-service/src/service/power.rs +++ b/type-c-service/src/service/power.rs @@ -74,7 +74,7 @@ impl<'a, Reg: Registration<'a>> Service<'a, Reg> { pub(super) async fn process_power_policy_event(&mut self, message: &PowerPolicyEventData) -> Result<(), Error> { match message { PowerPolicyEventData::Unconstrained(state) => self.process_unconstrained_state_change(state).await, - PowerPolicyEventData::ConsumerDisconnected => { + PowerPolicyEventData::ConsumerDisconnected(_) => { self.ucsi.psu_connected = false; // Notify OPM because this can affect battery charging capability status if self.ucsi.notifications_enabled.battery_charge_change() { diff --git a/type-c-service/tests/power.rs b/type-c-service/tests/power.rs index 189175c4..812521ed 100644 --- a/type-c-service/tests/power.rs +++ b/type-c-service/tests/power.rs @@ -6,14 +6,18 @@ use embassy_futures::join::join; use embassy_time::{TimeoutError, with_timeout}; use embedded_usb_pd::{PowerRole, type_c::ConnectionState}; use power_policy_interface::{ - capability::{ConsumerFlags, ConsumerPowerCapability, PsuType}, + capability::{ConsumerDisconnect, ConsumerFlags, ConsumerPowerCapability, PsuType}, service::event::Event as PowerPolicyEvent, }; use type_c_interface::{ control::pd::PortStatus, port::event::{PortEvent, PortStatusEventBitfield}, + port::max_sink_voltage::MaxSinkVoltage, util::POWER_CAPABILITY_5V_1A5, }; +use type_c_interface_mocks::controller::{ + FnCall as ControllerFnCall, max_sink_voltage::FnCall as MaxSinkVoltageFnCall, pd::FnCall as PdFnCall, +}; use type_c_service::controller::event::Event; use crate::common::{ @@ -114,11 +118,130 @@ impl Test for TestBasicConsumerFlow { assert_eq!(type_c_result.err(), Some(TimeoutError)); // Power policy service should broadcast a consumer disconnect event match power_policy_result { - Ok(PowerPolicyEvent::ConsumerDisconnected(psu)) => { + Ok(PowerPolicyEvent::ConsumerDisconnected(psu, _)) => { + assert!(ptr::eq(psu, port0.port)); + } + _ => panic!("Did not receive consumer disconnected event"), + } + } +} + +/// Test that changing the max sink voltage while a consumer is connected disables the sink path and +/// notifies the power policy, which broadcasts a `ConsumerDisconnected` event with the renegotiation +/// flag set. Setting the same voltage should do neither. +struct TestSinkDisableOnVoltageChange; + +impl Test for TestSinkDisableOnVoltageChange { + 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>, + ) { + // Bring up a connected consumer at 5V. + { + let mut mock0 = port0.mock.lock().await; + mock0.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() + })); + // Sink path is enabled when the power policy connects the consumer. + mock0.next_result_enable_sink_path.push_back(Ok(())); + } + + 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); + port0 + .port + .lock() + .await + .process_event(Event::PortEvent(PortEvent::StatusChanged(port_event))) + .await + .unwrap(); + + // Wait for the power policy to connect the consumer so the port is in the connected state. + 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; + + match power_policy_result { + Ok(PowerPolicyEvent::ConsumerConnected(psu, _)) => assert!(ptr::eq(psu, port0.port)), + _ => panic!("Did not receive consumer connected event"), + } + + // Setting the same voltage as the active contract must not disable the sink path or disconnect. + { + let mut mock0 = port0.mock.lock().await; + mock0.fn_calls.clear(); + mock0.next_result_set_max_sink_voltage.push_back(Ok(())); + } + port0.port.lock().await.set_max_sink_voltage(Some(5000)).await.unwrap(); + { + let mut mock0 = port0.mock.lock().await; + assert!( + matches!( + mock0.fn_calls.pop_front(), + Some(ControllerFnCall::MaxSinkVoltage( + MaxSinkVoltageFnCall::SetMaxSinkVoltage(_, Some(5000)) + )) + ), + "expected only the max sink voltage to be set without disabling the sink path" + ); + assert!(mock0.fn_calls.is_empty()); + } + // No disconnect should have been broadcast. + assert!(matches!( + with_timeout(DEFAULT_PER_CALL_TIMEOUT, power_policy_receiver.receive()).await, + Err(TimeoutError) + )); + + // Changing the max sink voltage should disable the sink path and notify the power policy. + { + let mut mock0 = port0.mock.lock().await; + mock0.fn_calls.clear(); + mock0.next_result_enable_sink_path.push_back(Ok(())); + mock0.next_result_set_max_sink_voltage.push_back(Ok(())); + } + port0.port.lock().await.set_max_sink_voltage(Some(9000)).await.unwrap(); + + // The power policy should broadcast a consumer disconnect with the renegotiation flag set. + match with_timeout(DEFAULT_PER_CALL_TIMEOUT, power_policy_receiver.receive()).await { + Ok(PowerPolicyEvent::ConsumerDisconnected(psu, flags)) => { assert!(ptr::eq(psu, port0.port)); + assert_eq!(flags, ConsumerDisconnect::none().with_renegotiation(true)); } _ => panic!("Did not receive consumer disconnected event"), } + + // The sink path should have been disabled before the new voltage was applied. + { + let mut mock0 = port0.mock.lock().await; + assert!( + matches!( + mock0.fn_calls.pop_front(), + Some(ControllerFnCall::Pd(PdFnCall::EnableSinkPath(_, false))) + ), + "expected the sink path to be disabled before the voltage change" + ); + assert!( + matches!( + mock0.fn_calls.pop_front(), + Some(ControllerFnCall::MaxSinkVoltage( + MaxSinkVoltageFnCall::SetMaxSinkVoltage(_, Some(9000)) + )) + ), + "expected the max sink voltage to be set" + ); + assert!(mock0.fn_calls.is_empty()); + } } } @@ -132,3 +255,14 @@ async fn test_basic_consumer_flow() { ) .await; } + +#[tokio::test] +async fn test_sink_disable_on_voltage_change() { + common::run_test( + DEFAULT_TEST_DURATION, + Default::default(), + Default::default(), + TestSinkDisableOnVoltageChange, + ) + .await; +}