TEL-05 review follow-ups: show unchanged_s, name a timeout, keep the poll task alive

Four follow-ups on the reviewed and approved TEL-05 work. Additive; no shipped
behaviour changes except the two failure paths below.

1. unchanged_s had no operator surface. DOCS.md told a reader "the transport
   tracks it as unchanged_s" and there was nowhere to look: main.py built the
   transport, scheduled run(), and never read the object again. The status page
   now shows it on the healthy P1 line. Still NOT thresholded and NOT folded
   into the age - that refusal was reviewed and upheld, because at the converged
   -10 W this controller aims for a 1 Wh register needs ~6 minutes to move, so
   any limit false-trips at the target operating point. The whole argument for
   leaving it to a human requires the human being able to see it.

2. The "equivalent mutant" note on the content_type guard was wrong, and the
   comment is downgraded to say so. web.Response(text=...) defaults to
   text/plain, so the fake meter CAN serve valid JSON under the wrong mimetype.
   Test added; shipped behaviour was already correct.

3. A timed-out poll logged an empty reason: str(asyncio.TimeoutError()) is "",
   so the status page read "last error:" and then nothing, on a hung meter, at
   the moment the battery had just gone to 0 W. Falls back to the class name.
   Note str(err), not `err or ...` - an exception object is always truthy.

4. submit() sat outside the try in poll_once() and run() had no except, so a
   raise would kill the poll task permanently and SILENTLY - safe (the age
   climbs, the controller commands 0 W) but indistinguishable from a dead meter.
   Both wrapped; poll_s is already the retry cadence, so no backoff.

Also a comment at the parse_homewizard range(phases) slice: a 3-phase meter
configured as 1-phase understates the capacity-tariff figure. Filed separately,
not fixed here.

242 checks in test_p1.py (236 before, 6 new). test_control 55, test_arbiter 18,
test_maintenance 21, all untouched and green. Each new check proved non-vacuous:
six mutations, six named reds, no suite aborts, sources restored byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa

goodwe_controller/CHANGELOG.md

@@ -38,7 +38,9 @@ thresholded: this controller regulates grid power toward ~0 W, and at a
 converged −10 W the export register needs six minutes to move by its 1 Wh
 resolution while the power figure legitimately repeats. Thresholding it at 30 s
 would rebuild the false-trip limit cycle at the exact operating point we aim
-for. Freeze detection needs the low-power case solved first, separately.
+for. Freeze detection needs the low-power case solved first, separately. It is
+shown on the status page's P1 line instead — leaving it unthresholded only
+holds up if a human can read it, so now they can.
 
 **TEL-04.** A third `meter_source`, `ha_signed`, reading **one signed** Home
 Assistant entity: positive = import, negative = export. That is the shape a

goodwe_controller/DOCS.md

@@ -196,6 +196,11 @@ repeats. Thresholding that at 30 s would rebuild the false-trip limit cycle at
 the exact operating point the controller aims for. Freeze detection is a
 separate problem and needs the low-power case solved first.
 
+You read it yourself instead: the add-on's status page shows it on the P1 line,
+as `… 4 rejected, measurement unchanged for 312 s`. On a house drawing real
+power that figure stays in the seconds; minutes of it while the load is clearly
+not near zero is the meter to go and look at.
+
 #### `sensor.p1_sample_age_s`
 
 Published over MQTT discovery whenever a broker is available: **seconds since the

goodwe_controller/app/main.py

@@ -97,6 +97,10 @@ class Controller:
         self.p1 = P1Ingest(phases=int(opts.get("meter_phases", 1)),
                            max_age_s=float(opts.get("meter_max_age_s", 30)))
         self.p1_enabled = is_enabled(opts)
+        # Set by amain() once the transport is built, so the status page can show
+        # what only the transport knows (homewizard_local's unchanged_s). Stays
+        # None when P1 is off, or under a transport that has no such counter.
+        self.p1_source = None
 
         # live state
         self.auto = bool(store.data.get("auto", opts.get("auto_start", False)))
@@ -365,10 +369,22 @@ class Controller:
                                     + (f" - last error: {self.p1.last_error}"
                                        if self.p1.last_error else "")})
             else:
