Ralim / IronOS

Open Source Soldering Iron firmware
https://ralim.github.io/IronOS/
GNU General Public License v3.0
7.28k stars 723 forks source link

Really strange QC3 bug with v2.14 on a Pinecil #823

Closed andyjbm closed 2 years ago

andyjbm commented 3 years ago

Right off the bat let's start by saying that this project is awesome and has turned my TS100 from a tugboat into a battleship.

But this issue is about a pinecil on V2.14.1

_This is a strange software phsyco bug that really shouldn't exist but does.

I have a Pinecil iron hot off the press Jan 2021 (sorry about the pun) from wherever they ship. Delivered with version 2.13.

I have TWO Anker power banks and a generic mains to USB PSU . They are:

  1. Anker powercord speed 10000 QC Model A1266 now referred to as babypack.
  2. Anker powercord Speed 20000 QC Model A1278 now referred to as bigdaddy.
  3. Some generic Chineese Mains QC3 charger Model KeKe-QC-04 (smiley face) now referred to as the mains USB

All bought within a month of each other about 3 years ago I guess.

Steps to reproduce the bug: Plug it in.

Now we've got that out of the way.

With pine64 supplied v2.13 firmware the Pinecil iron would fail to negotiate qc3 with any of the above power sources. It would see 5v and that was it. end of. Nada. But let's face it, the TS100 didn't even have USB-C.

So fastforward to flashing firmware 2.14,1 now reported by the iron as V2.14.2425902 17-01-21 and now: The iron is set to negotiate 20v in the settings The babypack negotiates 12v Yay! The Chineese mains USB negotiates 12v Yay! BUUUT the big daddy is stuck at 5v. Boo.

At 1st I thought that meant that the bigdaddy powerbank was broken somehow but: All of these 3 sources will negotiate 9v on my samsung S9+ using the same usb C cable. So there is some sort of QC3 communication going off with the bigdaddy.

I've used an inline usb A power meter to see whats' going off, so this is a true reflection of reality as reported by the meter and by the iron and S9+

Any ideas? How on earth does one power bank work but not the other from the same manfacturer series but one just has a bigger lipo cell???

I'm open to advice, further test requests etc etc. Really would like to get the iron working on the bigger powerpack because it err lasts longer... and of course because it would further the firmware cause and make the great even greater!

Your grateful user of an awesome firmware,

Andy.

Ralim commented 3 years ago

Alrighty,

So the first thing to note here is that QC is a very loose standard. In that the spec was not released officially; which means there are around 10 implementation's hanging around by different if vendors.

At one stage of the negotiations the host IC (charger) has to unlink the two datapins and pull down on one of them to indicate it supports QC.

What it most likely is that this charger does not implement this to spec.

Normally the device holds the pins in a certain state for 1.2 seconds to signal this, and then this should occur. However some power banks take a tad longer so this firmware allows slightly longer but uses the pins unlinking to signal that the charger has acknowledged.

This can cause issues on chargers that do not implement this properly as some of them see this as too long and timeout.

QC is a bit of a wild west of compatibility.

That all said, if you are able to get a trace of what voltages are on the USB data pins over the first 10 seconds of iron power on, can try and figure out why this one in particular is being fussy

andyjbm commented 3 years ago

Hi @Ralim, thanks for your reply.

Yeah I'd gathered that the QC spec is a bit flimsy shall we say. Great that the upgrade from 2.13 to 2.14.1 got me from no qc3 to 2 out of 3 devices now working!

I found it strange that of two devices from the same manufacturer who's only difference is capacity, one would work and one would not.

I'm gonna have to get a usb c sniffer and see what's going off, or maybe just butcher a cable and get them hooked up to my picoscope! Yeah maybe that's the best approach.

I'll keep you posted with the results...

Andy.

Ralim commented 3 years ago

Yeah QC is a "mess". I'm happy to add extra logic to extend support to more devices, though its a bit tough to do without having them on hand to test really. (Naturally I have slowly collected QC adapters and the firmware works on all of them 🙄 )

Its probably a slightly different chip being used due to different power output / different upstream manufacturer (not many power banks are made by the brand on the side).

