Refactor power policy to use direct async calls with power devices by RobertZ2011 · Pull Request #553 · OpenDevicePartnership/embedded-services

RobertZ2011 · 2025-11-03T22:40:21Z

Instead of IPC between the power policy task and power devices, direct async function calls are used through the new DeviceTrait trait.
Power policy now holds references to types that impl DeviceTrait instead of using channels
There's now a per-device channel for sending events to the power policy instead of a single shared channel
Remove type-stated state machines, these just ended up in a shared enum so there wasn't much benefit while resulting in duplication of common logic.
Introduce first power policy integration test for default consumer switching logic
Update examples
Introduce temporary bridge code until type-C code is similarly refactored (types in type_c_service::wrapper::proxy).

github-actions · 2025-11-04T23:15:51Z

Cargo Vet Audit Passed

cargo vet has passed in this PR. No new unvetted dependencies were found.

Copilot

Pull request overview

This PR refactors the power policy service architecture to use direct async calls instead of deferred IPC patterns. It introduces a proxy pattern for power device communication, extracts service message types into dedicated crates for better modularity, and adds comprehensive test coverage for the power policy service.

Changes:

Refactors power policy to use direct async communication with devices via a proxy pattern
Extracts thermal, debug, and battery service message types into separate reusable crates
Implements MCTP relay infrastructure in embedded-services for message routing
Adds test coverage for power policy consumer flows

Reviewed changes

