Skip to content
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

USBPDDebug: Check whether VPDO is EPR or PPS #1600

Merged
merged 11 commits into from
Apr 1, 2023
Merged

Conversation

Samuelrmlink
Copy link
Contributor

@Samuelrmlink Samuelrmlink commented Feb 27, 2023

  • Please check if the PR fulfills these requirements
  • [*] The changes have been tested locally
  • [] There are no breaking changes
  • What kind of change does this PR introduce?
    Bug fix

  • What is the current behavior?
    SPR PPS VPDOs are being classified as AVS EPR VPDOs.
    This results in an incorrect wattage value being displayed in the PD Debug menu.

  • What is the new behavior (if this is a feature change)?
    VPDOs are identified.

    • PPS - maximum current is shown. (As provided by the source)
    • EPR - maximum wattage (PDP value) is shown.
  • Other information:
    Referencing Table 6-13 and Table 6-14 from the "USB Power Delivery Specification Revision 3.1, Version 1.7" (Pages 138, 139)

    • It should be noted - this change has not yet been tested on EPR power adapters as I don't have any on hand.

@Ralim
Copy link
Owner

Ralim commented Feb 28, 2023

@River-Mochi
Could you give this a test; I'm away from hardware for testing for next few weeks. 🙏🏼

@River-Mochi
Copy link
Contributor

River-Mochi commented Feb 28, 2023

@Samuelrmlink I can't seem to find a working zip of this PR from above verified numbers, could you give me a link to it so I could download to test this?
Are there specific chargers I should focus on? I have a bunch of types, PPS/ non-PPS/ EPR 28V ?
one of each? what should I be checking for?

  • that all of them behave like they did before and no obvious things?
  • does V1 and V2 both have to be tested or only V2 to save me time?

if you have already tested on a bunch of chargers and V1 or V2, then I could focus more on the one you didn't test.
EPR can not be tested on V1 of course bc it doesn't support it. but I could test PPS chargers on V1.

@Samuelrmlink
Copy link
Contributor Author

@River-Mochi i have attached a build for the Pinecil and Pincilv2 here.
Pinecil_VPDOid.zip
Pinecilv2_VPDOid.zip

I have tested this with the Pinecilv1 using a PPS charger. I have not tested it with Pinecilv2 or an EPR charger since I don't have either of those yet. This PR does not change the negotiation, behavior, or policy - it only updates the USBPDDebug menu.

With previous firmware versions PPS power sources would be misinterpreted causing the Debug menu to show wattage ratings of 100w for a 45w charger in the debug menu. As far as I am aware this issue did not extend beyond the debug menu - so functionality of thd Iron was not affected.
This PR just makes the Debug menu more accurate by adding checks that differentiate PPS power profiles from EPR power profiles.
Screenshot_20230228_112723_Drive
PPS power profiles have a maximum current rating, while EPR profiles have a wattage rating. This was already implemented in the PDDebug menu - there just weren't sufficient checks previously.

@dandimi
Copy link

dandimi commented Feb 28, 2023

V1 tested on Baseus 120w gan pro, no issues, no reboots, in pps mode
tested on samsung 25w charger, negotiated 11v, works, no issues
tested on ravpower 45w powerbank with no pps support, negotiated 15v
all power check in debug menu seemed alright and accurate

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 5, 2023

Tested Devices: one Pinecil V1, and two Pinecil V2.

  • tested all 3 devices with 4 different sets of chargers:
  1. random 20V charger GaN
  2. PinePower portable PD 65w/20V GaN
  3. PPS charger with both 100W port and 65W port, both ports tested using both PD60 cables and PD240W cables.
  4. EPR 28V GaN charger tested with 240W EPR cable.

Results

  • PD debug menu looked normal
  • heating up on all was normal, wattages showed expected amounts
  • does not appear to cause any strange side effects
  • tested in Detailed screen and no-detail screen

Additional information

  • Reboot on start-up behavior seen on V2.
    This most likely has nothing to do with this repo, but I want to note this side behavior that happens sometimes with V2:
  • on Both V2 tested, sometimes when I plugged in the cable to the back for a split second, it would show the symbol for no tip installed and 5V but quickly jumped to normal. This did not happen every single time I plugged, rarely but I did catch it about 2 times in the 30 min I was doing bunch of random tests. sometimes the screen would just be black for a split second like it was rebooting and then it was fine. it did not matter which way the cable was flipped. The rebooting at startup happend with Kovol EPR charger that I happen to have handy in the room. symptom of seeing icon for no tip inserted was more rare. but the reboot on initial startup was like 1/8 times on Both V2.

