From 0fd1cd020f6ac4b479efe29fab69132889e7081d Mon Sep 17 00:00:00 2001 From: edennelson Date: Tue, 6 Oct 2026 11:45:10 -0700 Subject: [PATCH] fix(axp2101): make every AXP2101 register access land on the datasheet register Checked every AXP2101 register access in the firmware against the AXP2101 datasheet. Several used addresses or bits the AXP2101 does not have. This corrects them in place and removes the accesses that have nothing to land on. Where ESPHome test-bed work on the PhotoPainter reads the same registers (REG00, REG01, VBAT, VBUS, REG30 bit 2, battery percentage), the datasheet matched the hardware. The other registers follow the same datasheet. powerDownAXP2101() (runs at every panel shutdown): - IRQ status was cleared at 0x44-0x46. The AXP2101's IRQ status registers are 0x48-0x4A. 0x4A is written 0xDF because its bit 5 is reserved. - It also wrote a fourth IRQ-enable register (0x43) and a fourth IRQ-status register (0x47). The AXP2101 has three of each; these appear to be vestigial from older PMIC support. The writes were non-functional and non-erroring: the bus acknowledged them, so nothing reported a failure. Removed, with their constants. - REG26 reserved bit 7 was set. No longer set. - REG91 holds only the DLDO2 enable, but its write was logged as disabling BLDO1-2, CPUSLDO and DLDO1-2. Those other enables are REG90 bits 4-7, which the shutdown preserved. REG90 is now written 0x00 so the stated disables happen, and each log line states what its own write did. readAXP2101Data() (uncalled diagnostic, flagged in #155): - "VBUS" was read from 0x36 (the TS pin); VBUS is 0x38/0x39. - "System" was read from 0x38 (VBUS); VSYS is 0x3A/0x3B. - All three voltages used a 12-bit decode with 0.5/1.7/1.4 mV steps. The AXP2101 is 14-bit, (hi & 0x3F) << 8 | lo, at 1 mV per count. - REG30 was written 0xFF to enable all ADC channels, which also set reserved bits 7:6. Now 0x3F. - Battery presence and VBUS presence read swapped REG00 bits. - "Charging" read REG00 bit 2 (Battery in Active Mode). Charge direction is REG01 bits 6:5. The function remains uncalled. axp2101_adc_enable_bit0() (#155 battery reader): REG30 reserved bits 7:6 are written as 0 instead of echoed back. Two host-test expectations follow. #155's comments describing the diagnostic's now-fixed defects, and calling REG34 bits 7:6 "unused" (they are ch_dbg_en), are corrected. src/main.h: the unused duplicate AXP2101 block, left behind when fd0d73a moved these functions to display_service.cpp, gets the same corrections: VBUS/SYS ADC addresses, PWRON/PWROFF status at 0x20/0x21, IRQ status at 0x48-0x4A, the fourth IRQ registers removed, and the REG91 comment. The REG26 and REG80/90/91 handling follows the datasheet's sleep and wake sequence (Figure 6-2) and is otherwise unchanged. Not changed: rail policy beyond what the code already states (including ALDO3), fail-closed read/modify/write, the hardcoded 0x34 address and ambient Wire use, and when power state is committed. Co-Authored-By: Claude Opus 5.5 --- src/display_service.cpp | 68 +++++++++++------------------------ src/main.h | 22 ++++++------ src/sensor_axp2101.h | 23 ++++++------ tools/test_sensor_axp2101.cpp | 27 +++++++------- 4 files changed, 57 insertions(+), 83 deletions(-) diff --git a/src/display_service.cpp b/src/display_service.cpp index 734b88e..b9910a8 100644 --- a/src/display_service.cpp +++ b/src/display_service.cpp @@ -743,18 +743,16 @@ void cleanupPartialWriteOnDisconnect(void) { #define AXP2101_REG_POWER_WAKEUP_CTL 0x26 #define AXP2101_REG_ADC_CHANNEL_CTRL 0x30 #define AXP2101_REG_ADC_DATA_BAT_VOL_H 0x34 -#define AXP2101_REG_ADC_DATA_VBUS_VOL_H 0x36 -#define AXP2101_REG_ADC_DATA_SYS_VOL_H 0x38 +#define AXP2101_REG_ADC_DATA_VBUS_VOL_H 0x38 +#define AXP2101_REG_ADC_DATA_SYS_VOL_H 0x3A #define AXP2101_REG_BAT_PERCENT_DATA 0xA4 #define AXP2101_REG_PWRON_STATUS 0x20 #define AXP2101_REG_IRQ_ENABLE1 0x40 #define AXP2101_REG_IRQ_ENABLE2 0x41 #define AXP2101_REG_IRQ_ENABLE3 0x42 -#define AXP2101_REG_IRQ_ENABLE4 0x43 -#define AXP2101_REG_IRQ_STATUS1 0x44 -#define AXP2101_REG_IRQ_STATUS2 0x45 -#define AXP2101_REG_IRQ_STATUS3 0x46 -#define AXP2101_REG_IRQ_STATUS4 0x47 +#define AXP2101_REG_IRQ_STATUS1 0x48 +#define AXP2101_REG_IRQ_STATUS2 0x49 +#define AXP2101_REG_IRQ_STATUS3 0x4A #define AXP2101_REG_LDO_ONOFF_CTRL1 0x91 #define FONT_BASE_WIDTH 8 #define FONT_BASE_HEIGHT 8 @@ -1355,7 +1353,7 @@ void readAXP2101Data(){ } Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); Wire.write(AXP2101_REG_ADC_CHANNEL_CTRL); - Wire.write(0xFF); + Wire.write(0x3F); // all ADC channels; bits 7:6 are reserved error = Wire.endTransmission(); delay(10); Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); @@ -1368,9 +1366,9 @@ void readAXP2101Data(){ uint8_t status2 = Wire.read(); od_log_debug("Power Status 1: 0x%02X", status1); od_log_debug("Power Status 2: 0x%02X", status2); - bool batteryPresent = (status1 & 0x20) != 0; - bool charging = (status1 & 0x04) != 0; - bool vbusPresent = (status1 & 0x08) != 0; + bool batteryPresent = (status1 & 0x08) != 0; + bool charging = ((status2 >> 5) & 0x03) == 0x01; + bool vbusPresent = (status1 & 0x20) != 0; od_log_debug("Battery Present: %s", batteryPresent ? "Yes" : "No"); od_log_debug("Charging: %s", charging ? "Yes" : "No"); od_log_debug("VBUS Present: %s", vbusPresent ? "Yes" : "No"); @@ -1394,8 +1392,8 @@ void readAXP2101Data(){ if(Wire.available() >= 2){ uint8_t batVolH = Wire.read(); uint8_t batVolL = Wire.read(); - uint16_t batVolRaw = ((uint16_t)batVolH << 4) | (batVolL & 0x0F); - float batVoltage = batVolRaw * 0.5; + uint16_t batVolRaw = ((uint16_t)(batVolH & 0x3F) << 8) | batVolL; + float batVoltage = batVolRaw; od_log_debug("Battery Voltage: %.1f mV (%.2f V)", batVoltage, batVoltage / 1000.0); } } @@ -1407,8 +1405,8 @@ void readAXP2101Data(){ if(Wire.available() >= 2){ uint8_t vbusVolH = Wire.read(); uint8_t vbusVolL = Wire.read(); - uint16_t vbusVolRaw = ((uint16_t)vbusVolH << 4) | (vbusVolL & 0x0F); - float vbusVoltage = vbusVolRaw * 1.7; + uint16_t vbusVolRaw = ((uint16_t)(vbusVolH & 0x3F) << 8) | vbusVolL; + float vbusVoltage = vbusVolRaw; od_log_debug("VBUS Voltage: %.1f mV (%.2f V)", vbusVoltage, vbusVoltage / 1000.0); } } @@ -1420,8 +1418,8 @@ void readAXP2101Data(){ if(Wire.available() >= 2){ uint8_t sysVolH = Wire.read(); uint8_t sysVolL = Wire.read(); - uint16_t sysVolRaw = ((uint16_t)sysVolH << 4) | (sysVolL & 0x0F); - float sysVoltage = sysVolRaw * 1.4; + uint16_t sysVolRaw = ((uint16_t)(sysVolH & 0x3F) << 8) | sysVolL; + float sysVoltage = sysVolRaw; od_log_debug("System Voltage: %.1f mV (%.2f V)", sysVoltage, sysVoltage / 1000.0); } } @@ -1495,12 +1493,6 @@ void powerDownAXP2101(){ Wire.write(0x00); error = Wire.endTransmission(); } - if(error == 0){ - Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); - Wire.write(AXP2101_REG_IRQ_ENABLE4); - Wire.write(0x00); - error = Wire.endTransmission(); - } if(error == 0){ Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); Wire.write(AXP2101_REG_IRQ_STATUS1); @@ -1516,13 +1508,7 @@ void powerDownAXP2101(){ if(error == 0){ Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); Wire.write(AXP2101_REG_IRQ_STATUS3); - Wire.write(0xFF); - error = Wire.endTransmission(); - } - if(error == 0){ - Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); - Wire.write(AXP2101_REG_IRQ_STATUS4); - Wire.write(0xFF); + Wire.write(0xDF); // bit 5 is reserved error = Wire.endTransmission(); if(error == 0){ od_log_debug("All IRQs disabled and status cleared"); @@ -1553,29 +1539,18 @@ void powerDownAXP2101(){ Wire.write(0x00); error = Wire.endTransmission(); if(error == 0){ - od_log_debug("BLDO1-2, CPUSLDO, DLDO1-2 disabled"); + od_log_debug("DLDO2 disabled"); } else { - od_log_error("ERROR: Failed to disable BLDO/CPUSLDO/DLDO rails"); + od_log_error("ERROR: Failed to disable DLDO2"); } Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); Wire.write(AXP2101_REG_LDO_ONOFF_CTRL0); - error = Wire.endTransmission(); - uint8_t aldoEnable = 0x00; - if(error == 0){ - Wire.requestFrom(AXP2101_SLAVE_ADDRESS, (uint8_t)1); - if(Wire.available()){ - aldoEnable = Wire.read(); - } - } - aldoEnable &= ~0x0F; - Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); - Wire.write(AXP2101_REG_LDO_ONOFF_CTRL0); - Wire.write(aldoEnable); + Wire.write(0x00); error = Wire.endTransmission(); if(error == 0){ - od_log_debug("ALDO1-4 disabled"); + od_log_debug("ALDO1-4, BLDO1-2, CPUSLDO, DLDO1 disabled"); } else { - od_log_error("ERROR: Failed to disable ALDO rails"); + od_log_error("ERROR: Failed to disable ALDO/BLDO/CPUSLDO/DLDO1 rails"); } Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); Wire.write(AXP2101_REG_POWER_WAKEUP_CTL); @@ -1596,7 +1571,6 @@ void powerDownAXP2101(){ if(!(wakeupCtrl & 0x10)) { wakeupCtrl |= 0x10; } - wakeupCtrl |= 0x80; Wire.beginTransmission(AXP2101_SLAVE_ADDRESS); Wire.write(AXP2101_REG_POWER_WAKEUP_CTL); Wire.write(wakeupCtrl); diff --git a/src/main.h b/src/main.h index 430b3c1..2c3342e 100644 --- a/src/main.h +++ b/src/main.h @@ -312,8 +312,8 @@ static constexpr uint32_t DEFAULT_IDLE_HOLD_MS = 10000; #define AXP2101_SLAVE_ADDRESS 0x34 #define AXP2101_REG_POWER_STATUS 0x00 -#define AXP2101_REG_POWER_ON_STATUS 0x01 -#define AXP2101_REG_POWER_OFF_STATUS 0x02 +#define AXP2101_REG_POWER_ON_STATUS 0x20 +#define AXP2101_REG_POWER_OFF_STATUS 0x21 #define AXP2101_REG_DC_ONOFF_DVM_CTRL 0x80 #define AXP2101_REG_LDO_ONOFF_CTRL0 0x90 #define AXP2101_REG_DC_VOL0_CTRL 0x82 // DCDC1 voltage @@ -323,22 +323,20 @@ static constexpr uint32_t DEFAULT_IDLE_HOLD_MS = 10000; #define AXP2101_REG_ADC_CHANNEL_CTRL 0x30 #define AXP2101_REG_ADC_DATA_BAT_VOL_H 0x34 #define AXP2101_REG_ADC_DATA_BAT_VOL_L 0x35 -#define AXP2101_REG_ADC_DATA_VBUS_VOL_H 0x36 -#define AXP2101_REG_ADC_DATA_VBUS_VOL_L 0x37 -#define AXP2101_REG_ADC_DATA_SYS_VOL_H 0x38 -#define AXP2101_REG_ADC_DATA_SYS_VOL_L 0x39 +#define AXP2101_REG_ADC_DATA_VBUS_VOL_H 0x38 +#define AXP2101_REG_ADC_DATA_VBUS_VOL_L 0x39 +#define AXP2101_REG_ADC_DATA_SYS_VOL_H 0x3A +#define AXP2101_REG_ADC_DATA_SYS_VOL_L 0x3B #define AXP2101_REG_BAT_PERCENT_DATA 0xA4 #define AXP2101_REG_PWRON_STATUS 0x20 #define AXP2101_REG_BAT_DETECTION_CTRL 0x68 // Battery detection control #define AXP2101_REG_IRQ_ENABLE1 0x40 // IRQ enable register 1 #define AXP2101_REG_IRQ_ENABLE2 0x41 // IRQ enable register 2 #define AXP2101_REG_IRQ_ENABLE3 0x42 // IRQ enable register 3 -#define AXP2101_REG_IRQ_ENABLE4 0x43 // IRQ enable register 4 -#define AXP2101_REG_IRQ_STATUS1 0x44 // IRQ status register 1 -#define AXP2101_REG_IRQ_STATUS2 0x45 // IRQ status register 2 -#define AXP2101_REG_IRQ_STATUS3 0x46 // IRQ status register 3 -#define AXP2101_REG_IRQ_STATUS4 0x47 // IRQ status register 4 -#define AXP2101_REG_LDO_ONOFF_CTRL1 0x91 // LDO control register 1 (BLDO1-2, CPUSLDO, DLDO1-2) +#define AXP2101_REG_IRQ_STATUS1 0x48 // IRQ status register 1 +#define AXP2101_REG_IRQ_STATUS2 0x49 // IRQ status register 2 +#define AXP2101_REG_IRQ_STATUS3 0x4A // IRQ status register 3 +#define AXP2101_REG_LDO_ONOFF_CTRL1 0x91 // LDO control register 1 (DLDO2) // The deferred-work flags that used to be defined here are now file-static in // main.cpp: nothing outside it reads them, and the two that other translation diff --git a/src/sensor_axp2101.h b/src/sensor_axp2101.h index ab86e59..ff34b44 100644 --- a/src/sensor_axp2101.h +++ b/src/sensor_axp2101.h @@ -28,15 +28,14 @@ #define AXP2101_REG_VBAT_H_ADDR 0x34u // VBAT high byte; read as 2-byte block with 0x35 // Datasheet reg 0x00: bit 3 = battery presence, bit 5 = VBUS presence. -// The uncalled readAXP2101Data() diagnostic in display_service.cpp reads these -// the other way round (0x20 for battery, 0x08 for VBUS). That is a pre-existing -// defect in unreachable code, flagged in the pull request and left unchanged. #define AXP2101_POWER_STATUS_BATT_PRESENT_BIT (1u << 3) #define AXP2101_POWER_STATUS_VBUS_PRESENT_BIT (1u << 5) -// Datasheet reg 0x30: bit 0 enables the VBAT-voltage ADC channel. Every other -// bit is a different channel or an unrelated ADC control and must be preserved. +// Datasheet reg 0x30: bit 0 enables the VBAT-voltage ADC channel. Bits 5:1 are +// the other channel enables and must be preserved. Bits 7:6 are reserved (R0, +// default 0) and are written as 0. #define AXP2101_ADC_ENABLE_VBAT_BIT (1u << 0) +#define AXP2101_ADC_CTRL_RESERVED_MASK 0xC0u // Post-enable settling window before the first VBAT read is meaningful. Only // applied when the caller has just flipped bit 0 from 0 to 1. @@ -63,8 +62,9 @@ static inline bool axp2101_vbus_present(uint8_t status_reg) { return (status_reg & AXP2101_POWER_STATUS_VBUS_PRESENT_BIT) != 0u; } -// Returns the value to write back to reg 0x30 so bit 0 is set, with every other -// bit preserved. `out_channel_changed` receives `true` iff the caller has just +// Returns the value to write back to reg 0x30 so bit 0 is set, with the other +// channel enables preserved and the reserved bits 7:6 written as 0. +// `out_channel_changed` receives `true` iff the caller has just // flipped the channel from disabled to enabled -- the only case where the // caller must allow AXP2101_ADC_SETTLING_MS before the first VBAT sample. static inline uint8_t axp2101_adc_enable_bit0(uint8_t current, bool* out_channel_changed) { @@ -72,12 +72,13 @@ static inline uint8_t axp2101_adc_enable_bit0(uint8_t current, bool* out_channel if (out_channel_changed != NULL) { *out_channel_changed = was_off; } - return (uint8_t)(current | AXP2101_ADC_ENABLE_VBAT_BIT); + return (uint8_t)((current & (uint8_t)~AXP2101_ADC_CTRL_RESERVED_MASK) | + AXP2101_ADC_ENABLE_VBAT_BIT); } -// VBAT decoding. Reg 0x34 holds the six valid high bits (bits 7:6 are unused -// and must be masked), reg 0x35 holds the full low byte. Result is millivolts -// at 1 mV per count. +// VBAT decoding. Reg 0x34 bits 5:0 hold the high bits of the reading (bits 7:6 +// are the RW ch_dbg_en[1:0] debug field and must be masked), reg 0x35 holds the +// full low byte. Result is millivolts at 1 mV per count. static inline uint16_t axp2101_decode_vbat_mv(uint8_t hi, uint8_t lo) { const uint16_t high_bits = (uint16_t)(hi & 0x3Fu); return (uint16_t)((high_bits << 8) | (uint16_t)lo); diff --git a/tools/test_sensor_axp2101.cpp b/tools/test_sensor_axp2101.cpp index 50207ab..9d4721d 100644 --- a/tools/test_sensor_axp2101.cpp +++ b/tools/test_sensor_axp2101.cpp @@ -8,8 +8,8 @@ // Like tools/test_link_owner.cpp, this is as much a written-down statement of // the intended semantics as it is a test. It covers: // -// - VBAT decoding with upper unused bits clear and set -// - ADC bit-0 enable while preserving every other bit +// - VBAT decoding with reg 0x34 bits 7:6 (ch_dbg_en) clear and set +// - ADC bit-0 enable preserving the other channel enables, reserved 7:6 as 0 // - Battery-present status set and clear // - Default address selection for zero and 0xFF // - Configured address preservation @@ -87,7 +87,7 @@ static int fake_read_vbat_mv(FakeBus& bus, uint8_t addr) { } int main(void) { - // ---- VBAT decoding with upper unused bits clear ------------------------- + // ---- VBAT decoding with reg 0x34 bits 7:6 clear -------------------------- { // 0x0F B8 -- the ESPHome-observed sample -- must decode to 4024 mV. check(axp2101_decode_vbat_mv(0x0F, 0xB8) == 4024, "vbat decode 0x0FB8 == 4024 mV"); @@ -97,19 +97,19 @@ int main(void) { check(axp2101_decode_vbat_mv(0x3F, 0xFF) == 16383, "vbat decode 0x3FFF == 16383 mV"); } - // ---- VBAT decoding with upper unused bits SET --------------------------- + // ---- VBAT decoding with reg 0x34 bits 7:6 SET ---------------------------- { - // Bits 7:6 of reg 0x34 are unused; the decoder must mask them off so - // spurious garbage in that field cannot inflate the reading. + // Bits 7:6 of reg 0x34 are the ch_dbg_en debug field, not part of the + // reading; the decoder must mask them off so they cannot inflate it. check(axp2101_decode_vbat_mv(0xC0, 0x00) == 0, - "vbat unused bits 0xC0 masked -> 0 mV"); + "vbat ch_dbg_en bits 0xC0 masked -> 0 mV"); check(axp2101_decode_vbat_mv(0xCF, 0xB8) == 4024, - "vbat 0xCFB8 (unused bits set) decodes same as 0x0FB8"); + "vbat 0xCFB8 (ch_dbg_en set) decodes same as 0x0FB8"); check(axp2101_decode_vbat_mv(0xFF, 0xFF) == 16383, "vbat 0xFFFF decodes as 0x3FFF -> 16383 mV"); } - // ---- ADC bit-0 enable while preserving every other bit ------------------ + // ---- ADC bit-0 enable, preserving the other channel enables --------------- { // From all-zero: bit 0 flips 0->1, no other bit changes. bool changed = false; @@ -117,10 +117,11 @@ int main(void) { check(next == 0x01, "adc enable from 0x00 -> 0x01"); check(changed, "adc enable from 0x00 flags channel change"); - // From every other bit set: bit 0 flips 0->1, all other bits preserved. + // From every other bit set: bit 0 flips 0->1, bits 5..1 preserved, + // reserved bits 7:6 written as 0. changed = false; next = axp2101_adc_enable_bit0(0xFE, &changed); - check(next == 0xFF, "adc enable from 0xFE preserves 7..1, sets bit 0 -> 0xFF"); + check(next == 0x3F, "adc enable from 0xFE preserves 5..1, clears 7:6, sets bit 0 -> 0x3F"); check(changed, "adc enable from 0xFE flags channel change"); // Already enabled (0x01): value unchanged, no channel-change flag. @@ -129,10 +130,10 @@ int main(void) { check(next == 0x01, "adc enable from 0x01 stays 0x01"); check(!changed, "adc enable from 0x01 does not flag channel change"); - // Already enabled with siblings set: value unchanged, no flag. + // Already enabled with siblings set: no flag; reserved bit 7 written as 0. changed = true; next = axp2101_adc_enable_bit0(0xA5, &changed); - check(next == 0xA5, "adc enable from 0xA5 stays 0xA5 (bit 0 already set)"); + check(next == 0x25, "adc enable from 0xA5 keeps 5..0, clears reserved 7 -> 0x25"); check(!changed, "adc enable from 0xA5 does not flag channel change"); // NULL out-pointer must be tolerated.