fix: release-review findings for 4.1.0 — BLE settings list, LED rail, strict setting parse

.github/workflows/clang-tidy.yml

@@ -46,4 +46,6 @@ jobs:
             BirdsEye/led_modes.cpp \
             BirdsEye/led_animations.cpp \
             BirdsEye/sector_purple.cpp \
+            BirdsEye/local_time.cpp \
+            BirdsEye/setting_parse.cpp \
             BirdsEye/ble_stream.cpp

.github/workflows/compile-sketch.yml

@@ -37,7 +37,10 @@ jobs:
       # SensorEgg POC and the NeoPixel strip exactly as beta.yml does, so
       # the flag-on build is compile-checked on the PR rather than first
       # failing on the publish workflow. Everything else builds the
-      # master/release defaults (off).
+      # project.h defaults — which since 4.1.0 means the NeoPixel strip is
+      # ON (it is a core feature now; the flag defaults to 1) and only the
+      # SensorEgg POC is off. So the difference between the two arms is
+      # now just SensorEgg and the DovesLapTimer ref.
       FEATURE_FLAGS: ${{ (github.base_ref == 'BETA' || github.head_ref == 'BETA' || github.ref_name == 'BETA') && '-DBIRDSEYE_ENABLE_SENSOREGG=1 -DBIRDSEYE_ENABLE_NEOPIXEL=1' || '' }}
     # Build both XIAO nRF52840 variants. The Sense board has the onboard
     # LSM6DS3 IMU; the plain board does not (accelerometer logging degrades

ARCHITECTURE.md

@@ -142,8 +142,10 @@ to the matching `*_LOOP()`.
   `led_brightness` by day, `led_brightness_night` after dark (see
   *Local time* below). The strip's 5 V boost
   converter has its EN pin driven low in sleep, so System OFF really
-  powers the LEDs down. Gated on `BIRDSEYE_ENABLE_NEOPIXEL`: on in
-  beta, off (fully compiled out, no UICR write) in master/release.
+  powers the LEDs down. `BIRDSEYE_ENABLE_NEOPIXEL` is on in **every**
+  channel as of 4.1.0, which is what makes the strip a core feature — at
+  the price of a one-way, fleet-wide UICR NFC→GPIO conversion on the
+  first boot after updating.
 - **Course creator** (`course_creator` + `track_json` pure units, glued
   into the menu/pages/SD modules) — authors a track course on the device
   by walking to each cone and holding for a 3 s GPS average. Autocross

BirdsEye/BirdsEye.ino

@@ -99,6 +99,7 @@
 #include "haversine.h"
 #include "idle_policy.h"
 #include "local_time.h"
+#include "setting_parse.h"
 #include "neopixel.h"
 #include "replay.h"
 #include "sat_bars.h"
@@ -1018,46 +1019,63 @@ void setup() {
     }
     // NeoPixel strip (plan 0006). Both clamp back to the compiled-in
     // default on a missing or nonsense value, per the house idiom.