All that is required is an analog trace of the 0-3.3V analog voltages on the usb data pins. For QC the "usb-c" PD protocol is not used.

trvrnrth commented 3 years ago

To add some more anecdotal evidence to this I have an Anker PowerCore Speed 10000 (A1266) QC3 bank which works fine on a TS80 with stock firmware 1.07 but is very finnicky with IronOS. I've just re-checked with the latest 2.14.1 and I can get it to negotiate 9V after several attempts plugging and unplugging so by the sounds of it the timing is just a little bit outside of the range that the bank is happy with.

trvrnrth commented 3 years ago

I've just had a bit of a mess around and if I change the initial delay to i > 150 instead of i > 130 in the below that gets me up and running reliably. I presume that change would break other banks but... https://github.com/Ralim/IronOS/blob/1f6a3ad16728992d50e8992317ebf83edc8bafab/source/Core/Src/QC3.cpp#L144

trvrnrth commented 3 years ago

I appear to not be the only person on the internet to pick that magic number either: https://github.com/vdeconinck/QC3Control/blob/42e6240f4e23324a6190ae9bbd054481705bf103/src/QC3Control.h#L18

andyjbm commented 3 years ago

I've just had a bit of a mess around and if I change the initial delay to i > 150 instead of i > 130 in the below that gets me up and running reliably. I presume that change would break other banks but...

https://github.com/Ralim/IronOS/blob/1f6a3ad16728992d50e8992317ebf83edc8bafab/source/Core/Src/QC3.cpp#L144

Hi, So not having had chance to cut up a usb cable and take a snapshot of the handshake taking place with my pack I'll give this a try....

Andy.

andyjbm commented 3 years ago

Hi @trvrnrth @Ralim ,

Well yay! that did it. Changed the i>130 to i>150 as @trvrnrth suggested and now my 20000mAh pack also negotiates 12v

So I have a full complement of QC3 sources working with Pinecil.

Many thanks.

@Ralim if it would assist the cause I can still go ahead and observe the handshake on the usb data lines for you?

Other than that my issue is resolved.

Another happy bunny.

Andy.

Ralim commented 3 years ago

So this is interesting, as some chargers turn on the pulldown at around 1.2 seconds, and if you hold for longer than around 1.3 seconds they then cancel entering QC.

