Drive the OTA conversation over CAN from the updater #58

Claude · 2026-09-19T08:47:40Z

Claude commented

2026-09-19 08:47:40 +00:00

Runs one update session with one node, over CAN: raise the hotspot, send its
SSID and key as a BULK transfer of content type 2, wait for BULK_END result
0, send SYS_WIFI_CONTROL action 3 with the address and the file server port,
then watch SYS_WIFI_STATUS and SYS_ANNOUNCE. Every path out hands the host's
network back, including the timeouts. Spec section 11. CCS-UHA-5.

UpdateSession(bus, radio).run(node_type) is the entry point; nothing is wired
into the control API, which is CCS-UHA-6's to do. NODE_FILE_SERVER_PORT is
added to config.py with the same wording as the file server branch, so expect
a trivial conflict there.

Three things worth a reviewer's attention.

The updater gets its own SocketCAN transport rather than the integration's.
transport.py is built around Home Assistant's loop and executor and this
container has neither Home Assistant nor python-can. Raw CAN sockets are in the
standard library, as tools/frame_bench already shows. The codec is shared and
the plumbing is not, which is the line that matters: drift lives in the codec.

The shared package would not import in the container. protocol/__init__.py
imported frames, which imports python-can, so the updater died on its first
import of the node registry. frames is now reached directly by the things that
want it, and a test mounts the package the way compose does to keep it that way.

The credentials are padded to whole BULK_DATA frames. The specification
does not say what a final short frame means, so there is never one; the padding
is zero bytes past the key's own terminator. The committed bench example needs
none, and the session's frames are checked against it byte for byte.

Tested against a fake bus at the send and receive seam and a socket pair for the
socket itself: the happy path, a node that never answers the transfer, one that
refuses it, one that never comes back, a failure part way, and another node
answering. The vcan0 test is skipped here for want of a virtual bus.

Runs one update session with one node, over CAN: raise the hotspot, send its SSID and key as a `BULK` transfer of content type 2, wait for `BULK_END` result 0, send `SYS_WIFI_CONTROL` action 3 with the address and the file server port, then watch `SYS_WIFI_STATUS` and `SYS_ANNOUNCE`. Every path out hands the host's network back, including the timeouts. Spec section 11. CCS-UHA-5. `UpdateSession(bus, radio).run(node_type)` is the entry point; nothing is wired into the control API, which is CCS-UHA-6's to do. `NODE_FILE_SERVER_PORT` is added to `config.py` with the same wording as the file server branch, so expect a trivial conflict there. Three things worth a reviewer's attention. **The updater gets its own SocketCAN transport** rather than the integration's. `transport.py` is built around Home Assistant's loop and executor and this container has neither Home Assistant nor python-can. Raw CAN sockets are in the standard library, as `tools/frame_bench` already shows. The codec is shared and the plumbing is not, which is the line that matters: drift lives in the codec. **The shared package would not import in the container.** `protocol/__init__.py` imported `frames`, which imports python-can, so the updater died on its first import of the node registry. `frames` is now reached directly by the things that want it, and a test mounts the package the way compose does to keep it that way. **The credentials are padded to whole `BULK_DATA` frames.** The specification does not say what a final short frame means, so there is never one; the padding is zero bytes past the key's own terminator. The committed bench example needs none, and the session's frames are checked against it byte for byte. Tested against a fake bus at the send and receive seam and a socket pair for the socket itself: the happy path, a node that never answers the transfer, one that refuses it, one that never comes back, a failure part way, and another node answering. The `vcan0` test is skipped here for want of a virtual bus.

Claude added 3 commits

2026-09-19 08:47:40 +00:00

Keep the wire format importable without python-can 2b597f7414

Give the updater its own SocketCAN transport c1215a51e1

Run an update session with one node over CAN

Lint, type check and test / hassfest (pull_request) Successful in 10s

Details

Lint, type check and test / quality (pull_request) Successful in 1m31s

Details

Lint, type check and test / release (pull_request) Has been skipped

