diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index dbcdd85..8141987 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/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 diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index d924f7d..a6f8718 100644 --- a/goodwe_controller/DOCS.md +++ b/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 diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 13d0403..2d23337 100644 --- a/goodwe_controller/app/main.py +++ b/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() diff --git a/goodwe_controller/app/p1.py b/goodwe_controller/app/p1.py index fc5f189..3efe5e3 100644 --- a/goodwe_controller/app/p1.py +++ b/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: - await self.poll_once() + 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: diff --git a/goodwe_controller/test_p1.py b/goodwe_controller/test_p1.py index 328a20f..fdd24d2 100644 --- a/goodwe_controller/test_p1.py +++ b/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)}")