Would you be able to test with trying the if (i == 140) { line to be larger?

Would like to narrow down if this is an issue with the unit trying to charge into 9/12/20V mode to early for these supplies or if they are not passign through the pullup well and the extra pulldown makes them then not work.

trvrnrth commented 3 years ago

This probably isn't the answer you'd like but I've just tested by taking out the call to QC_DM_PullDown entirely, leaving the initial delay set to 130. Occasionally negotiation will work as I'd initially observed but it only becomes reliable when I raise the initial delay to 150.

trvrnrth commented 3 years ago

In case it helps this also works for me:

  QC_DPlusZero_Six();

  // Delay 1.25 seconds
  osDelay(1250);
  if (QC_DM_PulledDown()) {
    // We have a QC capable charger
    osDelay(250);
    QC_Seek9V();
Ralim commented 3 years ago

Would be curious if the old code is reliable with a pull-up on the D- pin? Wondering if your device has a pulldown internally thats causing grief?

trvrnrth commented 3 years ago

@Ralim I'm happy enough to test alternative negotiation code if you've got something specific in mind.

VaZso commented 3 years ago

I have checked the latest (v2.15) firmware with "Baseus 18W" power supply and it still does not work like in v2.14, but it works well at 12V using the previous firmware.

Pinecil starts displaying 5.1V, then voltage goes up till around 9V, then goes down back to around 5V in similar steps. However, the firmware works well using USB PD-capable chargers and power bank I tried and also using another power supply from the same manufacturer I use to charge my phone.

It is strange that older firmwares work well using this power supply while the latest two firmwares are not.

ZipperZ commented 3 years ago

Similar as @VaZso Pinecil with v2.15 starts at ~5V, then when enabling the heater it obviously spits out an error that voltage is low, but after this error the voltage rises to 9V and heater can be enabled successfully at low power. It does not negotiate higher than 9V, but the both supplies (wall and powerbank) are capable and can be triggered using "USB-Tester". Have tested with Baseus 45W power-bank and 100w no-name wall adapter

PD seems to work flawlessly on all supplies that I have.

Ralim commented 3 years ago

In theory the build over here should have an extra menu item for a QC Mode that might let you test different settings for the negotiation.

If you could give the different options (0-3) an attempt would be good to know. Note: I dont have hardware to hand, so may be totally broken to (sorry 🙏🏼 )

ZipperZ commented 3 years ago

Sorry for being a n00b, but I assume that I have to compile that branch? If I will manage to somehow to compile Makefile project (?) under windows, then I will for sure give it a try (Cygwin is the correct keyword to begin the journey?)

Ralim commented 3 years ago

Oh sorry, no compiling required. GitHub has already done it for you.

Find the latest commit and click the green tick to get the details of the CI system, then go to the details and find the "artefact" for your model (aka zip file)

Or if in doubt go to the actions tab in GitHub's, and find the newest one for that branch. Then find the one called CI (not CodeQL) and click on it. Once that loads at the very bottom of the page should be the zip files

VaZso commented 3 years ago

I haven't looked inside the code, but downloaded image from f77373a which works on Pinecil, but QC Mode option always shows 9, I can not change it to something else.

ZipperZ commented 3 years ago

I haven't looked inside the code, but downloaded image from f77373a which works on Pinecil, but QC Mode option always shows 9, I can not change it to something else.

Yes, same here, option stuck on '9'. But I am getting higher voltages from both supplies

ZipperZ commented 3 years ago

GitHub is something... A compiler...

Have modified this function: `

static bool settings_displayQCMode(void) { printShortDescription(SettingsItemIndex::QCMode, 5); OLED::printNumber(systemSettings.QCNegotiationMode, 2, FontStyle::LARGE); return false;}

`

And on 5 power sources (2 car adaptors - Anker and Baseus, 2 power banks Huawei and Baseus, 1 No-name wall adapter) getting the same results:

So whatever else was done made the major influence and not this parameter?

VaZso commented 3 years ago

I did the same modification before I went from home. :)

For me, I had the same result:

VaZso commented 3 years ago

Is there any way to write some debug-like information out of Pinecil? It would be good to see what happens inside...

I think negotiation itself is theoretically good in my case, so I think that is the reason of the timing change of different methods which was tested above had not improved it - at least not with my power supply.

Practically what I see is it sets QC2, then tries to go above ~8V and if it was possible, it switches to QC3 mode. That is what I see on displayed voltage when it comes up to 8.XV, then goes back to 5V.

If I force the software to remain at QC2, both of my chargers can set the voltage to 9V. If I allow it to switch to QC3, then one of my chargers goes up to 12V and the other drops to 5V.

Currently I don't see where the negotiation continues after QCMode = QCState::QC_3; and a return happens, but that is the point where my other power supply fails to set the voltage.

However, that power supply is also QC3 capable (as per written on it) and it can also go up to 12V at 1.5A. Additionally, old Pinecil software was able to use it at 12V.

Ralim commented 3 years ago

@Vazso

Is there any way to write some debug-like information out of Pinecil? It would be good to see what happens inside...

Debugging information can be written out the uart if there is a need for it, though given we are not reading the pins with an adc, measuring the signals externally is usually far more telling than the high/low readings from firmware.

I think negotiation itself is theoretically good in my case, so I think that is the reason of the timing change of different methods which was tested above had not improved it - at least not with my power supply.

Practically what I see is it sets QC2, then tries to go above ~8V and if it was possible, it switches to QC3 mode. That is what I see on displayed voltage when it comes up to 8.XV, then goes back to 5V.

If I force the software to remain at QC2, both of my chargers can set the voltage to 9V. If I allow it to switch to QC3, then one of my chargers goes up to 12V and the other drops to 5V.

Currently I don't see where the negotiation continues after QCMode = QCState::QC_3; and a return happens, but that is the point where my other power supply fails to set the voltage.

After this negotiation is "done" and then the iron will return and the hunt-and-seek code to track the target voltage will then run. Where it will emit the state pulses to step voltage up or down to track to the user set voltage. If the charger is a QC2 charger it should ignore these pulses and maintain its prior negotiated voltage. (QC2 is subset of QC3). Currently the code is setup to do QC entry, which will land at 9V. Then it will emit pulses to raise the 9V up to 12V if 12V is configured. If your charger is QC2 then setting the iron to 9V should prevent these pulses being generated (so long as your usb cable has a a low voltage drop), i.e. rated for >2A).