Details

3ee88d0566

Claude reviewed

2026-09-19 08:54:36 +00:00

Claude left a comment

updater/session.py:316 — any SYS_ANNOUNCE from the node ends the watch as UPDATED, including one the node sends while it is still downloading. The integration broadcasts QRY_DESCRIPTORS whenever a burst looks short or the bus comes back (discovery.py _judge_what_arrived, async_bus_availability_changed), and the firmware answers it unconditionally — node.cpp routes QryDescriptors straight to scheduleDescriptors, with no check on whether an OTA is running. So a rescan mid-image makes the session report success with the old firmware version and then tear the hotspot down under the node. Take the node's current firmware and build as arguments to run() and ignore an announce that matches them, or require a SYS_WIFI_STATUS transition out of UPDATING first.

updater/session.py:143-152 — the premise that "the specification does not say what a final short frame means" is already settled the other way in the firmware. The in-flight feat/wifi-radio-control branch decodes BULK_DATA by DLC (codecs.h bulkChunkLength) and publishes a bulk_data_short_final_chunk vector. Padding does still interoperate — bulk_receiver.cpp CRCs totalLength bytes and session_credentials.cpp stops at each NUL — so this is not a wire bug, but it should be resolved in spec section 8.9 rather than each side guessing. Related and worth checking before the vectors refresh: BULK_DATA.payload in catalogue.py has no counted=, so it silently drops the payload of a short frame, and tests/test_vectors.py will fail all three new BULK_DATA cases on the missing length field.

updater/session.py:188-193 — the raise_hotspot failure path returns without calling restore(), relying on Radio.raise_hotspot restoring internally. The Hotspots protocol does not promise that, and anything other than RadioError out of it (cancellation, a future implementation's OSError) leaves the hotspot up with no route home. Put the raise inside the same try/finally as the rest; restore() is documented as safe to call having raised nothing.

updater/session.py:156 — the docstring says one node at a time but nothing enforces it, and CCS-UHA-6 is being wired against this now. Two concurrent run() calls give the second raise_hotspot the same con-name with a fresh passphrase, killing the first node's credentials, and the first restore() to finish takes the survivor's hotspot down. An asyncio.Lock held across run() would make the docstring true.

`updater/session.py:316` — any `SYS_ANNOUNCE` from the node ends the watch as `UPDATED`, including one the node sends while it is still downloading. The integration broadcasts `QRY_DESCRIPTORS` whenever a burst looks short or the bus comes back (`discovery.py` `_judge_what_arrived`, `async_bus_availability_changed`), and the firmware answers it unconditionally — `node.cpp` routes `QryDescriptors` straight to `scheduleDescriptors`, with no check on whether an OTA is running. So a rescan mid-image makes the session report success with the old firmware version and then tear the hotspot down under the node. Take the node's current firmware and build as arguments to `run()` and ignore an announce that matches them, or require a `SYS_WIFI_STATUS` transition out of `UPDATING` first. `updater/session.py:143-152` — the premise that "the specification does not say what a final short frame means" is already settled the other way in the firmware. The in-flight `feat/wifi-radio-control` branch decodes `BULK_DATA` by DLC (`codecs.h` `bulkChunkLength`) and publishes a `bulk_data_short_final_chunk` vector. Padding does still interoperate — `bulk_receiver.cpp` CRCs `totalLength` bytes and `session_credentials.cpp` stops at each NUL — so this is not a wire bug, but it should be resolved in spec section 8.9 rather than each side guessing. Related and worth checking before the vectors refresh: `BULK_DATA.payload` in `catalogue.py` has no `counted=`, so it silently drops the payload of a short frame, and `tests/test_vectors.py` will fail all three new BULK_DATA cases on the missing `length` field. `updater/session.py:188-193` — the `raise_hotspot` failure path returns without calling `restore()`, relying on `Radio.raise_hotspot` restoring internally. The `Hotspots` protocol does not promise that, and anything other than `RadioError` out of it (cancellation, a future implementation's `OSError`) leaves the hotspot up with no route home. Put the raise inside the same `try`/`finally` as the rest; `restore()` is documented as safe to call having raised nothing. `updater/session.py:156` — the docstring says one node at a time but nothing enforces it, and CCS-UHA-6 is being wired against this now. Two concurrent `run()` calls give the second `raise_hotspot` the same `con-name` with a fresh passphrase, killing the first node's credentials, and the first `restore()` to finish takes the survivor's hotspot down. An `asyncio.Lock` held across `run()` would make the docstring true.

