Surface which uplink case the Pi is in before an install #68

Merged
Claude merged 7 commits from feat/uplink-before-an-install into feat/firmware-updates 2026-09-19 23:25:13 +00:00
Collaborator

Pressing install can cost the Home Assistant connection, and nothing said so in advance. radio.py already worked the case out; this reports it through GET /api/status and shows it on the bridge.

/api/status gains a radio block: uplink (ethernet, wireless or none), the connection and device it is on, and drops_uplink. Only a wireless uplink on the hotspot's own radio comes down, so drops_uplink is the fact worth reading — one on another radio is wireless and costs nothing. The predicate raise_hotspot already used is now shared rather than written twice. nmcli runs on the service loop and the API answers on threads, so UplinkReport hands the question over the way BusLink hands over an update. No NetworkManager means nulls, not a guess: "ethernet" would read as a promise that an install costs nothing.

In the integration it is one sensor on the bridge device, sensor.bridge_uplink, beside the check button. It is a property of the Pi rather than of any node, so it is not on each node's update entity — that would repeat one fact per controller and make it look like the controller's. The state is the case; update_drops_uplink and connection are attributes, so "which network am I about to lose" is answerable. No entity category, like the check button: the two things you want before an install sit together rather than one of them under Diagnostic. It is information and never a gate — someone in a field may want the update anyway, and that is theirs to decide.

The updater is absent as often as not, and that degrades the way everything else does: unknown, never an error and never a broken entity. An older updater with no radio in its status reads the same. The FirmwareUpdates coordinator asks the two questions independently, so one failing does not take the other's answer with it. The integration reads nothing of the host's network itself.

Tested: all three cases plus unknown on both sides — radio.py through its fake nmcli, the API through ControlApi, the service end to end through a request thread, and the integration through aioclient_mock. The status route is answered by an autouse fixture in tests/test_firmware_updates.py, because the first mock registered for a URL is the one that answers.

Closes CCS-UHA-12.

Pressing install can cost the Home Assistant connection, and nothing said so in advance. `radio.py` already worked the case out; this reports it through `GET /api/status` and shows it on the bridge. `/api/status` gains a `radio` block: `uplink` (`ethernet`, `wireless` or `none`), the connection and device it is on, and `drops_uplink`. Only a wireless uplink on the hotspot's own radio comes down, so `drops_uplink` is the fact worth reading — one on another radio is `wireless` and costs nothing. The predicate `raise_hotspot` already used is now shared rather than written twice. nmcli runs on the service loop and the API answers on threads, so `UplinkReport` hands the question over the way `BusLink` hands over an update. No NetworkManager means nulls, not a guess: "ethernet" would read as a promise that an install costs nothing. In the integration it is one sensor on the bridge device, `sensor.bridge_uplink`, beside the check button. It is a property of the Pi rather than of any node, so it is not on each node's update entity — that would repeat one fact per controller and make it look like the controller's. The state is the case; `update_drops_uplink` and `connection` are attributes, so "which network am I about to lose" is answerable. No entity category, like the check button: the two things you want before an install sit together rather than one of them under Diagnostic. It is information and never a gate — someone in a field may want the update anyway, and that is theirs to decide. The updater is absent as often as not, and that degrades the way everything else does: unknown, never an error and never a broken entity. An older updater with no `radio` in its status reads the same. The `FirmwareUpdates` coordinator asks the two questions independently, so one failing does not take the other's answer with it. The integration reads nothing of the host's network itself. Tested: all three cases plus unknown on both sides — `radio.py` through its fake nmcli, the API through `ControlApi`, the service end to end through a request thread, and the integration through `aioclient_mock`. The status route is answered by an autouse fixture in `tests/test_firmware_updates.py`, because the first mock registered for a URL is the one that answers. Closes CCS-UHA-12.
Show what an install costs the connection on the bridge
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 8s
Lint, type check and test / quality (pull_request) Successful in 4m44s
Lint, type check and test / release (pull_request) Has been skipped
a5156862aa
Claude left a comment

Three points, all on the integration side. The updater side and the _in_the_way share look right, and ruff, mypy and the suite are green.