in all cases, if I waited a couple seconds everything was normal and it did not continue to reboot. I did not see it happen plugging into the 20V USB-C chargers, however I also did not test them enough times to know for sure. Pinecil V2 still works fine, it's just this odd reboot occasionally on start.

  • The above did not happen a single time on the Pinecil V1 on any of the 4 chargers tested from 20V to EPR ( on epr of course the V1 only works at 20V since charger is backwards compatible with non-epr devices).

Otherwise, looks fine, nothing jumped out as affecting operation on Pinecil V1 or V2.

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 6, 2023

PPS power profiles have a maximum current rating, while EPR profiles have a wattage rating. This was already implemented in the PDDebug menu - there just weren't sufficient checks previously.

@Samuelrmlink since you dont have V2 yet, I made a video so you can see it.

  • PD debug on 28V EPR charger and Pinecil V2. It normally just shows up as
  • for Kovol (off-brand) epr charger.

8 28V 5. 0A ( no idea why there is a space there but there is always the space in amps)

  • on the PD debug display, EPR chargers always showed 28V 5A as far as the menu display and doesn't show us in watts on Pinecil V2.
Pinecilv2-EPR28V.mp4

Copy link
Contributor

@River-Mochi River-Mochi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested here #1600 (comment)

@Samuelrmlink
Copy link
Contributor Author

@River-Mochi
Thanks for the video - there were a few things in it that stood out to me.

  1. PDO index number jumps from 6 to 8 (skipping 7) with your EPR charger.
  2. Your 28 volt EPR PDO is displaying as though it were a PPS profile (???)

Looking over the code we have here and the USB-PD spec I haven't been able to spot why your EPR PDO is displaying as current. It should be displayed as wattage, since that is how EPR PDOs are framed in the PD spec.
It may be possible that your charger is not following the spec??

Do you have a PD analyzer?

@Samuelrmlink
Copy link
Contributor Author

On a secondary note - I just added another commit to this PR to change the appearance of the current readout.
See below.

Old:
1 5V 3. 0A
2 9V 3. 0A
etc....

New:
1 5V 3.00A
2 9V 3.00A
etc....

See Commit 9a16eb8
Builds:

Pinecil-ctz.zip
Pinecilv2-ctz.zip

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 6, 2023

  1. PDO index number jumps from 6 to 8 (skipping 7) with your EPR charger.
  1. I think it's always been 8 for EPR . I've seen on the V2. 8 28V 5. 0A
  2. I just Tested APPLE 140W 28V EPR, it's also category "8". I can ask other people who have the other Brands too. we have tested 8-10 brands at least now.
  3. I'm pretty sure ever since I've been testing EPR with Pinecil V2 in Aug 2022, it's always shown as 28V 5A inside PD Debug menu and not as watts.
  4. . I will ask some of the other EPR owners to confirm I even have old videos I made when EPR was not working initially (Ralim had to make some changes because of interpretation of the EPR protocol between some charger makers ).

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 6, 2023

Do you have a PD analyzer?

yes I have the KM002C (chargerlabs) with the PC software to save logs.

It may be possible that your charger is not following the spec??

  • I don't totally trust the Kovol 140W 28V to follow the specs,. company did other wierd things too, the 2nd port on this is a QC2.0 not even QC3.0. that is why I dont recommend it to people, it works but there are many better ones.

  • Apple 140W 28V I have should follow spec. they sit on the USB-IF board so they get exceptions put in to accomodate for what they are doing. technically we can say apple would be compliant bc they make it so. but these chargers are meant to have the weird apple tip at the other end and not for USB-C to C EPR cable. but it works since they have to comply bc they sit on the board.

Ralim could confirm what the PD Debug menu is supposed to do but I don't see any time where EPR has not been 28V 5A (or rather it has not show watts there afaik for any EPR charger in the past).

  • Are you a member of the Pinecil Live Chat by chance? I just put out a call to all EPR owners and they are all writing in their results now. and it's all showing the same. These are people with Anker 737 BATTERY PD3.1 28V, and the Voltme 140W-28V EPR. all different brands and they all confirm we all have exact same message. Note these people are using the older Firmware and not this new beta I'm testing here.
  • we have never seen watts displayed in the PD debug menu.
  • you can join the Live community Pinecil chat here and see all messages from past 20 min about this. https://wiki.pine64.org/wiki/Pinecil#Live_Community_Chat

