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

[Timdebruijn]

Third of three PRs from the codebase review. Production code this time, not tests.

The release gate had no gate

check_version.sh recognised a release tag as /^v[0-9]/ while release.yml triggers on the far wider v*. So:

GITHUB_REF_NAME=vtest bash tools/check_version.sh   # exit 0, nothing checked

vtest, v-wip and a bare v all built and published a release with no version check at all — and release.yml names its release ${GITHUB_REF_NAME#v}, so vtest would have shipped as "test".

The discriminator is now the ref type, which is what it always meant. A branch is never read as a version whatever it is called — the false positive the old name-shape guard existed to avoid.

It also gets a --self-test, run in CI on every push rather than only at tag time; a broken gate should surface before the release that needs it.

The first version of that self-test was itself vacuous: it passed each tag as an argument, and every case still passed with the bug reintroduced, because the argument path was never the broken one. It now drives the environment with no argument, exactly as release.yml does.

Dependencies

Three caret ranges (^7.4.3 ×2, ^1.7.3) — not pins; they accept any later minor. .pio/ is gitignored and there is no lockfile, so the resolved set lived only in whatever machine built last. This builds an OTA image, where "the version that shipped" has to be reconstructible from the tag.

Both were already resolving to the versions now named, and both are the latest release upstream — this pins current behaviour rather than changing it.

The clock

main.cpp narrowed nowMs() to uint32_t on the way into breadcrumbs::tick() — forty lines below a comment warning that millis() wraps every 49.7 days and that casting a wrapped value to uint64 does not un-wrap it.

The record exists to say "this life reached this uptime", so the bridges it was built for — the long-lived ones — were exactly the ones it misreported, as a plausible smaller number: a 60-day life read as 10.3 days.

Storage now holds seconds in the same uint32_t — 136 years, struct still sixteen bytes, CRC window and RTC-RAM layout untouched. Nothing is lost: tick() only ever wrote once per second.

⚠ External behaviour change

/api/v1/diagnostics still publishes previous_uptime_ms in milliseconds, but the value is now always a whole number of seconds. Accuracy is unchanged — it was already only sampled once per second — but a consumer reading the low digits will see zeros where it used to see noise. The field now also carries values past 2³², and the payload is tested for that.

MQTT throttle

PublishThrottle::Sample held its id as a std::string, so every publish rebuilt the whole vector: one allocation per channel, freed 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" — and this was the one place downstream that put them back on the heap. Every id is an inline constexpr constant with static storage, so the pointer is safe to borrow.

The comparison is now an explicit strcmp: it would have silently become a pointer comparison otherwise, which happens to work for interned constants and is not something to depend on.

Verification

1024 native cases · check_layering.sh · the new self-test (mutation-proven against the original bug) · a real waveshare-rs485-can build with no new warnings.

[Timdebruijn]

Two additions since the description above, both from reviewing this work rather than from the original findings.

A fallback when GITHUB_REF_TYPE is unset. Making that variable the sole release signal turned something that has never been absent into a single point of failure for the gate — and the failure mode is silence. With no ref type at all, a v-shaped name is now checked by the old heuristic instead of waved through. A branch wrongly read as a version is a loud failure on a PR; a release skipping its version check is a silent one.

Home Assistant discovery had its own serialiser, without the size check. home_assistant_discovery.cpp carried a private serialise() that was finish() minus the needed > maxBytes check. That was the wrong path to be missing it: discovery payloads are the largest this device publishes (every entity repeats the device block) and identity strings are unbounded std::strings filled from whatever a device reports over RS485.

It now uses the shared finish() at kMaxPayloadBytes. Over that, the entity is dropped rather than published oversized — the convention every other payload here follows. The largest entity the fixture produces is well under half the ceiling, and the test asserts that headroom rather than a bare "it fits".

Proving it needed care: removing an upper bound is strictly more permissive, so a test that only checks real payloads fit cannot detect its absence. The assertion goes through buildDiscoveryEntities with a device reporting an absurd model name — unbounded, that fixture publishes 14 oversized entities; bounded, none.

[Timdebruijn]

Review found a defect I introduced two commits back — worth reading before merging

Renaming heartbeatUptimeMs → heartbeatUptimeSeconds changed a field's unit without changing the layout. The CRC covers the same twelve bytes, so a record left in RTC RAM by the previous firmware still validates — and is then read as seconds.

RTC_NOINIT survives exactly the reset an OTA performs. So on the first boot into that firmware, a bridge genuinely up for three days would have published three thousand days. A thousandfold error, on every device in the fleet, through /api/v1/diagnostics. It self-corrects on the following boot — which is what would have made it hard to believe when someone reported it.

Fixed by folding a schema tag into the CRC. Not stored, so it costs no RTC bytes; a record written under a different schema fails validation and reads as cold.

⚠ Second external behaviour change, on top of the one in the description

Every device will report one cold start when this firmware lands, and its boot count restarts from 1. That is the price of the fix, and it is the honest answer — those bytes genuinely cannot be interpreted. The previous life is reported as unknown rather than as a wrong number.

Bump kStorageSchema whenever a field's meaning, unit or width changes.

Also in this commit

crc32() became crc32Update(), carrying the running value so the checksum can span the tag and the struct.

And a property the same commit had quietly dropped: the old millisecond throttle compared with <, so it could never store a value smaller than the one already there — rewriting it as == gave that up. esp_timer_get_time() does not run backwards so nothing reaches it, but tick() takes the clock as an argument, so the property can be asserted rather than merely asserted about.

Both mutation-proven: removing the schema tag fails the upgrade test; == in place of <= fails the monotonic one.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.