Three points, all on the integration side. The updater side and the `_in_the_way` share look right, and ruff, mypy and the suite are green.
@ -152,0 +208,4 @@
return UplinkCase(
kind=Uplink(radio["uplink"]),
connection=connection if isinstance(connection, str) else None,
drops_uplink=bool(radio.get("drops_uplink")),
Author
Collaborator

bool(...) turns a missing or null drops_uplink into False, i.e. "an install costs nothing" — the one wrong way to be wrong here, and the opposite of how connection two lines down is handled. Either treat a non-bool as unknown (drop the whole case to None, as a bad uplink already does) or carry it as bool | None.

`bool(...)` turns a missing or null `drops_uplink` into False, i.e. "an install costs nothing" — the one wrong way to be wrong here, and the opposite of how `connection` two lines down is handled. Either treat a non-bool as unknown (drop the whole case to `None`, as a bad `uplink` already does) or carry it as `bool | None`.
@ -241,0 +301,4 @@
async def _async_update_data(self) -> Offered:
"""Ask what is cached and what installing it would cost."""
return Offered(
versions=await self._asked(self.client.async_offered_versions, {}),
Author
Collaborator

The PR says the two questions are asked independently so one failing does not take the other's answer. Nothing pins it — collapsing both into a single try around the two calls passes the whole suite. Add a test where /api/status raises and /api/nodes answers, asserting the offered version still shows.

The PR says the two questions are asked independently so one failing does not take the other's answer. Nothing pins it — collapsing both into a single `try` around the two calls passes the whole suite. Add a test where `/api/status` raises and `/api/nodes` answers, asserting the offered version still shows.
@ -1513,0 +1634,4 @@
assert uplink_shown(hass) == ("ethernet", False)
async def test_the_pi_on_wifi_says_an_install_costs_the_connection(
Author
Collaborator

No integration test covers a wireless uplink with drops_uplink: false — the wireless-on-another-radio case. Replacing drops_uplink=bool(radio.get("drops_uplink")) in firmware.py with Uplink(radio["uplink"]) is Uplink.WIRELESS passes all 63 tests here. That is the one distinction the sensor exists to make, so it should be pinned: add a case with uplink: "wireless", device: "wlan1", drops_uplink: false asserting the sensor reads wireless with the attribute False.

No integration test covers a `wireless` uplink with `drops_uplink: false` — the wireless-on-another-radio case. Replacing `drops_uplink=bool(radio.get("drops_uplink"))` in `firmware.py` with `Uplink(radio["uplink"]) is Uplink.WIRELESS` passes all 63 tests here. That is the one distinction the sensor exists to make, so it should be pinned: add a case with `uplink: "wireless"`, `device: "wlan1"`, `drops_uplink: false` asserting the sensor reads `wireless` with the attribute False.
Take the review: a cost left unsaid is unknown, not free
Some checks failed
Lint, type check and test / hassfest (pull_request) Successful in 9s
Lint, type check and test / quality (pull_request) Failing after 30s
Lint, type check and test / release (pull_request) Has been skipped
80e6ec3243
Take the review: pin the two cases the tests walked past
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 9s
Lint, type check and test / quality (pull_request) Successful in 1m41s
Lint, type check and test / release (pull_request) Has been skipped
8cbf7309d3
Claude left a comment

Both earlier points are properly pinned now. Two things left.

README.md is not updated. Every other bridge entity is described there — the check button has its own paragraph at line 109 — and past rounds on this stack asked for exactly that ("say in the README what the firmware entity shows"). A user-facing entity that is the whole point of the ticket should get a couple of lines: what it reads, what update_drops_uplink means, and that it is information rather than a gate.

Both earlier points are properly pinned now. Two things left. `README.md` is not updated. Every other bridge entity is described there — the check button has its own paragraph at line 109 — and past rounds on this stack asked for exactly that ("say in the README what the firmware entity shows"). A user-facing entity that is the whole point of the ticket should get a couple of lines: what it reads, what `update_drops_uplink` means, and that it is information rather than a gate.
@ -189,0 +243,4 @@
return None
return {"update_drops_uplink": case.drops_uplink, "connection": case.connection}
async def async_added_to_hass(self) -> None:
Author
Collaborator

The sensor adds a listener but never asks for a first refresh, so it depends on some other entity doing it. CampervanFirmwareUpdate.async_added_to_hass calls async_request_refresh, which is the only reason the tests here see a value — they all announce a node first. With no node announced the sensor reads unknown for up to fifteen minutes even though the updater is answering: set up the entry with /api/status returning a wireless radio and no announce, and uplink_shown gives ('unknown', None).

That is also the van whose bus will not come up, which test_the_uplink_is_there_with_the_bus_down covers — it passes on unknown, so nothing catches this. And even on a healthy van the reading someone checks before pressing install can be a poll old. Add await self.coordinator.firmware.async_request_refresh() here, as the update entity does.

The sensor adds a listener but never asks for a first refresh, so it depends on some other entity doing it. `CampervanFirmwareUpdate.async_added_to_hass` calls `async_request_refresh`, which is the only reason the tests here see a value — they all `announce` a node first. With no node announced the sensor reads `unknown` for up to fifteen minutes even though the updater is answering: set up the entry with `/api/status` returning a wireless radio and no `announce`, and `uplink_shown` gives `('unknown', None)`. That is also the van whose bus will not come up, which `test_the_uplink_is_there_with_the_bus_down` covers — it passes on `unknown`, so nothing catches this. And even on a healthy van the reading someone checks before pressing install can be a poll old. Add `await self.coordinator.firmware.async_request_refresh()` here, as the update entity does.
Take the review: ask before a node has ever announced
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 9s
Lint, type check and test / quality (pull_request) Successful in 1m42s
Lint, type check and test / release (pull_request) Has been skipped
52b600f748
Claude left a comment

Three points, one of them worth acting on before merge.

**custom_components/campervan/sensor.py:255** — await self.coordinator.firmware.async_refresh()inasync_added_to_hassholds up config entry setup.EntityPlatform._async_add_entitiesawaits each entity's add in sequence, andinit.py:96awaitsasync_forward_entry_setups, so setting up the integration now waits on two sequential updater requests at REQUEST_TIMEOUT(10s each). A refused loopback connection is instant, but the 10s is documented for exactly the case that bites here — the updater container still starting alongside Home Assistant — so a restart can stall sensor setup for up to 20s. Fire it rather than await it:self.coordinator.firmware.config_entry.async_create_background_task(...), or hass.async_create_task. It still bypasses the debouncer, and test_the_uplink_shows_before_any_node_has_announcedalready ends onasync_block_till_done(). (Separately, _async_update_data` could gather the two calls instead of awaiting them in turn.)

**updater/radio.py:430** — nothing exercises REPORT_TIMEOUT. Drop the timeout argument entirely and the suite still passes, while a request thread would then block forever on a loop that never gets round to the coroutine. A test with a loop busy past the deadline would pin both the value and TimeoutError` staying in the handler.

**README.md:120** — the three cases named there (wire / nothing / wlan0) are not the three states the sensor shows (ethernet/none/wireless), and "only the last costs anything" reads as wireless implying a cost. A wireless uplink on a second radio shows wirelesswithupdate_drops_uplink: false—test_a_wireless_uplink_off_the_hotspots_radio_costs_nothing` covers it. Say the attribute is the answer, not the state.

Three points, one of them worth acting on before merge. **`custom_components/campervan/sensor.py:255** — `await self.coordinator.firmware.async_refresh()` in `async_added_to_hass` holds up config entry setup. `EntityPlatform._async_add_entities` awaits each entity's add in sequence, and `__init__.py:96` awaits `async_forward_entry_setups`, so setting up the integration now waits on two sequential updater requests at `REQUEST_TIMEOUT` (10s each). A refused loopback connection is instant, but the 10s is documented for exactly the case that bites here — the updater container still starting alongside Home Assistant — so a restart can stall sensor setup for up to 20s. Fire it rather than await it: `self.coordinator.firmware.config_entry.async_create_background_task(...)`, or `hass.async_create_task`. It still bypasses the debouncer, and `test_the_uplink_shows_before_any_node_has_announced` already ends on `async_block_till_done()`. (Separately, `_async_update_data` could gather the two calls instead of awaiting them in turn.) **`updater/radio.py:430** — nothing exercises `REPORT_TIMEOUT`. Drop the timeout argument entirely and the suite still passes, while a request thread would then block forever on a loop that never gets round to the coroutine. A test with a loop busy past the deadline would pin both the value and `TimeoutError` staying in the handler. **`README.md:120** — the three cases named there (wire / nothing / `wlan0`) are not the three states the sensor shows (`ethernet` / `none` / `wireless`), and "only the last costs anything" reads as wireless implying a cost. A wireless uplink on a second radio shows `wireless` with `update_drops_uplink: false` — `test_a_wireless_uplink_off_the_hotspots_radio_costs_nothing` covers it. Say the attribute is the answer, not the state.
@ -117,6 +117,19 @@ the van's uplink could not reach, and an image offered but not fetched whole
are each reported as themselves. The last two especially, because either one
otherwise looks exactly like nothing new having been published.
Beside the button is an uplink sensor, because an install can cost the very
Author
Collaborator

The three cases named here (wire / nothing / wlan0) are not the three states the sensor shows (ethernet / none / wireless), and "only the last costs anything" reads as wireless implying a cost. A wireless uplink on a second radio shows wireless with update_drops_uplink: false. Point at the attribute as the answer rather than the state.

The three cases named here (wire / nothing / `wlan0`) are not the three states the sensor shows (`ethernet` / `none` / `wireless`), and "only the last costs anything" reads as wireless implying a cost. A wireless uplink on a second radio shows `wireless` with `update_drops_uplink: false`. Point at the attribute as the answer rather than the state.
@ -189,0 +252,4 @@
# Nothing else would ask on a bus nothing announces on, and that is the
# van most likely to be updated. Asked for outright rather than
# requested: the debounced request would spend its immediate turn here
# and hold back the one an update entity makes as its node arrives.
Author
Collaborator

Awaited here, this holds up config entry setup: EntityPlatform._async_add_entities awaits each add in turn and __init__.py:96 awaits the forward, so setup now waits on two sequential updater requests at REQUEST_TIMEOUT (10s each). The 10s is documented for a slow updater start, which is exactly the restart case where both containers come up together. Schedule it instead — config_entry.async_create_background_task or hass.async_create_task — which still bypasses the debouncer, and the new test already ends on async_block_till_done().

Awaited here, this holds up config entry setup: `EntityPlatform._async_add_entities` awaits each add in turn and `__init__.py:96` awaits the forward, so setup now waits on two sequential updater requests at `REQUEST_TIMEOUT` (10s each). The 10s is documented for a slow updater start, which is exactly the restart case where both containers come up together. Schedule it instead — `config_entry.async_create_background_task` or `hass.async_create_task` — which still bypasses the debouncer, and the new test already ends on `async_block_till_done()`.
@ -351,0 +427,4 @@
asking.close()
return None
try:
return waiting.result(REPORT_TIMEOUT.total_seconds())
Author
Collaborator

REPORT_TIMEOUT is untested: remove the argument and the suite still passes, while a request thread would block forever on a busy loop. A test with the loop occupied past the deadline would pin the value and keep TimeoutError in the handler below.

`REPORT_TIMEOUT` is untested: remove the argument and the suite still passes, while a request thread would block forever on a busy loop. A test with the loop occupied past the deadline would pin the value and keep `TimeoutError` in the handler below.
Take the review: setting up must not wait on the updater
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 9s
Lint, type check and test / quality (pull_request) Successful in 1m41s
Lint, type check and test / release (pull_request) Has been skipped
4029a0e038
Author
Collaborator

Third round taken in 4029a0e:

  • The refresh is scheduled rather than awaited, so setting the entry up never waits on the updater. The undebounced call stays, and the comment now says both why it is not the debounced one and why it is not awaited.
  • REPORT_TIMEOUT is pinned by a test where nmcli dawdles past it; dropping the argument from waiting.result() now fails.
  • The README paragraph names the sensor's own states and makes update_drops_uplink, not wireless, the answer to what it costs.

One suggestion not taken: gathering the two calls in _async_update_data. Nothing waits on that refresh any more, the poll is a quarter of an hour apart, and asking in sequence keeps the two answers plainly independent — which is the property the earlier round asked for a test for. Happy to change it if you still think the concurrency is worth it.

Third round taken in 4029a0e: - The refresh is scheduled rather than awaited, so setting the entry up never waits on the updater. The undebounced call stays, and the comment now says both why it is not the debounced one and why it is not awaited. - `REPORT_TIMEOUT` is pinned by a test where nmcli dawdles past it; dropping the argument from `waiting.result()` now fails. - The README paragraph names the sensor's own states and makes `update_drops_uplink`, not `wireless`, the answer to what it costs. One suggestion not taken: gathering the two calls in `_async_update_data`. Nothing waits on that refresh any more, the poll is a quarter of an hour apart, and asking in sequence keeps the two answers plainly independent — which is the property the earlier round asked for a test for. Happy to change it if you still think the concurrency is worth it.
Claude left a comment

All three of the last round's points are taken, and nothing here blocks merge. The sequential _async_update_data is fine as argued — nothing waits on it now. Four minor things, none of which need to happen before merge.

All three of the last round's points are taken, and nothing here blocks merge. The sequential `_async_update_data` is fine as argued — nothing waits on it now. Four minor things, none of which need to happen before merge.
README.md Outdated
@ -120,0 +123,4 @@
traffic while that stands: `ethernet`, `wireless` or `none`. Whether it costs
anything is `update_drops_uplink` beside it rather than the state, because only
an uplink on the radio the access point wants has to come down — a wire, a
second radio, and nothing at all all cost nothing. When one does come down,
Author
Collaborator

"nothing at all all cost nothing" reads as a typo on the way past. "a wire, a second radio, or nothing at all costs nothing".

"nothing at all all cost nothing" reads as a typo on the way past. "a wire, a second radio, or nothing at all costs nothing".
@ -109,6 +110,47 @@ class CheckOutcome:
failed: tuple[str, ...]
class Uplink(StrEnum):
Author
Collaborator

Naming: this mirrors updater.radio.UplinkKind, but updater.radio.Uplink is a different type (device/connection/wireless). Two names swapped across the pair of implementations is the sort of thing the vocabulary test exists to stop. Calling it UplinkKind here would make the mirror obvious and free the name.

Naming: this mirrors `updater.radio.UplinkKind`, but `updater.radio.Uplink` is a different type (device/connection/wireless). Two names swapped across the pair of implementations is the sort of thing the vocabulary test exists to stop. Calling it `UplinkKind` here would make the mirror obvious and free the name.
@ -189,0 +256,4 @@
# Scheduled rather than awaited, because setting the entry up must not
# wait on the updater — a Pi restart brings both containers up at once,
# and one still starting has ten seconds to answer.
self.hass.async_create_task(
Author
Collaborator

The fix itself is not pinned: change this back to await self.coordinator.firmware.async_refresh() and the whole suite still passes, because every test ends on async_block_till_done() either way. Every other point in this review loop got a test; this one is the regression that would be silent. A status route held on an asyncio.Event would do it — assert async_setup returns while the route is still blocked, then release and block till done.

The fix itself is not pinned: change this back to `await self.coordinator.firmware.async_refresh()` and the whole suite still passes, because every test ends on `async_block_till_done()` either way. Every other point in this review loop got a test; this one is the regression that would be silent. A status route held on an `asyncio.Event` would do it — assert `async_setup` returns while the route is still blocked, then release and block till done.
@ -351,0 +428,4 @@
return None
try:
return waiting.result(REPORT_TIMEOUT.total_seconds())
except RadioError, TimeoutError, CancelledError:
Author
Collaborator

except RadioError, TimeoutError, CancelledError: — valid on 3.14 (PEP 758) and the target is py314, but it is the only unparenthesised except tuple in the repo; transport.py, updater/firmware.py and custom_components/campervan/firmware.py all use parentheses. Match them.

`except RadioError, TimeoutError, CancelledError:` — valid on 3.14 (PEP 758) and the target is `py314`, but it is the only unparenthesised except tuple in the repo; `transport.py`, `updater/firmware.py` and `custom_components/campervan/firmware.py` all use parentheses. Match them.
Take the review: pin the scheduling, and name what it mirrors
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 9s
Lint, type check and test / quality (pull_request) Successful in 1m42s
Lint, type check and test / release (pull_request) Has been skipped
f864e59da8
Author
Collaborator

Fourth round taken in f864e59:

  • The scheduling is pinned. test_setting_the_entry_up_does_not_wait_on_the_updater holds /api/status on an event and asserts setup returns anyway, then that the answer catches up once it is let go. Putting await back fails it.
  • Uplink is UplinkKind in the integration too, so it is named after the thing it mirrors.
  • The README line reads properly now.

The unparenthesised except is not a style slip: ruff format strips those parentheses on py314, and CI runs ruff format --check. The repo already reads that way wherever there is no as binding — updater/firmware.py:239 and :494, tools/frame_bench/examples.py:61. Parenthesised ones all bind a name, which the formatter requires parentheses for.

Fourth round taken in f864e59: - The scheduling is pinned. `test_setting_the_entry_up_does_not_wait_on_the_updater` holds `/api/status` on an event and asserts setup returns anyway, then that the answer catches up once it is let go. Putting `await` back fails it. - `Uplink` is `UplinkKind` in the integration too, so it is named after the thing it mirrors. - The README line reads properly now. The unparenthesised `except` is not a style slip: `ruff format` strips those parentheses on `py314`, and CI runs `ruff format --check`. The repo already reads that way wherever there is no `as` binding — `updater/firmware.py:239` and `:494`, `tools/frame_bench/examples.py:61`. Parenthesised ones all bind a name, which the formatter requires parentheses for.
Claude left a comment

Nothing left to act on. The scheduling test bites (restoring the await fails it), the missing-cost guard bites, and lint, format, mypy and the suite are green on py3.14.

Nothing left to act on. The scheduling test bites (restoring the `await` fails it), the missing-cost guard bites, and lint, format, mypy and the suite are green on py3.14.
Claude merged commit 0b84e91f1b into feat/firmware-updates 2026-09-19 23:25:13 +00:00
Claude deleted branch feat/uplink-before-an-install 2026-09-19 23:25:13 +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/CampervanHomeAssistant!68
No description provided.