-    if (getSetting("led_brightness", buf, sizeof(buf))) {
-      const int b = atoi(buf);
+    //
+    // That idiom needs setting_parse::parseIntSetting, not atoi(): atoi
+    // answers 0 for "" and for "garbage", and for every setting below
+    // EXCEPT rev_limit and temp1_alert_c, 0 is inside the accepted range.
+    // led_brightness 0 disables the LEDs and never raises the 5 V boost
+    // rail, so a blank value read as a deliberate "off" and looked exactly
+    // like dead hardware. parseIntSetting rejects a non-integer outright,
+    // the range check then fails, and the compiled-in default stands.
+    int parsedSetting = 0;
+    if (getSetting("led_brightness", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int b = parsedSetting;
       if (b >= 0 && b <= 255) settingLedBrightness = (uint8_t)b;
     }
-    if (getSetting("rev_limit", buf, sizeof(buf))) {
-      const int r = atoi(buf);
+    if (getSetting("rev_limit", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int r = parsedSetting;
       // Floor keeps a garbled value from parking the scale at zero;
       // ceiling matches the tach filter's ~20k true-RPM limit.
       if (r >= 1000 && r <= 20000) settingRevLimit = r;
     }
-    if (getSetting("overrev_limit", buf, sizeof(buf))) {
-      const int r = atoi(buf);
+    if (getSetting("overrev_limit", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int r = parsedSetting;
       // 0 (the default) disables the whole-chain overrev flash; any
       // other value clamps to the same band as rev_limit.
       if (r == 0) settingOverrevLimit = 0;
       else if (r >= 1000 && r <= 20000) settingOverrevLimit = r;
     }
-    if (getSetting("temp1_alert_c", buf, sizeof(buf))) {
-      const int t = atoi(buf);
+    if (getSetting("temp1_alert_c", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int t = parsedSetting;
       // Celsius. Floor above any plausible ambient so a garbled value
       // can't latch the alert at power-on; ceiling past any real EGT.
       if (t >= 50 && t <= 1200) settingTemp1AlertC = t;
     }
     // Local time (plan 0010). The band is the pure unit's, not a literal
     // here, so ±14 h has one home. Out of band keeps the 0 default —
     // i.e. UTC — which is exactly the pre-0010 behaviour.
-    if (getSetting("utc_offset_min", buf, sizeof(buf))) {
-      const int o = atoi(buf);
+    if (getSetting("utc_offset_min", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int o = parsedSetting;
       if (local_time::isValidOffsetMinutes(o)) settingUtcOffsetMin = (int16_t)o;
     }
-    if (getSetting("led_brightness_night", buf, sizeof(buf))) {
-      const int b = atoi(buf);
+    if (getSetting("led_brightness_night", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int b = parsedSetting;
       if (b >= 0 && b <= 255) settingLedBrightnessNight = (uint8_t)b;
     }
-    if (getSetting("led_day_start_hour", buf, sizeof(buf))) {
-      const int h = atoi(buf);
+    if (getSetting("led_day_start_hour", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int h = parsedSetting;
       if (h >= 0 && h <= 23) settingLedDayStartHour = (uint8_t)h;
     }
-    if (getSetting("led_night_start_hour", buf, sizeof(buf))) {
-      const int h = atoi(buf);
+    if (getSetting("led_night_start_hour", buf, sizeof(buf)) &&
+        setting_parse::parseIntSetting(buf, &parsedSetting)) {
+      const int h = parsedSetting;
       if (h >= 0 && h <= 23) settingLedNightStartHour = (uint8_t)h;
     }
     crossingThresholdMeters = settingLapDetectionDistance;
@@ -1078,8 +1096,10 @@ void setup() {
   // beta channel): the one-time UICR NFC->GPIO write needs direct NVMC
   // access, which is illegal once the SoftDevice is up. Also before
   // wdtSetup() so the one-time self-reset can't race the watchdog. Needs
-  // SETTINGS_SETUP (led_brightness) — a no-op unless
-  // BIRDSEYE_ENABLE_NEOPIXEL is set (beta channel only).
+  // SETTINGS_SETUP (led_brightness). Since 4.1.0 this runs on EVERY
+  // channel — BIRDSEYE_ENABLE_NEOPIXEL defaults to 1 — so the first boot
+  // of any 4.1.0+ image is the one that spends the NFC pads and resets
+  // once. See project.h.
   NEOPIXEL_SETUP();
 
   // Camera auto-record: load the persisted Insta360 serial + init the FSM

BirdsEye/bluetooth.ino

@@ -8,6 +8,11 @@
 #include "camera_ble.h"
 #include "filename_validator.h"
 #include "firmware_ota.h"
+// For SETTINGS_JSON_CAPACITY and getSetting/setSetting. This module used
+// them via Arduino's concatenation of BirdsEye.ino's includes; naming the
+// dependency follows camera_ble.ino and keeps the SLIST buffer sizes tied
+// to the settings module that owns them.
+#include "settings.h"
 
 // Target connection interval in 1.25 ms units: 12 = 15 ms, the fastest an
 // Apple central is permitted to accept from an accessory. See bleTuneLink().
@@ -1040,7 +1045,13 @@ void processSettingsCommand() {
       return;
     }
 
-    char fileBuf[512];
+    // The SECOND parser of /SETTINGS.json (settings.ino has the other).
+    // Both are sized by SETTINGS_JSON_CAPACITY so they can never drift
+    // again — see the comment on it in settings.h for what happened when
+    // they did. static, not stack: 2 KB in one frame is more than the loop
+    // task's budget wants, and it matches the house idiom in
+    // sd_functions.ino ("keeps JSON_BUFFER_SIZE off the stack").
+    static char fileBuf[SETTINGS_JSON_CAPACITY];
     int bytesRead = settingsFile.read(fileBuf, sizeof(fileBuf) - 1);
     settingsFile.close();
     releaseSDAccess(SD_ACCESS_TRACK_PARSE);
@@ -1056,7 +1067,8 @@ void processSettingsCommand() {
     }
     fileBuf[bytesRead] = '\0';
 
-    StaticJsonDocument<512> doc;
+    static StaticJsonDocument<SETTINGS_JSON_CAPACITY> doc;
+    doc.clear();
     DeserializationError err = deserializeJson(doc, fileBuf);
     if (err != DeserializationError::Ok) {
       debug(F("BLE: SLIST - JSON parse error: "));

BirdsEye/display_pages.ino

@@ -741,9 +741,12 @@ void displayPage_gps_pace() {
   display.setTextColor(DISPLAY_TEXT_WHITE);
   const int lineHeight = 21;
   if (engineStopped) {
-    display.setCursor(0, lineHeight);
+    // 7 chars, NOT 8 with a leading space: size 3 is an 18 px advance, so
+    // " STOPPED" needs 144 px on a 128 px panel and the trailing D was
+    // clipped on every render. 7 x 18 = 126 fits, and x=1 centres it.
+    display.setCursor(1, lineHeight);
     display.setTextSize(3);
-    display.print(F(" STOPPED"));
+    display.print(F("STOPPED"));
   } else if (sprintModeIsActive() && !activeTimerRunActive()) {
     // Sprint mode, between runs — no live pace to compare (see lap page).
     display.setCursor(0, lineHeight);

BirdsEye/led_modes.cpp

@@ -78,9 +78,19 @@ led_frame::Rgb evalStatus(const StatusAction& a, StatusState& s, float value,
     s.active = false;
     return a.invalidColor;
   }
+  // A caller can hand us clearBelow ABOVE threshold — overrev_limit and
+  // rev_limit clamp independently, so overrev_limit <= rev_limit *
+  // kRevClearFrac makes the overrev action's release point sit above its
+  // own trip point. Left alone, a value in that inverted band sets the
+  // latch on one frame and clears it on the next: a 15 Hz strobe of the
+  // whole chain instead of the intended 100 ms flash. A release point
+  // above the trip point is never meaningful, so collapse it — the action
+  // degrades to a plain threshold with no hysteresis, which is right.
+  const float clearBelow =
+      a.clearBelow > a.threshold ? a.threshold : a.clearBelow;
   if (!s.active && value >= a.threshold) {
     s.active = true;
-  } else if (s.active && value < a.clearBelow) {
+  } else if (s.active && value < clearBelow) {
     s.active = false;
   }
   if (!s.active) {

BirdsEye/neopixel.ino

@@ -337,15 +337,49 @@ void NEOPIXEL_WAKE() {
 #else  // !BIRDSEYE_ENABLE_NEOPIXEL
 
 ///////////////////////////////////////////
-// SUBSYSTEM COMPILED OUT (the master/release default — see project.h)
+// SUBSYSTEM COMPILED OUT
 //
-// No UICR write, no pin driving, no Adafruit_NeoPixel dependency in
-// the image. The pads stay exactly as the chip shipped.
+// Since 4.1.0 no shipped channel takes this branch — the flag defaults
+// to 1 (see project.h). It is reached only by a build that forces
+// -DBIRDSEYE_ENABLE_NEOPIXEL=0, which is why the already-converted case
+// below matters more than it looks: the boards most likely to run such a
+// build are ones that already ran a flag-on one.
+//
+// No UICR write, no Adafruit_NeoPixel dependency in the image. On a
+// board that has never run a flag-on build the pads stay exactly as the
+// chip shipped, and this file drives nothing at all.
+//
+// ONE exception, and it is a power bug if you remove it: a board that
+// HAS run a flag-on build carries the one-way UICR NFC->GPIO conversion
+// forever, and a later flag-off image (a beta unit updating to a prod
+// release) inherits it. With stubs that truly do nothing, P0.09 — the
+// boost converter's EN — is left in its reset state (input, disconnected)
+// for the whole session AND through System OFF, where a driven-LOW level
+// is the only thing that holds the rail down (the same retention that
+// caused the "blue conn LED stays on after sleep" report, subsystem 10).
+// EN floating on the Adafruit boost module reads as enabled, so "off"
+// keeps the 5 V rail and 11 idle WS2812s alive on a device with no power
+// switch — a flat pack in a day or two.
+//
+// So: drive EN low, but ONLY when the conversion has already happened.
+// PROTECT clear (0) means the pads are already GPIO. On an unconverted
+// board the bit is set, this is skipped, and the promise above holds
+// exactly — we never touch a pad the user didn't opt into.
 ///////////////////////////////////////////
 
-void NEOPIXEL_SETUP() {}
+static void npxHoldConvertedBoostOff() {
+  if ((NRF_UICR->NFCPINS & UICR_NFCPINS_PROTECT_Msk) != 0) {
+    return;  // pads still NFC — never touched by a flag-off build
+  }
+  pinMode(NEOPIXEL_PIN_BOOST_EN, OUTPUT);
+  digitalWrite(NEOPIXEL_PIN_BOOST_EN, LOW);
+}
+
+void NEOPIXEL_SETUP() { npxHoldConvertedBoostOff(); }
 void NEOPIXEL_LOOP() {}
-void NEOPIXEL_SLEEP() {}
+// Re-assert before System OFF: the level is retained there, and that is
+// the case that costs a battery rather than a few mA of run current.
+void NEOPIXEL_SLEEP() { npxHoldConvertedBoostOff(); }
 void NEOPIXEL_WAKE() {}
 void neopixelNotifyPurpleSector() {}
 

BirdsEye/project.h

@@ -31,10 +31,10 @@
 #ifdef FIRMWARE_VERSION_OVERRIDE
   #define FIRMWARE_VERSION _BE_TOSTRING(FIRMWARE_VERSION_OVERRIDE)
 #else
-  // The 4.0.0 release cut (matches the v4.0.0 tag). The webapp still keys
+  // The 4.1.0 release cut (matches the v4.1.0 tag). The webapp still keys
   // the track JSON budget off this — 8 KB at or above 3.2.0 — so never
   // stamp a build below that line again.
-  #define FIRMWARE_VERSION "4.0.0"
+  #define FIRMWARE_VERSION "4.1.0"
 #endif
 
 ///////////////////////////////////////////
@@ -84,21 +84,31 @@
 
 // ---- NeoPixel strip (11 px: 2 status + 9-px pace/RPM strip) ----
 //
-// 0 (default — master and release): the whole subsystem is compiled out.
-// The module's entry points become no-ops and, critically, the firmware
-// NEVER writes UICR->NFCPINS and never drives pins 30/31 (P0.09/P0.10,
-// the NFC pads) — a flag-off build leaves the pads exactly as it found
-// them.
+// 1 (default — master, beta and release all ship this as of 4.1.0): the
+// strip is a CORE feature, present in every image so a logger works the
+// moment someone wires LEDs to it. On first boot NEOPIXEL_SETUP()
+// converts the NFC pads to GPIO by programming UICR->NFCPINS and
+// self-resets once so the pin latch takes effect. After that: pin 30 =
+// boost converter EN, pin 31 = WS2812 data. See plan 0006 and neopixel.h.
 //
-// 1 (the beta channel passes -DBIRDSEYE_ENABLE_NEOPIXEL=1): on first
-// boot NEOPIXEL_SETUP() converts the NFC pads to GPIO by programming
-// UICR->NFCPINS (a ONE-WAY change — undoing it needs a full chip erase,
-// i.e. a bootloader reflash; accepted, NFC is never used on this
-// hardware) and self-resets once so the pin latch takes effect. After
-// that: pin 30 = boost converter EN, pin 31 = WS2812 data. See plan
-// 0006 and neopixel.h.
+// KNOW WHAT THIS COSTS, because it is charged to every unit in the field,
+// not just the ones with LEDs on them. The UICR write is ONE-WAY — undoing
+// it needs a full chip erase, i.e. a bootloader reflash over USB — so the
+// first boot after updating to 4.1.0 permanently spends the NFC pads and
+// reboots itself once, on every device, wired for LEDs or not. That was the
+// deliberate 4.1.0 decision (NFC is not used on this hardware and the pads
+// are otherwise idle); it is recorded here rather than in a commit message
+// because nothing about a later build can undo it.
+//
+// 0 (no shipped channel sets this; -DBIRDSEYE_ENABLE_NEOPIXEL=0 forces it):
+// the subsystem is compiled out — no Adafruit_NeoPixel dependency, and on a
+// board that has NOT already been converted the firmware never writes UICR
+// and never drives pins 30/31. On one that HAS (it ran a flag-on build
+// before), the stubs still hold the boost EN pin low, because a floating EN
+// leaves the 5 V rail up through System OFF — see the #else block in
+// neopixel.ino.
 #ifndef BIRDSEYE_ENABLE_NEOPIXEL
-  #define BIRDSEYE_ENABLE_NEOPIXEL 0
+  #define BIRDSEYE_ENABLE_NEOPIXEL 1
 #endif
 
 ///////////////////////////////////////////

BirdsEye/setting_parse.cpp

@@ -0,0 +1,56 @@
+#include "setting_parse.h"
+
+#include <limits.h>
+
+namespace setting_parse {
+namespace {
+
+bool isSpace(char c) {
+  return c == ' ' || c == '\t' || c == '\r' || c == '\n' || c == '\v' ||
+         c == '\f';
+}
+
+}  // namespace
+
+bool parseIntSetting(const char* s, int* out) {
+  if (s == nullptr || out == nullptr) {
+    return false;
+  }
+  const char* p = s;
+  while (isSpace(*p)) {
+    p++;
+  }
+  bool negative = false;
+  if (*p == '+' || *p == '-') {
+    negative = (*p == '-');
+    p++;
+  }
+  if (*p < '0' || *p > '9') {
+    return false;  // no digits at all — "" / " " / "abc" / "-" / "+"
+  }
+  // Accumulate in long long so overflow is detected rather than wrapped.
+  // A settings file is hand-editable, so "99999999999" must be rejected,
+  // not folded into some in-range value.
+  long long acc = 0;
+  while (*p >= '0' && *p <= '9') {
+    acc = acc * 10 + (*p - '0');
+    if (acc > 4294967296LL) {
+      return false;  // far past any int; stop before acc itself overflows
+    }
+    p++;
+  }
+  while (isSpace(*p)) {
+    p++;
+  }
+  if (*p != '\0') {
+    return false;  // trailing junk — "12abc", "1.5", "0x10"
+  }
+  const long long value = negative ? -acc : acc;
+  if (value < (long long)INT_MIN || value > (long long)INT_MAX) {
+    return false;
+  }
+  *out = (int)value;
+  return true;
+}
+
+}  // namespace setting_parse