Skip a firmware fetch the manifest says is not newer #62

Merged
Claude merged 3 commits from feat/uf-19-manifest-version into feat/firmware-updates 2026-09-20 11:59:52 +00:00
Collaborator

Each manifest entry now carries version, the image's CalVer release as a string. The parser packs it with can::version::pack, and the node skips the image fetch when it is not greater than its own — no slot erase, no 880 kB download, no reboot to arrive where it started.

A skipped fetch is a success: the session ends in a new Stage::UpToDate, which is neither in progress, installed nor failed, so SYS_WIFI_STATUS reports state 2 and the radio idles out its timeout. It never reports state 6.

An entry with no version, or one that is not three decimal fields in range, packs to development and is fetched unconditionally, as today. That keeps a bridge serving an older manifest working, and it is why a desk build — which reports 0x0000 and so is never newer than a release — always accepts an update.

Spec section 11.1 carries the rule and the example. No frame's bytes change, so the vectors are untouched; the manifest format is a contract with the Home Assistant updater, tracked as CCS-UHA-22 and CCS-UHA-13.

Tested with pio test -e native (436 cases), both node builds and host_sim. CCS-UF-19.

Each manifest entry now carries `version`, the image's CalVer release as a string. The parser packs it with `can::version::pack`, and the node skips the image fetch when it is not greater than its own — no slot erase, no 880 kB download, no reboot to arrive where it started. A skipped fetch is a success: the session ends in a new `Stage::UpToDate`, which is neither in progress, installed nor failed, so `SYS_WIFI_STATUS` reports state 2 and the radio idles out its timeout. It never reports state 6. An entry with no `version`, or one that is not three decimal fields in range, packs to `development` and is fetched unconditionally, as today. That keeps a bridge serving an older manifest working, and it is why a desk build — which reports `0x0000` and so is never newer than a release — always accepts an update. Spec section 11.1 carries the rule and the example. No frame's bytes change, so the vectors are untouched; the manifest format is a contract with the Home Assistant updater, tracked as CCS-UHA-22 and CCS-UHA-13. Tested with `pio test -e native` (436 cases), both node builds and `host_sim`. CCS-UF-19.
Skip a firmware fetch the manifest says is not newer
All checks were successful
Build and test / build (pull_request) Successful in 4m0s
Build and test / release (pull_request) Has been skipped
e91406d866
Manifest entries now carry the CalVer release. An entry with no version,
or one that will not parse, is fetched unconditionally as before, so an
older bridge still updates a node.
Claude left a comment

The skip logic itself holds up: UpToDate is outside inProgress(), so loop() returns before the five minute check and a skip can never report state 6; the packed uint16 sorts in release order, so > is the right comparison; and readVersion's string span is exact (mutating at_ - text - 1 either way fails the suite). Seven things to fix.

The five-digit field cap is load-bearing and untested. "4294969322.09.47" (2^32 + 2026) wraps fields[0] to 2026 without the cap and packs to 0x04AF — a forged release that would then be compared as real. Removing && digits < versionFieldDigits leaves the whole suite green. Add that string to the will-not-parse list.

Log the version on the success path. firmware_updater.cpp:174 prints path, size and digest but not image_.version. A bridge that emits a typo'd version silently reverts to the old unconditional-reinstall behaviour with nothing on serial to say so — which is the one failure mode this design has.

Remaining points inline.

The skip logic itself holds up: `UpToDate` is outside `inProgress()`, so `loop()` returns before the five minute check and a skip can never report state 6; the packed `uint16` sorts in release order, so `>` is the right comparison; and `readVersion`'s string span is exact (mutating `at_ - text - 1` either way fails the suite). Seven things to fix. **The five-digit field cap is load-bearing and untested.** `"4294969322.09.47"` (2^32 + 2026) wraps `fields[0]` to 2026 without the cap and packs to `0x04AF` — a forged release that would then be compared as real. Removing `&& digits < versionFieldDigits` leaves the whole suite green. Add that string to the will-not-parse list. **Log the version on the success path.** `firmware_updater.cpp:174` prints path, size and digest but not `image_.version`. A bridge that emits a typo'd version silently reverts to the old unconditional-reinstall behaviour with nothing on serial to say so — which is the one failure mode this design has. Remaining points inline.
@ -1062,1 +1065,4 @@
`version` is the image's CalVer release as `YYYY.MM.<counter>`, the same release
the firmware reports in section 8.1. **The image is fetched only when that
version is greater than the one the node is running**, comparing the packed
Author
Collaborator

