Report battery voltage from a configured AXP2101 - #155
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a new extern global access against the repo’s documented guidance and has a small, concrete maintainability issue (redundant double-scan) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds AXP2101 VBAT acquisition into the existing battery-voltage reporting flow when an AXP2101 is present in the runtime sensor configuration, keeping the existing cache + BLE manufacturer-data encoding behavior and preserving BQ27220/analog-sense fallback behavior.
Changes:
- Introduces
sensor_axp2101.{h,cpp}with pure helpers for VBAT decode, address defaulting, and targeted ADC bit-0 enable/preserve, plus the production I2C read path. - Extends
readBatteryVoltageUncached()to consult the AXP2101 path before falling back to the analog battery-sense pin. - Adds a standalone host test (
tools/test_sensor_axp2101.cpp) and documents it intools/README.md.
File summaries
| File | Description |
|---|---|
| tools/test_sensor_axp2101.cpp | New host-side test for AXP2101 helper semantics (decode, ADC bit preservation, address defaulting, failure handling). |
| tools/README.md | Documents the new standalone host test and how to build it. |
| src/sensor_axp2101.h | New AXP2101 helper/header API, including pure inline helpers for host testing. |
| src/sensor_axp2101.cpp | New production implementation reading VBAT from the configured AXP2101 via OpenDisplay’s Wire init/restore path. |
| src/display_service.cpp | Wires AXP2101 voltage acquisition into the normal battery voltage path ahead of analog-sense fallback. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…3-PhotoPainter)
The normal battery-reading path (readBatteryVoltageUncached) checked the
BQ27220 gauge and then the analog battery-sense pin, but never the
AXP2101. On the Waveshare ESP32-S3-PhotoPainter -- whose official
esp32-s3-wspp preset configures an AXP2101 (sensor type 3) and no analog
battery-sense pin -- it returned -1.0 and updatemsdata() encoded the
voltage as zero in the BLE MSD advertisement.
Add an AXP2101 arm to the same battery path that:
- selects the PMIC from the parsed SensorData (no board or PhotoPainter
flag), respecting the configured bus and sensor-address defaulting
(0 / 0xFF -> 0x34, any other 7-bit address respected verbatim),
- reuses initOrRestoreWireForBus() so bus switching stays centralised,
- confirms battery presence via power-status reg 0x00 bit 3,
- enables only ADC channel bit 0 in reg 0x30, preserving every other
bit, and allows a short settling delay ONLY when the channel just
flipped 0->1,
- decodes VBAT from regs 0x34/0x35 as (hi & 0x3F) << 8 | lo at
1 mV per count, and
- fails soft with -1.0f + od_log_warn on any I2C failure so the known
post-panel-shutdown bus loss stays a separate observation.
The existing BQ27220 precedence, analog-sense fallback, and 30-second
top-level cache are preserved. No heap. No changes to the vendored
protocol header, PMIC rail policy, ALDO3/ALDO4, audio, panel shutdown,
Wire.end, GPIO47/48, ws_pp_init, PhotoPainter detection flags, or the
esp32-s3-wspp preset.
Flagged but deliberately not fixed here: readAXP2101Data() in
display_service.cpp decodes VBAT as a 12-bit ((H<<4)|(L&0x0F)) * 0.5 mV
field, writes 0xFF to reg 0x30 (enabling every ADC channel and stomping
unrelated bits), and reads battery presence at bit 5 and VBUS at bit 3,
which is the reverse of the datasheet. The function has no callers, so
none of that is reachable, and CLAUDE.md asks that pre-existing problems
be flagged rather than fixed as a side effect of unrelated work. Happy to
follow up separately, either against the shared helpers added here or by
removing the function.
Host test tools/test_sensor_axp2101.cpp covers every case listed in the
host-test plan: VBAT decoding with unused bits clear and set, targeted
ADC-enable that preserves every other bit, battery-present set/clear,
address defaulting for 0 and 0xFF, configured-address preservation, no
configured AXP2101, and short/failed I2C reads. Standalone build; not
part of the PlatformIO build. Registered in tools/README.md next to the
other host harnesses.
Verified: host test 49/49 checks pass under -fsanitize=undefined,address;
all 12 environments in .github/firmware-targets.json compile clean; and a
four-state before/after hardware cycle on a PhotoPainter (positive) and a
XIAO ESP32-C3 (negative) shows the PhotoPainter reading ~4.14 V into the
BLE advertisement where mainline encoded zero, with the C3 serial logs
byte-identical between unmodified main and this commit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9ea8bad to
3bda527
Compare
|
Both review comments are addressed in Changes
Re-verification against
The PR description has been updated with the new figures. One correction worth flagging: an earlier revision claimed a 148-byte free-heap difference on the C3. Re-capturing |
Fixes #154
Summary
Add AXP2101 VBAT acquisition to the existing battery-voltage path when the runtime sensor configuration contains
OD_SENSOR_TYPE_AXP2101.This change is limited to voltage acquisition. PMIC rail policy, panel shutdown, audio hardware, shared-I2C shutdown handling, device detection, and website presets remain unchanged.
Intended behavior
Flagged, deliberately not fixed here:
readAXP2101Data()While working in this file we found that the pre-existing
readAXP2101Data()diagnostic reads theAXP2101 using AXP192 register semantics. Every value it decodes is affected:
((H<<4)|(L&0x0F))field with AXP192 step sizes — 0.5 mV forVBAT, 1.7 mV for VBUS, 1.4 mV for SYS. The AXP2101 encodes these as six valid high bits followed
by a full low byte, at 1 mV per count.
0xFFto ADC-control register0x30, enabling every ADC channel and overwriting everyunrelated control bit, where only bit 0 is needed.
is the reverse of the datasheet.
The function has no callers anywhere in the tree — only a declaration in
display_service.handits own definition — so none of this is reachable at runtime today.
The mismatch appears confined to this one function.
initAXP2101()andpowerDownAXP2101()use adifferent register set and are exercised on every boot, and the
0xFFwrites in the power-down pathare legitimate write-one-to-clear on the IRQ status registers, not the same bug.
It is left unchanged here on purpose.
CLAUDE.mdasks that pre-existing problems be flagged ratherthan fixed as a side effect of unrelated work, and nothing in the hardware test cycle below could
exercise a fix to unreachable code. I intend to fix it myself in a follow-up — see Follow-up work
at the end.
Scope of impact
esp32-s3-wsppis the only preset in the opendisplay.org catalog withsensor_type: 3, and it setsbattery_sense_pin: 0xFF. No existing configuration has both an AXP2101 and an analog battery-sense pin, so the new AXP2101-before-analog precedence displaces nothing. That preset also omitsi2c_addr_7bitentirely, which parses as0— the0/0xFF→0x34address defaulting is what makes the fix work on real hardware, not a theoretical convenience.Testing
Host tests
49 checks exercising VBAT decode, ADC bit-0 enable/preserve, address selection, battery-presence flag, and failed read handling. Compiled and run off-target with
-fsanitize=undefined,address. Seetools/test_sensor_axp2101.cpp.The pure helpers are covered directly. The fake-bus scenarios re-implement the production call sequence rather than driving it, so ordering changes inside
axp2101BatteryVoltageVolts()would not fail the test; the Wire I/O and the call ordering are what the hardware cycle covers.CI build matrix
All 12 environments in
.github/firmware-targets.jsoncompile clean at every one of the four teststates, in both the production and diagnostic build profiles. No environment regressed.
Hardware test cycle
Four firmware states were built, flashed and serial-captured on two physical devices — eight
captures, all taken in a single session with an identical observation window (boot through setup
completion, then a fixed 150-second hold), so every comparison is like-for-like:
00-main01-main-diagOD_LOG_DEBUG02-fix-diag03-fix-mainesp32-s3-N16R8)esp32-c3-N4)00-mainvs03-fix-main(PhotoPainter): the complete diff, after timestamp normalization, isthe free-heap line plus the two post-shutdown I²C warnings explained under Known separate issue
below. Boot, configuration load, display refresh, panel force-off and BLE advertising are otherwise
line-for-line identical.
01-main-diagvs02-fix-diag(PhotoPainter):02reads 4137 mV at t=1.983 s and 4138 mV att=36.425 s from the AXP2101, and that voltage reaches the BLE advertisement —
02publishes… 9D 03, i.e. voltage low byte0x9Dwith the status bit-8 flag set:0x019D= 413 × 10 mV =4130 mV, the 10 mV quantization of 4138 mV.
01publishes… 00 02, encoding zero.C3 negative control: free heap is 134 824 B in all four states, and no AXP2101 log line
appears anywhere in any capture — the arm is never entered on a device with no AXP2101 in its
sensor configuration. The only textual differences between the captures are one partially-written
line caught by the serial monitor attaching a few milliseconds earlier in one run, and the
chip-temperature byte in the manufacturer data, which drifts between runs.
Footprint (
esp32-s3-N16R8):00-main03-fix-mainThe 396-byte heap difference is not the new code's footprint — static RAM is byte-identical. It is
resident heap arising from the I²C transactions the read performs during boot. It tracks the fix and
not the diagnostics:
00-mainand01-main-diagboth report 168 168 B,02-fix-diagand03-fix-mainboth report 167 772 B.Patch-ID:
cef298c94ca80e567edb198d702b50ec29c1081b— the fix commit is byte-for-byte identicalwhether applied alone or on top of the diagnostic commit, confirming the two are independent.
Negative-coverage limitation
Negative testing was performed on the ESP32-C3 only. A second, S3-class negative device (TRMNL 7.5-inch OG DIY Kit, XIAO ESP32-S3 Plus) was planned but the unit was not delivered. Given the C3 result above and the preset-catalog scope, the remaining untested surface is a second S3 board with no AXP2101 configured.
Known separate issue: the PMIC is unreachable after panel shutdown
The PhotoPainter logs above show two I²C warnings after the panel powers down. This pull request is
what makes them visible, so they need explaining.
At the end of the boot render the device runs its shutdown path:
powerDownAXP2101()disables theALDO1–4 rails among others and puts the PMIC to sleep, then
pwrmgm()callsWire.end()and drivesGPIO47/GPIO48 — this preset's bus pins — high as push-pull outputs. Every PMIC transaction after
that point fails with
ESP_ERR_INVALID_STATE.The cause is known from separate isolation testing on this hardware, not from this pull request: the
schematic ties
Audio_VCCto ALDO3, and de-energizing it leaves the ES8311/ES7210 I²C pinsunpowered, so their protection diodes clamp the shared SDA/SCL lines and take down every device on
the bus, the PMIC included. The software teardown in
pwrmgm()would be enough on its own as well;both happen here.
Nothing in this change fixes, hides, or works around that. In that window the reader returns
-1.0f, exactly as it does for any other failed transaction, and the voltage this pull request addsis the pre-shutdown reading.
What this costs, and where. The read itself is cheap, and it is what every AXP2101 board pays:
three register reads on the configured bus — power status, ADC control, and the VBAT pair — plus one
register write and a 2 ms settling delay whenever the VBAT ADC channel reads back disabled. On the
PhotoPainter it does read back disabled on both pre-shutdown samples in the capture, so each read
there is three reads, one write and 2 ms. That happens at most once per cache interval.
The 69 ms figure is not that cost. It is a transaction timing out against a bus that has already
been torn down, and it only arises in the window after
pwrmgm(false)has run. That teardown isgated on
sensor_type == OD_SENSOR_TYPE_AXP2101— the same predicate as the new read — so the setof boards that can see it is exactly the set that enters this path, and
esp32-s3-wsppis currentlythe only preset configuring one. In practice that means the PhotoPainter, after its panel shutdown,
and nothing else.
Within that window it is consistent: 69 ms on every attempt in the diagnostic capture (43.710 →
43.779 s, and identically at 53 s, 63 s and 73 s). In production the read is driven by the
60-second manufacturer-data update rather than by the 30-second cache, so the
03-fix-maincaptureshows one stall per minute, at 120 s and 180 s and nowhere else. Mainline never attempted a PMIC
read at all, so it never paid this. It resolves with the underlying problem, which I intend to fix
myself — see Follow-up work below.
Follow-up work
Both problems this pull request flags are mine to fix, not requests for someone else to pick up.
Unless you would rather they were handled differently, I will take them in order once this merges,
each as its own issue followed by its own pull request, using the same pattern as this one — an
isolated commit against a pinned baseline, host tests, the full CI matrix, and a before/after
hardware capture on every available device:
readAXP2101Data()— correct its VBAT decode, targeted ADC enable and status-bit readingagainst the shared helpers added here, or remove the function if you would prefer that. Small,
and it needs a decision from you on which of the two you want.
lifecycle that keeps the powered devices on the shared bus reachable, with the audio parts put
into their documented low-power states rather than having their rail removed. I have the
isolation testing behind it and will bring the evidence with the issue.
Happy to reorder, split, or drop either if that does not match how you would like this area handled.
AI-enabled workflow disclosure: this change was produced with AI assistance under a human-driven, human-in-the-loop process. GitHub Copilot was the harness; planning, implementation, review, and log analysis were carried out across multiple AI harnesses and models, each stage reviewed by a human operator. All hardware observations were produced on physical devices by that operator, and the raw serial captures, build output, and flash logs are retained as evidence.