diff --git a/src/Power.cpp b/src/Power.cpp index cd9bfa7ea4..f230318f67 100644 --- a/src/Power.cpp +++ b/src/Power.cpp @@ -784,7 +784,7 @@ class ADS1115BatteryLevel : public AnalogBatteryLevel sum += _ads.computeVolts(raw); } // Piggyback a toggle-engine watchdog on this same throttle interval. - // Only re-arm when VBUS is absent — calling this while attached + // Only re-arm when VBUS is absent - calling this while attached // would restart CC toggling and could glitch an active sink attach. if (_aw35615.isReady() && !_aw35615.isVbusPresent()) { _aw35615.rearmToggle(); @@ -812,7 +812,7 @@ class ADS1115BatteryLevel : public AnalogBatteryLevel bool vbus = _aw35615.isVbusPresent(); if (!vbus) { - // VBUS just went away (or has been away) — make sure the CC + // VBUS just went away (or has been away) - make sure the CC // toggle engine is re-armed so the next attach gets detected. _aw35615.rearmToggle(); } @@ -830,7 +830,7 @@ class ADS1115BatteryLevel : public AnalogBatteryLevel if (_aw35615.isReady()) { concurrency::LockGuard guard(spiLock); // Charging == VBUS present AND we're attached as a sink. - // (isSinkAttached() is a latched result — safe to trust here since + // (isSinkAttached() is a latched result - safe to trust here since // isVbusIn() above keeps re-arming toggle on every detach.) return _aw35615.isVbusPresent() && _aw35615.isSinkAttached(); } @@ -1097,6 +1097,20 @@ void Power::shutdown() #endif } +// Consecutive readings only: a battery-less board's floating divider drifts in and out of the +// "battery present" window, and a count that survived the gaps would deep-sleep a USB-powered node. +bool updateLowVoltageCounter(uint8_t &counter, bool hasBattery, bool hasUsb, uint16_t battMv, uint16_t cutoffMv) +{ + if (!hasBattery || hasUsb || battMv >= cutoffMv) { + counter = 0; + return false; + } + + if (counter < UINT8_MAX) + counter++; + return counter > LOW_VOLTAGE_READINGS_BEFORE_SHUTDOWN; +} + /// Reads power status to powerStatus singleton. // // TODO(girts): move this and other axp stuff to power.h/power.cpp. @@ -1229,16 +1243,16 @@ void Power::readPowerStatus() // is 2.0 to 2.5V, current OCV min is set to 3100 that is large enough. // - if (batteryLevel && powerStatus2.getHasBattery() && !powerStatus2.getHasUSB()) { - if (batteryLevel->getBattVoltage() < OCV[NUM_OCV_POINTS - 1]) { - low_voltage_counter++; - LOG_DEBUG("Low voltage counter: %d/10", low_voltage_counter); - if (low_voltage_counter > 10) { - LOG_INFO("Low voltage detected, trigger deep sleep"); - powerFSM.trigger(EVENT_LOW_BATTERY); - } - } else { - low_voltage_counter = 0; + if (batteryLevel) { + // getBattVoltage() reports pack voltage; the OCV table is per cell. + const bool shutdownNow = + updateLowVoltageCounter(low_voltage_counter, powerStatus2.getHasBattery(), powerStatus2.getHasUSB(), + batteryLevel->getBattVoltage(), OCV[NUM_OCV_POINTS - 1] * NUM_CELLS); + if (low_voltage_counter) + LOG_DEBUG("Low voltage counter: %d/%d", low_voltage_counter, LOW_VOLTAGE_READINGS_BEFORE_SHUTDOWN); + if (shutdownNow) { + LOG_INFO("Low voltage detected, trigger deep sleep"); + powerFSM.trigger(EVENT_LOW_BATTERY); } } } diff --git a/src/Power.h b/src/Power.h index f21511ec08..6a7bd21643 100644 --- a/src/Power.h +++ b/src/Power.h @@ -30,6 +30,12 @@ #define NUM_CELLS 1 #endif +/// Consecutive below-cutoff readings needed before the low-battery deep sleep fires. +static constexpr uint8_t LOW_VOLTAGE_READINGS_BEFORE_SHUTDOWN = 10; + +/// Advance the low-battery shutdown counter by one reading; true once the device should deep sleep. +bool updateLowVoltageCounter(uint8_t &counter, bool hasBattery, bool hasUsb, uint16_t battMv, uint16_t cutoffMv); + #if HAS_TELEMETRY && !MESHTASTIC_EXCLUDE_ENVIRONMENTAL_SENSOR #include "modules/Telemetry/Sensor/nullSensor.h" #if __has_include() @@ -98,7 +104,7 @@ class Power : public concurrency::OSThread virtual int32_t runOnce() override; void setStatusHandler(meshtastic::PowerStatus *handler) { statusHandler = handler; } const uint16_t OCV[11] = {OCV_ARRAY}; - bool isLowBattery() { return low_voltage_counter >= 10; }; + bool isLowBattery() { return low_voltage_counter >= LOW_VOLTAGE_READINGS_BEFORE_SHUTDOWN; }; #ifdef ARCH_ESP32 int beforeLightSleep(void *unused); diff --git a/src/platform/esp32/main-esp32.cpp b/src/platform/esp32/main-esp32.cpp index f7879a333f..74b8e0865f 100644 --- a/src/platform/esp32/main-esp32.cpp +++ b/src/platform/esp32/main-esp32.cpp @@ -345,16 +345,24 @@ void cpuDeepSleep(uint32_t msecToWake) #endif 34, 35, 37}; - for (int i = 0; i < sizeof(rtcGpios); i++) - rtc_gpio_isolate((gpio_num_t)rtcGpios[i]); +#ifdef BUTTON_PIN + const int wakeButton = config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN; +#else + const int wakeButton = -1; +#endif + // Isolating a pad holds it, and ext1 cannot re-arm a held pad - skip the pin we wake on. + for (int i = 0; i < sizeof(rtcGpios); i++) { + if (rtcGpios[i] != wakeButton) + rtc_gpio_isolate((gpio_num_t)rtcGpios[i]); + } #endif - // FIXME, disable internal rtc pullups/pulldowns on the non isolated pins. for inputs that we aren't using - // to detect wake and in normal operation the external part drives them hard. + // FIXME, disable internal rtc pullups/pulldowns on the non isolated pins. for inputs that we aren't using + // to detect wake and in normal operation the external part drives them hard. #ifdef BUTTON_PIN - // Only GPIOs which are have RTC functionality can be used in this bit map: 0,2,4,12-15,25-27,32-39. + // Only GPIOs which are have RTC functionality can be used in this bit map: 0,2,4,12-15,25-27,32-39. #if SOC_RTCIO_HOLD_SUPPORTED && SOC_PM_SUPPORT_EXT_WAKEUP - uint64_t gpioMask = (1ULL << (config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN)); + uint64_t gpioMask = (1ULL << wakeButton); #endif #ifdef ALT_BUTTON_WAKE gpioMask |= (1ULL << BUTTON_PIN_ALT); diff --git a/src/sleep.cpp b/src/sleep.cpp index 2b6f58843c..5b224bae9e 100644 --- a/src/sleep.cpp +++ b/src/sleep.cpp @@ -317,7 +317,9 @@ void doDeepSleep(uint32_t msecToWake, bool skipPreflight = false, bool skipSaveN #else pinMode(BUTTON_PIN, INPUT); #endif - gpio_hold_en((gpio_num_t)BUTTON_PIN); + // A held pad ignores ext1_wakeup_prepare()'s re-route to RTC, so never hold the pin we wake on. + if (config.device.button_gpio && config.device.button_gpio != BUTTON_PIN) + gpio_hold_en((gpio_num_t)BUTTON_PIN); } #endif #ifdef SENSECAP_INDICATOR diff --git a/test/test_low_battery_shutdown/test_main.cpp b/test/test_low_battery_shutdown/test_main.cpp new file mode 100644 index 0000000000..24ff195ffe --- /dev/null +++ b/test/test_low_battery_shutdown/test_main.cpp @@ -0,0 +1,146 @@ +// Unit tests for updateLowVoltageCounter() in src/Power.cpp - the gate on the low-battery deep sleep. +// +// Power::readPowerStatus() calls this once per Power thread cycle (20s) with the freshly probed +// battery state. When it returns true the device takes EVENT_LOW_BATTERY -> stateLowBattSDS -> +// doDeepSleep(config.power.sds_secs), and sds_secs defaults to UINT32_MAX, so a false positive parks +// a node for the ~24.8-day clamp with only RST or a power cycle to recover it. That asymmetry is why +// the counter has to be conservative: a missed shutdown costs a flat battery, a spurious one costs +// the whole node. +// +// The contract is that only *consecutive* confirmed-low readings count. The regression guarded is +// the original shape, where the reset lived inside the "battery present and not on USB" guard rather +// than beside it. A board with no battery reads a floating divider that drifts across the 2600mV +// battery-present threshold, so each excursion into the window bumped the counter and nothing outside +// the window ever cleared it; eleven such flickers, spread over any span at all, deep-slept a healthy +// USB-powered node. Reported for Heltec V4 on USB with no battery in meshtastic/firmware#11796. +// +// Also pins the cutoff as a pack voltage. readPowerStatus() passes OCV[NUM_OCV_POINTS-1] * NUM_CELLS; +// it previously passed the bare single-cell OCV point, which no multi-cell pack can fall below, so +// those boards would have discharged to destruction instead of shutting down. +#include "Arduino.h" +#include "TestUtil.h" +#include +#include + +// Declared here rather than via Power.h, which pulls in the ADC and telemetry sensor headers. +// A signature change breaks the link rather than silently diverging from the definition. +bool updateLowVoltageCounter(uint8_t &counter, bool hasBattery, bool hasUsb, uint16_t battMv, uint16_t cutoffMv); + +// LOW_VOLTAGE_READINGS_BEFORE_SHUTDOWN, spelled out so a change to it has to be a deliberate edit here. +static constexpr uint8_t kReadingsBeforeShutdown = 10; + +// The default LiIon curve's lowest OCV point, and a single-cell pack comfortably below it. +static constexpr uint16_t kCutoffMv = 3100; +static constexpr uint16_t kLowMv = 2900; +static constexpr uint16_t kHealthyMv = 3900; + +// One reading of a battery-backed node running off its battery - the only case that may ever count. +static bool lowReading(uint8_t &counter, uint16_t battMv = kLowMv, uint16_t cutoffMv = kCutoffMv) +{ + return updateLowVoltageCounter(counter, true, false, battMv, cutoffMv); +} + +void setUp(void) {} +void tearDown(void) {} + +void test_an_unbroken_run_of_low_readings_shuts_down(void) +{ + uint8_t counter = 0; + + for (uint8_t i = 0; i < kReadingsBeforeShutdown; i++) + TEST_ASSERT_FALSE_MESSAGE(lowReading(counter), "must not fire before the full run is seen"); + + TEST_ASSERT_TRUE(lowReading(counter)); +} + +void test_a_healthy_reading_clears_the_run(void) +{ + uint8_t counter = 0; + for (uint8_t i = 0; i < kReadingsBeforeShutdown; i++) + lowReading(counter); + + TEST_ASSERT_FALSE(lowReading(counter, kHealthyMv)); + TEST_ASSERT_EQUAL_UINT8(0, counter); + TEST_ASSERT_FALSE_MESSAGE(lowReading(counter), "the run restarts from zero, it does not resume"); +} + +// #11796: the battery-less board. Its floating divider reads "no battery" as often as it reads a +// phantom one, and it is never on a detectable USB rail, so the gaps are the only thing that can +// save it. Interleaving them must hold the counter at zero however long this runs. +void test_no_battery_reading_clears_the_run(void) +{ + uint8_t counter = 0; + + for (int cycle = 0; cycle < 50; cycle++) { + TEST_ASSERT_FALSE(lowReading(counter)); + TEST_ASSERT_FALSE(updateLowVoltageCounter(counter, false, false, kLowMv, kCutoffMv)); + TEST_ASSERT_EQUAL_UINT8_MESSAGE(0, counter, "a reading with no battery resets, it does not skip"); + } +} + +void test_usb_power_clears_the_run(void) +{ + uint8_t counter = 0; + for (uint8_t i = 0; i < kReadingsBeforeShutdown; i++) + lowReading(counter); + + TEST_ASSERT_FALSE(updateLowVoltageCounter(counter, true, true, kLowMv, kCutoffMv)); + TEST_ASSERT_EQUAL_UINT8(0, counter); +} + +// A pack sitting exactly on the cutoff is not below it; the OCV table's last point is a valid voltage. +void test_the_cutoff_is_exclusive(void) +{ + uint8_t counter = 0; + TEST_ASSERT_FALSE(lowReading(counter, kCutoffMv)); + TEST_ASSERT_EQUAL_UINT8(0, counter); + + TEST_ASSERT_FALSE(lowReading(counter, kCutoffMv - 1)); + TEST_ASSERT_EQUAL_UINT8(1, counter); +} + +// The caller scales by NUM_CELLS. Against the bare single-cell point a 2S pack never reads low at all. +void test_the_cutoff_is_a_pack_voltage(void) +{ + constexpr uint16_t twoCellCutoffMv = kCutoffMv * 2; + constexpr uint16_t flatTwoCellPackMv = 6000; + + uint8_t counter = 0; + for (uint8_t i = 0; i <= kReadingsBeforeShutdown; i++) + TEST_ASSERT_EQUAL(i == kReadingsBeforeShutdown, lowReading(counter, flatTwoCellPackMv, twoCellCutoffMv)); + + counter = 0; + for (uint8_t i = 0; i <= kReadingsBeforeShutdown; i++) + TEST_ASSERT_FALSE_MESSAGE(lowReading(counter, flatTwoCellPackMv, kCutoffMv), + "unscaled cutoff: the regression that never shuts a 2S pack down"); +} + +// The counter is a uint8_t and the caller keeps calling after it fires, so it must saturate. Were it +// to wrap, the node would come back up, count to 255 again and re-sleep in an unattended loop. +void test_the_counter_saturates_rather_than_wrapping(void) +{ + uint8_t counter = 0; + + for (int i = 0; i < 400; i++) { + const bool shutdown = lowReading(counter); + if (i >= kReadingsBeforeShutdown) + TEST_ASSERT_TRUE_MESSAGE(shutdown, "once tripped it stays tripped until a reading clears it"); + } + TEST_ASSERT_EQUAL_UINT8(UINT8_MAX, counter); +} + +void setup() +{ + initializeTestEnvironment(); + UNITY_BEGIN(); + RUN_TEST(test_an_unbroken_run_of_low_readings_shuts_down); + RUN_TEST(test_a_healthy_reading_clears_the_run); + RUN_TEST(test_no_battery_reading_clears_the_run); + RUN_TEST(test_usb_power_clears_the_run); + RUN_TEST(test_the_cutoff_is_exclusive); + RUN_TEST(test_the_cutoff_is_a_pack_voltage); + RUN_TEST(test_the_counter_saturates_rather_than_wrapping); + exit(UNITY_END()); +} + +void loop() {}