Close the release gate, pin the last three deps, and stop truncating the clock

.github/workflows/ci.yml

@@ -90,6 +90,13 @@ jobs:
       - name: Layering invariants
         run: bash tools/check_layering.sh
 
+      # The release gate had no gate of its own. It recognised a release tag as /^v[0-9]/ while
+      # release.yml triggers on "v*", so vtest, v-wip and a bare v all built a release with no
+      # version check at all. Run here, on every push, rather than only at tag time -- a broken
+      # gate should surface before the release that needs it.
+      - name: Release version gate self-test
+        run: bash tools/check_version.sh --self-test
+
       - name: Embedded JS syntax
         run: python3 tools/check_web_js.py
 

platformio.ini

@@ -122,7 +122,7 @@ lib_deps =
     ; the Home Assistant discovery generator are tested on the host -- which is exactly where
     ; a wrong value_template or device_class would otherwise stay invisible until Home
     ; Assistant quietly shows nothing.
-    bblanchon/ArduinoJson@^7.4.3
+    bblanchon/ArduinoJson@7.4.3
 
 ; ---------------------------------------------------------------------------------------
 ; Target firmware. One environment per supported board (the HELIOGRAPH_BOARD_* flag picks
@@ -301,8 +301,17 @@ lib_deps =
     esp32async/AsyncTCP@3.5.0
     https://github.com/ESP32Async/ESPAsyncWebServer#v3.12.0
     miq19/eModbus@1.7.4
-    bblanchon/ArduinoJson@^7.4.3
-    bertmelis/espMqttClient@^1.7.3
+    ; EXACT, like everything above it. These two carried carets (^7.4.3, ^1.7.3), which is not
+    ; a pin: it accepts any later minor. There is no lockfile -- .pio/ is gitignored -- so the
+    ; resolved set lives only in whatever machine built last, and a CI runner with a cold cache
+    ; could produce a different firmware from the same commit without anything saying so. That
+    ; matters more here than in most projects: this builds an OTA image, and "the version that
+    ; shipped" has to be reconstructible from the tag.
+    ;
+    ; Both were already resolving to the versions named here, and both are the latest release
+    ; upstream (checked 2026-08-28), so this pins the current behaviour rather than changing it.
+    bblanchon/ArduinoJson@7.4.3
+    bertmelis/espMqttClient@1.7.3
 
 ; The board Tim runs in production (16 MB flash, PCF85063 RTC, no relays).
 [env:waveshare-rs485-can]

src/device/bridge_info.h

@@ -113,7 +113,7 @@ struct BridgeInfo {
     /// reason is not here: it is this boot's resetReason, already above.
     uint32_t bootCount        = 0;
     bool     breadcrumbsCold  = true;
-    uint32_t previousUptimeMs = 0;
+    uint64_t previousUptimeMs = 0;
     uint32_t previousFirmware = 0;
 
     /// The board this firmware is running on. Reported to Home Assistant as the bridge

src/diagnostics/breadcrumbs.cpp

@@ -15,21 +15,41 @@
 namespace heliograph::breadcrumbs {
 namespace {
 
+/// What the twelve checksummed bytes MEAN. Not stored -- it is folded into the CRC, so it
+/// costs no RTC bytes and a record written under a different schema simply fails validation.
+///
+/// This exists because renaming heartbeatUptimeMs to heartbeatUptimeSeconds changed a field's
+/// UNIT without changing the layout, so an old record still CRC'd correctly and was read back
+/// as warm: a bridge that had been up three days reported three thousand. That is not a
+/// plausible-looking error, it is a thousandfold one, and it would have happened once on every
+/// device in the fleet at its first update into that firmware.
+///
+/// Bump this whenever a field's meaning, unit or width changes. The cost of bumping is one
+/// cold start -- the boot count restarts and the previous life is reported as unknown, which
+/// is the honest answer, because the bytes genuinely cannot be interpreted.
+inline constexpr uint32_t kStorageSchema = 2;
+
 /// Plain CRC32 (reflected, poly 0xEDB88320), byte at a time, no table. Twelve bytes once a
 /// second does not justify 1 KB of lookup table in a build where RAM is the scarce resource.
-uint32_t crc32(const uint8_t* data, size_t len) {
-    uint32_t crc = 0xFFFFFFFFu;
+///
+/// Takes and returns the RUNNING value, without the final inversion, so a checksum can span
+/// more than one buffer -- the schema tag and the struct are two.
+uint32_t crc32Update(uint32_t crc, const uint8_t* data, size_t len) {
     for (size_t i = 0; i < len; ++i) {
         crc ^= data[i];
         for (int b = 0; b < 8; ++b) {
             crc = (crc >> 1) ^ (0xEDB88320u & (0u - (crc & 1u)));
         }
     }
-    return crc ^ 0xFFFFFFFFu;
+    return crc;
 }
 
 uint32_t storageCrc(const Storage& s) {
-    return crc32(reinterpret_cast<const uint8_t*>(&s), offsetof(Storage, crc));
+    uint32_t crc = 0xFFFFFFFFu;
+    crc = crc32Update(crc, reinterpret_cast<const uint8_t*>(&kStorageSchema),
+                      sizeof kStorageSchema);
+    crc = crc32Update(crc, reinterpret_cast<const uint8_t*>(&s), offsetof(Storage, crc));
+    return crc ^ 0xFFFFFFFFu;
 }
 
 }  // namespace
@@ -42,26 +62,32 @@ BootRecord begin(Storage& storage, uint32_t runningFirmware) {
         // field says what it was running. Its death reason is this boot's
         // esp_reset_reason(), which the caller already exposes -- nothing to store.
         record.coldStart        = false;
-        record.previousUptimeMs = storage.heartbeatUptimeMs;
+        record.previousUptimeMs = static_cast<uint64_t>(storage.heartbeatUptimeSeconds) * 1000ULL;
         record.previousFirmware = storage.runningFirmware;
         record.bootCount        = storage.bootCount + 1;
     }
     // Cold path and warm path converge: write this life's record. On cold, bootCount in
     // the record defaulted to 1.
     storage.bootCount         = record.bootCount;
-    storage.heartbeatUptimeMs = 0;
+    storage.heartbeatUptimeSeconds = 0;
     storage.runningFirmware   = runningFirmware;
     storage.crc               = storageCrc(storage);
     return record;
 }
 
-void tick(Storage& storage, uint32_t uptimeMs) {
-    // One write per second of uptime. The comparison also handles the first call (heartbeat
-    // starts at 0) and is immune to loop() pace.
-    if (uptimeMs < storage.heartbeatUptimeMs + 1000) {
+void tick(Storage& storage, uint64_t uptimeMs) {
+    // One write per second of uptime, which is also why storing seconds loses nothing. Takes
+    // the full 64-bit clock: the caller used to narrow it to uint32 here, and casting a
+    // wrapped value to a wider type does not un-wrap it.
+    const uint32_t seconds = static_cast<uint32_t>(uptimeMs / 1000);
+    // <=, not !=. The old millisecond comparison was implicitly monotonic -- it could never
+    // write a value smaller than the one stored -- and an equality check quietly gave that up.
+    // esp_timer_get_time() does not run backwards, so nothing reaches this today; the property
+    // is worth keeping anyway, because the field's whole meaning is "how far this life got".
+    if (seconds <= storage.heartbeatUptimeSeconds) {
         return;
     }
-    storage.heartbeatUptimeMs = uptimeMs;
+    storage.heartbeatUptimeSeconds = seconds;
     storage.crc               = storageCrc(storage);
 }
 

src/diagnostics/breadcrumbs.h

@@ -50,7 +50,13 @@ namespace heliograph::breadcrumbs {
 /// once-per-four-billion event that still only costs one fabricated boot count.
 struct Storage {
     uint32_t bootCount;
-    uint32_t heartbeatUptimeMs;
+    /// SECONDS, not milliseconds. As milliseconds this wrapped at 49.7 days and the record
+    /// exists precisely to describe a bridge that had been up a long time: a 60-day life
+    /// reported ~10 days, which reads as a plausible number rather than as an error. Seconds
+    /// in the same uint32 reach 136 years and keep this struct at sixteen bytes, so the CRC
+    /// window and the RTC layout are untouched. Nothing is lost -- tick() only writes once
+    /// per second, so the millisecond digits were never meaningful.
+    uint32_t heartbeatUptimeSeconds;
     /// (major<<16)|(minor<<8)|patch of the image that is running. "The previous life died
     /// right after an OTA" is the single most valuable thing this record can show.
     uint32_t runningFirmware;
@@ -86,7 +92,11 @@ struct BootRecord {
     uint32_t bootCount = 1;
     /// The previous life's last heartbeat: "it had been up this long when it died".
     /// Meaningless on a cold start; the payload reports it absent then.
-    uint32_t previousUptimeMs = 0;
+    ///
+    /// Still milliseconds, and still what /api/v1/diagnostics publishes as
+    /// previous_uptime_ms -- the storage changed, the interface did not. 64-bit because a
+    /// 32-bit millisecond count is the wrap this moved away from.
+    uint64_t previousUptimeMs = 0;
     /// The image the previous life was running, same encoding as Storage::runningFirmware.
     uint32_t previousFirmware = 0;
 };
@@ -98,6 +108,6 @@ BootRecord begin(Storage& storage, uint32_t runningFirmware);
 
 /// Heartbeat: remembers how far this life got. Throttled internally to one write per second
 /// of uptime -- calling it every loop() pass is fine and expected.
-void tick(Storage& storage, uint32_t uptimeMs);
+void tick(Storage& storage, uint64_t uptimeMs);
 
 }  // namespace heliograph::breadcrumbs

src/main.cpp

@@ -1980,7 +1980,7 @@ void loop() {
 
     // Breadcrumb heartbeat: "this life reached this uptime". Throttled inside to one RTC-RAM
     // write per second.
-    breadcrumbs::tick(g_breadcrumbStore, static_cast<uint32_t>(nowMs()));
+    breadcrumbs::tick(g_breadcrumbStore, nowMs());
 
     g_wifi.loop(nowMs());
     startOutputs();  // no-op until there is a network, and only ever runs once

src/outputs/mqtt/home_assistant_discovery.cpp

@@ -8,6 +8,8 @@
 #include <cstdio>
 
 #include "device/command.h"
+#include "json_limits.h"
+#include "mqtt_payloads.h"
 #include "relays/drm.h"
 
 namespace heliograph::mqtt {
@@ -185,16 +187,16 @@ void addDeviceBlock(JsonObject entity, const BridgeInfo& bridge, const DeviceIde
     device["via_device"] = bridge.bridgeId;
 }
 
+// Was a private copy of json_limits::finish() with the size check taken out: it caught an
+// overflowed document but let a well-formed one grow to whatever the heap allowed. Every other
+// payload on this device is bounded, and discovery payloads are the LARGEST ones -- each
+// carries the full device block -- so this was the one path without the guard, on the output
+// that publishes the most bytes.
+//
+// Now the shared one, at the same ceiling the state payloads use. One point of truth: a change
+// to how a payload is bounded should not need finding in two places.
 bool serialise(const JsonDocument& doc, std::string& out) {
-    if (doc.overflowed()) {
-        return false;
-    }
-    const size_t needed = measureJson(doc);
-    std::string  buffer;
-    buffer.resize(needed + 1);
-    buffer.resize(serializeJson(doc, buffer.data(), buffer.size()));
-    out = std::move(buffer);
-    return true;
+    return json_limits::finish(doc, out, kMaxPayloadBytes);
 }
 
 }  // namespace

src/outputs/mqtt/publish_policy.cpp

@@ -4,6 +4,7 @@
 
 #include <algorithm>
 #include <cmath>
+#include <cstring>
 
 namespace heliograph::mqtt {
 
@@ -45,7 +46,7 @@ bool PublishThrottle::shouldPublish(const DeviceState& state, uint64_t nowMs) {
             continue;
         }
         const auto    it       = std::find_if(lastPublished_.begin(), lastPublished_.end(),
-                                              [&m](const Sample& s) { return s.id == m.id; });
+                                              [&m](const Sample& s) { return std::strcmp(s.id, m.id) == 0; });
         const Sample* previous = it == lastPublished_.end() ? nullptr : &*it;
         if (previous == nullptr) {
             return true;  // a channel appeared, e.g. a second MPPT was detected

src/outputs/mqtt/publish_policy.h

@@ -50,7 +50,19 @@ class PublishThrottle {
     double deadbandFor(MeasurementType type) const;
 
     struct Sample {
-        std::string id;
+        /// Borrowed, not owned -- and safe to borrow: an id is either one of the
+        /// `inline constexpr const char*` constants in measurement.h or, in a few tests, a
+        /// bare string literal. Both have static storage duration, so both outlive every
+        /// throttle that points at them. What would NOT be safe is an id built at runtime,
+        /// which nothing does and measurement.h's own contract forbids.
+        ///
+        /// This was a std::string, which rebuilt the whole vector on every publish: one
+        /// allocation per channel, freed again on the next, at least once a minute per device
+        /// for the life of the bridge. measurement.h keeps ids as const char* for exactly that
+        /// reason -- it records 80-120 short-lived allocations per poll as "a classic
+        /// fragmentation hazard" on a device with no defragmenting allocator -- and this was
+        /// the one place downstream that put them back on the heap.
+        const char* id;
         double      value;
         bool        valid;
         bool        stale;

test/test_mqtt/test_main.cpp

@@ -435,6 +435,47 @@ static void test_writable_driver_lists_its_bounds() {
 
 // --- Home Assistant discovery --------------------------------------------------------------
 
+// Discovery payloads are the largest this device publishes -- every entity repeats the whole
+// device block -- and they were the one path that went out unbounded, through a private copy of
+// json_limits::finish() with the size check removed. They share the state payloads' ceiling now.
+//
+// Asserted BOTH ways, because either alone is worthless: real payloads must still be published
+// (a ceiling that refuses them breaks Home Assistant discovery outright and silently), and the
+// ceiling must actually refuse something (or it is decoration).
+static void test_every_discovery_payload_fits_the_ceiling_it_now_shares() {
+    Rig              r;
+    const auto       state  = r.poll();
+    const auto       bridge = makeBridge();
+    const MqttTopics topics(kDefaultBaseTopic, bridge.bridgeId);
+    const auto entities = buildDiscoveryEntities(state, bridge, topics, topics.availability(),
+                                                 kDefaultDiscoveryPrefix, bridge.bridgeId);
+
+    TEST_ASSERT_TRUE_MESSAGE(entities.size() > 3, "the fixture must produce real entities");
+    size_t largest = 0;
+    for (const auto& e : entities) {
+        TEST_ASSERT_FALSE_MESSAGE(e.payload.empty(), "an entity was serialised to nothing");
+        largest = std::max(largest, e.payload.size());
+    }
+    // Room to spare, not "just fits": the device block grows when a field is added, and a
+    // ceiling this sits under by a hair would start dropping entities on the next one.
+    TEST_ASSERT_LESS_THAN_UINT32(kMaxPayloadBytes / 2, largest);
+
+    // AND THE BOUND BITES, through the real builder rather than beside it. Identity strings
+    // are unbounded std::strings filled from whatever a device reports, and every entity
+    // repeats the device block -- so a device with an absurd model name is the reachable way
+    // over the ceiling. Asserting this through buildDiscoveryEntities is what makes the guard
+    // testable at all: removing an upper bound is strictly MORE permissive, so a test that
+    // only checks real payloads fit cannot detect its absence.
+    //
+    // Over the ceiling the entity is dropped rather than published oversized. That is the
+    // convention every other payload on this device already follows.
+    DeviceState huge  = r.poll();
+    huge.identity.model = std::string(kMaxPayloadBytes, 'M');
+    const auto dropped = buildDiscoveryEntities(huge, bridge, topics, topics.availability(),
+                                                kDefaultDiscoveryPrefix, bridge.bridgeId);
+    TEST_ASSERT_EQUAL_UINT32(0, dropped.size());
+}
+
 static void test_discovery_creates_an_entity_per_supported_measurement() {
     Rig        r;
     const auto state  = r.poll();
@@ -1626,6 +1667,7 @@ int main(int, char**) {
     RUN_TEST(test_capabilities_payload_reports_read_only);
     RUN_TEST(test_writable_driver_lists_its_bounds);
     RUN_TEST(test_discovery_creates_an_entity_per_supported_measurement);
+    RUN_TEST(test_every_discovery_payload_fits_the_ceiling_it_now_shares);
     RUN_TEST(test_discovery_metadata_matches_the_measurement_type);
     RUN_TEST(test_value_template_reads_the_right_key);
     RUN_TEST(test_availability_tracks_the_bridge_not_the_inverter);