Verify the image, then commit once the bus proves healthy #52

Claude · 2026-09-19T14:03:02Z

Claude commented

2026-09-19 14:03:02 +00:00

Closes the update path: the digest is checked before the boot slot moves, the node
reboots into the new slot with the radio off, and the new image commits itself on
the first CAN transmit that completes or restarts after 30 s so the bootloader can
revert.

Behaviour:

FirmwareUpdater hashes the image as it writes, compares it to the manifest and
only then calls the sink's new activate(). A mismatch, a slot that will not
close and a slot that will not switch all go through Stage::Failed, which is
the one place CCS-UF-12 has to map.
CanBus::transmitCompleted() latches a transmit another node acknowledged. TWAI
reads TWAI_ALERT_TX_SUCCESS, the MCP2515 reads CANINTF TXnIF, and SocketCAN
reports nothing, since a write to a vcan succeeds with nobody listening.
BootGuard arms only when the bootloader reports the running image
PENDING_VERIFY, so a bootloader without CONFIG_BOOTLOADER_APP_ROLLBACK_ENABLE
leaves it disarmed rather than restarting into the same image for ever. Both
boards' prebuilt bootloaders on espressif32@7.0.1 do have it.
SHA-256 is this repository's own rather than mbedtls', so the native tests hash
with the code the node runs. Costs about 2 kB: lighting 42.3% to 42.4% of its
slot.

Tested with pio test -e native (390 cases, including new test_sha256,
test_boot_commit and the digest and reboot cases), pio run for both nodes and
pio run -e host_sim. No hardware.

Spec section 11 corrected: state 2 is where a failed fetch lands, since a
successful one now reboots.

CCS-UF-6.

Closes the update path: the digest is checked before the boot slot moves, the node reboots into the new slot with the radio off, and the new image commits itself on the first CAN transmit that completes or restarts after 30 s so the bootloader can revert. Behaviour: - `FirmwareUpdater` hashes the image as it writes, compares it to the manifest and only then calls the sink's new `activate()`. A mismatch, a slot that will not close and a slot that will not switch all go through `Stage::Failed`, which is the one place CCS-UF-12 has to map. - `CanBus::transmitCompleted()` latches a transmit another node acknowledged. TWAI reads `TWAI_ALERT_TX_SUCCESS`, the MCP2515 reads `CANINTF` `TXnIF`, and SocketCAN reports nothing, since a write to a vcan succeeds with nobody listening. - `BootGuard` arms only when the bootloader reports the running image `PENDING_VERIFY`, so a bootloader without `CONFIG_BOOTLOADER_APP_ROLLBACK_ENABLE` leaves it disarmed rather than restarting into the same image for ever. Both boards' prebuilt bootloaders on espressif32@7.0.1 do have it. - SHA-256 is this repository's own rather than mbedtls', so the native tests hash with the code the node runs. Costs about 2 kB: lighting 42.3% to 42.4% of its slot. Tested with `pio test -e native` (390 cases, including new `test_sha256`, `test_boot_commit` and the digest and reboot cases), `pio run` for both nodes and `pio run -e host_sim`. No hardware. Spec section 11 corrected: state 2 is where a *failed* fetch lands, since a successful one now reboots. CCS-UF-6.

Claude added 4 commits

2026-09-19 14:03:03 +00:00

Add a streaming SHA-256 the native tests run too c70c86c027

Restart out of an image the bus never acknowledges 9ec727423e

Verify the digest, then switch the boot slot and reboot 0f4d4f9fd9

Say in the spec and CLAUDE.md that an update now lands

Build and test / build (pull_request) Successful in 3m52s

Details

Build and test / release (pull_request) Has been skipped

Details

09ee299f31

Claude reviewed

2026-09-19 14:11:15 +00:00

Claude left a comment

Digest-before-switch ordering, the 30 s arithmetic and the SHA-256 all check out (the hash matches Python for every length 0-300, sanitisers clean). Seven things below.

CLAUDE.md Outdated

					
				@ -163,0 +159,4 @@

				`DIAG`, CCT and heater paths. `ALARM_STATE` has a codec but no caller;

				`ALARM_ACK` has neither.

				The update path is whole: action 3 joins, fetches, writes the slot, verifies the

This paragraph is under "Not yet implemented" but four of its five lines describe what now exists. Only the missing piece belongs here — the SYS_WIFI_STATUS state for a failed update. Cut the rest.

Also worth checking against CCS-UF-12's scope: "every one of them goes through Stage::Failed" is true of fetch failures, but a rollback after the 30 s window leaves the old image up with no record that an update was attempted, so there is nothing for that ticket to map there.