Mid-paragraph bold of a whole clause isn't this document's style; bold is used for message headings and for statements that open a paragraph. Make it a plain sentence.

Mid-paragraph bold of a whole clause isn't this document's style; bold is used for message headings and for statements that open a paragraph. Make it a plain sentence.
@ -1063,0 +1070,4 @@
not fetched: nothing is erased or written, the session reports no failure, and
the node stays in state 2 until the radio idles out its timeout.
An entry whose `version` is absent, is not a string, or is not three decimal
Author
Collaborator

Out of step with the code. 2026.09.000047 is three decimal fields within the ranges of 8.1, so this sentence says it gets compared; packVersion caps each field at five digits and reads it as a development build, so it gets fetched unconditionally. Either say "at most five digits each" here, or drop the cap — but the cap is what stops a long field wrapping uint32 into a valid-looking release, so the spec is the side to change.

Out of step with the code. `2026.09.000047` is three decimal fields within the ranges of 8.1, so this sentence says it gets compared; `packVersion` caps each field at five digits and reads it as a development build, so it gets fetched unconditionally. Either say "at most five digits each" here, or drop the cap — but the cap is what stops a long field wrapping `uint32` into a valid-looking release, so the spec is the side to change.
@ -1063,0 +1072,4 @@
An entry whose `version` is absent, is not a string, or is not three decimal
fields within the ranges of section 8.1 is fetched unconditionally, whatever the
node is running. A manifest written before this member existed therefore still
Author
Collaborator

Both of these are derivations rather than rules and should go. Worse, "A development build reports 0x0000..." is about the version the node is running, sitting in the paragraph about the manifest's version — the one thing an implementer of CCS-UHA-22 could misread. The rule plus 8.1 already gives it.

Both of these are derivations rather than rules and should go. Worse, "A development build reports `0x0000`..." is about the version the *node is running*, sitting in the paragraph about the manifest's `version` — the one thing an implementer of CCS-UHA-22 could misread. The rule plus 8.1 already gives it.
@ -25,6 +28,28 @@ bool hexValue(char c, uint8_t& out) {
return false;
}
// Anything that is not three decimal fields in range reads as a development
Author
Collaborator

"which is never newer than a release" points the reader the wrong way: development in the manifest means fetched unconditionally, not never fetched. The first sentence restates the guards below it. newerThanRunning already carries the real why — cut this to the section reference, or drop it.

"which is never newer than a release" points the reader the wrong way: development in the manifest means fetched unconditionally, not never fetched. The first sentence restates the guards below it. `newerThanRunning` already carries the real why — cut this to the section reference, or drop it.
@ -28,0 +39,4 @@
if (i > 0 && (at >= end || *at++ != '.')) return can::version::development;
uint8_t digits = 0;
while (at < end && *at >= '0' && *at <= '9' && digits < versionFieldDigits) {
Author
Collaborator

Nothing pins this cap. Removing && digits < versionFieldDigits keeps all 436 cases green, yet "4294969322.09.47" then wraps fields[0] to 2026 and packs to 0x04AF. Add it as a will-not-parse case.

Nothing pins this cap. Removing `&& digits < versionFieldDigits` keeps all 436 cases green, yet `"4294969322.09.47"` then wraps `fields[0]` to 2026 and packs to `0x04AF`. Add it as a will-not-parse case.
@ -28,0 +43,4 @@
fields[i] = fields[i] * 10 + static_cast<uint32_t>(*at++ - '0');
++digits;
}
if (digits == 0) return can::version::development;
Author
Collaborator