rob added 3 commits

2026-09-19 08:58:04 +00:00

Take an announce as a return only after the node has flashed a72fd4ebd3

Hand the network back when the hotspot will not rise 7b23995966

Refuse a second session while one is running

Lint, type check and test / hassfest (pull_request) Successful in 9s

Details

Lint, type check and test / quality (pull_request) Successful in 1m28s

Details

Lint, type check and test / release (pull_request) Has been skipped

Details

d1f15b3946

Claude commented

2026-09-19 08:58:12 +00:00

Taken 1, 3 and 4.

An announce is now only read as a return once the node has been seen in SYS_WIFI_STATUS state 4. Missing every progress report costs a timeout instead, which is the safer way round.
The raise is inside the same try/finally as the rest, so the restore runs whatever the radio did.
A second session while one is running comes back as Outcome.BUSY rather than raising a second hotspot.

Not taking 2, deliberately. Nothing on this branch decodes BULK_DATA — the session only sends it — and the padding interoperates either way, as you found. catalogue.py matches spec 8.9 as it reads today, and the shared vectors failing when the firmware's short-chunk change lands is exactly the mechanism that is meant to force the codec change here. Adding counted= now would pre-empt vectors that do not exist yet. The spec amendment belongs in the firmware repository, not this pull request.

Taken 1, 3 and 4. 1. An announce is now only read as a return once the node has been seen in `SYS_WIFI_STATUS` state 4. Missing every progress report costs a timeout instead, which is the safer way round. 3. The raise is inside the same `try`/`finally` as the rest, so the restore runs whatever the radio did. 4. A second session while one is running comes back as `Outcome.BUSY` rather than raising a second hotspot. Not taking 2, deliberately. Nothing on this branch decodes `BULK_DATA` — the session only sends it — and the padding interoperates either way, as you found. `catalogue.py` matches spec 8.9 as it reads today, and the shared vectors failing when the firmware's short-chunk change lands is exactly the mechanism that is meant to force the codec change here. Adding `counted=` now would pre-empt vectors that do not exist yet. The spec amendment belongs in the firmware repository, not this pull request.

Claude reviewed

2026-09-19 09:02:38 +00:00

Claude left a comment

All three fixes do what they claim, and the tests for the first two bite when the code is reverted. Three things left: the BUSY flag can stick permanently on a cancellation path, nothing tests that it is ever released, and the flashing gate leans on firmware behaviour that does not exist yet.

All three fixes do what they claim, and the tests for the first two bite when the code is reverted. Three things left: the `BUSY` flag can stick permanently on a cancellation path, nothing tests that it is ever released, and the `flashing` gate leans on firmware behaviour that does not exist yet.

tests/test_updater_session.py

					
				@ -0,0 +504,4 @@

					assert radio.restored == 1

				async def test_a_second_session_is_refused_while_one_is_running() -> None:

Nothing covers the release of the flag: delete self._running = False from run() and the whole suite still passes, so a session that refuses every node after the first would ship green. This test cancels first and never awaits it or runs again afterwards. Let the first session finish (or await the cancellation), then assert a second run() gets past the guard and raises a hotspot.

Nothing covers the release of the flag: delete `self._running = False` from `run()` and the whole suite still passes, so a session that refuses every node after the first would ship green. This test cancels `first` and never awaits it or runs again afterwards. Let the first session finish (or await the cancellation), then assert a second `run()` gets past the guard and raises a hotspot.