Is your charger that is failing to maintain the 9V output QC3 certified? Does it support 12V?

However, that power supply is also QC3 capable (as per written on it) and it can also go up to 12V at 1.5A. Additionally, old Pinecil software was able to use it at 12V.

The QC code has not been changed, only that it will wait a little longer before negotiation as it gives the PD code longer to finish. This is by design as the QC is really only provided as a worst case situation, as 9 and 12V are very low power levels.

QC spec allows voltage negotiation at any time, so if your charger does not support the different timing its most likely not certified.

If QC2 only mode would fix your charger, that could be added as an option.

@ZipperZ You can check the Pull request to see what was changed. For modes 0 and 1 keep the longer 2 second timeout and are just toggling the extra pulldown on and off. Mode 0 is the same path as the firmware without any of these patches. If mode 0 on this fork works for you but the latest master build doesn't; then the most likely thing is that its a timing issue and the slight changes in code execution time are pushing you either side of your chargers timing requirements.

Given that modes 2 and 3 don't work, you charger certainly seems to require the longer than spec entry time, but thats already part of the firmware. 😓

Ralim commented 3 years ago

I did also fix the typo in the settings menu, sorry about that 😢 Late night code editing.

VaZso commented 3 years ago
Is there any way to write some debug-like information out of Pinecil?
It would be good to see what happens inside...

Debugging information can be written out the uart if there is a need for it, though given we are not reading the pins with an adc, measuring the signals externally is usually far more telling than the high/low readings from firmware.

I think negotiation itself is theoretically good in my case, so I think that is the reason of the timing change of different methods which was tested above had not improved it - at least not
with my power supply.

Practically what I see is it sets QC2, then tries to go above ~8V and if it was possible, it switches to QC3 mode.
That is what I see on displayed voltage when it comes up to 8.XV, then goes back to 5V.

If I force the software to remain at QC2, both of my chargers can set the voltage to 9V.
If I allow it to switch to QC3, then one of my chargers goes up to 12V and the other drops to 5V.

Currently I don't see where the negotiation continues after QCMode = QCState::QC_3; and a return happens, but that is the point where my other power supply fails to set the voltage.

After this negotiation is "done" and then the iron will return and the hunt-and-seek code to track the target voltage will then run. Where it will emit the state pulses to step voltage up or down to track to the user set voltage. If the charger is a QC2 charger it should ignore these pulses and maintain its prior negotiated voltage. (QC2 is subset of QC3). Currently the code is setup to do QC entry, which will land at 9V. Then it will emit pulses to raise the 9V up to 12V if 12V is configured. If your charger is QC2 then setting the iron to 9V should prevent these pulses being generated (so long as your usb cable has a a low voltage drop), i.e. rated for >2A).

Both of my power supplies I tried land at 9V at first, then the firmware starts to emit pulses which causes one of the power supplies to increase voltage, but other one drops down to 5V at this phase. I have tried to limit the pulses to 1 which caused the first one to only decrease the voltage slightly (which is as expected) and the other one dropped to 5V the same way as before. If I skip these pulses completely, both power supplies remain at 9V.

Also, if I modify the code to call the decrease routine, it works on the first power supply and does not modify the behaviour of second one.