+                # ⚠️ unchanged_s is REPORTED, never thresholded and never folded
+                # into the age - see HomeWizardLocalSource.unchanged_s for why
+                # (at the converged -10 W this controller aims for, a 1 Wh
+                # register needs ~6 minutes to move, so any limit false-trips at
+                # the exact operating point we target). The whole argument for
+                # leaving it unthresholded is that a human interprets it, which
+                # requires a human being able to see it - so here it is. getattr:
+                # only homewizard_local has one, and p1_source is None until
+                # amain() builds the transport.
+                unchanged = getattr(self.p1_source, "unchanged_s", None)
                 out.append({"ok": True, "warn": False,
                             "text": f"P1 meter ({o.get('meter_source')}): {self.p1.net_w:g} W, "
                                     f"{age:.0f} s old, {self.p1.samples} telegrams, "
-                                    f"{self.p1.parse_errors} rejected"})
+                                    f"{self.p1.parse_errors} rejected"
+                                    + (f", measurement unchanged for {unchanged:.0f} s"
+                                       if unchanged is not None else "")})
         rows = [("battery SoC", self.soc, o.get("soc_entity")),
                 ("battery power", self.batt, o.get("batt_entity"))]
         if not self.p1_enabled:
@@ -540,6 +556,7 @@ async def amain() -> None:
         # unit - would be computed from a fraction of the data.
         p1_source = build_source(opts, controller.p1, session, broker)
         if p1_source is not None:
+            controller.p1_source = p1_source     # so checks() can report on it
             tasks.append(asyncio.create_task(p1_source.run()))
 
         await stop.wait()

goodwe_controller/app/p1.py