@Samuelrmlink
Copy link
Contributor Author

Samuelrmlink commented Mar 6, 2023

Do you have a PD analyzer?

yes I have the KM002C (chargerlabs) with the PC software to save logs.
<
Could you go ahead and take two captures? One with the Apple EPR charger, and the other with your generic EPR charger?
(I also have the ChargerLabs KM002C unit and the appropriate software for it - so I can open their file format.)

It may be possible that your charger is not following the spec??

  • I don't totally trust the Kovol 140W 28V to follow the specs, they were so cheap the other port on this is a QC2.0 not even QC3.0. that is why I dont recommend it to people, it works but there are so many better ones.
  • Apple 140W 28V I have should follow spec. they sit on the USB-IF board so they get exceptions put in to accomodate for what they are doing. technically we can say apple would be compliant bc they make it so. but these chargers are meant to have the weird apple tip at the other end and not for USB-C to C EPR cable. but it works since they have to comply bc they sit on the board.
    Absolutely. Apple's chargers are pretty much guaranteed to follow the spec since they are the top company on the board & spearheading the USB PD spec.
  • I don't understand USB-PD code but does Ralim allow it to display watts instead of Amps in that section. All chargers on all Pinecil V1 and V2 ever since PD Debug menu came out in 2.17 release have shown for me as Amps inside PD Debug and never have I seen watts. I've tested a few chargers and seen test results from other people, no watts are inside PD debug menu. maybe this needs to be enabled?
    Yeah it's possible that my interpretation of the specification document may be flawed.
    I don't have the EPR-capable power source to test/compare against.
    I just got my Pinecilv2 in the mail two days ago - but don't have any EPR chargers.

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 6, 2023

This is my Apple EPR PD3.1, 28V 140W charger. it is showing differently from the off-brand Kovol.
shows a "9" stage.

9 15-28V 140W
image

IMG_3558.mov

@River-Mochi
Copy link
Contributor

@Samuelrmlink your beta for V2 doesn't have the BLE built in. is it possible you bring in one of Ralim's newer approved versions of the beta so that I can test your features using also PineSAM which gives me extra information.
it will be a beta version that in Advanced menu there is a BLE checkbox to enable/disable.
it was slated to be released with 2.21 stable.

@Samuelrmlink
Copy link
Contributor Author

Samuelrmlink commented Mar 6, 2023

Last night’s build has BLE.
https://github.com/Ralim/IronOS/files/10894694/Pinecilv2-ctz.zip

I noticed that BLE can take a minute before it shows up on your other device though.

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 6, 2023

I noticed that BLE can take a minute before it shows up on your other device though.

just in general you have a long delay pairing BLE pinecil V2 to PineSam?

I usually plug in V2, start the script and then if it doesn't connect on Browser first time, I hit F5. after that even if I quit the terminal script and re-open, then it's faster to connect in the Browser bc pinesam script connected Pinecil to my PC bluetooth already.

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 7, 2023

Attached PD analyzer Logs and Videos of PD Debug for:

@River-Mochi
Copy link
Contributor

@Samuelrmlink did you get a chance to look at the logs I attached; are you good now for this PR?

@River-Mochi
Copy link
Contributor

River-Mochi commented Mar 9, 2023

fwiw. I tested in Windows 11, and PineSAM does not connect if I first connect Pinecil to "windows bluetooth"

Pinecil has to be disconnected from all other OS/devices. and then when I open PineSAM, and let the script do all the backend connecting - then the browser opens automatically and connects to BLE Pinecil automatically and it's good. if the PC is a little slow working with the backend script, then I hit "F5" on the browser and it works.

Copy link
Owner

@Ralim Ralim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise this seems good to me, dont have a suitable charger for testing.
Has this been tested as working (Comments are not super clear)

@River-Mochi
Copy link
Contributor

River-Mochi commented Apr 1, 2023

Has this been tested as working (Comments are not super clear)

yes it has been tested by at least 2 people and it works. I also sent some of my PD logs to Samuel as requested but did not hear back if he did anything with that.

I tested with PPS charger. and Dancho/dandimi (test results here above #1600 (comment)) tested with PPS charger.

Dancho/dandimi said this code helped him, his PPS charger no longer reboots.

I tested also with EPR chargers and it does not seem to cause any harm or break anything.

@Ralim Ralim merged commit 445069c into Ralim:dev Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants