Nice work!

[septillion-git]

Heyy,

Cool you forked QC2Controll for QC3! Lucky I saw it at Hackaday (since I can't read all articles anymore). I would have liked a message from you and I bed Hugatry as well 😉

But I do have some notes 😁

The abbreviation "PD" is a bit confusing. In combination with USB it's commenly used as USB Power Delivery.

I made the library on the notes of Hugatry. This included the resistors I picked. It's nice to see you found an error in it for QC3 chargers. But I do have to note, the resistors I picked where compatible with both 5V and 3,3V Arduino's. So that might be a good note to mention in the differences between QC2 and QC3. And try to make QC3Controll work with 3,3V Arduino's as well (new values or a 3,3V Arduino schematic).

Inline functions should be in .h

Macros (#define) are a relic from C. C++ has nicer and saver ways of doing it. In this case, static const.

Doubles are overkill, a float will do ;) Makes no difference on ATmega's but on 32-bit is does. Even a float is a bit of a hack because floats != decimal point. But I get why you would like it to make it simple and intuitive.

There was no need to break compatibility for setVoltage() ;) There is something called function overloading 😄

I personally think the voltage ramping is more annoying then useful. If I set a voltage I want to get there as quick as possible. Do the discrete voltages really need the 60ms of waiting?

And although not every charger goes to 20V, isn't it a bit much to make this a setting? 😕

0 is a valid pin on an Arduino. Although on a lot it's a UART pin, it's not very nice to exclude it.

[vdeconinck]

True, I should have pinged you :-( . I guess Github makes it too easy to fork... Sorry for that.
I used PD for "portable devices" because that's what they use in the official Battery Charging docs such as this one. But it's true that it's misleading...
Inlines in .h, noted :-). And yeah, I'm an old relic programmer. static const it will be.
double vs float, agreed. Same thing for Arduino but yeah, float makes more sense.

My first idea was to only use milliVolt versions to keep everything simple and avoid rounding errors (I now take care of them but well), then I thought it would be cool to keep some backwards compatibility as much as possible, but I'm still not satisfied. As you say, setVoltage() is not compatible anymore with QC2 chargers, and that's on purpose because I didn't want "setVoltage(8.8); setVoltage(9.0)" to drop to 5V in between because we would switch from continuous to discrete values...
As for overloading, yeah, I admit that's an interesting option, but having a different behaviour between setVoltage(9) and setVoltage(9.0) would be misleading too I guess...

I admit the 60ms is a rather conservative value. I took it from a charging port controller datasheet - https://www.upi-semi.com/files/1889/1b8dae21-e91a-11e6-97d5-f1910565ec6d - which mentions minimum 20, typical 40 and maximum 60ms for tGLITCH_V_CHANGE. The goal is to make sure it's an actual state change request and not a "glitch" (quick increment/decremetn). It's a pity we don't have the actual QC specs though.... Might be intersting to see how chargers behave with shorter delays...

20V is completely untested for now. A user thought his charger was class B compliant, but it was not the case, so I have no idea if it works. The goal of declaring if we're using a class A or B charger is mostly used to stop incrementing voltage past 12V if using a class A charger. In fact, it does not harm to continue incrementing, but the internal voltage (returned by getVoltage) will become irrelevant.
In fact, I'm also questioning the use of getVoltage as QC chargers give no feedback. For example, one of my QC3 chargers does not go down to 3.6V but stops at 4V, and it can be misleading for newcomers to have a getVoltage() function that reports inaccurate values.
Maybe we should ditch getVoltage() altogether, and then there's no need to differentiate between class A and B

Oh right, pin 0 is valid. I guess it would be wiser to use -1...

I see you have filed other issues, but that will be for later ;-)

[septillion-git]

No worries! I like it's picked up 😉 And all my comments are just my two cents 😄

Yeah, I know why you used PD. But if you don't open that datasheet you might think of USB Power Delivery 😕

And yeah, I started with C as well but I do enjoy the nicer C++ alternatives. Although sometimes they are a bit more complicated but they are so much easier to debug!

About the different behavior, does that really matter? For a normal user it makes no difference if the charger is in discrete or continuous mode. Even from a discrete mode increseVoltage() and decreaseVoltage() can be called. And an advanced user can have a better look at the datasheet. But I think it would be a nice idea to add setVoltage(uint16_t voltage, bool forceContinuouss). That way you can use a int to set it continuous. But setVoltage(uint16_t voltage) can just be an inline call to setVoltage(voltage, false) to stay compatible. But like I said, I don't think it really matters for a normal end user if the charger is set discrete or continuous.