Anyway, voltage drop does not take effect when the heater is not working as power consumption of the hardware is really low when heater is turned off (the cause of voltage drop is the current flowing through the wire's resistance which basically depends on the length of cable, the exact material [ie. copper] and its diameter).

Is your charger that is failing to maintain the 9V output QC3 certified? Does it support 12V?

I don't know if it has valid certification but it has QC logo including a "3" around it, a "Qualcomm Quick Charge 3.0" text and a lot of logos including FCC / CE signs and is capable of delivering 12V at 1.5A. It has two outputs and it was manufactured by UGREEN I have a smaller version manufactured by Baseus and same problem which only has one USB output and also supports 12V 1.5A

The other power supply which works was also manufactured by Baseus and it has a QC3-capable and a non-QC port and 12V 1.5A rating.

I have another power supply of same manufacturer which supports USB PD and also has a USB A connector, that one supports 20V at 1.5A through USB A which works. If I know well, PD only works through Type-C, so it should negotiate using QC3.

However, that power supply is also QC3 capable (as per written on it) and it can also go up to 12V at 1.5A.
Additionally, old Pinecil software was able to use it at 12V.

The QC code has not been changed, only that it will wait a little longer before negotiation as it gives the PD code longer to finish. This is by design as the QC is really only provided as a worst case situation, as 9 and 12V are very low power levels.

QC spec allows voltage negotiation at any time, so if your charger does not support the different timing its most likely not certified.

If QC2 only mode would fix your charger, that could be added as an option.

Now I have tried older firmwares and I was wrong, nor 2.14, nor 2.14.1 work with the charger(s) above. However, v2.13 works perfectly.

I have tried to figure out what could be the cause but I haven't found it yet - till this point (see later). It was a very early version of Pinecil firmware which was not separated for different hardware yet... however, as I see the basic QC negotiation codes are not changed since then, but FUSB302 driver had a bit stronger modification and there is a separate thread for power handling...

At first, when I have disabled USB-PD, my charger started negotiating 12V over QC3 instead of dropping back to 5V, so the root cause is coming from "PD handling".

Basically QC_resync() function is called from Power.cpp in a function named power_check(), which checks if FUSB302 is present and calls QC_resync() only when PD discovery is completed or timed out, otherwise returns without calling it. How long does it take for a PD-capable power supply to negotiate? As far as I can see, the code waits at least 1 second at first but I have not dig deep enough to count if it tries to reset, etc... However, my PD chargers seem to negotiate very quickly - does it necessary to wait that long for a timeout (I don't know what the PD specification states)?

Instead, I moved QC_resync() from here to POWThread.cpp. Currently, I put a 1s delay after usb_pd_detect() check which calls fusb302_start_processing() function, then I call QC_resync() function.

That way all of my chargers are working, including those which were dropped back to 5V and my PD chargers. However, if I increase delay further to 2s, then my chargers drop back to 5V and also drop to 5V when I call it after let's 10s, while my other QC3 charger accepts signals to go up to 12V also after this 10s delay. I haven't checked yet further what is the timeframe it accepts these signals.

So, it seems timeout of PD discovery takes relatively long time and my QC3 charger does not handling voltage modifier pulses after a specific timeout which you wrote it should handle. One of them is Baseus TC012QC and another one is UGREEN CD161, both are having this voltage drop problem and both work if QC negotiation starts earlier.

I think if the longer timeout is necessary for PD negotiation, a setup option which could limit (or extend) this timeout would be the best option. So instead of limiting to QC2, I would add a maximal timeout value for PD negotiation. If most of the PD-capable chargers are fast in this relation, then I may add a shorter value as default, otherwise a value which can be reduced would be very welcome.

I think it should solve a lot of problems covered by this and another "issue" mentioning regression in QC handling which worked in earlier firmware while I understand PD is the most important power source of this hardware.

What is written in specification about timeout?

Sorry for the long message, I was digging into the code while this message was composed and it took a few hours to reach the end of it while I have tried to find a way to get my power supplies working...

Ralim commented 3 years ago

Hi,

Indeed it is a long comment, but a good one.

To summerise:

  1. The required delay for PD negotiation is taking too long for the timeout to occur, which in turn is upsetting some of your chargers
  2. If the QC3 pulses are not started quickly after entering QC mode the chargers do not like this and reset to 5V
  3. QC2 mode works because there are no QC3 pulses sent

PD calls for a few seconds (around 2) but I have a supply here that will spend up to 3 seconds polling for SOP to complete before it announces anything, so for it to work on all cables around 4-5 seconds is the minimum required for timing out waiting for the capabilities announcement.

Of course this is a little messy as well as its all over the place how adapters behave, other ones only give you 500ms to react :). And others will kill all PD negotiation as soon as you do anything with the data pins (thus why QC is held back). Even though by PD spec you are not allowed to also support QC on the type-c port, many overseas manufacturers do not listen to this :)

