Surface which uplink case the Pi is in before an install #68
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/uplink-before-an-install"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Pressing install can cost the Home Assistant connection, and nothing said so in advance.
radio.pyalready worked the case out; this reports it throughGET /api/statusand shows it on the bridge./api/statusgains aradioblock:uplink(ethernet,wirelessornone), the connection and device it is on, anddrops_uplink. Only a wireless uplink on the hotspot's own radio comes down, sodrops_uplinkis the fact worth reading — one on another radio iswirelessand costs nothing. The predicateraise_hotspotalready used is now shared rather than written twice. nmcli runs on the service loop and the API answers on threads, soUplinkReporthands the question over the wayBusLinkhands 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_uplinkandconnectionare 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
radioin its status reads the same. TheFirmwareUpdatescoordinator 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.pythrough its fake nmcli, the API throughControlApi, the service end to end through a request thread, and the integration throughaioclient_mock. The status route is answered by an autouse fixture intests/test_firmware_updates.py, because the first mock registered for a URL is the one that answers.Closes CCS-UHA-12.
Three points, all on the integration side. The updater side and the
_in_the_wayshare 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")),bool(...)turns a missing or nulldrops_uplinkinto False, i.e. "an install costs nothing" — the one wrong way to be wrong here, and the opposite of howconnectiontwo lines down is handled. Either treat a non-bool as unknown (drop the whole case toNone, as a baduplinkalready does) or carry it asbool | 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, {}),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
tryaround the two calls passes the whole suite. Add a test where/api/statusraises and/api/nodesanswers, 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(No integration test covers a
wirelessuplink withdrops_uplink: false— the wireless-on-another-radio case. Replacingdrops_uplink=bool(radio.get("drops_uplink"))infirmware.pywithUplink(radio["uplink"]) is Uplink.WIRELESSpasses all 63 tests here. That is the one distinction the sensor exists to make, so it should be pinned: add a case withuplink: "wireless",device: "wlan1",drops_uplink: falseasserting the sensor readswirelesswith the attribute False.Both earlier points are properly pinned now. Two things left.
README.mdis 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, whatupdate_drops_uplinkmeans, and that it is information rather than a gate.@ -189,0 +243,4 @@return Nonereturn {"update_drops_uplink": case.drops_uplink, "connection": case.connection}async def async_added_to_hass(self) -> None: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_hasscallsasync_request_refresh, which is the only reason the tests here see a value — they allannouncea node first. With no node announced the sensor readsunknownfor up to fifteen minutes even though the updater is answering: set up the entry with/api/statusreturning a wireless radio and noannounce, anduplink_showngives('unknown', None).That is also the van whose bus will not come up, which
test_the_uplink_is_there_with_the_bus_downcovers — it passes onunknown, so nothing catches this. And even on a healthy van the reading someone checks before pressing install can be a poll old. Addawait self.coordinator.firmware.async_request_refresh()here, as the update entity does.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 atREQUEST_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(...), orhass.async_create_task. It still bypasses the debouncer, andtest_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 exercisesREPORT_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 andTimeoutError` 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 showswirelesswithupdate_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 wholeare each reported as themselves. The last two especially, because either oneotherwise looks exactly like nothing new having been published.Beside the button is an uplink sensor, because an install can cost the veryThe 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 showswirelesswithupdate_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.Awaited here, this holds up config entry setup:
EntityPlatform._async_add_entitiesawaits each add in turn and__init__.py:96awaits the forward, so setup now waits on two sequential updater requests atREQUEST_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_taskorhass.async_create_task— which still bypasses the debouncer, and the new test already ends onasync_block_till_done().@ -351,0 +427,4 @@asking.close()return Nonetry:return waiting.result(REPORT_TIMEOUT.total_seconds())REPORT_TIMEOUTis 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 keepTimeoutErrorin the handler below.Third round taken in
4029a0e:REPORT_TIMEOUTis pinned by a test where nmcli dawdles past it; dropping the argument fromwaiting.result()now fails.update_drops_uplink, notwireless, 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.All three of the last round's points are taken, and nothing here blocks merge. The sequential
_async_update_datais fine as argued — nothing waits on it now. Four minor things, none of which need to happen before merge.@ -120,0 +123,4 @@traffic while that stands: `ethernet`, `wireless` or `none`. Whether it costsanything is `update_drops_uplink` beside it rather than the state, because onlyan uplink on the radio the access point wants has to come down — a wire, asecond radio, and nothing at all all cost nothing. When one does come down,"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):Naming: this mirrors
updater.radio.UplinkKind, butupdater.radio.Uplinkis 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 itUplinkKindhere 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(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 onasync_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 anasyncio.Eventwould do it — assertasync_setupreturns while the route is still blocked, then release and block till done.@ -351,0 +428,4 @@return Nonetry:return waiting.result(REPORT_TIMEOUT.total_seconds())except RadioError, TimeoutError, CancelledError:except RadioError, TimeoutError, CancelledError:— valid on 3.14 (PEP 758) and the target ispy314, but it is the only unparenthesised except tuple in the repo;transport.py,updater/firmware.pyandcustom_components/campervan/firmware.pyall use parentheses. Match them.Fourth round taken in
f864e59:test_setting_the_entry_up_does_not_wait_on_the_updaterholds/api/statuson an event and asserts setup returns anyway, then that the answer catches up once it is let go. Puttingawaitback fails it.UplinkisUplinkKindin the integration too, so it is named after the thing it mirrors.The unparenthesised
exceptis not a style slip:ruff formatstrips those parentheses onpy314, and CI runsruff format --check. The repo already reads that way wherever there is noasbinding —updater/firmware.py:239and:494,tools/frame_bench/examples.py:61. Parenthesised ones all bind a name, which the formatter requires parentheses for.Nothing left to act on. The scheduling test bites (restoring the
awaitfails it), the missing-cost guard bites, and lint, format, mypy and the suite are green on py3.14.