fix #4367 make USB power detection work correctly on seeed trackers (#4376)

for wio tracker 1110 and 1000-E and possibly other nrf52 boards.
The problem was that nrf52 power stuff wasn't generating regular
powerstatus notifications (because that code was guarded by a batteryLevel
check which was null for those boards).  So I've cleaned up the battery status stuff
a bit and we now have fewer special cases.
Tested on a 1000-E, tracker 1110 and a rak4631 board.

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

src/Power.cpp

@@ -461,7 +461,7 @@ class AnalogBatteryLevel : public HasBatteryLevel
 #endif
 };
 
-AnalogBatteryLevel analogLevel;
+static AnalogBatteryLevel analogLevel;
 
 Power::Power() : OSThread("Power")
 {
@@ -560,6 +560,10 @@ bool Power::setup()
 {
     bool found = axpChipInit() || analogInit();
 
+#ifdef NRF_APM
+    found = true;
+#endif
+
     enabled = found;
     low_voltage_counter = 0;
 
@@ -589,10 +593,16 @@ void Power::shutdown()
 // TODO(girts): move this and other axp stuff to power.h/power.cpp.
 void Power::readPowerStatus()
 {
+    int32_t batteryVoltageMv = -1; // Assume unknown
+    int8_t batteryChargePercent = -1;
+    OptionalBool usbPowered = OptUnknown;
+    OptionalBool hasBattery = OptUnknown; // These must be static because NRF_APM code doesn't run every time
+    OptionalBool isCharging = OptUnknown;
+
     if (batteryLevel) {
-        bool hasBattery = batteryLevel->isBatteryConnect();
+        hasBattery = batteryLevel->isBatteryConnect() ? OptTrue : OptFalse;
-        uint32_t batteryVoltageMv = 0;
+        usbPowered = batteryLevel->isVbusIn() ? OptTrue : OptFalse;
-        int8_t batteryChargePercent = 0;
+        isCharging = batteryLevel->isCharging() ? OptTrue : OptFalse;
         if (hasBattery) {
             batteryVoltageMv = batteryLevel->getBattVoltage();
             // If the AXP192 returns a valid battery percentage, use it
@@ -607,102 +617,90 @@ void Power::readPowerStatus()
                                              0, 100);
             }
         }
+    }
 
-        OptionalBool NRF_USB = OptFalse;
+// FIXME: IMO we shouldn't be littering our code with all these ifdefs.  Way better instead to make a Nrf52IsUsbPowered subclass
-
+// (which shares a superclass with the BatteryLevel stuff)
+// that just provides a few methods.  But in the interest of fixing this bug I'm going to follow current
+// practice.
 #ifdef NRF_APM // Section of code detects USB power on the RAK4631 and updates the power states.  Takes 20 seconds or so to detect
                // changes.
 
-        static nrfx_power_usb_state_t prev_nrf_usb_state = (nrfx_power_usb_state_t)-1; // -1 so that state detected at boot
+    nrfx_power_usb_state_t nrf_usb_state = nrfx_power_usbstatus_get();
-        nrfx_power_usb_state_t nrf_usb_state = nrfx_power_usbstatus_get();
+    // LOG_DEBUG("NRF Power %d\n", nrf_usb_state);
 
-        // If state changed
+    // If changed to DISCONNECTED
-        if (nrf_usb_state != prev_nrf_usb_state) {
+    if (nrf_usb_state == NRFX_POWER_USB_STATE_DISCONNECTED)
-            // If changed to DISCONNECTED
+        isCharging = usbPowered = OptFalse;
-            if (nrf_usb_state == NRFX_POWER_USB_STATE_DISCONNECTED) {
+    // If changed to CONNECTED / READY
-                powerFSM.trigger(EVENT_POWER_DISCONNECTED);
+    else
-                NRF_USB = OptFalse;
+        isCharging = usbPowered = OptTrue;
-            }
-            // If changed to CONNECTED / READY
-            else {
-                powerFSM.trigger(EVENT_POWER_CONNECTED);
-                NRF_USB = OptTrue;
-            }
 
-            // Cache the current state
-            prev_nrf_usb_state = nrf_usb_state;
-        }
 #endif
-        // Notify any status instances that are observing us
+
-        const PowerStatus powerStatus2 = PowerStatus(
+    // Notify any status instances that are observing us
-            hasBattery ? OptTrue : OptFalse, batteryLevel->isVbusIn() || NRF_USB == OptTrue ? OptTrue : OptFalse,
+    const PowerStatus powerStatus2 = PowerStatus(hasBattery, usbPowered, isCharging, batteryVoltageMv, batteryChargePercent);
-            batteryLevel->isCharging() || NRF_USB == OptTrue ? OptTrue : OptFalse, batteryVoltageMv, batteryChargePercent);
+    LOG_DEBUG("Battery: usbPower=%d, isCharging=%d, batMv=%d, batPct=%d\n", powerStatus2.getHasUSB(),
-        LOG_DEBUG("Battery: usbPower=%d, isCharging=%d, batMv=%d, batPct=%d\n", powerStatus2.getHasUSB(),
+              powerStatus2.getIsCharging(), powerStatus2.getBatteryVoltageMv(), powerStatus2.getBatteryChargePercent());
-                  powerStatus2.getIsCharging(), powerStatus2.getBatteryVoltageMv(), powerStatus2.getBatteryChargePercent());
+    newStatus.notifyObservers(&powerStatus2);
-        newStatus.notifyObservers(&powerStatus2);
 #ifdef DEBUG_HEAP
-        if (lastheap != memGet.getFreeHeap()) {
+    if (lastheap != memGet.getFreeHeap()) {
-            LOG_DEBUG("Threads running:");
+        LOG_DEBUG("Threads running:");
-            int running = 0;
+        int running = 0;
-            for (int i = 0; i < MAX_THREADS; i++) {
+        for (int i = 0; i < MAX_THREADS; i++) {
-                auto thread = concurrency::mainController.get(i);
+            auto thread = concurrency::mainController.get(i);
-                if ((thread != nullptr) && (thread->enabled)) {
+            if ((thread != nullptr) && (thread->enabled)) {
-                    LOG_DEBUG(" %s", thread->ThreadName.c_str());
+                LOG_DEBUG(" %s", thread->ThreadName.c_str());
-                    running++;
+                running++;
-                }
             }
-            LOG_DEBUG("\n");
-            LOG_DEBUG("Heap status: %d/%d bytes free (%d), running %d/%d threads\n", memGet.getFreeHeap(), memGet.getHeapSize(),
-                      memGet.getFreeHeap() - lastheap, running, concurrency::mainController.size(false));
-            lastheap = memGet.getFreeHeap();
         }
+        LOG_DEBUG("\n");
+        LOG_DEBUG("Heap status: %d/%d bytes free (%d), running %d/%d threads\n", memGet.getFreeHeap(), memGet.getHeapSize(),
+                  memGet.getFreeHeap() - lastheap, running, concurrency::mainController.size(false));
+        lastheap = memGet.getFreeHeap();
+    }
 #ifdef DEBUG_HEAP_MQTT
-        if (mqtt) {
+    if (mqtt) {
-            // send MQTT-Packet with Heap-Size
+        // send MQTT-Packet with Heap-Size
-            uint8_t dmac[6];
+        uint8_t dmac[6];
-            getMacAddr(dmac); // Get our hardware ID
+        getMacAddr(dmac); // Get our hardware ID
-            char mac[18];
+        char mac[18];
-            sprintf(mac, "!%02x%02x%02x%02x", dmac[2], dmac[3], dmac[4], dmac[5]);
+        sprintf(mac, "!%02x%02x%02x%02x", dmac[2], dmac[3], dmac[4], dmac[5]);
 
-            auto newHeap = memGet.getFreeHeap();
+        auto newHeap = memGet.getFreeHeap();
-            std::string heapTopic =
+        std::string heapTopic =
-                (*moduleConfig.mqtt.root ? moduleConfig.mqtt.root : "msh") + std::string("/2/heap/") + std::string(mac);
+            (*moduleConfig.mqtt.root ? moduleConfig.mqtt.root : "msh") + std::string("/2/heap/") + std::string(mac);
-            std::string heapString = std::to_string(newHeap);
+        std::string heapString = std::to_string(newHeap);
-            mqtt->pubSub.publish(heapTopic.c_str(), heapString.c_str(), false);
+        mqtt->pubSub.publish(heapTopic.c_str(), heapString.c_str(), false);
-            auto wifiRSSI = WiFi.RSSI();
+        auto wifiRSSI = WiFi.RSSI();
-            std::string wifiTopic =
+        std::string wifiTopic =
-                (*moduleConfig.mqtt.root ? moduleConfig.mqtt.root : "msh") + std::string("/2/wifi/") + std::string(mac);
+            (*moduleConfig.mqtt.root ? moduleConfig.mqtt.root : "msh") + std::string("/2/wifi/") + std::string(mac);
-            std::string wifiString = std::to_string(wifiRSSI);
+        std::string wifiString = std::to_string(wifiRSSI);
-            mqtt->pubSub.publish(wifiTopic.c_str(), wifiString.c_str(), false);
+        mqtt->pubSub.publish(wifiTopic.c_str(), wifiString.c_str(), false);
-        }
+    }
 #endif
 
 #endif
 
-        // If we have a battery at all and it is less than 0%, force deep sleep if we have more than 10 low readings in
+    // If we have a battery at all and it is less than 0%, force deep sleep if we have more than 10 low readings in
-        // a row. NOTE: min LiIon/LiPo voltage is 2.0 to 2.5V, current OCV min is set to 3100 that is large enough.
+    // a row. NOTE: min LiIon/LiPo voltage is 2.0 to 2.5V, current OCV min is set to 3100 that is large enough.
-        //
+    //
-        if (powerStatus2.getHasBattery() && !powerStatus2.getHasUSB()) {
+    if (batteryLevel && powerStatus2.getHasBattery() && !powerStatus2.getHasUSB()) {
-            if (batteryLevel->getBattVoltage() < OCV[NUM_OCV_POINTS - 1]) {
+        if (batteryLevel->getBattVoltage() < OCV[NUM_OCV_POINTS - 1]) {
-                low_voltage_counter++;
+            low_voltage_counter++;
-                LOG_DEBUG("Low voltage counter: %d/10\n", low_voltage_counter);
+            LOG_DEBUG("Low voltage counter: %d/10\n", low_voltage_counter);
-                if (low_voltage_counter > 10) {
+            if (low_voltage_counter > 10) {
 #ifdef ARCH_NRF52
-                    // We can't trigger deep sleep on NRF52, it's freezing the board
+                // We can't trigger deep sleep on NRF52, it's freezing the board
-                    LOG_DEBUG("Low voltage detected, but not triggering deep sleep\n");
+                LOG_DEBUG("Low voltage detected, but not triggering deep sleep\n");
 #else
-                    LOG_INFO("Low voltage detected, triggering deep sleep\n");
+                LOG_INFO("Low voltage detected, triggering deep sleep\n");
-                    powerFSM.trigger(EVENT_LOW_BATTERY);
+                powerFSM.trigger(EVENT_LOW_BATTERY);
 #endif
-                }
-            } else {
-                low_voltage_counter = 0;
             }
+        } else {
+            low_voltage_counter = 0;
         }
-    } else {
-        // No power sensing on this board - tell everyone else we have no idea what is happening
-        const PowerStatus powerStatus3 = PowerStatus(OptUnknown, OptUnknown, OptUnknown, -1, -1);
-        newStatus.notifyObservers(&powerStatus3);
     }
 }
 
@@ -1051,4 +1049,4 @@ bool Power::axpChipInit()
 #else
     return false;
 #endif
-}
+}

src/PowerFSM.cpp

@@ -26,7 +26,7 @@
 static bool isPowered()
 {
 // Circumvent the battery sensing logic and assumes constant power if no battery pin or power mgmt IC
-#if !defined(BATTERY_PIN) && !defined(HAS_AXP192) && !defined(HAS_AXP2101)
+#if !defined(BATTERY_PIN) && !defined(HAS_AXP192) && !defined(HAS_AXP2101) && !defined(NRF_APM)
     return true;
 #endif
 

variants/tracker-t1000-e/variant.h

@@ -40,6 +40,9 @@ extern "C" {
 #define NUM_ANALOG_INPUTS (6)
 #define NUM_ANALOG_OUTPUTS (0)
 
+// Use the native nrf52 usb power detection
+#define NRF_APM
+
 #define PIN_3V3_EN (32 + 6)     // P1.6, Power to Sensors
 #define PIN_3V3_ACC_EN (32 + 7) // P1.7, Power to Acc
 
@@ -147,4 +150,4 @@ extern "C" {
  *        Arduino objects - C++ only
  *----------------------------------------------------------------------------*/
 
-#endif // _VARIANT_TRACKER_T1000_E_
+#endif // _VARIANT_TRACKER_T1000_E_

variants/wio-tracker-wm1110/variant.h

@@ -43,6 +43,9 @@ extern "C" {
 
 #define WIRE_INTERFACES_COUNT 1
 
+// We rely on the nrf52840 USB controller to tell us if we are hooked to a power supply
+#define NRF_APM
+
 #define PIN_3V3_EN (32 + 1) // P1.01, Power to Sensors
 
 #define PIN_WIRE_SDA (0 + 5) // P0.05
@@ -108,4 +111,4 @@ extern "C" {
  *        Arduino objects - C++ only
  *----------------------------------------------------------------------------*/
 
-#endif // _VARIANT_WIO_TRACKER_WM1110_
+#endif // _VARIANT_WIO_TRACKER_WM1110_