type-c-service/max_sink_voltage: Port recovery logic from v1#920
type-c-service/max_sink_voltage: Port recovery logic from v1#920RobertZ2011 wants to merge 1 commit into
Conversation
Pull in the recovery logic from v1 and add test coverage.
There was a problem hiding this comment.
Pull request overview
This PR ports the v1 “recovery” behavior for set_max_sink_voltage by arming the sink-ready timer as a fallback mechanism after temporarily disconnecting the sink path. It also renames the shared-state field from a “timeout” concept to an absolute “deadline” to better reflect how the timer is used. Finally, it adds an integration test that exercises the max-sink-voltage disconnect/reconnect flow and validates power-policy notifications.
Changes:
- Rename
SharedState’s sink-ready tracking fromsink_ready_timeouttosink_ready_deadlineand update call sites/tests accordingly. - Factor out
check_sink_ready_timeout_duration(is_epr)and reuse it for both normal contract transitions and max-sink-voltage recovery. - Add an integration test to validate that a max sink voltage change triggers a temporary consumer disconnect and subsequent recovery.
Step-by-step review guide
-
Shared-state semantics change (timeout → deadline)
SharedStatenow stores an absoluteInstant(sink_ready_deadline) instead of a conceptually ambiguous “timeout”.- This matters because both the controller event loop and out-of-band operations (like
set_max_sink_voltage) rely on the same shared timer state.
-
Centralized sink-ready timeout duration computation
check_sink_ready_timeout_duration(is_epr)standardizes how long the system waits before synthesizing a sink-ready event.- This ensures consistent behavior across “normal” PD transitions and the new max-sink-voltage recovery path.
-
Max sink voltage recovery logic
- When changing max sink voltage while connected as a consumer, the code disables the sink path, notifies the power policy of a temporary disconnect, and arms the sink-ready deadline as a recovery mechanism.
- Non-obvious detail: this recovery depends on the controller event receiver being able to observe the newly armed deadline even if no further hardware events occur.
-
Test coverage
- The new test simulates: connect as sink → call
set_max_sink_voltage(None)→ expectConsumerDisconnected(renegotiation=true)→ wait for sink-ready recovery → expectConsumerConnectedagain. - This is important coverage for a tricky cross-service flow (controller ↔ type-c ↔ power-policy), especially for controllers that don’t consistently emit sink-ready events.
- The new test simulates: connect as sink → call
Potential issues
| # | Severity | File | Description | Code |
|---|---|---|---|---|
| 1 | 🔴 High | type-c-service/src/controller/event_receiver.rs:122-126 |
wait_event captures sink_ready_deadline once; if another task arms the deadline while wait_event is blocked with None, the timer path won’t fire and recovery can stall indefinitely without a port event. |
let timeout = self.shared_state.lock().await.sink_ready_deadline; |
| 2 | 🟢 Low | type-c-service/tests/power.rs:611 |
Panic message is incorrect for the ConsumerDisconnected match arm, reducing debuggability when the test fails. |
panic!("Did not receive provider connected event") |
| 3 | 🟢 Low | type-c-service/src/controller/state.rs:6 |
Doc comment still says “timeout” after renaming field to sink_ready_deadline, which can confuse readers about whether this is a duration or an absolute time. |
/// Sink ready timeout |
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| type-c-service/tests/power.rs | Adds an integration test for max-sink-voltage disconnect + recovery; updates existing sink-ready assertions for the renamed deadline accessor. |
| type-c-service/src/controller/state.rs | Renames shared sink-ready tracking field/accessor to sink_ready_deadline. |
| type-c-service/src/controller/power.rs | Extracts check_sink_ready_timeout_duration and updates sink-ready timeout logic to use the renamed deadline field. |
| type-c-service/src/controller/max_sink_voltage.rs | Arms sink-ready recovery deadline when max sink voltage changes force a temporary disconnect. |
| type-c-service/src/controller/event_receiver.rs | Updates event receiver to use sink_ready_deadline and clear it when synthesizing sink-ready. |
Comments suppressed due to low confidence (1)
type-c-service/src/controller/event_receiver.rs:126
wait_eventreadssink_ready_deadlineonce into a localtimeoutand then awaits either the port event or that captured deadline. If another task setssink_ready_deadlinewhilewait_eventis already blocked (e.g.,set_max_sink_voltagearms recovery while the event loop is waiting withNone), the timer branch will never fire and recovery can stall indefinitely when no port events arrive. Consider adding an explicit wake-up/notification path when arming the deadline (e.g., a Signal/Channel inSharedState, or a dedicated loopback variant) sowait_eventcan re-evaluate the deadline immediately.
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;
} else {
| assert_eq!(flags, ConsumerDisconnect::none().with_renegotiation(true)); | ||
| assert!(ptr::eq(psu, port0.port)); | ||
| } | ||
| _ => panic!("Did not receive provider connected event"), |
| @@ -4,20 +4,20 @@ use embassy_time::Instant; | |||
| #[derive(Copy, Clone)] | |||
| pub struct SharedState { | |||
| /// Sink ready timeout | |||
Pull in the recovery logic from v1 and add test coverage.