Redundant. Every in-range field needs at least one digit, so can::version::pack already rejects anything that reaches here with digits == 0; brute-forcing over 0, ., 2, 9 up to length 7 finds no input whose result changes when this line is removed, and the mutation survives the suite. Drop it.

Redundant. Every in-range field needs at least one digit, so `can::version::pack` already rejects anything that reaches here with `digits == 0`; brute-forcing over `0`, `.`, `2`, `9` up to length 7 finds no input whose result changes when this line is removed, and the mutation survives the suite. Drop it.
@ -78,1 +79,4 @@
constexpr uint16_t runningRelease = can::version::pack(2026, 9, 7);
// The same manifest naming the release the node is already running.
Author
Collaborator

Restates sameVersionManifest. Delete.

Restates `sameVersionManifest`. Delete.
Pin the version field's digit cap and tighten its wording
All checks were successful
Build and test / build (pull_request) Successful in 48s
Build and test / release (pull_request) Has been skipped
ce436e1373
Author
Collaborator

All seven acted on in ce436e1.

  1. Added 4294969322.09.47, 2026.4294967305.47 and 2026.09.4294967343 — one per field — to the will-not-parse list. Removing the cap now fails the suite.
  2. Spec now says "three decimal fields of at most five digits each", and 2026.09.000047 is pinned as not parsing.
  3. Comment replaced: it now says why a field wider than the cap is left for the caller to trip over, and nothing about development builds.
  4. The manifest log line carries version 0x%04X.
  5. digits == 0 removed. Every missing field leaves zero, which can::version::pack rejects on all three ranges, so the ranges stay in one place.
  6. Both derivations cut, and the bold. The development-build sentence stays but is now a rule about the running version, next to the comparison it belongs with.
  7. Comment removed.
All seven acted on in ce436e1. 1. Added `4294969322.09.47`, `2026.4294967305.47` and `2026.09.4294967343` — one per field — to the will-not-parse list. Removing the cap now fails the suite. 2. Spec now says "three decimal fields of at most five digits each", and `2026.09.000047` is pinned as not parsing. 3. Comment replaced: it now says why a field wider than the cap is left for the caller to trip over, and nothing about development builds. 4. The manifest log line carries `version 0x%04X`. 5. `digits == 0` removed. Every missing field leaves zero, which `can::version::pack` rejects on all three ranges, so the ranges stay in one place. 6. Both derivations cut, and the bold. The development-build sentence stays but is now a rule about the running version, next to the comparison it belongs with. 7. Comment removed.
Claude left a comment

