Stop a low charge voltage request from raising the charge voltage - #333
Merged
Conversation
Both setChargeVoltage implementations subtracted a bias from the argument before comparing it against their step table. The argument is unsigned, so a request below the lowest step wrapped around and then clamped to the opposite end of the table: on the AXP192 a request for 4.0V configured 4.36V, and on the AXP2101 the same request selected the reserved code 0. Asking for a gentler charge voltage gave a harsher one. The AXP2101 table also carried a 4.6V step that the part does not have - its constant-voltage register goes up to 4.4V - so a request that reached that entry wrapped the index back onto the reserved code as well. Compare in millivolts against the real steps instead, take the highest step that does not exceed the request, and fall back to the lowest step when the request is under all of them. Every request inside the supported range keeps its previous result.
Review pointed out that the AXP2101 constant-voltage register is documented differently across datasheet revisions: the current ones reserve code 0 while an early one gave it to 4.6V. The change had asserted flatly that no step above 4.4V exists, which only holds for the revisions available now. State what every revision agrees on instead, and give the reason for holding a high request at 4.4V: no battery this library runs on charges to 4.6V, so the code whose meaning depends on the silicon is worth avoiding. Both headers now carry the supported steps and what happens on either side of them, which nothing stated before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is wrong
Both
setChargeVoltageimplementations subtracted a bias from the argument before comparing it against their step table:The argument is unsigned, so a request below the lowest step wraps around and is then clamped to the opposite end of the table. Asking for a gentler charge voltage produces a harsher one:
A sketch that sets a lower ceiling for a small cell —
M5.Power.setChargeVoltage(4000)— configures 4.36 V on the AXP192 instead.The AXP2101 table also carried a 4.6 V entry. The current datasheet revisions document the constant-voltage register as 4.0 V through 4.4 V with code 0 reserved, so a request that reached that entry wrapped the index onto the reserved code as well.
What changed
Compare in millivolts against the real steps, take the highest step that does not exceed the request, and fall back to the lowest step when the request is under all of them. Every request inside the supported range keeps its previous result — only the two broken edges move.
Both headers now state the supported steps and what happens on either side of them, which nothing documented before.
Notes
uint16_t, so a negative argument still converts to a large positive one and selects the highest step. That is unchanged by this PR and is better addressed where the setter contract itself is revised.setChargeCurrenthas the same shape of problem (a request below the lowest step is raised to it, and on the AXP2101 the 25/50/75 mA steps are unreachable). Left out of this PR deliberately: it changes whatsetChargeCurrent(0)means, which deserves its own discussion.Verified on hardware
Register read-back after each request, with the original value restored afterwards: