Conversation
|
@mschawe do you have any hardware to test this on? |
Yes, using a stm32g0b1re on the NUCLEO-G0B1RE + X-NUCLEO-DRP1M1. I am sourcing power using the MP4246 programable buck-boost converter. I am also using the USB Power Delivery Tester PRO for further testing. It has a passthrough feature to monitor sourcing to various sinks (power banks, mostly). My end-goal is to have a functional dual role port utilizing this crate. |
|
Thank you for implementing the source feature! Nice to hear that you have such a sophisticated test setup. I am planning to create a hardware test setup for CI, similar to what you are using (two X-NUCLEO-DRP1M1 on top of some Nucleo variant). Did you also test the existing sink mode, potentially even EPR? Until now, we had no access to any compliance testing device. I will have a look at your changes, in case you tell me they are ready to review. |
If you have ideas to improve the structure, please let me know. I am open to changing it. |
I have not tested sinking nor EPR formally. If you have any formal tests with the PassMark tool you want me to run, or any example programs you want me to try to port quickly to my setup, please let me know! Informally, I can say that I on my setup, I currently have sourcing and sinking working separately. I currently have my CC pins switching between As for the state of the PR, I have added tests for the main source negotiation and re-negotiation state traversals, alongside test for state traversals of the 3 (power, fast power, and data) role-swapping state machines. While I am a bit hesitant, this is probably ready to start reviewing. I will continue to write more tests! |
There was a problem hiding this comment.
I have not tested sinking nor EPR formally. If you have any formal tests with the PassMark tool you want me to run, or any example programs you want me to try to port quickly to my setup, please let me know!
I will think about it, I don't know yet what to test, in order to confirm compliance. Do you have experience with that?
While I am a bit hesitant, this is probably ready to start reviewing.
Don't worry, we can go back and forth a bit. Thank you for your effort so far! I left some first comments, I will tackle the big policy_engine soon!
You added role swap methods to the source device policy manager, but I assume the sink's DPM also needs some features like these for role swapping.
I think it would be nice to add a runnable example for a source and dual-role device. I can also try to create that example for the platform that I have (STM32G431).
| ElectricCurrent::new::<centiampere>(self.raw_max_current().into()) | ||
| } | ||
|
|
||
| pub fn v_safe_5v(max_current_10ma: u16) -> Self { |
There was a problem hiding this comment.
clippy will probably fail for missing public docstrings
| pub struct SourceCapabilities(pub(crate) Vec<PowerDataObject, 16>); | ||
|
|
||
| impl SourceCapabilities { | ||
| pub fn new_with_pdos(pdos: Vec<PowerDataObject, 16>) -> Self { |
There was a problem hiding this comment.
I suggest to make 16 some constant with a name.
| self.transmit(Message::new_with_data(header, Data::EprMode(mdo))).await | ||
| } | ||
|
|
||
| /// Wait for the sink to request a capability with the a Request Message. |
There was a problem hiding this comment.
| /// Wait for the sink to request a capability with the a Request Message. | |
| /// Wait for the sink to request a capability with a Request Message. |
| /// Request a certain power level from the source. | ||
| pub async fn request_power(&mut self, power_source_request: request::PowerSource) -> Result<(), ProtocolError> { | ||
| // Only sinks can request from a supply. | ||
| assert!(matches!(self.default_header.port_power_role(), PowerRole::Sink)); |
There was a problem hiding this comment.
Maybe we could adjust the existing assertions as well.
| debug_assert!(matches!(self.default_header.port_power_role(), PowerRole::Sink)); |
| self.transmit(message).await | ||
| } | ||
|
|
||
| pub async fn transmit_source_capabilities( |
There was a problem hiding this comment.
I think it makes sense to split the protocol layer into source and sink halves, and a common piece that is shared.
|
|
||
| /// Hard reset power supply to vSafe5V via vSafe0V | ||
| /// | ||
| /// A Hard Reset shall not cause any change to either the Rp/Rd resistor being asserted |
There was a problem hiding this comment.
Is that relevant for the device, or rather the driver?
| } | ||
| } | ||
|
|
||
| // FIXME: Split DPM traits between base, epr, and dual roles |
| impl SourceDevicePolicyManager for DummySourceDevice { | ||
| fn evaluate_request( | ||
| &mut self, | ||
| request: &crate::protocol_layer::message::data::request::PowerSource, |
There was a problem hiding this comment.
maybe consider importing part of that
| ) -> impl Future<Output = crate::source::device_policy_manager::CapabilityResponse> { | ||
| async { | ||
| if request.object_position() < 8 { | ||
| crate::source::device_policy_manager::CapabilityResponse::Accept |
There was a problem hiding this comment.
import up to device_policy_manager?
| impl Timer for DummyTimer { | ||
| async fn after_millis(_milliseconds: u64) { | ||
| // Never time out | ||
| pending().await |
There was a problem hiding this comment.
what is the problem with waiting forever?
3 participants
An initial implementation of the USB-PD-R3.2 V1.1 source state machine. I tried my best to mimic paradigms in the sink crate, even if other structures could be preferable. (ex. making a
Statetrait, or implementing atomic sequences for some of the state machines rather than state-by-state).This is still missing Source EPR support. Alongside this, now that both Sourcing and Sinking have implementations, it would now be possible to implement a Dual Role Port. This implementation would allow users to define DPMs for both Sourcing and Sinking in one structure.