@@ -840,6 +840,10 @@ def parse_homewizard(doc, phases: int, *, now: datetime | None = None,
     imp, exp = split_signed(doc.get("active_power_w"))
 
     pi = pe = None
+    # ⚠️ range(phases), not the legs the meter served: a 3-phase meter configured
+    # as meter_phases: 1 yields a per-phase tuple covering ONE leg of three.
+    # Control is unaffected (it uses the connection figure), but the
+    # capacity-tariff peak is understated. Filed separately - not fixed here.
     legs = [doc.get(f"active_power_l{i + 1}_w") for i in range(phases)]
     if all(v is not None for v in legs):
         pairs = [split_signed(v) for v in legs]
@@ -947,10 +951,24 @@ class HomeWizardLocalSource:
         return max(0.0, time.monotonic() - self._changed_mono)
 
     async def run(self) -> None:
-        """Long-lived task: poll, submit, sleep, forever."""
+        """Long-lived task: poll, submit, sleep, forever.
+
+        ⚠️ Nothing but cancellation may end this loop. An escaping exception
+        would kill the poll task for the lifetime of the add-on, and it would do
+        it QUIETLY: the failure mode is safe (no submissions, the age climbs, the
+        watchdog holds the battery at 0 W) but it looks identical to a dead
+        meter, so the operator goes hunting the wrong device. Retry on the next
+        tick instead - poll_s is already the retry cadence, so no backoff.
+        """
         _LOG.info("P1 ingest: polling %s every %.1fs", self.url, self.poll_s)
         while True:
+            try:
                 await self.poll_once()
+            except asyncio.CancelledError:
+                raise
+            except Exception as err:  # noqa: BLE001 - the task must outlive it
+                _LOG.warning("P1 meter poll %s: %s - retrying in %.1fs", self.url,
+                             str(err) or type(err).__name__, self.poll_s)
             await asyncio.sleep(self.poll_s)
 
     async def poll_once(self) -> bool:
@@ -965,33 +983,43 @@ class HomeWizardLocalSource:
                 # content_type=None: the meter's own firmware is the authority on
                 # what it serves, and refusing a reading over a Content-Type
                 # header would be a fabricated outage.
-                # ⚠️ Known EQUIVALENT MUTANT: dropping this argument leaves all
-                # 236 checks green, because aiohttp's own json_response - which
-                # the fake meter in test_p1.py uses - always sets
-                # application/json, so no test can serve valid JSON under a
-                # wrong header. Recorded here rather than left for the next
-                # reviewer to rediscover. It is kept because a real HomeWizard
-                # firmware that ever answers text/plain would otherwise read as
-                # a dead meter and hold the battery at 0 W.
+                # ⚠️ Kept because a real HomeWizard firmware that ever answers
+                # text/plain would otherwise read as a dead meter and hold the
+                # battery at 0 W. (This was once recorded here as an equivalent
+                # mutant - it is not. aiohttp's json_response always sets
+                # application/json, but web.Response(text=...) defaults to
+                # text/plain, so the fake meter CAN serve valid JSON under the
+                # wrong header, and test_p1.py now does.)
                 doc = await resp.json(content_type=None)
         except asyncio.CancelledError:
             raise
         except Exception as err:  # noqa: BLE001 - any read failure is a gap
             self.connected = False
             self.poll_errors += 1
-            self.ingest.reject(f"meter poll {self.url}: {err}")
+            # ⚠️ str() of a bare asyncio.TimeoutError is the EMPTY STRING, so
+            # f"...: {err}" renders "last error:" and then nothing - on the
+            # status page, on a hung meter, at the moment the battery has just
+            # dropped to 0 W and someone is reading that line to find out why.
+            # The class name is the only thing that says "it timed out".
+            # ⚠️ str(err), not `err or ...`: an exception object is ALWAYS truthy,
+            # empty message or not, so the `or` would never reach the fallback.
+            self.ingest.reject(
+                f"meter poll {self.url}: {str(err) or type(err).__name__}")
             return False
 
         self.connected = True
         try:
             sample = parse_homewizard(doc, self.ingest.phases)
+            # ⚠️ Submitted unconditionally, INCLUDING a document identical to the
+            # last one. The response is the arrival; the number is not.
+            # Inside the try on purpose: submit() outside it would put a raise on
+            # a path with no handler at all, killing the poll task permanently -
+            # safely (the age climbs) but silently. run() backstops the rest.
+            self.ingest.submit(sample)
+            self._note(doc)
         except P1Error as err:
             self.ingest.reject(err)
             return False
-        # ⚠️ Submitted unconditionally, INCLUDING a document identical to the
-        # last one. The response is the arrival; the number is not.
-        self.ingest.submit(sample)
-        self._note(doc)
         return True
 
     def _note(self, doc) -> None:

goodwe_controller/test_p1.py

@@ -11,6 +11,7 @@ computes the wrong quarter-hour figure whenever the telegram cadence changes.
 """
 
 import asyncio
+import json
 import sys
 import time
 from datetime import datetime, timedelta, timezone
@@ -957,11 +958,18 @@ class _FakeMeter:
         self.doc = doc
         self.status = 200
         self.body = None          # set to raw text to serve something unparseable
+        self.delay = 0.0          # set to seconds to imitate a meter that hangs
         self.hits = 0
 
     async def handle(self, request):
         from aiohttp import web
         self.hits += 1
+        if self.delay:
+            await asyncio.sleep(self.delay)
+        # ⚠️ web.Response(text=...) defaults to text/plain. That is not just the
+        # "unparseable body" path: fed VALID json it serves a good document under
+        # the wrong mimetype, which is the only way to reach the content_type
+        # guard in poll_once - json_response can never produce it.
         if self.body is not None:
             return web.Response(text=self.body, status=self.status)
         return web.json_response(self.doc, status=self.status)
@@ -1122,6 +1130,98 @@ check("no failed poll ever became a sample", ing.samples == 2)
 check("a 200 OK marks the transport connected even when its body is refused",
       src.connected is True)
 
+# --------------------------------------------------------------------------- #
+print("homewizard_local: a wrong header, a hung meter, a submit that throws")
+# Three failure shapes that all end the same way if they are mishandled - no
+# sample, a climbing age, the battery at 0 W - and each of which would send the
+# operator hunting the wrong device.
+
+
+async def _header_and_timeout(sess, port, meter):
+    out = {}
+    # Valid JSON under text/plain: exactly what content_type=None is for.
+    meter.body = json.dumps(hw_doc(350.0, l1=350.0))
+    ing = P1Ingest(phases=1, max_age_s=30.0)
+    src = HomeWizardLocalSource(sess, ing, "127.0.0.1", port=port, poll_s=1.0)
+    out["mimetype"] = await src.poll_once()
+    out["ing"] = ing
+    meter.body = None
+
+    # A meter that takes the connection and then does not answer.
+    meter.delay = 0.6
+    ing_t = P1Ingest(phases=1, max_age_s=30.0)
+    slow = HomeWizardLocalSource(sess, ing_t, "127.0.0.1", port=port,
+                                 poll_s=1.0, timeout_s=0.05)
+    out["timeout"] = await slow.poll_once()
+    out["timeout_err"] = ing_t.last_error
+    meter.delay = 0.0
+    return out
+
+
+r = asyncio.run(_hw_rig(_header_and_timeout))
+# ⚠️ A real firmware answering text/plain must not read as a dead meter. Drop
+# content_type=None from poll_once and this goes red with "unexpected mimetype".
+check("valid JSON under the wrong Content-Type is still a reading",
+      r["mimetype"] is True and r["ing"].samples == 1 and r["ing"].net_w == 350.0)
+# ⚠️ str(asyncio.TimeoutError()) is the EMPTY STRING. Without the class-name
+# fallback the status page reads "last error:" and then nothing, on a hung
+# meter, at the moment someone is reading that line to find out why the battery
+# went to 0 W.
+check("a timed-out poll names the fault instead of logging an empty reason",
+      r["timeout"] is False and "TimeoutError" in (r["timeout_err"] or ""))
+
+
+async def _submit_rejects(sess, port, meter):
+    """submit() raising P1Error must be a rejection, not an escaping exception."""
+    class _P1Boom(P1Ingest):
+        def submit(self, sample):
+            raise P1Error("register went backwards")
+
+    ing = _P1Boom(phases=1, max_age_s=30.0)
+    src = HomeWizardLocalSource(sess, ing, "127.0.0.1", port=port, poll_s=1.0)
+    try:
+        ok = await src.poll_once()
+    except Exception as err:      # noqa: BLE001 - an escape IS the failure
+        ok = err
+    return ok, ing.parse_errors
+
+
+ok, errs = asyncio.run(_hw_rig(_submit_rejects))
+check("a submit that rejects the sample is handled, not left to escape",
+      ok is False and errs == 1)
+
+
+async def _submit_explodes(sess, port, meter):
+    """And an UNEXPECTED raise must not kill the poll task for good."""
+    class _Boom(P1Ingest):
+        def submit(self, sample):
+            raise RuntimeError("kaboom")
+
+    src = HomeWizardLocalSource(sess, _Boom(phases=1, max_age_s=30.0),
+                                "127.0.0.1", port=port, poll_s=0.05)
+    task = asyncio.create_task(src.run())
+    await asyncio.sleep(0.3)
+    alive = not task.done()
+    task.cancel()
+    # ⚠️ BaseException, and the same reason built() exists: if run() loses its
+    # guard the task is already dead HOLDING the RuntimeError, and awaiting it
+    # re-raises - aborting the whole suite with a traceback instead of reddening
+    # the check that names the rule. The death is what `alive` records; catching
+    # it here is bookkeeping, not leniency.
+    try:
+        await task
+    except BaseException:  # noqa: BLE001
+        pass
+    return alive, src.polls
+
+
+# ⚠️ The silent-death case. A dead poll task fails SAFE - the age climbs and the
+# controller commands 0 W - but it looks exactly like a dead meter, so the
+# operator spends the outage power-cycling hardware that was never at fault.
+alive, polls = asyncio.run(_hw_rig(_submit_explodes))
+check("a raise inside a poll does not permanently kill the polling task",
+      alive is True and polls > 1)
+
 # --------------------------------------------------------------------------- #
 print("homewizard_local: selection by config")
 
@@ -1295,6 +1395,53 @@ Controller({"meter_source": SOURCE_HOMEWIZARD}, None, _Store(), pub_hw).publish(
 check("with homewizard_local selected, p1_age is published",
       "p1_age" in pub_hw.last and isinstance(pub_hw.last["p1_age"], float))
 
+# --------------------------------------------------------------------------- #
+print("unchanged_s has somewhere an operator can read it")
+# ⚠️ The counter is deliberately NOT thresholded and NOT folded into the age -
+# at the converged -10 W this controller aims for, a 1 Wh register needs six
+# minutes to move, so any limit false-trips at the target operating point. That
+# refusal only holds up if a HUMAN can interpret the number instead, and DOCS.md
+# tells them "the transport tracks it as unchanged_s". Before this, nothing ever
+# read the transport object back: connected, polls, poll_errors and unchanged_s
+# were all write-only, and the documented signal existed nowhere an operator
+# could see it.
+
+
+def p1_rows(ctl):
+    """The P1 status line(s), or the exception that stopped checks() making one.
+
+    Same reason as built(): a status line that raises would abort the suite with
+    a traceback instead of reddening the check that names the rule.
+    """
+    try:
+        return [c["text"] for c in ctl.checks() if c["text"].startswith("P1 meter")]
+    except Exception as err:  # noqa: BLE001 - a raise here is itself the failure
+        print(f"       checks() raised {type(err).__name__}: {err}")
+        return err
+
+
+def _fed(source):
+    ctl = Controller({"meter_source": source}, None, _Store(), _Pub())
+    ctl.p1.submit(make_sample(source, 350.0, 0.0, phases=1,
+                              ingest_mono=time.monotonic()))
+    return ctl
+
+
+hw_ctl = _fed(SOURCE_HOMEWIZARD)
+hw_src = HomeWizardLocalSource(None, hw_ctl.p1, "127.0.0.1")
+hw_src._note(hw_doc(350.0, l1=350.0))
+hw_ctl.p1_source = hw_src            # what amain() does once the transport exists
+rows = p1_rows(hw_ctl)
+check("the healthy P1 status line reports the transport's unchanged_s",
+      isinstance(rows, list) and len(rows) == 1 and "unchanged" in rows[0])
+
+# ⚠️ getattr, not attribute access: p1_source is None until amain() builds one,
+# and ha_dsmr/ha_signed/mqtt_p1 have no such counter at all. Reaching for it
+# directly would turn the whole status page into a 500 on every other transport.
+ha_rows = p1_rows(_fed(SOURCE_HA))
+check("...and the line is unharmed on a transport that has no such counter",
+      isinstance(ha_rows, list) and len(ha_rows) == 1 and "unchanged" not in ha_rows[0])
+
 print()
 if fails:
     print(f"{len(fails)} of {total} FAILED: {', '.join(fails)}")