As for t_{GLITCH_V_CHANGE}, the datasheet is a bit vague 😕 Indeed a pity the full specs are not public. But I thought it's only used when going to continues mode.

About the 20V, not going past 12V for a Class A is nice but on the other end you have the same kind of problem for chargers that will not go to 3,6V. Also, for protection it's not great as well. Most voltage regulators on Arduino's should be able to handle 20V as well. But, as I mention in QC2Control, even 12V is to high for some Arduino's 😢. I'm still in doubt what causes it. Maybe just fake IC's 😛 But that's why Hugatry suggested the optional diodes. But I think a note of caution would be better then a "fake" protection in the form of a Class A lock. Aka, nice idea but I don't think it's worth it.

I think it's nice to keep getVoltage(). Yes, it only gives the last voltage that was attempted to set but more libraries do the same. For example, the Servo library. And I think in a lot of applications the user wants to remember the set voltage and I think it's a pity if you need to store it twice (the user and the library). If the library already stores it, just make it available I would say. The behavior is clearly stated in the documentation.

If you want to make an invalid pin -1 (which is indeed a common conversion) you need to make the variable signed ((signed) char of int8_t). That leaves 128 possible pins. I think that would be enough for all Arduino variants but I'm not 100% sure... (Okay, an Arduino with more then 128 looks pretty useless to me as well 😛)

Alright, back to work 👼 Just let me know what you think 👍

[vdeconinck]

This issue looks more and more like a forum thread... but I like that :-)
As for "for a normal user it makes no difference if the charger is in discrete or continuous mode", I thought that too but in fact there is a big difference : if you are at 9V and you want to reach 12V, the behaviour is very different depending on the mode.
If both 9V and 12V were reached in discrete mode, you get a step.
If you request 12V in continuous (that is 15 increments of 200mV), you get a ramp.
But if you request 12V in discrete mode while 9V was reached in continuous, then you get a drop down to 5V before going back up to 12V. That's because the only way to exit continuous mode is through a request of 5V (confirmed by my tests). I admit I don't see the reason why they forced that intermediate 5V, but practically speaking, I guess it means designers did not consider switching back and forth between modes was a normal use.
The logic now is ;

• set5V(), set9V(), set12V() mean discrete
• setVoltage(), setMilliVoltage() means continuous
  Of course, there are many options, but this one is simple at least.

tGLITCH_V_CHANGE, the patent for the initial QuickCharge mentions that the goal is to filter noise and supurious spikes on D+, that's all the information I have...). My understanding is that QC was initally just used to select a charge voltage once and then leave it as is, so 60ms is not a big deal. But continuous mode is precisely used to adjust voltage finely during the charge cycle to compensate for the varying voltage drop in the cable as the charge current changes.
To achieve that goal, they selected as signalling through very quick changes (around 1ms) precisely to be well below the discrete filter threshold and avoid confusing QC2 chargers. At least that's my understanding...

The "powering the Arduino" question was already a challenge with a source between 5 and 12V and the solution you proposed was clever and safe (although, as you say, an official Arduino recommends a range of 7-12V and accepts limits as far as 6-20V) , but of course the issue is worse with a source between 3.6 and 20 The diode trick is nice to decrease by a few volts, but you'd need a bunch of them if you want to bring it under 12V from a source at 20V. On the other hand, if you require less than 5V from the source, the diodes would eventually make the voltage drop below the minimal voltage required for the Arduino. So in the end... I decided to leave it to the user :-).

And yeah, -1 as signed or 0xFF as unsigned, out of the pin range is much better than 0 anyway :-).

I'll do the changes when I have time on friday or something. Thanks for sharing your views :-)

[septillion-git]

Sorry for that! 👼

About the voltage changes, you're right 😊 So I would change it like I said (might do it tonight). Aka, make going back to discrete mode a parameter for setVoltage(byte). An let the default behavior be in line with QC2Control. If you just use the setMilliVoltage() when dealing with QC3, there is no problem 😄

About t_{GLITCH_V_CHANGE}, I wasn't aware that also applied to the discrete mode. The datasheet you mention in the documentation wasn't very clear. But if it needs it, fine 😊

Yeah, the diode solution only works for 5V to 12V range. The 3,6V to 20V range is tricky. Might be best to extend the documentation. 20V is out of spec for the original MIC5205 regulator but I've seen Pro Mini's with regulators who are fine with 20V (78L05 or something). On the other end, 3,6V is out of spec to run a ATmega at 16MHz, especially taking in account the drop out voltage of the regulator. So might leave it to the user to provide external circuitry for that. (7805, buck-converter, buck boost, separate supply etc). Between 4V (minimum to run @16mhz via regulator) and 16V the Arduino should be fine (although some already die at 12V 😒).