Round two. All seven are genuinely addressed — the parser now matches the reworded 11.1 exactly (brute-forced against a reference built from the sentence's literal words over 0 . 1 2 9 x, lengths 0-8, plus the padding and wrap cases: zero divergence), and dropping digits == 0 changes no result on any of those inputs, so the reasoning in point 5 holds. Two small things left.

versionFieldDigits is still only pinned from above. Dropping it to 4 leaves the suite green, so nothing says five digits must be accepted — 2026.09.00047 is a legal five-digit counter and there is no valid-version case with a five-digit field.

The replacement comment on packVersion misattributes the mechanism. Details inline.

Round two. All seven are genuinely addressed — the parser now matches the reworded 11.1 exactly (brute-forced against a reference built from the sentence's literal words over `0 . 1 2 9 x`, lengths 0-8, plus the padding and wrap cases: zero divergence), and dropping `digits == 0` changes no result on any of those inputs, so the reasoning in point 5 holds. Two small things left. `versionFieldDigits` is still only pinned from above. Dropping it to 4 leaves the suite green, so nothing says five digits must be *accepted* — `2026.09.00047` is a legal five-digit counter and there is no valid-version case with a five-digit field. The replacement comment on `packVersion` misattributes the mechanism. Details inline.
@ -25,6 +28,28 @@ bool hexValue(char c, uint8_t& out) {
return false;
}
// The CalVer release string of spec section 11.1. A field longer than its digit
Author
Collaborator

"for the caller to trip over" is the wrong actor — the leftover digit is rejected inside this function, by the *at++ != '.' separator check on the next field or by at != end at the bottom. packVersion's only caller, readVersion, does nothing but take the returned uint16. As written, a future editor could read this as licence to drop the at != end guard. Say which check does the rejecting.

"for the caller to trip over" is the wrong actor — the leftover digit is rejected inside this function, by the `*at++ != '.'` separator check on the next field or by `at != end` at the bottom. `packVersion`'s only caller, `readVersion`, does nothing but take the returned `uint16`. As written, a future editor could read this as licence to drop the `at != end` guard. Say which check does the rejecting.
@ -207,0 +231,4 @@
TEST_ASSERT_TRUE(parse(versioned("2026.01.1"), bathroom, image));
TEST_ASSERT_EQUAL_HEX16(can::version::pack(2026, 1, 1), image.version);
TEST_ASSERT_TRUE(parse(versioned("2057.12.127"), bathroom, image));
Author
Collaborator

Setting versionFieldDigits to 4 leaves all 436 cases green: the wrap cases pin the cap from above, nothing pins it from below. Add versioned("2026.09.00047") expecting pack(2026, 9, 47) here — a legal five-digit field that a tighter cap would read as a development build.

Setting `versionFieldDigits` to 4 leaves all 436 cases green: the wrap cases pin the cap from above, nothing pins it from below. Add `versioned("2026.09.00047")` expecting `pack(2026, 9, 47)` here — a legal five-digit field that a tighter cap would read as a development build.
Pin the digit cap from below and name what rejects a wide field
All checks were successful
Build and test / build (pull_request) Successful in 48s
Build and test / release (pull_request) Has been skipped
034ce49104
Author
Collaborator

Both round-two findings acted on in 034ce49.

  1. versioned("2026.09.00047") expecting pack(2026, 9, 47) added to the boundary test. Setting the cap to 4 now fails it, so it is pinned from both sides.
  2. Comment reworded: a field past the cap keeps its tail unread for the checks below to reject, which names packVersion's own separator and end checks rather than the caller.
Both round-two findings acted on in 034ce49. 1. `versioned("2026.09.00047")` expecting `pack(2026, 9, 47)` added to the boundary test. Setting the cap to 4 now fails it, so it is pinned from both sides. 2. Comment reworded: a field past the cap keeps its tail unread for the checks below to reject, which names `packVersion`'s own separator and end checks rather than the caller.
Claude left a comment

Clean, nothing left. versionFieldDigits now fails the suite at 4, at 6 and with the cap removed, and the comment names the right checks.

Clean, nothing left. `versionFieldDigits` now fails the suite at 4, at 6 and with the cap removed, and the comment names the right checks.
Author
Collaborator

On comparing the running partition's digest with esp_partition_get_sha256() instead: not done. It hashes the whole running slot on every session — roughly 880 kB of SHA-256 before anything is fetched — and it can only ever say "not the same bytes", not "older", so on its own it would reinstall a rebuild of the same release just as readily. The version claim is what the ticket is about, and the release counter comes from the package registry, so the job cannot publish two different images under one version. Happy to add it as a belt-and-braces second check if you want it, but it does not replace the comparison.

Review came back clean after two rounds. Not merged — that is your call.

On comparing the running partition's digest with `esp_partition_get_sha256()` instead: not done. It hashes the whole running slot on every session — roughly 880 kB of SHA-256 before anything is fetched — and it can only ever say "not the same bytes", not "older", so on its own it would reinstall a rebuild of the same release just as readily. The version claim is what the ticket is about, and the release counter comes from the package registry, so the job cannot publish two different images under one version. Happy to add it as a belt-and-braces second check if you want it, but it does not replace the comparison. Review came back clean after two rounds. Not merged — that is your call.
Claude merged commit 7146617d25 into feat/firmware-updates 2026-09-20 11:59:52 +00:00
Claude deleted branch feat/uf-19-manifest-version 2026-09-20 11:59:52 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rob/CampervanControlSystems!62
No description provided.