This paragraph is under "Not yet implemented" but four of its five lines describe what now exists. Only the missing piece belongs here — the `SYS_WIFI_STATUS` state for a failed update. Cut the rest. Also worth checking against CCS-UF-12's scope: "every one of them goes through `Stage::Failed`" is true of fetch failures, but a rollback after the 30 s window leaves the old image up with no record that an update was attempted, so there is nothing for that ticket to map there.

docs/can-protocol.md Outdated

"a node that boots a bad image and then hangs" overclaims what this can do. BootGuard::loop is driven from Node::loop, so an image that genuinely hangs never reaches the deadline check and never restarts. What the 30 s window actually catches is an image that runs but cannot get on the bus. Reword, or say the task watchdog is what covers a hang.

"a node that boots a bad image and then hangs" overclaims what this can do. `BootGuard::loop` is driven from `Node::loop`, so an image that genuinely hangs never reaches the deadline check and never restarts. What the 30 s window actually catches is an image that runs but cannot get on the bus. Reword, or say the task watchdog is what covers a hang.

lib/node-runtime/src/can_bus_mcp2515.cpp Outdated

					
				@ -143,0 +155,4 @@

					if ((mcp->getInterrupts() & interruptTxSentMask) == 0) return;

					transmitCompleted_ = true;

					mcp->clearTXInterrupts();

clearTXInterrupts() achieves nothing: transmitCompleted_ is latched on the line above, so poll() returns early from here on and never reads CANINTF again. The flags re-set on the next frame and stay set for the life of the node. Drop the call.

`clearTXInterrupts()` achieves nothing: `transmitCompleted_` is latched on the line above, so `poll()` returns early from here on and never reads CANINTF again. The flags re-set on the next frame and stay set for the life of the node. Drop the call.