Copilot reviewed 26 out of 32 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
type-c-service/src/wrapper/proxy.rs	New proxy pattern for power device communication channels
power-policy-service/src/*.rs	Refactored to use direct device trait calls instead of action state machine
thermal-service-messages/	New crate containing thermal service request/response types
debug-service-messages/	New crate containing debug service request/response types
battery-service-messages/	New crate containing battery service request/response types
embedded-service/src/relay/	New MCTP relay infrastructure for message serialization
embedded-service/src/event.rs	New event sender/receiver traits
embedded-service/src/power/policy/device.rs	Refactored device with InternalState and DeviceTrait
espi-service/src/mctp.rs	MCTP relay implementation using new infrastructure
power-policy-service/tests/	New test files for consumer flows
examples/std/src/bin/*.rs	Updated examples for new architecture

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

type-c-service/src/wrapper/backing.rs

embedded-service/src/event.rs

Copilot

Pull request overview

Copilot reviewed 28 out of 38 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

power-policy-service/src/provider.rs

embedded-service/src/event.rs

embedded-service/src/power/policy/policy.rs

type-c-service/src/wrapper/power.rs

embedded-service/src/power/policy/device.rs

power-policy-service/src/consumer.rs

embedded-service/src/power/policy/device.rs

type-c-service/src/wrapper/backing.rs

Copilot

Pull request overview

Copilot reviewed 28 out of 40 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

power-policy-service/src/provider.rs

power-policy-service/src/consumer.rs

power-policy-service/src/provider.rs

embedded-service/src/power/policy/policy.rs

Copilot

Pull request overview

Copilot reviewed 28 out of 40 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

embedded-service/src/event.rs

embedded-service/src/power/policy/device.rs

embedded-service/src/event.rs

Copilot

Pull request overview

Copilot reviewed 32 out of 45 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 32 out of 45 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

embedded-service/src/event.rs

embedded-service/src/power/policy/device.rs

power-policy-service/src/task.rs

Copilot

Pull request overview

Copilot reviewed 32 out of 45 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 33 out of 48 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

power-policy-service/src/provider.rs

tullom

Checked out locally and ran the examples. Great job with this!

jerrysxie · 2026-02-04T03:06:28Z

What is the impact of this change on code size? Can we check the size of some of the examples before and after?

* Move away from type-state type machine * Introduce first proper integration test * Create temporary bridge with type-C code

RobertZ2011 · 2026-02-04T18:44:59Z

What is the impact of this change on code size? Can we check the size of some of the examples before and after

With this PR, the rt685s-evk/type_c example shows a 19.6KiB size reduction vs ODP main (223.5KiB vs 243.1KiB).

asasine

I didn't identify anything fundamentally broken and all of my comments ultimately boiled down to nits/suggestions so approved.

asasine · 2026-02-05T20:06:10Z

embedded-service/src/power/policy/policy.rs

+        // Panic safety: The index is guaranteed to be within bounds since it comes from the select_slice result
+        #[allow(clippy::unwrap_used)]


This isn't a sufficient guarantee as the underlying list may have changed between creating the futures and their completion. There's both a panic risk here and a validity risk as index may no longer refer to the same device in that case, even if it is still within the list.

asasine · 2026-02-05T20:07:19Z

embedded-service/src/power/policy/policy.rs

+    pub async fn wait_request(&self) -> Request {
+        let mut futures = heapless::Vec::<_, 16>::new();
+        for device in self.devices().iter_only::<device::Device<'static, D, R>>() {
+            // TODO: check this at compile time


Check ~~this~~ what?

asasine · 2026-02-05T20:10:06Z

embedded-service/src/power/policy/device.rs

+
 /// Device struct
-pub struct Device<const CHANNEL_SIZE: usize> {
+pub struct Device<'a, D: Lockable, R: Receiver<RequestData>>


Please add documentation as to how the struct and trait with identical names are different, how they're meant to be used, and other design decisions. That the struct wraps the trait rather than implementing the trait is confusing at first glance. Consider other names too: trait DeviceTrait should probably be trait Device and struct Device is probably a different semantic thing entirely.

asasine · 2026-02-05T20:12:04Z

embedded-service/src/power/policy/device.rs

 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
-struct InternalState {
+pub struct InternalState {


If it's "internal state," why is it public? That alone is smelly to me. My intuition says it should be private to some module (idk which).

asasine · 2026-02-05T20:13:47Z

embedded-service/src/power/policy/device.rs

+        let result = if matches!(
+            self.state,
+            State::Idle | State::ConnectedConsumer(_) | State::ConnectedProvider(_)
+        ) {
+            Ok(())
+        } else {
+            Err(Error::InvalidState(
+                &[StateKind::Idle, StateKind::ConnectedConsumer],
+                self.state.kind(),
+            ))
+        };


IMO a complex matches! that's nested in other expressions is probably better served by an explicit match

Suggested change

let result = if matches!(

self.state,

State::Idle | State::ConnectedConsumer(_) | State::ConnectedProvider(_)

) {

Ok(())

} else {

Err(Error::InvalidState(

&[StateKind::Idle, StateKind::ConnectedConsumer],

self.state.kind(),

))

};

let result = match self.state {

State::Idle | State::ConnectedConsumer(_) | State::ConnectedProvider(_) => Ok(()),

_ => Err(Error::InvalidState(

&[StateKind::Idle, StateKind::ConnectedConsumer],

self.state.kind(),

)),

};

asasine · 2026-02-05T20:17:25Z

embedded-service/src/power/policy/device.rs

+    pub state: Mutex<GlobalRawMutex, InternalState>,
+    /// Reference to hardware
+    pub device: &'a D,
+    /// Event receiver
+    pub receiver: Mutex<GlobalRawMutex, R>,


Can struct Device become externally mutable? Given its public methods, a RwLock<Device> seems appropriate since many of the methods would be &self, with only some methods on the inner device needing &mut self

RobertZ2011 self-assigned this Nov 3, 2025

RobertZ2011 force-pushed the power-policy-direct-async-call branch from 1588510 to aa3e2e0 Compare November 4, 2025 23:15

github-actions bot added the cargo vet PRs pending auditor review label Nov 4, 2025

RobertZ2011 force-pushed the power-policy-direct-async-call branch from cda7d9a to 3467d89 Compare January 13, 2026 18:07

Copilot AI review requested due to automatic review settings January 13, 2026 18:07

Copilot started reviewing on behalf of RobertZ2011 January 13, 2026 18:07 View session

RobertZ2011 changed the base branch from main to v0.2.0 January 13, 2026 18:07

Copilot AI reviewed Jan 13, 2026

View reviewed changes

type-c-service/src/wrapper/backing.rs Outdated Show resolved Hide resolved

embedded-service/src/event.rs Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings January 22, 2026 19:47

RobertZ2011 force-pushed the power-policy-direct-async-call branch from 4be5bff to a838d81 Compare January 22, 2026 19:47

Copilot started reviewing on behalf of RobertZ2011 January 22, 2026 19:47 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings January 22, 2026 21:54

RobertZ2011 force-pushed the power-policy-direct-async-call branch from d3aa53a to 57c10a6 Compare January 22, 2026 21:54

Copilot started reviewing on behalf of RobertZ2011 January 22, 2026 21:54 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

RobertZ2011 force-pushed the power-policy-direct-async-call branch from 57c10a6 to fbe2f13 Compare January 22, 2026 22:42

Copilot AI review requested due to automatic review settings January 22, 2026 22:45

RobertZ2011 force-pushed the power-policy-direct-async-call branch from fbe2f13 to fd95083 Compare January 22, 2026 22:45

Copilot started reviewing on behalf of RobertZ2011 January 22, 2026 22:46 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

embedded-service/src/event.rs Show resolved Hide resolved

embedded-service/src/power/policy/device.rs Show resolved Hide resolved

embedded-service/src/event.rs Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings January 29, 2026 00:07

RobertZ2011 force-pushed the power-policy-direct-async-call branch from 31b3b00 to 69a1d6b Compare January 29, 2026 00:07

Copilot started reviewing on behalf of RobertZ2011 January 29, 2026 00:07 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings January 29, 2026 19:46

Copilot started reviewing on behalf of RobertZ2011 January 29, 2026 19:47 View session

RobertZ2011 force-pushed the power-policy-direct-async-call branch from bbc2ee1 to d29f8b6 Compare January 29, 2026 19:51

Copilot AI reviewed Jan 29, 2026

View reviewed changes

embedded-service/src/event.rs Show resolved Hide resolved

embedded-service/src/power/policy/device.rs Show resolved Hide resolved

power-policy-service/src/task.rs Outdated Show resolved Hide resolved

Copilot AI reviewed Jan 29, 2026

View reviewed changes

RobertZ2011 changed the title ~~Power policy direct async call~~ Refactor power policy to use direct async calls Jan 29, 2026

RobertZ2011 changed the title ~~Refactor power policy to use direct async calls~~ Refactor power policy to use direct async calls with power devices Jan 29, 2026

RobertZ2011 marked this pull request as ready for review January 29, 2026 22:27

Copilot AI review requested due to automatic review settings January 29, 2026 22:27

RobertZ2011 requested review from a team as code owners January 29, 2026 22:27

RobertZ2011 requested review from asasine, gjpmsft, madeleyneVaca, seanbrns, thdesil and williampMSFT January 29, 2026 22:27

Copilot started reviewing on behalf of RobertZ2011 January 29, 2026 22:27 View session

RobertZ2011 requested a review from tullom January 29, 2026 22:27

RobertZ2011 enabled auto-merge (squash) January 29, 2026 22:28

Copilot AI reviewed Jan 29, 2026

View reviewed changes

power-policy-service/src/provider.rs Show resolved Hide resolved

kurtjd approved these changes Jan 29, 2026

View reviewed changes

tullom approved these changes Jan 29, 2026

View reviewed changes

RobertZ2011 added 4 commits February 4, 2026 10:32

power-policy-service: Convert to async calls

15c3f2e

* Move away from type-state type machine * Introduce first proper integration test * Create temporary bridge with type-C code

Fix-up cfu_buffer_complete example

4c5bf4d

Update pico de gallo lockfile to make CI happy

575e945

Audit updated dependencies and new dependencies for tests

97c2fe1

RobertZ2011 force-pushed the power-policy-direct-async-call branch from 0a7d305 to 97c2fe1 Compare February 4, 2026 18:33

jerrysxie approved these changes Feb 5, 2026

View reviewed changes

asasine approved these changes Feb 6, 2026

View reviewed changes

		// Panic safety: The index is guaranteed to be within bounds since it comes from the select_slice result
		#[allow(clippy::unwrap_used)]

Conversation

RobertZ2011 commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cargo Vet Audit Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

tullom left a comment

Choose a reason for hiding this comment

Uh oh!

jerrysxie commented Feb 4, 2026

Uh oh!

RobertZ2011 commented Feb 4, 2026

Uh oh!

asasine left a comment

Choose a reason for hiding this comment

Uh oh!

asasine Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

asasine Feb 5, 2026

Choose a reason for hiding this comment

RobertZ2011 commented Nov 3, 2025 •

edited

Loading

github-actions bot commented Nov 4, 2025 •

edited

Loading