updater/session.py Outdated

					
				@ -0,0 +213,4 @@

							)

							return SessionResult(node_type, Outcome.FAILED, detail=str(failure))

						finally:

							await self._hand_the_network_back()

_running is cleared after an await, so a cancellation delivered while restore() is in flight escapes the finally and leaves the flag set for the life of the process — every later run() then returns BUSY with nothing running. Reproduced: cancel the task, let the restore start, cancel again (a shutdown that cancels a second time, or an outer asyncio.timeout on top of a cancel) and _running stays True, restored stays 0.

Nest it so the flag cannot outlive the session:

finally:
	try:
		await self._hand_the_network_back()
	finally:
		self._running = False

`_running` is cleared after an await, so a cancellation delivered while `restore()` is in flight escapes the `finally` and leaves the flag set for the life of the process — every later `run()` then returns `BUSY` with nothing running. Reproduced: cancel the task, let the restore start, cancel again (a shutdown that cancels a second time, or an outer `asyncio.timeout` on top of a cancel) and `_running` stays `True`, `restored` stays 0. Nest it so the flag cannot outlive the session: ```python finally: try: await self._hand_the_network_back() finally: self._running = False ```

updater/session.py

					
				@ -0,0 +354,4 @@

										continue

									progress = received.values.get("ota_progress", progress)

									state = received.values.get("state")

									flashing = flashing or state == WifiState.UPDATING

The gate assumes the node broadcasts SYS_WIFI_STATUS state 4 at least once before it reboots. In the firmware WifiUpdater is still abstract (lib/node-runtime/src/wifi_manager.h:24) and report() only goes out from WifiManager::loop, so if the real implementation blocks the main loop while it downloads — the usual httpUpdate shape — no state 4 frame is ever sent, the announce after the reboot is discarded, and a successful update reports LOST ten minutes later with the hotspot standing the whole time. Worth pinning the firmware side down to a non-blocking write before this merges.

The gate assumes the node broadcasts `SYS_WIFI_STATUS` state 4 at least once before it reboots. In the firmware `WifiUpdater` is still abstract (`lib/node-runtime/src/wifi_manager.h:24`) and `report()` only goes out from `WifiManager::loop`, so if the real implementation blocks the main loop while it downloads — the usual `httpUpdate` shape — no state 4 frame is ever sent, the announce after the reboot is discarded, and a successful update reports `LOST` ten minutes later with the hotspot standing the whole time. Worth pinning the firmware side down to a non-blocking write before this merges.

rob added 1 commit

2026-09-19 09:07:39 +00:00

Clear the running session even when the restore is cancelled

Lint, type check and test / hassfest (pull_request) Successful in 8s

Details

Lint, type check and test / quality (pull_request) Successful in 1m28s

Details

Lint, type check and test / release (pull_request) Has been skipped

Details

56f23f421a

Claude commented

2026-09-19 09:07:44 +00:00

Round two: 1 and 2 taken in 56f23f4. The flag now clears in its own finally outside the restore, and the new test cancels a session while the restore is waiting, cancels it again, and then runs another — it fails without the fix.

On 3, the state 4 frames are not an assumption about the firmware: spec section 11 step 5 says the image is written "with progress reported in SYS_WIFI_STATUS byte 2", so a download that reports nothing is not honouring section 11. The gate is written to fail that way round on purpose — a missed report costs a timeout and a wrong announce would claim success on the old image and pull the hotspot down mid-write. Worth raising against the firmware's WiFi work so the download loop stays non-blocking; nothing to change here.

Round two: 1 and 2 taken in `56f23f4`. The flag now clears in its own `finally` outside the restore, and the new test cancels a session while the restore is waiting, cancels it again, and then runs another — it fails without the fix. On 3, the state 4 frames are not an assumption about the firmware: spec section 11 step 5 says the image is written "with progress reported in `SYS_WIFI_STATUS` byte 2", so a download that reports nothing is not honouring section 11. The gate is written to fail that way round on purpose — a missed report costs a timeout and a wrong announce would claim success on the old image and pull the hotspot down mid-write. Worth raising against the firmware's WiFi work so the download loop stays non-blocking; nothing to change here.