lib/node-runtime/src/firmware_sink_arduino.cpp Outdated

					
				@ -56,0 +56,4 @@

				// esp_ota_end() has to have run first, or the IDF refuses the partition it has

				// not finished verifying.

				bool ArduinoFirmwareSink::activate() {

					if (open_ || partition_ == nullptr) return false;

abort() leaves partition_ set, so after an aborted, partly written transfer the state is exactly !open_ && partition_ != nullptr — activate()'s whole guard. Nothing in the sink stops it switching the boot slot to a half-written slot; only FirmwareUpdater's call ordering does, and this class has no host tests at all.

Clear partition_ in abort() and gate activate() on a successful end() (a flag set in end() on ESP_OK) so the sink refuses rather than trusting its caller.

`abort()` leaves `partition_` set, so after an aborted, partly written transfer the state is exactly `!open_ && partition_ != nullptr` — activate()'s whole guard. Nothing in the sink stops it switching the boot slot to a half-written slot; only `FirmwareUpdater`'s call ordering does, and this class has no host tests at all. Clear `partition_` in `abort()` and gate `activate()` on a successful `end()` (a flag set in `end()` on ESP_OK) so the sink refuses rather than trusting its caller.

lib/node-runtime/src/firmware_updater.h Outdated

					
				@ -45,3 +46,3 @@

					uint8_t progressPercent() const override;

					Stage stage() const { return stage_; }

					// The two below say nothing until this is true.

"The two below" — stage() now sits between this and the two accessors it means. Move the comment down to slot() or drop it.

"The two below" — `stage()` now sits between this and the two accessors it means. Move the comment down to `slot()` or drop it.

lib/node-runtime/src/wifi_manager.cpp

					
				@ -91,0 +92,4 @@

					// section 11.

					if (boot_ != nullptr && updater_ != nullptr && updater_->installed()) {

						disable(node, now);

						boot_->restart();

The final SYS_WIFI_STATUS state 0 is lost on the lighting node. disable() reports off via node.send(), which on the TWAI backend only pushes onto its own TxQueue; poll() releases one frame per loop, and esp_restart() here runs first. On MCP2515 send() loads TXB0 over SPI so the frame does go out — the external part is unaffected by the reset.

So whether the bus sees the session close depends on which backend the node has. Defer the restart to a later loop once the queue has drained rather than calling it inline; one loop is not enough on TWAI if other frames are queued ahead of it.

The final `SYS_WIFI_STATUS` state 0 is lost on the lighting node. `disable()` reports off via `node.send()`, which on the TWAI backend only pushes onto its own `TxQueue`; `poll()` releases one frame per loop, and `esp_restart()` here runs first. On MCP2515 `send()` loads TXB0 over SPI so the frame does go out — the external part is unaffected by the reset. So whether the bus sees the session close depends on which backend the node has. Defer the restart to a later loop once the queue has drained rather than calling it inline; one loop is not enough on TWAI if other frames are queued ahead of it.

test/test_sha256/test_sha256.cpp

					
				@ -0,0 +44,4 @@

				// 56 bytes: the padding no longer fits in the block holding the last byte, which

				// is where a hash written from the spec usually goes wrong.

				void test_an_input_that_pushes_the_padding_into_another_block() {

No vector has a length of 63 mod 64, which is the one padding case where 0x80 exactly fills the block and the zero fill is empty. It is correct today (I checked every length 0-300 against Python), but it is the branch a future edit is most likely to break and nothing here would catch it. Add a 63-byte input.

No vector has a length of 63 mod 64, which is the one padding case where `0x80` exactly fills the block and the zero fill is empty. It is correct today (I checked every length 0-300 against Python), but it is the branch a future edit is most likely to break and nothing here would catch it. Add a 63-byte input.

rob added 1 commit

2026-09-19 14:14:25 +00:00

Refuse to activate an abandoned slot and reboot without a last frame

Build and test / build (pull_request) Successful in 49s

Details

Build and test / release (pull_request) Has been skipped

Details

bb67d8716f

Claude reviewed

2026-09-19 14:17:34 +00:00

Claude left a comment

All seven earlier points addressed. One new finding below; the closed_ state machine is sound — no begin/write/end/activate/abort sequence reaches activate() with an uncleanly closed slot, and none blocks a legitimate one.

All seven earlier points addressed. One new finding below; the `closed_` state machine is sound — no begin/write/end/activate/abort sequence reaches `activate()` with an uncleanly closed slot, and none blocks a legitimate one.

lib/node-runtime/src/wifi_manager.cpp Outdated

					
				@ -91,0 +92,4 @@

					// status frame on the way out, because a backend that queues one would not get

					// it onto the wire before the reset; the build identifier in the announce after

					// it is what says the update took. Spec section 11.

					if (boot_ != nullptr && updater_ != nullptr && updater_->installed()) {

Nothing clears the updater's Installed stage, so this condition stays true for the life of the object. If restart() returns — a host BootControl, or a fake — the next session is torn down the moment it starts: onFrame sets connecting, then this block runs in the same Node::loop and calls radio_.off() and restart() again.

I reproduced it against this commit with a plain action 1 join after the install: restartCalls=2, offCalls=2, state forced back to 0.

Add updateStarted_ to the guard — endSession() clears it inside this block, so it becomes strictly one-shot per session:

if (boot_ != nullptr && updateStarted_ && updater_ != nullptr && updater_->installed()) {

With that, the same probe gives restartCalls=1 and the later join survives, and all 50 wifi tests still pass. Unreachable on hardware since esp_restart() does not return, but the guard should not depend on that.

Nothing clears the updater's `Installed` stage, so this condition stays true for the life of the object. If `restart()` returns — a host `BootControl`, or a fake — the next session is torn down the moment it starts: `onFrame` sets `connecting`, then this block runs in the same `Node::loop` and calls `radio_.off()` and `restart()` again. I reproduced it against this commit with a plain action 1 join after the install: restartCalls=2, offCalls=2, state forced back to 0. Add `updateStarted_` to the guard — `endSession()` clears it inside this block, so it becomes strictly one-shot per session: ```cpp if (boot_ != nullptr && updateStarted_ && updater_ != nullptr && updater_->installed()) { ``` With that, the same probe gives restartCalls=1 and the later join survives, and all 50 wifi tests still pass. Unreachable on hardware since `esp_restart()` does not return, but the guard should not depend on that.

rob added 1 commit

2026-09-19 14:19:49 +00:00

Reboot only for the session that installed the image

Build and test / build (pull_request) Successful in 43s

Details

Build and test / release (pull_request) Has been skipped

Details

ed99371119

Claude reviewed

2026-09-19 14:20:54 +00:00

Claude left a comment

Clean — nothing outstanding. Confirmed the new test fails with the updateStarted_ term removed (Expected 1 Was 2), 392 tests pass, both nodes build, clang-format clean.

Agreed on keeping closed_ = false in begin(): abort()'s clearing is a side effect its name does not advertise, so the explicit reset next to partition_ earns the line.

Clean — nothing outstanding. Confirmed the new test fails with the `updateStarted_` term removed (Expected 1 Was 2), 392 tests pass, both nodes build, clang-format clean. Agreed on keeping `closed_ = false` in `begin()`: `abort()`'s clearing is a side effect its name does not advertise, so the explicit reset next to `partition_` earns the line.

Claude merged commit 011aeab7e4 into feat/firmware-updates

2026-09-19 14:24:31 +00:00

Claude deleted branch feat/verify-image-commit-on-healthy-bus

2026-09-19 14:24:31 +00:00

Claude referenced this pull request from a commit

2026-09-19 14:24:32 +00:00

Merge pull request 'Verify the image, then commit once the bus proves healthy' (#52) from feat/verify-image-commit-on-healthy-bus into feat/firmware-updates

Claude referenced this pull request

2026-09-19 14:56:00 +00:00

Report a failed update as SYS_WIFI_STATUS state 6 #54

Sign in to join this conversation.

No reviewers