Skip to content

type-c-service/wrapper: Add recovery logic to set_max_sink_voltage#907

Draft
RobertZ2011 wants to merge 1 commit into
OpenDevicePartnership:stable-v0.1.yfrom
RobertZ2011:set-max-sink-voltage-recovery
Draft

type-c-service/wrapper: Add recovery logic to set_max_sink_voltage#907
RobertZ2011 wants to merge 1 commit into
OpenDevicePartnership:stable-v0.1.yfrom
RobertZ2011:set-max-sink-voltage-recovery

Conversation

@RobertZ2011

Copy link
Copy Markdown
Contributor

Since the PD controller is responsible for picking a contract, we can't determine if a renegotiation will actually happen. The current behavior can cause a failure to consume power if no renegotiation happens. Start a sink ready timeout when limiting the maximum sink voltage to allow us to recover in cases where no renegotiation happens.

@RobertZ2011 RobertZ2011 self-assigned this Jun 26, 2026
Since the PD controller is responsible for picking a contract, we can't
determine if a renegotiation will actually happen. The current behavior
can cause a failure to consume power if no renegotiation happens. Start
a sink ready timeout when limiting the maximum sink voltage to allow us
to recover in cases where no renegotiation happens.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a recovery mechanism to set_max_sink_voltage in the Type‑C controller wrapper by starting a “sink ready” timeout when the max sink voltage is changed. The motivation is that the PD controller’s internal contract selection logic may not actually renegotiate after a max-voltage limit is set, which can otherwise leave the system failing to consume power. The change reuses the existing sink-ready-timeout infrastructure (which already broadcasts a sink-ready event on timeout) by ensuring a deadline is set even when no “new contract” event ever occurs.

Changes:

  • Factor the sink-ready timeout calculation into a helper (check_sink_ready_timeout_duration) and reuse it for both contract transitions and the new max-voltage recovery path.
  • Update set_max_sink_voltage / process_set_max_sink_voltage to take a mutable PortState so it can arm sink_ready_deadline after disabling the sink path and before requesting renegotiation.
  • Adjust the SetMaxSinkVoltage controller-command path to plumb PortState into process_set_max_sink_voltage.

Step-by-step review guide

  1. Shared timeout computation

    • Introduces check_sink_ready_timeout_duration(is_epr) and uses it inside check_sink_ready_timeout.
    • Matters because the new recovery logic needs to arm the same timeout with consistent timing (SPR vs EPR).
  2. Recovery logic in set_max_sink_voltage

    • set_max_sink_voltage now locks wrapper state and obtains &mut PortState so it can set sink_ready_deadline.
    • In process_set_max_sink_voltage, after disabling the sink path (and before requesting renegotiation), it arms sink_ready_deadline if not already set. This is the key behavioral change: it ensures a future timeout-driven “sink ready” event can fire even if renegotiation never produces a new-contract indication.
  3. Command path plumbing

    • The SetMaxSinkVoltage arm in process_port_command now fetches the per-port PortState and passes it to process_set_max_sink_voltage.
    • Matters because this path runs through the generic PD command handler, so it must be able to mutate the same per-port deadline used by the timeout event loop.

Potential issues

# Severity File Description Code
1 🟢 Low type-c-service/src/wrapper/pd.rs:315-331 The SetMaxSinkVoltage match arm redundantly re-runs lookup_local_port(command.port) even though local_port was already validated earlier in process_port_command, adding avoidable nesting/duplication. match self.registration.pd_controller.lookup_local_port(command.port) { ... }

Comment on lines 315 to 331
controller::PortCommandData::SetMaxSinkVoltage(voltage_mv) => {
match self.registration.pd_controller.lookup_local_port(command.port) {
Ok(local_port) => {
self.process_set_max_sink_voltage(controller, local_port, voltage_mv)
.await
match state
.port_states_mut()
.get_mut(local_port.0 as usize)
.ok_or(PdError::InvalidPort)
{
Ok(port_state) => {
self.process_set_max_sink_voltage(controller, port_state, local_port, voltage_mv)
.await
}
Err(e) => Err(e),
}
}
Err(e) => Err(e),
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants