From fb5ed7e4eecafc64e544b43576077c791228b0e0 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Thu, 15 Oct 2026 17:37:53 -0700 Subject: [PATCH] Stop nRF52 automatic LEDs before shutdown and sleep --- src/helpers/NRF52Board.cpp | 15 ++ src/helpers/nrf52/ShutdownLeds.h | 39 +++++ test/fixtures/nrf52_shutdown_leds.cpp | 199 ++++++++++++++++++++++++++ test/test_nrf52_shutdown_leds.py | 143 ++++++++++++++++++ 4 files changed, 396 insertions(+) create mode 100644 src/helpers/nrf52/ShutdownLeds.h create mode 100644 test/fixtures/nrf52_shutdown_leds.cpp create mode 100644 test/test_nrf52_shutdown_leds.py diff --git a/src/helpers/NRF52Board.cpp b/src/helpers/NRF52Board.cpp index 9e869f253..ca49d7181 100644 --- a/src/helpers/NRF52Board.cpp +++ b/src/helpers/NRF52Board.cpp @@ -4,6 +4,7 @@ #include "NRF52VoltagePolicy.h" #include "Nrf52BootloaderVersion.h" #include "nrf52/SoftDeviceState.h" +#include "nrf52/ShutdownLeds.h" #include #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 diff --git a/src/helpers/nrf52/ShutdownLeds.h b/src/helpers/nrf52/ShutdownLeds.h new file mode 100644 index 000000000..760a1f451 --- /dev/null +++ b/src/helpers/nrf52/ShutdownLeds.h @@ -0,0 +1,39 @@ +#pragma once + +#include + +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(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 diff --git a/test/fixtures/nrf52_shutdown_leds.cpp b/test/fixtures/nrf52_shutdown_leds.cpp new file mode 100644 index 000000000..80efe350d --- /dev/null +++ b/test/fixtures/nrf52_shutdown_leds.cpp @@ -0,0 +1,199 @@ +#include +#include +#include + +#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 + +std::vector 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")); + } + } + } + } +} diff --git a/test/test_nrf52_shutdown_leds.py b/test/test_nrf52_shutdown_leds.py new file mode 100644 index 000000000..50670227d --- /dev/null +++ b/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 +#include +#include +#include +#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 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 +#include +std::vector 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()