So if I understand, these would be the following options to allow you to use your QC chargers:

  1. Option to disable PD (QC only mode)
  2. Option to reduce PD negotiation timeout to minimum (2 seconds)
  3. QC2 only mode (Disable voltage hunting)

I would prefer options that don't touch the PD stack as its far more important generally, but providing a default off option to reduce the timeout seems reasonable.

VaZso commented 3 years ago

Hi,

Hi,

Indeed it is a long comment, but a good one.

To summerise:

  1. The required delay for PD negotiation is taking too long for the timeout to occur, which in turn is upsetting some of your chargers
  2. If the QC3 pulses are not started quickly after entering QC mode the chargers do not like this and reset to 5V
  3. QC2 mode works because there are no QC3 pulses sent

Exactly. Also, I suspect other users who were speaking about the same issue may have chargers with the same behaviour.

PD calls for a few seconds (around 2) but I have a supply here that will spend up to 3 seconds polling for SOP to complete before it announces anything, so for it to work on all cables around 4-5 seconds is the minimum required for timing out waiting for the capabilities announcement.

Is it also an out of specs behaviour or simply it was not specified in protocol description?

Of course this is a little messy as well as its all over the place how adapters behave, other ones only give you 500ms to react :). And others will kill all PD negotiation as soon as you do anything with the data pins (thus why QC is held back). Even though by PD spec you are /not allowed/ to also support QC on the type-c port, many overseas manufacturers do not listen to this :)

So if I understand well, accepting QC3 pulses over Type-C port is also an unexpected behaviour of some chargers and that is why QC3 pulses should not happen too early, as it will prevent those chargers which are slow but accepting QC3 over Type-C to negotiate in PD mode.

So if I understand, these would be the following options to allow you to use your QC chargers:

  1. Option to disable PD (QC only mode)
  2. Option to reduce PD negotiation timeout to minimum (2 seconds)
  3. QC2 only mode (Disable voltage hunting)

3) I think that would cause all of my QC3 chargers to stuck at 9V instead of 12 at least now. Although QC2 seems to be able to provide 12V, if I disable these pulses, it will remain on 9V.

2) Right, but my chargers do not work if I put a delay in thread starting of 2 seconds, but they work if I only wait 1 second. So for my chargers, the accepted delay should be somewhere between 1 and 2 seconds.

1) I would not really like to use that option as I prefer to use my Pinecil using a PD-capable adapter as it provides reasonable amount of power at 20V. However, others should be backup adapters like a QC3 car socket adapter or a QC3 charger laying around and I would like to use the maximal available voltage per power supplies. I don't prefer messing around the menu system every time I connect it to another power supplies and it may also happen if I connect it to my PD charger over Type-C or to my power bank through Type-C, they may not fall back to Type-C (as you wrote) and I am not allowed to use full power unless going through menu system to enable PD again.

I would prefer options that don't touch the PD stack as its far more important generally, but providing a default off option to reduce the timeout seems reasonable.

I understand it may be a more complicated solution to modify the PD stack in general as it is a complex solution.

How about doing a similar workaround instead what I did?

As restoreSettings runs prior starting threads, an option set earlier should be available when any threads are started. In POWtask, there is a call for power_check() around every 100ms, that is where it checks for PolicyEngine::setupCompleteOrTimedOut() to be true for calling QC_resync() in case of PD negotiation fails. It also calls PPSTimerCallback()

So from here, it may be a call of something like NegotiationTimeoutReached() with similar content inside like above which basically checks for xTaskGetTickCount() is greater than the value set in configuration (if enabled). I think it may have a range of maybe 1000-5000 milliseconds in 100 ms or 200 ms steps. 100 ms step is available as TICK_100ms which comes from configTICK_RATE_HZ defined as TICK_SECOND, so TICK_SECOND/10 does the same or TICK_SECOND/20 gives 200 ms steps which can be multiplied by the value in settings.

So, NegotiationTimeoutReached() is defined in policy_engine.h, it may have a call (if it is possible here) or pick up a value generated in "NegotiationTimeoutReached()" where it should return true right after pdNegotiationComplete check. However, in pdHasNegotiated() call, it also should have the check for this timeout if pdNegotiationComplete is not true to return false.

