Stop nRF52 automatic LEDs before shutdown and sleep

src/helpers/NRF52Board.cpp

@@ -4,6 +4,7 @@
 #include "NRF52VoltagePolicy.h"
 #include "Nrf52BootloaderVersion.h"
 #include "nrf52/SoftDeviceState.h"
+#include "nrf52/ShutdownLeds.h"
 #include <target.h>
 #include "UserGpioPinPolicy.h"
 
@@ -339,6 +340,10 @@ void NRF52Board::initiateShutdown(uint8_t reason) {
 void NRF52Board::enterSystemOff(uint8_t reason) {
   MESH_DEBUG_PRINTLN("PWRMGT: Entering SYSTEMOFF (%s)", getShutdownReasonString(reason));
 
+  // This path can run before BLE begin() during boot voltage protection.
+  // autoConnLed(false) is safe then too and stops future automatic LED writes.
+  Bluefruit.autoConnLed(false);
+
   // Record shutdown reason in GPREGRET2
   uint8_t sd_enabled = 0;
   mesh_nrf52::softdeviceIsEnabled(sd_enabled);
@@ -352,6 +357,7 @@ void NRF52Board::enterSystemOff(uint8_t reason) {
   // Flush serial buffers
   Serial.flush();
   delay(100);
+  mesh_nrf52::turnOffShutdownLeds();
 
   // Enter SYSTEMOFF
   if (sd_enabled) {
@@ -556,6 +562,11 @@ float NRF52Board::getMCUTemperature() {
 }
 
 void NRF52Board::shutdownPeripherals() {
+  // Stop Bluefruit's LED owner before setting a final OFF latch. Its timer
+  // stop is queued, so allow the existing shutdown delay to drain a pending
+  // blinky callback before the final write below (also safe before BLE begin).
+  Bluefruit.autoConnLed(false);
+
   // Power off the display if any
 #ifdef DISPLAY_CLASS
   if (display.isOn()) {
@@ -613,10 +624,13 @@ void NRF52Board::shutdownPeripherals() {
   // Flush serial buffers
   Serial.flush();
   delay(100);
+  mesh_nrf52::turnOffShutdownLeds();
 }
 
 void NRF52Board::powerOff() {
   shutdownPeripherals();
+  // Board-specific hooks may signal button release after calling the base.
+  mesh_nrf52::turnOffShutdownLeds();
 
   // Enter SYSTEMOFF
   uint8_t sd_enabled = 0;
@@ -633,6 +647,7 @@ void NRF52Board::enterDeepSleep(uint32_t secs) {
     powerOff(); // Not wake up
   } else {
     shutdownPeripherals();
+    mesh_nrf52::turnOffShutdownLeds();
 
 // Clear FPU interrupt flags to avoid insomnia
 // see errata 87 for details

src/helpers/nrf52/ShutdownLeds.h

@@ -0,0 +1,39 @@
+#pragma once
+
+#include <Arduino.h>
+
+namespace mesh_nrf52 {
+
+inline void turnOffShutdownLed(int32_t pin) {
+  // Several variants use -1, 0xff or PINS_COUNT for an absent LED. Never
+  // convert one of these sentinels into an actual GPIO during shutdown.
+  if (pin < 0 || static_cast<uint32_t>(pin) >= PINS_COUNT) return;
+  if (digitalPinToPinName(pin) == 0xff) return;
+  // Set the latch before enabling the output, including early boot shutdown.
+  digitalWrite(pin, !LED_STATE_ON);
+  pinMode(pin, OUTPUT);
+}
+
+inline void turnOffShutdownLeds() {
+  // Bluefruit uses LED_BLUE. Prefer it over the legacy LED_CONN alias: some
+  // variants retain an unused LED_CONN definition without a corresponding pin.
+#if defined(LED_BLUE)
+  turnOffShutdownLed(LED_BLUE);
+#elif defined(LED_CONN)
+  turnOffShutdownLed(LED_CONN);
+#endif
+#if defined(LED_BUILTIN)
+  turnOffShutdownLed(LED_BUILTIN);
+#endif
+#if defined(LED_PIN)
+  turnOffShutdownLed(LED_PIN);
+#endif
+#if defined(LED_RED)
+  turnOffShutdownLed(LED_RED);
+#endif
+#if defined(LED_GREEN)
+  turnOffShutdownLed(LED_GREEN);
+#endif
+}
+
+} // namespace mesh_nrf52

test/fixtures/nrf52_shutdown_leds.cpp

@@ -0,0 +1,199 @@
+#include <Arduino.h>
+#include <algorithm>
+#include <stdexcept>
+
+#define LED_STATE_ON 1
+#define LED_BLUE 36
+#define LED_BUILTIN 35
+#define LED_PIN LED_BUILTIN
+#define LED_GREEN LED_BUILTIN
+#define LED_RED (-1)
+#define P_LORA_RESET 40
+#define P_LORA_BUSY 41
+#define P_LORA_NSS 42
+#define P_LORA_DIO_1 43
+#define P_LORA_SCLK 44
+#define P_LORA_MISO 45
+#define P_LORA_MOSI 46
+#define DISPLAY_CLASS 1
+#define HAS_GPS 1
+#define GPS_VRTC_EN 10
+#define GPS_RESET 11
+#define GPS_SLEEP_INT 12
+#define GPS_RTC_INT 13
+#define GPS_EN 14
+#define PIN_DRV_EN 15
+#define PIN_BAT_ADC_EN 16
+#define PIN_3V3_EN 17
+#define MESH_DEBUG_PRINTLN(...) do {} while (0)
+#define POWER_SYSTEMOFF_SYSTEMOFF_Enter 1
+#define NRF_ERROR_SOFTDEVICE_NOT_ENABLED 2
+#define pdMS_TO_TICKS(ms) (ms)
+
+#include <helpers/nrf52/ShutdownLeds.h>
+
+std::vector<Event> events;
+int levels[PINS_COUNT];
+bool timer_active = false, timer_stop_pending = false, automatic_owner = true;
+bool ble_started = false;
+uint8_t sd_enabled_mock = 0;
+uint32_t now = 0;
+struct Exit {};
+
+void event(const char* name) { events.push_back({name, 0, 0}); }
+struct BluefruitMock {
+  void autoConnLed(bool enabled) {
+    assert(!enabled);
+    event("stop-auto-led");
+    automatic_owner = enabled;
+    // The real SDK accepts this before begin(), when the timer handle is NULL.
+    if (ble_started) timer_stop_pending = true;
+  }
+} Bluefruit;
+
+uint32_t millis() { return now; }
+int digitalRead(int) { return LOW; }
+void detachInterrupt(int) { event("detach-radio-irq"); }
+void delayMicroseconds(int) {}
+void delay(unsigned ms) {
+  now += ms;
+  if (timer_active) {
+    // SDK blinky_cb does not test _led_conn: a callback already ready to run
+    // can still execute before the queued timer stop is processed.
+    event("pending-blinky");
+    digitalWrite(LED_BLUE, !levels[LED_BLUE]);
+  }
+  if (timer_stop_pending) {
+    timer_active = false;
+    timer_stop_pending = false;
+    event("timer-stopped");
+  }
+}
+void vTaskDelay(uint32_t) { event("timed-sleep"); }
+void NVIC_SystemReset() { event("reset"); throw Exit{}; }
+namespace mesh_nrf52 {
+int softdeviceIsEnabled(uint8_t& enabled) { enabled = sd_enabled_mock; return 0; }
+}
+struct Register {
+  Register& operator=(uint32_t) { event("systemoff"); return *this; }
+};
+struct PowerRegisters { Register SYSTEMOFF; uint8_t GPREGRET2; } power_registers;
+PowerRegisters* NRF_POWER = &power_registers;
+int sd_power_gpregret_clr(int, int) { return 0; }
+int sd_power_gpregret_set(int, int) { return 0; }
+uint32_t sd_power_system_off() { event("systemoff"); throw Exit{}; }
+struct SerialMock { void flush() { event("flush"); } } Serial;
+struct SpiMock { void setPins(int,int,int) {} void begin() { event("spi"); } } SPI;
+struct RadioMock { void powerOff() { event("radio-off"); } } radio_driver;
+struct LocationMock { void stop() { event("gps-off"); } } location;
+struct SensorsMock { LocationMock* getLocationProvider() { return &location; } } sensors;
+struct DisplayMock {
+  bool isOn() { return true; }
+  void turnOff() { event("display-off"); }
+} display;
+
+class NRF52Board {
+public:
+  virtual ~NRF52Board() {}
+  virtual void shutdownPeripherals();
+  virtual void powerOff();
+  void enterDeepSleep(uint32_t secs);
+  void enterSystemOff(uint8_t reason);
+  void reboot() { NVIC_SystemReset(); }
+};
+class MeshTrackerX1Board : public NRF52Board {
+public:
+  void shutdownPeripherals() override;
+};
+
+@METHODS@
+@X1_HOOK@
+
+size_t first(const char* name) {
+  for (size_t i = 0; i < events.size(); ++i) {
+    if (events[i].name == name) return i;
+  }
+  return events.size();
+}
+size_t lastWrite(int pin) {
+  size_t result = events.size();
+  for (size_t i = 0; i < events.size(); ++i) {
+    if (events[i].name == "write" && events[i].pin == pin) result = i;
+  }
+  return result;
+}
+void prepare(bool started, bool connected, bool sd) {
+  events.clear();
+  for (int& level : levels) level = LED_STATE_ON;
+  now = 0;
+  ble_started = started;
+  // Advertising uses a timer; a connected central leaves a solid LED on.
+  timer_active = started && !connected;
+  timer_stop_pending = false;
+  automatic_owner = true;
+  sd_enabled_mock = sd;
+}
+void verify(bool had_timer, bool peripherals, const char* exit_event) {
+  assert(!automatic_owner && !timer_active && !timer_stop_pending);
+  assert(levels[LED_BLUE] == !LED_STATE_ON);
+  assert(levels[LED_BUILTIN] == !LED_STATE_ON);
+  assert(first("stop-auto-led") < lastWrite(LED_BLUE));
+  if (had_timer) {
+    assert(first("stop-auto-led") < first("pending-blinky"));
+    assert(first("timer-stopped") < lastWrite(LED_BLUE));
+  }
+  if (peripherals) {
+    assert(first("radio-off") < first("gps-off"));
+    assert(first("gps-off") < lastWrite(LED_BLUE));
+  } else {
+    assert(first("radio-off") == events.size());
+    assert(first("gps-off") == events.size());
+  }
+  if (exit_event) assert(lastWrite(LED_BLUE) < first(exit_event));
+  // Subsequent BLE event writes are gated by the same _led_conn flag as SDK.
+  if (automatic_owner) digitalWrite(LED_BLUE, LED_STATE_ON);
+  assert(levels[LED_BLUE] == !LED_STATE_ON);
+}
+
+int main() {
+  NRF52Board base;
+  MeshTrackerX1Board tracker;
+  for (bool started : {false, true}) {
+    for (bool connected : {false, true}) {
+      for (bool sd : {false, true}) {
+        const bool had_timer = started && !connected;
+        prepare(started, connected, sd);
+        base.shutdownPeripherals();
+        verify(had_timer, true, nullptr);
+        // Repeating shutdown is safe, including a never-initialized BLE stack.
+        base.shutdownPeripherals();
+        verify(had_timer, true, nullptr);
+
+        prepare(started, connected, sd);
+        try { base.powerOff(); } catch (const Exit&) {}
+        verify(had_timer, true, "systemoff");
+        prepare(started, connected, sd);
+        try { base.enterDeepSleep(0); } catch (const Exit&) {}
+        verify(had_timer, true, "systemoff");
+        prepare(started, connected, sd);
+        try { base.enterDeepSleep(10); } catch (const Exit&) {}
+        verify(had_timer, true, "timed-sleep");
+
+        // Early low-voltage boot shutdown must not start GPS/radio peripherals.
+        prepare(started, connected, sd);
+        try { base.enterSystemOff(42); } catch (const Exit&) {}
+        verify(had_timer, false, "systemoff");
+
+        // Regression for #3567: inherited powerOff must dispatch the X1 hook.
+        prepare(started, connected, sd);
+        try { tracker.powerOff(); } catch (const Exit&) {}
+        verify(had_timer, true, "systemoff");
+        for (int pin : {GPS_VRTC_EN, GPS_RESET, GPS_SLEEP_INT, GPS_RTC_INT,
+                        GPS_EN, PIN_DRV_EN, PIN_BAT_ADC_EN, PIN_3V3_EN}) {
+          assert(levels[pin] == LOW);
+          assert(lastWrite(pin) < first("systemoff"));
+        }
+      }
+    }
+  }
+}

test/test_nrf52_shutdown_leds.py

@@ -0,0 +1,143 @@
+#!/usr/bin/env python3
+"""Run real nRF52 shutdown paths with queued Bluefruit LED callbacks on the host.
+
+No radio/current measurements are implied. The Bluefruit mock models the SDK's
+NULL-safe autoConnLed(false), deferred timer stop, and unguarded blinky callback.
+"""
+from pathlib import Path
+import os
+import shutil
+import subprocess
+import tempfile
+import unittest
+
+from test_replay_reset_integration import extract_braced
+
+ROOT = Path(__file__).resolve().parents[1]
+BOARD = ROOT / "src/helpers/NRF52Board.cpp"
+FIXTURE = ROOT / "test/fixtures/nrf52_shutdown_leds.cpp"
+
+ARDUINO = r'''
+#pragma once
+#include <cstdint>
+#include <vector>
+#include <string>
+#include <cassert>
+#define LOW 0
+#define HIGH 1
+#define OUTPUT 1
+#define INPUT 0
+#define PINS_COUNT 64
+struct Event { std::string name; int pin; int value; };
+extern std::vector<Event> events;
+extern int levels[PINS_COUNT];
+inline int digitalPinToPinName(int pin) { return pin == 62 ? 0xff : pin; }
+inline void digitalWrite(int pin, int value) {
+  assert(pin >= 0 && pin < PINS_COUNT && pin != 62);
+  levels[pin] = value; events.push_back({"write", pin, value});
+}
+inline void pinMode(int pin, int mode) {
+  assert(pin >= 0 && pin < PINS_COUNT && pin != 62);
+  events.push_back({"mode", pin, mode});
+}
+'''
+
+
+def compile_source(testcase, source, defines=(), expect_success=True):
+    compiler = shutil.which("c++") or shutil.which("g++")
+    testcase.assertIsNotNone(compiler, "run this host-only suite where c++ is available")
+    with tempfile.TemporaryDirectory(prefix="nrf52-shutdown-leds-") as folder:
+        work = Path(folder)
+        (work / "Arduino.h").write_text(ARDUINO)
+        (work / "case.cpp").write_text(source)
+        binary = work / ("case.exe" if os.name == "nt" else "case")
+        command = [compiler, "-std=c++11", "-Wall", "-Wextra", "-Werror",
+                   "-I", str(work), "-I", str(ROOT / "src"),
+                   *["-D" + flag for flag in defines], str(work / "case.cpp"),
+                   "-o", str(binary)]
+        if os.name != "nt":
+            command[1:1] = ["-fsanitize=address,undefined", "-fno-omit-frame-pointer",
+                           "-fno-pie", "-no-pie"]
+        built = subprocess.run(command, text=True, capture_output=True)
+        testcase.assertEqual(built.returncode, 0, built.stderr)
+        ran = subprocess.run([str(binary)], text=True, capture_output=True)
+        if expect_success:
+            testcase.assertEqual(ran.returncode, 0, ran.stdout + ran.stderr)
+        else:
+            testcase.assertNotEqual(ran.returncode, 0, "negative control did not fail")
+
+
+def lifecycle_source(mutation=None):
+    source = BOARD.read_text()
+    if mutation == "automatic-owner-left-running":
+        source = source.replace("Bluefruit.autoConnLed(false);", "// negative control")
+    elif mutation == "final-latch-left-on":
+        source = source.replace("mesh_nrf52::turnOffShutdownLeds();", "// negative control")
+    methods = "\n".join(extract_braced(source, signature) for signature in (
+        "void NRF52Board::shutdownPeripherals()",
+        "void NRF52Board::powerOff()",
+        "void NRF52Board::enterDeepSleep(uint32_t secs)",
+        "void NRF52Board::enterSystemOff(uint8_t reason)",
+    ))
+    x1 = (ROOT / "variants/meshtracker_x1/MeshTrackerX1Board.h").read_text()
+    hook = extract_braced(x1, "void shutdownPeripherals() override")
+    hook = hook.replace("void shutdownPeripherals() override",
+                        "void MeshTrackerX1Board::shutdownPeripherals()", 1)
+    return FIXTURE.read_text().replace("@METHODS@", methods).replace("@X1_HOOK@", hook)
+
+
+class Nrf52ShutdownLedsTests(unittest.TestCase):
+    def test_real_shutdown_paths_stop_led_owner_then_clear_latches(self):
+        compile_source(self, lifecycle_source())
+
+    def test_automatic_led_owner_negative_control(self):
+        compile_source(self, lifecycle_source("automatic-owner-left-running"),
+                       expect_success=False)
+
+    def test_final_latch_negative_control(self):
+        compile_source(self, lifecycle_source("final-latch-left-on"),
+                       expect_success=False)
+
+    def test_aliases_polarity_and_absent_led_sentinels(self):
+        source = r'''
+#include <Arduino.h>
+#include <helpers/nrf52/ShutdownLeds.h>
+std::vector<Event> events;
+int levels[PINS_COUNT];
+int main() {
+  for (int& level : levels) level = LED_STATE_ON;
+  mesh_nrf52::turnOffShutdownLeds();
+  for (const Event& e : events) {
+    assert(e.pin == EXPECT_PIN);
+    if (e.name == "write") assert(e.value == !LED_STATE_ON);
+  }
+  assert(events.size() == EXPECT_EVENTS);
+  for (size_t i = 0; i < events.size(); i += 2) {
+    assert(events[i].name == "write" && events[i + 1].name == "mode");
+  }
+}
+'''
+        cases = (
+            # Preferred LED_BLUE avoids expanding an obsolete broken alias.
+            ("LED_STATE_ON=1", "LED_BLUE=36", "LED_CONN=UNUSED_PIN_NAME",
+             "LED_BUILTIN=36", "LED_PIN=36", "LED_RED=36", "LED_GREEN=36",
+             "EXPECT_PIN=36", "EXPECT_EVENTS=10"),
+            ("LED_STATE_ON=0", "LED_BLUE=-1", "LED_BUILTIN=35", "LED_PIN=255",
+             "LED_RED=PINS_COUNT", "LED_GREEN=62", "EXPECT_PIN=35", "EXPECT_EVENTS=2"),
+            ("LED_STATE_ON=0", "LED_CONN=12", "EXPECT_PIN=12", "EXPECT_EVENTS=2"),
+            ("LED_STATE_ON=1", "EXPECT_PIN=-1", "EXPECT_EVENTS=0"),
+        )
+        for case in cases:
+            with self.subTest(defines=case):
+                compile_source(self, source, case)
+
+    def test_meshtracker_inherits_base_poweroff_and_keeps_peripheral_hook(self):
+        x1 = (ROOT / "variants/meshtracker_x1/MeshTrackerX1Board.h").read_text()
+        self.assertNotIn("void powerOff(", x1)
+        self.assertIn("NRF52Board::shutdownPeripherals();", x1)
+        # Catch an include-only helper that never reaches production shutdown.
+        self.assertIn('#include "nrf52/ShutdownLeds.h"', BOARD.read_text())
+
+
+if __name__ == "__main__":
+    unittest.main()