Power policy remove intrusive list#731
Draft
RobertZ2011 wants to merge 11 commits intoOpenDevicePartnership:v0.2.0from
Draft
Power policy remove intrusive list#731RobertZ2011 wants to merge 11 commits intoOpenDevicePartnership:v0.2.0from
RobertZ2011 wants to merge 11 commits intoOpenDevicePartnership:v0.2.0from
Conversation
* Also move PSU state onto PSU trait
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Type-C and power policy integration by moving Type-C definitions into type-c-service, switching from embedded_services::power::policy to the standalone power-policy-service, and restructuring wrapper/backing state to remove the previous type-erased intrusive-list-based power policy plumbing.
Changes:
- Migrate Type-C types/APIs into
type-c-service::type_cand update all call sites (service, wrapper, drivers, examples). - Replace legacy embedded power policy interfaces with
power-policy-service(PSU trait, events, capabilities) and update tests/examples accordingly. - Rework wrapper backing storage to hold per-port state/proxy in
Portstructs with mutexes and direct power policy event senders.
Reviewed changes
Copilot reviewed 60 out of 74 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| type-c-service/src/wrapper/vdm.rs | Removes old power-policy generics and uses per-port locked state for VDM notifications. |
| type-c-service/src/wrapper/proxy.rs | Refactors proxy to implement power_policy_service::psu::Psu and adds per-device state/name. |
| type-c-service/src/wrapper/power.rs | Switches power policy event types/capabilities to power-policy-service and adjusts signatures. |
| type-c-service/src/wrapper/pd.rs | Updates PD command handling to use per-port mutex state and new PSU state types. |
| type-c-service/src/wrapper/message.rs | Repoints message types to power-policy-service and crate::type_c definitions. |
| type-c-service/src/wrapper/dp.rs | Drops old power-policy generics from wrapper impl. |
| type-c-service/src/wrapper/cfu.rs | Uses new controller state storage and detaches via power-policy-service events. |
| type-c-service/src/wrapper/backing.rs | Major backing refactor: removes DynPortState, introduces Port + per-port mutex state and policy sender. |
| type-c-service/src/type_c/mod.rs | Introduces new type_c module (ControllerId, capability helpers, constants). |
| type-c-service/src/type_c/external.rs | Updates imports and wiring for external Type-C messaging after module move. |
| type-c-service/src/type_c/event.rs | Adjusts error import source. |
| type-c-service/src/type_c/controller.rs | Switches contracts to power_policy_service::capability::PowerCapability. |
| type-c-service/src/type_c/comms.rs | Adds comms message definitions for Type-C service. |
| type-c-service/src/task.rs | Updates task generics and registration flow for new PSU/power policy context types. |
| type-c-service/src/service/vdm.rs | Updates Service impl to be generic over PSU and uses crate::type_c types. |
| type-c-service/src/service/ucsi.rs | Updates Service impl generics for PSU integration. |
| type-c-service/src/service/power.rs | Updates power-policy event matching to new power-policy-service event enum. |
| type-c-service/src/service/port.rs | Repoints Type-C controller/external types to crate::type_c and updates Service generics. |
| type-c-service/src/service/pd.rs | Updates Service generics and imports for PSU integration. |
| type-c-service/src/service/mod.rs | Makes Service generic over PSU; replaces old power policy message types with new service event type. |
| type-c-service/src/service/controller.rs | Updates Service generics and imports for moved Type-C types. |
| type-c-service/src/lib.rs | Exposes type_c module and updates event imports in crate root/tests. |
| type-c-service/src/driver/tps6699x.rs | Adapts driver contract parsing and power capability mapping to new helpers/types. |
| type-c-service/Cargo.toml | Adds power-policy-service + bitvec deps and forwards defmt/log features. |
| time-alarm-service-messages/Cargo.toml | Enables zerocopy derive feature via workspace dependency specification. |
| supply-chain/audits.toml | Adds/updates audit entries (e.g., jiff, portable-atomic-util). |
| power-policy-service/tests/consumer.rs | Updates tests to new PSU API/types and splits tests for lifetime/logging constraints. |
| power-policy-service/tests/common/mod.rs | Reworks test harness around new service::Service + psu::event::EventReceivers. |
| power-policy-service/tests/common/mock.rs | Updates mock PSU device to implement new Psu trait and emit new PSU events. |
| power-policy-service/src/task.rs | Removes legacy top-level task runner. |
| power-policy-service/src/service/task.rs | Adds new task runner consuming PSU events and driving service::Service. |
| power-policy-service/src/service/provider.rs | Adds provider power allocation logic for new service architecture. |
| power-policy-service/src/service/mod.rs | Introduces new power policy Service core with PSU-device slice and internal state. |
| power-policy-service/src/service/event.rs | Adds service-level event enum for downstream notification. |
| power-policy-service/src/service/context.rs | Adds context for charger registration and initialization in new architecture. |
| power-policy-service/src/service/consumer.rs | Refactors consumer selection/charger configuration logic to new PSU references and state model. |
| power-policy-service/src/service/config.rs | Updates config to use new capability::PowerCapability. |
| power-policy-service/src/psu/mod.rs | Replaces legacy device registration model with Psu trait + State + PsuState. |
| power-policy-service/src/psu/event.rs | Adds PSU-originated event model and multi-receiver dispatcher. |
| power-policy-service/src/provider.rs | Removes legacy provider implementation (moved into service/provider.rs). |
| power-policy-service/src/lib.rs | Re-exports new module layout (capability/psu/service) and removes legacy PowerPolicy. |
| power-policy-service/src/charger.rs | Rehomes charger state machine and messaging under power-policy-service. |
| power-policy-service/src/capability.rs | Expands capability definitions and renames consumer/provider flag wrappers. |
| power-policy-service/Cargo.toml | Adds deps required by new layout (embedded-batteries-async, num_enum, bitfield). |
| examples/std/src/lib/type_c/mock_controller.rs | Updates example mock controller to new type-c-service module paths and capability helpers. |
| examples/std/src/bin/type_c/unconstrained.rs | Updates example wiring for new power-policy service and per-port PSU registration. |
| examples/std/src/bin/type_c/ucsi.rs | Updates UCSI example wiring for new power-policy service and moved Type-C types. |
| examples/std/src/bin/type_c/service.rs | Updates basic Type-C service example to new power-policy task/event wiring. |
| examples/std/src/bin/type_c/external.rs | Removes the old external messaging example binary. |
| examples/std/src/bin/type_c/basic.rs | Updates imports to new type_c_service::type_c module. |
| examples/std/src/bin/power_policy.rs | Updates power policy example to new service/task and PSU event model; removes old comms receiver task. |
| examples/std/Cargo.toml | Removes type-c-external bin entry. |
| examples/std/Cargo.lock | Updates lockfile for new deps and removed legacy ones. |
| examples/rt685s-evk/src/bin/type_c_cfu.rs | Updates embedded example wiring for new power policy service/task and wrapper types. |
| examples/rt685s-evk/src/bin/type_c.rs | Updates embedded example wiring for new power policy service/task and wrapper types. |
| examples/rt685s-evk/Cargo.lock | Updates lockfile for new deps and removed legacy ones. |
| examples/rt685s-evk/.cargo/config.toml | Adjusts formatting and changes DEFMT_LOG default to info. |
| examples/rt633/Cargo.lock | Updates lockfile to include new power-policy-service package. |
| examples/pico-de-gallo/Cargo.lock | Updates lockfile to include new power-policy-service package. |
| embedded-service/src/type_c/mod.rs | Removes embedded-service Type-C module (moved to type-c-service). |
| embedded-service/src/power/policy/policy.rs | Removes embedded-service power policy context/IPC (superseded by power-policy-service). |
| embedded-service/src/power/policy/mod.rs | Removes embedded-service power policy module definitions. |
| embedded-service/src/power/policy/charger.rs | Removes embedded-service charger module (moved to power-policy-service). |
| embedded-service/src/power/mod.rs | Removes embedded-service power module entrypoint. |
| embedded-service/src/lib.rs | Stops exporting event/power/type_c modules and removes power policy init call. |
| embedded-service/src/event.rs | Removes embedded-service event sender/receiver traits. |
| embedded-service/Cargo.toml | Drops embedded-service dependency on embedded-batteries-async. |
| battery-service/src/lib.rs | Temporarily disables power policy comms handling (commented TODO block). |
| battery-service/src/context.rs | Adds dependency on power_policy_service::capability::PowerCapability and comments out power-info update. |
| battery-service/src/acpi.rs | Updates PowerCapability import to power-policy-service. |
| battery-service/Cargo.toml | Adds power-policy-service dependency and forwards defmt/log features. |
| Cargo.toml | Adds power-policy-service as a workspace dependency path. |
| Cargo.lock | Updates lockfile for new/updated deps and removed legacy ones. |
Comments suppressed due to low confidence (1)
power-policy-service/src/service/consumer.rs:239
- The error log string has an extra leading '(' (
"(({}): ..."), which will show up in logs and makes the message harder to search/parse. Please fix the format string to consistently match the other log messages in this module.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
259
to
266
| /// Register the Type-C service with the power policy service | ||
| pub fn register_comms<PD: Lockable + 'static, PR: Receiver<policy::RequestData> + 'static>( | ||
| pub fn register_comms( | ||
| &'static self, | ||
| power_policy_context: &power_policy::policy::Context<PD, PR>, | ||
| ) -> Result<(), intrusive_list::Error> | ||
| where | ||
| PD::Inner: embedded_services::power::policy::device::DeviceTrait, | ||
| { | ||
| power_policy_context.register_message_receiver(&self.power_policy_event_publisher) | ||
| _power_policy_context: &power_policy_service::service::context::Context, | ||
| ) -> Result<(), intrusive_list::Error> { | ||
| // TODO | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
register_comms is currently a stub (// TODO + unconditional Ok(())). Since task_closure relies on this to hook the Type-C service into power-policy events, this will silently leave wait_power_policy_event() unsubscribed and the service will never process power policy changes. Please implement the registration against power_policy_service::service::context::Context (or remove the call/feature until wiring exists) so failures are surfaced and power policy events flow end-to-end.
Comment on lines
+215
to
+218
| /// Send an event to all registered listeners | ||
| async fn broadcast_event(&mut self, _message: ServiceEvent<'a, PSU>) { | ||
| // TODO | ||
| } |
There was a problem hiding this comment.
broadcast_event is a no-op (// TODO). This means consumer/provider connect/disconnect and unconstrained state changes are never published to listeners, so downstream services (e.g., Type-C service) cannot react to power-policy state. Please implement event publication (e.g., via a broadcaster in context::Context or an injected publisher) or remove the unused subscriber plumbing until events are supported.
Comment on lines
+174
to
+176
| let (psu_state, mut port_state) = if let Some(port) = self.ports.get(local_port.0 as usize) { | ||
| (port.proxy.lock().await.psu_state, port.state.lock().await) | ||
| } else { |
There was a problem hiding this comment.
Potential deadlock: this locks port.proxy and then port.state, but other code (e.g. CFU update path) locks in the opposite order. Please enforce a single lock acquisition order for Port fields, or avoid holding both locks at once (e.g., copy the needed psu_state in a short scope, drop the proxy guard, then lock the port state).
Comment on lines
+144
to
+152
| for port in self.ports { | ||
| let (port_state, port_proxy) = (port.state.lock().await, port.proxy.lock().await); | ||
| info!("Controller{}: checking power device", controller_id.0); | ||
| if power.state.state() != power::policy::device::State::Detached { | ||
| if port_proxy.psu_state.psu_state != power_policy_service::psu::PsuState::Detached { | ||
| info!("Controller{}: Detaching power device", controller_id.0); | ||
| power.sender.send(policy::RequestData::Detached).await; | ||
| port_state | ||
| .power_policy_sender | ||
| .send(power_policy_service::psu::event::EventData::Detached) | ||
| .await; |
There was a problem hiding this comment.
Potential deadlock: this path locks port.state and port.proxy together, but other paths (e.g. PD command handling) lock proxy before state. Please standardize the lock order across the wrapper and avoid awaiting (send().await) while holding multiple mutex guards (extract the needed fields, drop guards, then perform async work).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
intrusive_list.SenderandReceivertraits in favor of directly using embassy channels to simplify code (can be added back later if needed).'staticlifetime constraints in service code and update integration tests accordingly.