So that way a timeout check could be applied in policy_engine while still there is an option to completely turn it off to have the very same behaviour what it has now. I think this would be the most proper solution which does not really modify PD handling (however, policy_engine is part of FUSB302 driver). Basically, I don't think an initial but simple timeout value in that driver is a really bad thing...

Alternatively, if you don't want to touch policy_engine as it is part of the FUSB302 driver, then the same solution may be applied directly in power_check() function by checking if a new (theoretical) timeout function is enabled and check if xTaskGetTickCount() > (TICK_SECOND/10)*value_set is true prior checking FUSB203_present. If timeout has reached, it may call QC_resync() once, the rest can be intact - it may set a variable which is initially zero/false but set on the first call, then never fires up again, so a PD charger which waits longer but don't respond to QC may not affected, especially if it also sets this value when pdHasNegotiated() call has true as result, so it does not fire up when a PD negotiation seems to be happened.

I would prefer adding an additional timeout check in PolicyEngine written above as it could be a natural part of the system. I feel it does not hurt doing that this way and modification of the complex background of PD negotiation can be intact.

What is your opinion?

VaZso commented 3 years ago

Okay, so I have done the modification I suggested above and it works for me so far also with my QC chargers and also with my PD chargers. There is a new setting called "PD timeout" which can be set from 0 to 50 where 0 means disabled which is the default value.

My QC3 power supplies work up to value 15 which means it handles timeout after around 1500 ms. On values 16 and 17, my chargers are sometimes work but otherwise not, so QC3 starts to fail on these units.

Maybe 50 alias 5 seconds is a bit too high value for practical use.

I will try to upload the modified code as a pull request soon for further testing and revision. Currently it compiles for all hardware and languages except for MHP30 which I haven't checked yet.

It was only tested on Pinecil.

VaZso commented 3 years ago

I have opened a pull request here: https://github.com/Ralim/IronOS/pull/994

If everything go right, others may also check if it improves anything using their QC3 power supplies which should also work well using PD power supplies.

...or at least that is what I expect. :)

Ralim commented 3 years ago

Hia,

Thanks for the PR.

I've added comments on the PR and the CI will want some love (same issue is also flagged in comments).

Is it also an out of specs behaviour or simply it was not specified in protocol description?

Grey area, not what I would interpret as in spec, but it's valid by not breaking anything I could see at the time.

Your comment has lost all formatting so it's a little hard to follow, but as you have rolled your idea into the PR going to go with those comments.

VaZso commented 3 years ago

Hi,

Okay, I will modify what you suggested and set the state machine to PESinkSourceUnresponsive instead.

Anyway, I haven't done such this before and I don't know GitHUB that deeply... how could I resolve the issues you wrote? Should I open another pull request or is there an appropriate option for that?

Thanks your thoughts.

Ralim commented 3 years ago

Ah, so for GitHub you have a few options.

The preferred way is you just keep commiting to the branch you used to open the PR and GitHub will pick up any following commits and pull them in too.

Once you have addressed a comment you can click the resolved button on it, or if you change the line the comment was made on it will automatically mark it outdated.

The alternative is to close the PR and then open a new one, but I prefer the former method as it stops spam of PR's and keeps the history.

VaZso commented 3 years ago

Thank you.

I think I have addressed most of what you wrote there, except the timeout handling which I currently don't see how should it handle better and maybe setting the state machine to PESinkSourceUnresponsive is not in the best place.

Please add you comments if you know how to do things better.

For me, it still works as before and now also MHP30 compiles without warning as POW_PD parts are now in place... so automatic build should also happen I think.

VaZso commented 3 years ago

Sorry for delay, @Ralim has just merged this code to the repository, so if someone may has an issue with some QC3 power supplies, it may worth a try to set PD timeout and try if it solves it.

For me, it helped on some Baseus and Ugreen chargers while my PD chargers are still happy.

discip commented 2 years ago

@VaZso So this can be closed than? 😊

VaZso commented 2 years ago

@discip I think yes, but I am afraid I am not allowed to close this issue as I am not the author or owner. :)

I have not tested latest release yet but it still worked using my power supplies a few months ago when I have checked actual latest code.