Power policy direct async call #553
Conversation
RobertZ2011
force-pushed
the
power-policy-direct-async-call
branch
from
1588510 to
aa3e2e0
Compare
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. |
RobertZ2011
force-pushed
the
power-policy-direct-async-call
branch
from
cda7d9a to
3467d89
Compare
There was a problem hiding this comment.
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.
| state: RefCell::new(InternalState::new( | ||
| intermediate.storage, | ||
| // Safe because both have N elements | ||
| power_senders | ||
| .into_array() | ||
| .unwrap_or_else(|_| panic!("Failed to create power events")), | ||
| )), |
There was a problem hiding this comment.
The code calls InternalState::new() but the implementation shows InternalState::try_new() which returns Option<Self>. This will cause a compilation error. The method should be try_new and the result should be unwrapped or handled properly.
| state: RefCell::new(InternalState::new( | |
| intermediate.storage, | |
| // Safe because both have N elements | |
| power_senders | |
| .into_array() | |
| .unwrap_or_else(|_| panic!("Failed to create power events")), | |
| )), | |
| state: RefCell::new( | |
| InternalState::try_new( | |
| intermediate.storage, | |
| // Safe because both have N elements | |
| power_senders | |
| .into_array() | |
| .unwrap_or_else(|_| panic!("Failed to create power events")), | |
| ) | |
| .unwrap_or_else(|| panic!("Failed to create internal state")), | |
| ), |
| /// Common event sender trait | ||
| pub trait Sender<E> { | ||
| /// Attempt to send an event | ||
| /// | ||
| /// Return none if the event cannot currently be sent | ||
| fn try_send(&mut self, event: E) -> Option<()>; | ||
| /// Send an event | ||
| fn send(&mut self, event: E) -> impl Future<Output = ()>; | ||
| } | ||
|
|
||
| /// Common event receiver trait | ||
| pub trait Receiver<E> { | ||
| /// Attempt to receive an event | ||
| /// | ||
| /// Return none if there are no pending events | ||
| fn try_next(&mut self) -> Option<E>; | ||
| /// Receiver an event |
There was a problem hiding this comment.
The Sender and Receiver traits lack documentation. Consider adding doc comments explaining their purpose, when they should be used, and any invariants. For example, document whether try_send returning None means the channel is full or the receiver is closed.
| /// Common event sender trait | |
| pub trait Sender<E> { | |
| /// Attempt to send an event | |
| /// | |
| /// Return none if the event cannot currently be sent | |
| fn try_send(&mut self, event: E) -> Option<()>; | |
| /// Send an event | |
| fn send(&mut self, event: E) -> impl Future<Output = ()>; | |
| } | |
| /// Common event receiver trait | |
| pub trait Receiver<E> { | |
| /// Attempt to receive an event | |
| /// | |
| /// Return none if there are no pending events | |
| fn try_next(&mut self) -> Option<E>; | |
| /// Receiver an event | |
| /// Abstraction over components that can send events of type `E`. | |
| /// | |
| /// This trait provides a common interface for event senders backed by | |
| /// channel-like primitives (for example, [`DynamicSender`]). It allows code | |
| /// to be written against this trait instead of a concrete channel type. | |
| /// | |
| /// # Semantics | |
| /// | |
| /// - [`Sender::try_send`] is **non-blocking**: it attempts to enqueue an | |
| /// event immediately and returns: | |
| /// - `Some(())` if the event was successfully enqueued. | |
| /// - `None` if the event cannot currently be sent. In this crate's | |
| /// implementations, this covers both the channel being full and the | |
| /// receiver side being closed. | |
| /// - [`Sender::send`] returns a future that will **asynchronously** send | |
| /// the event according to the underlying implementation's semantics. | |
| pub trait Sender<E> { | |
| /// Attempt to send an event without blocking. | |
| /// | |
| /// Returns `Some(())` on success. | |
| /// | |
| /// Returns `None` if the event cannot currently be sent. For the | |
| /// provided `DynamicSender` implementation, this occurs when the | |
| /// underlying channel is full or the receiver side has been closed. | |
| fn try_send(&mut self, event: E) -> Option<()>; | |
| /// Send an event, waiting asynchronously until it can be enqueued. | |
| /// | |
| /// The returned future completes once the event has been sent according | |
| /// to the semantics of the underlying sender implementation. | |
| fn send(&mut self, event: E) -> impl Future<Output = ()>; | |
| } | |
| /// Abstraction over components that can receive events of type `E`. | |
| /// | |
| /// This trait provides a common interface for event receivers backed by | |
| /// channel-like primitives (for example, [`DynamicReceiver`]). It allows | |
| /// consumers to be written against this trait instead of a concrete channel | |
| /// type. | |
| /// | |
| /// # Semantics | |
| /// | |
| /// - [`Receiver::try_next`] is **non-blocking**: it attempts to receive an | |
| /// event immediately and returns: | |
| /// - `Some(event)` if an event is available. | |
| /// - `None` if no event can currently be received. In this crate's | |
| /// implementations, this covers both the channel being empty and the | |
| /// sender side being closed. | |
| /// - [`Receiver::wait_next`] returns a future that will **asynchronously** | |
| /// wait for and yield the next event. | |
| pub trait Receiver<E> { | |
| /// Attempt to receive the next event without blocking. | |
| /// | |
| /// Returns `Some(event)` if an event is immediately available. | |
| /// | |
| /// Returns `None` if there are no pending events. For the provided | |
| /// `DynamicReceiver` implementation, this occurs when the underlying | |
| /// channel is empty or the sender side has been closed. | |
| fn try_next(&mut self) -> Option<E>; | |
| /// Receive the next event, waiting asynchronously until one is available. |
1 participant
No description provided.