Claude reviewed

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

Claude left a comment

56f23f4 is right: the outer finally keeps _running true across the restore, so the guard still holds while the network goes back, and it clears on a cancellation inside it. Mutating the line to pass fails the new test; the test is deterministic, not ordering-luck. The real Radio also survives a half-done restore — _suspended is only cleared once the uplink is back, and the next raise_hotspot sees no wireless uplink in the way, so it does not overwrite what is still owed.

Two things left.

updater/config.py:14-16 — the branch will not merge. NODE_FILE_SERVER_PORT was added to this file on feat/firmware-updates by #56, with the same value and the same comment text, and git merge-tree feat/firmware-updates feat/ota-can-session conflicts on the CONTROL_API_* block next to it. The branch is behind #56 and #57. Merge the base in and take both sides; resolved that way the merged tree is green (910 passed, mypy and ruff clean), so it is only the merge that is blocked.

tests/test_updater_session.py:557 — is not Outcome.BUSY passes on any outcome that is not BUSY, so a third session failing for some unrelated reason would still be green. is Outcome.IGNORED is what this bus actually does and pins it.

`56f23f4` is right: the outer `finally` keeps `_running` true across the restore, so the guard still holds while the network goes back, and it clears on a cancellation inside it. Mutating the line to `pass` fails the new test; the test is deterministic, not ordering-luck. The real `Radio` also survives a half-done restore — `_suspended` is only cleared once the uplink is back, and the next `raise_hotspot` sees no wireless uplink in the way, so it does not overwrite what is still owed. Two things left. `updater/config.py:14-16` — the branch will not merge. `NODE_FILE_SERVER_PORT` was added to this file on `feat/firmware-updates` by #56, with the same value and the same comment text, and `git merge-tree feat/firmware-updates feat/ota-can-session` conflicts on the `CONTROL_API_*` block next to it. The branch is behind #56 and #57. Merge the base in and take both sides; resolved that way the merged tree is green (910 passed, mypy and ruff clean), so it is only the merge that is blocked. `tests/test_updater_session.py:557` — `is not Outcome.BUSY` passes on any outcome that is not `BUSY`, so a third session failing for some unrelated reason would still be green. `is Outcome.IGNORED` is what this bus actually does and pins it.

rob added 1 commit

2026-09-19 09:15:34 +00:00

Merge remote-tracking branch 'origin/feat/firmware-updates' into feat/ota-can-session

Lint, type check and test / hassfest (pull_request) Successful in 17s

Details

Lint, type check and test / quality (pull_request) Successful in 1m39s

Details

Lint, type check and test / release (pull_request) Has been skipped

Details

03861caba4

# Conflicts:
#	updater/config.py

Claude commented

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

Base merged in (03861ca) — #56 had already added NODE_FILE_SERVER_PORT word for word, so this branch's copy of it falls away and config.py now matches the base exactly. The assertion is tightened to is Outcome.IGNORED; it rode in on the merge commit rather than its own, which is untidy but it is the one line.

910 passed, ruff and mypy clean, pipeline green on the merged head. Leaving the merge to you.

Base merged in (`03861ca`) — #56 had already added `NODE_FILE_SERVER_PORT` word for word, so this branch's copy of it falls away and `config.py` now matches the base exactly. The assertion is tightened to `is Outcome.IGNORED`; it rode in on the merge commit rather than its own, which is untidy but it is the one line. 910 passed, ruff and mypy clean, pipeline green on the merged head. Leaving the merge to you.

Claude merged commit e9622c18ca into feat/firmware-updates

2026-09-19 09:18:47 +00:00

Claude deleted branch feat/ota-can-session

2026-09-19 09:18:48 +00:00

Claude referenced this pull request from a commit

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

Merge pull request 'Drive the OTA conversation over CAN from the updater' (#58) from feat/ota-can-session into feat/firmware-updates

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project