-
Notifications
You must be signed in to change notification settings - Fork 45
Power policy clean up #711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v0.2.0
Are you sure you want to change the base?
Power policy clean up #711
Conversation
* Move away from type-state type machine * Introduce first proper integration test * Create temporary bridge with type-C code
Cargo Vet Audit Failed
If the unvetted dependencies are not neededPlease modify Cargo.toml file to avoid including the dependencies. If the unvetted dependencies are neededPost a new comment with the questionnaire below to the PR to help the auditors vet the dependencies. Copy and paste the questionnaire as a new comment and provide your answers:1. What crates (with version) need to be audited? 2. How many of the crates are version updates vs new dependencies? 3. To confirm none of the already included crates serve your needs, please provide a brief description of the purpose of the new crates. 4. Any extra notes to the auditors to help with their audits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Refactors power/thermal/host-relay messaging to remove legacy EC memory-map types, introduce shared “*-messages” crates with explicit serialization, and modernize service initialization patterns.
Changes:
- Introduces
embedded_services::relaywith MCTP routing helpers and new per-service*-messagescrates (battery/thermal/debug/time-alarm). - Refactors
power-policy-serviceinto a generic, testable policy core with new unit tests and updated device state-machine handling. - Updates eSPI + debug + battery services to use the new relay/messages flow and gates hardware/defmt-only modules from desktop test builds.
Reviewed changes
Copilot reviewed 53 out of 70 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| thermal-service/src/lib.rs | Refactors thermal service into an explicit Service instance with storage provided by caller; updates comms receive types. |
| thermal-service/src/fan.rs | Threads &Service into fan auto-control logic instead of using global wrappers. |
| thermal-service/src/context.rs | Simplifies context (removes MCTP queue/buffer) and switches MPTF queue to thermal-service-messages. |
| thermal-service/Cargo.toml | Adds dependency + defmt feature wiring for thermal-service-messages. |
| thermal-service-messages/src/lib.rs | New no_std message types + serialization for thermal requests/responses/errors. |
| thermal-service-messages/Cargo.toml | Defines the new message crate and feature flags. |
| supply-chain/audits.toml | Adds cargo-vet audit entries and minor trusted metadata tweak. |
| power-policy-service/tests/consumer.rs | Adds async integration-style tests for consumer attach/detach and swapping. |
| power-policy-service/tests/common/mod.rs | Adds shared harness utilities and a policy task runner for tests. |
| power-policy-service/tests/common/mock.rs | Adds a mock device implementing the policy device trait. |
| power-policy-service/src/task.rs | Reworks task entry to accept a prebuilt policy instance (removes singleton init). |
| power-policy-service/src/provider.rs | Refactors provider connection logic to new generic policy/device model. |
| power-policy-service/src/policy/policy.rs | New policy context abstraction with device/charger registration + request selection. |
| power-policy-service/src/policy/mod.rs | Reshapes policy module exports and adjusts Error::InvalidState signature. |
| power-policy-service/src/policy/device.rs | New device state machine + device container abstractions for policy. |
| power-policy-service/src/policy/charger.rs | Updates charger types/import paths; aligns NodeContainer usage. |
| power-policy-service/src/lib.rs | Makes PowerPolicy generic over device locks/receivers; rewires request processing. |
| power-policy-service/src/consumer.rs | Refactors consumer selection + charger configuration to new context/device APIs. |
| power-policy-service/src/config.rs | Updates config to use local PowerCapability type. |
| power-policy-service/src/charger.rs | Repoints charger wrapper imports to the new policy charger module. |
| power-policy-service/Cargo.toml | Adds deps needed by new policy core + introduces dev-deps for tests. |
| partition-manager/partition-manager/Cargo.toml | Removes defmt from default features. |
| keyboard-service/src/lib.rs | Derives Debug for KeyboardError. |
| examples/std/src/lib/type_c/mock_controller.rs | Updates mock Type-C controller to new wrapper generics and power-policy request types. |
| examples/std/Cargo.toml | Bumps deps and adds new message crates to std example. |
| examples/rt685s-evk/Cargo.toml | Adds time-alarm services and embedded-mcu-hal dependency. |
| examples/rt633/Cargo.toml | Formatting + dependency bumps (e.g., embedded-batteries-async, bq40z50-rx). |
| examples/pico-de-gallo/Cargo.toml | Adds a new standalone example workspace manifest. |
| espi-service/src/task.rs | Removes EC memory map buffer init/validation; updates event loop to “response” path. |
| espi-service/src/mctp.rs | Adds hardcoded ODP MCTP relay type list via new relay macro. |
| espi-service/src/lib.rs | Gates eSPI modules from tests to allow workspace tests on Windows. |
| espi-service/src/espi_service.rs | Replaces legacy EC mem-map routing with relay-based MCTP request/response handling. |
| espi-service/Cargo.toml | Adds relay dependencies and message crates; wires defmt features. |
| embedded-service/src/type_c/external.rs | Removes external Type-C command definitions module (legacy API removal). |
| embedded-service/src/relay/mod.rs | Adds generic message serialization traits + MCTP relay macro and routing helper. |
| embedded-service/src/power/policy/policy.rs | Removes legacy embedded-service power-policy context implementation. |
| embedded-service/src/power/policy/device.rs | Removes legacy embedded-service power-policy device implementation. |
| embedded-service/src/power/policy/action/policy.rs | Removes legacy policy action typestate module. |
| embedded-service/src/power/policy/action/mod.rs | Removes legacy action kind definitions. |
| embedded-service/src/power/policy/action/device.rs | Removes legacy device action typestate module. |
| embedded-service/src/power/mod.rs | Removes legacy power module root. |
| embedded-service/src/lib.rs | Removes legacy ec_type/power/type_c module wiring; adds relay + event modules. |
| embedded-service/src/event.rs | Introduces common Sender/Receiver traits for dynamic channels. |
| embedded-service/src/ec_type/structure.rs | Deletes legacy EC memory-map structures. |
| embedded-service/src/ec_type/protocols/mptf.rs | Deletes legacy MPTF protocol enum. |
| embedded-service/src/ec_type/protocols/mod.rs | Deletes legacy protocol module aggregator. |
| embedded-service/src/ec_type/protocols/debug.rs | Deletes legacy debug protocol enum. |
| embedded-service/src/ec_type/protocols/acpi.rs | Deletes legacy ACPI battery command enum. |
| embedded-service/src/ec_type/message.rs | Deletes legacy StdHostRequest/StdHostPayload host message types. |
| embedded-service/src/ec_type/generator/ec_memory_map.yaml | Deletes EC memory-map generator input. |
| embedded-service/src/ec_type/generator/ec-memory-generator.py | Deletes EC memory-map code generator script. |
| embedded-service/src/comms.rs | Tightens message data to Any + Send + Sync and updates downcast APIs accordingly. |
| embedded-service/Cargo.toml | Moves embassy-futures to normal deps (used by relay/context code). |
| debug-service/src/task.rs | Switches debug-to-host messages to debug-service-messages types. |
| debug-service/src/lib.rs | Gates debug module from tests to avoid defmt dependency on desktop. |
| debug-service/src/debug_service.rs | Updates mailbox receive to new DebugRequest type + uses new buffer size const. |
| debug-service/Cargo.toml | Adds debug-service-messages dependency + defmt feature plumbing. |
| debug-service-messages/src/lib.rs | New no_std debug request/response/error message types + serialization. |
| debug-service-messages/Cargo.toml | Defines the new debug message crate and feature flags. |
| battery-service/src/task.rs | Refactors battery task to accept an explicit Service + device list; returns structured init errors. |
| battery-service/src/lib.rs | Switches ACPI request handling to message crates and sends responses via comms. |
| battery-service/src/device.rs | Reuses battery_service_messages::DeviceId instead of defining a local type. |
| battery-service/src/context.rs | Refactors ACPI command processing to typed requests/responses + updated power-policy comms types. |
| battery-service/src/acpi.rs | Replaces legacy host payload types with battery-service-messages responses. |
| battery-service/Cargo.toml | Adds battery-service-messages, heapless, and power-policy-service deps; wires defmt feature. |
| battery-service-messages/Cargo.toml | Adds new battery message crate manifest. |
| Cargo.toml | Adds new workspace members and dependency versions/paths for message crates. |
| .vscode/settings.json | Adds the new example manifest to watched TOMLs. |
| .github/workflows/check.yml | Adds CI job to clippy the new pico-de-gallo example. |
| .github/workflows/cargo-vet.yml | Bumps cargo-vet version and installs via cargo +stable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using unwrap() here makes Service::new panic if registration fails (e.g., duplicate node, list corruption). Since new already returns Result, propagate registration errors (map to Error or return a dedicated init error) instead of unwrapping.
| if service.register_sensor(sensor).is_err() { | |
| return Err(Error); | |
| } | |
| } | |
| for fan in fans { | |
| if service.register_fan(fan).is_err() { | |
| return Err(Error); | |
| } |
| } | ||
| }; | ||
| let mut state = self.state.lock().await; | ||
| let mut policy_state = self.state.lock().await; |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This holds self.state's mutex guard (policy_state) across multiple .awaits, including locking per-device mutexes and invoking locked_device.connect_provider(...).await and post_provider_connected(...).await. Holding async mutex guards across await points is a common source of deadlocks and priority inversions. Recommendation: limit the scope of policy_state to only the synchronous computations/updates (e.g., compute target power + update cached policy state), then drop the guard before awaiting on device locks/hardware calls; reacquire if you must mutate policy state after the awaited operations.
| .devices() | ||
| .iter_only::<device::Device<POLICY_CHANNEL_SIZE>>() | ||
| { | ||
| for device in self.context.devices().iter_only::<device::Device<'_, D, R>>() { |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This holds self.state's mutex guard (policy_state) across multiple .awaits, including locking per-device mutexes and invoking locked_device.connect_provider(...).await and post_provider_connected(...).await. Holding async mutex guards across await points is a common source of deadlocks and priority inversions. Recommendation: limit the scope of policy_state to only the synchronous computations/updates (e.g., compute target power + update cached policy state), then drop the guard before awaiting on device locks/hardware calls; reacquire if you must mutate policy state after the awaited operations.
| let mut locked_state = device.state.lock().await; | ||
| let mut locked_device = device.device.lock().await; |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This holds self.state's mutex guard (policy_state) across multiple .awaits, including locking per-device mutexes and invoking locked_device.connect_provider(...).await and post_provider_connected(...).await. Holding async mutex guards across await points is a common source of deadlocks and priority inversions. Recommendation: limit the scope of policy_state to only the synchronous computations/updates (e.g., compute target power + update cached policy state), then drop the guard before awaiting on device locks/hardware calls; reacquire if you must mutate policy state after the awaited operations.
| locked_device.connect_provider(target_power).await?; | ||
| self.post_provider_connected(&mut policy_state, requester_id, target_power) | ||
| .await; |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This holds self.state's mutex guard (policy_state) across multiple .awaits, including locking per-device mutexes and invoking locked_device.connect_provider(...).await and post_provider_connected(...).await. Holding async mutex guards across await points is a common source of deadlocks and priority inversions. Recommendation: limit the scope of policy_state to only the synchronous computations/updates (e.g., compute target power + update cached policy state), then drop the guard before awaiting on device locks/hardware calls; reacquire if you must mutate policy state after the awaited operations.
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DebugError opts out of deserialization, but the relay layer (SerializableResult::deserialize) needs to deserialize error results on the receiving side (e.g., host tooling/tests, or any consumer of these message crates). Implement deserialize similarly to ThermalError (map discriminant -> enum, ignore buffer if empty) so that error results can be decoded correctly.
| fn deserialize(discriminant: u16, _buffer: &[u8]) -> Result<Self, MessageSerializationError> { | |
| DebugError::try_from(discriminant) | |
| .map_err(|_| MessageSerializationError::UnknownMessageDiscriminant(discriminant)) |
| .await | ||
| .expect("comms_send is infallible") | ||
| } | ||
| Err(e) => error!("Battery service command failed: {:?}", e), |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On ACPI request failures, the service only logs an error and does not send any response. That can cause the host-side request to hang/timeout indefinitely. Recommendation: send an explicit error result/response back over comms when process_acpi_cmd returns Err (e.g., using an AcpiBatteryResult/error payload) so callers always get a reply.
| Err(e) => { | |
| error!("Battery service command failed: {:?}", e); | |
| // Send an explicit error response so callers always receive a reply | |
| self.comms_send( | |
| crate::EndpointID::External(embedded_services::comms::External::Host), | |
| &e, | |
| ) | |
| .await | |
| .expect("comms_send is infallible") | |
| } |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with_pec is a fixed 100-byte buffer, but src_slice.len() is derived from port_event.length and can exceed 99, which will panic on slicing/copy or indexing. Recommendation: either (a) validate and reject oversize packets before copying, or (b) allocate from an appropriately sized buffer (e.g., reuse ASSEMBLY_BUF_SIZE + 1) and bound-check port_event.length against that capacity.
| } | ||
|
|
||
| impl State { | ||
| /// Returns the correpsonding state kind |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'correpsonding' to 'corresponding'.
| /// Returns the correpsonding state kind | |
| /// Returns the corresponding state kind |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code sets destination_endpoint_id based on the responding service (source_service), but the comment indicates this should be the host's address/EID. As written, responses may be addressed to the wrong endpoint and be dropped by the recipient. Recommendation: plumb the requester's endpoint ID/EID from the deserialized inbound packet into HostResultMessage (or an equivalent response context) and use that to populate destination_endpoint_id for the reply.
| // Use the original requester's endpoint/EID as the destination for the reply. | |
| destination_endpoint_id: mctp_rs::EndpointId::Id(result.requester_endpoint_id), |
No description provided.