admin/goodwe-addon
1
0
Fork 0
Code Issues Pull Requests 4 Actions Packages Projects Releases Wiki Activity

TEL-01 review fixes: stop the age sensor tripping installs that have no meter

Browse Source 
T-1, and it was a fleet-wide trip to zero. publish() emitted p1_age
unconditionally, and published_age_s counts from P1Ingest.__init__ when no
sample has ever arrived. With meter_source defaulting to off, every existing
install would have published sensor.p1_sample_age_s climbing without bound; the
ESP32 does `has_state() && state >= max_age_s` and forces the layer-1 failsafe,
so each of them would have pinned its inverter at 0 W within 30 s. Exactly the
opposite of the zero-regression the off default was for. The key is now omitted
from the payload AND from MQTT discovery when P1 is off, so the entity does not
exist at all - which is the status quo, and what has_state() is testing for.
The predicate is one function, is_enabled(), because the grid reading, the task
start and the discovery announcement have to agree or this comes back.

T-2, connect no longer manufactures a sample. get_states returns whatever HA
currently holds, which after a Core restart is a RestoreEntity value of unknown
age; stamping it with ingest_ts=now reset the age and reported a fresh meter
that could have been dead for an hour. run()'s own docstring already said a
reconnect must emit nothing - the code disagreed with it, and a test asserted
the violation. The cache is still primed, so the first real state_changed
builds a complete sample; the age just stays honest until one arrives.

T-3, gaps are no longer filled with the last held value. The averager held a
sample forward across any interval, so a meter dying at 5 kW and returning ten
minutes later credited 5 kW x 600 s to the capacity-tariff accumulator - a
fabricated peak on a permanent record. The hold is capped at max_age_s: past
that the stretch is walked so block boundaries still land correctly, but
nothing accumulates and elapsed does not grow, which is what finally makes the
comment about a gap dragging the billed average down true. Same threshold for
control and billing: a reading too old to steer by is too old to bill by.

T-4, the out-of-order/duplicate guard is covered. It was untested, and the
reason is worth recording: the obvious assertion passes without the guard,
because the negative interval is separately refused by the covered > 0 test.
What the guard prevents is the timestamp REWIND, which only shows up one sample
later as a re-integrated window. The test now goes one sample later.

T-6, DOCS was wrong about latency. meter_max_age_s and stale_input_s stack, so
meter death to 0 W is 45 s and not 30. Documented as a table with both clocks.

Also documented the T-5 asymmetry rather than papering over it: the age
measures arrival, not change, so a stuck MQTT bridge republishing its last
telegram still looks fresh. Correct on ha_dsmr, not detectable on mqtt_p1
without a change-detector. Written up as a known limit.

Writing the T-1 test caught a second defect in the test itself: it recorded
only MQTT topics, and object_id lives in the payload, so "the age sensor is not
announced" had been passing for the wrong reason.

test_p1.py: 99 -> 122 checks. 14 mutations run, all 14 red, files restored
byte-identical - including one per fix above.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa
This commit is contained in:
glenn schrooyen
2026-08-24 23:16:31 +02:00
co-authored by Claude Opus 5
parent f47f1f0129
commit 4bd659c499
5 changed files with 245 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
goodwe_controller/app/main.py
+16 -5
View File
@@ -39,7 +39,7 @@ from .control import Tuning, compute, maintenance_charge_floor, peak_at_risk
from .hass import HomeAssistant
from .maintenance import IDLE, MaintConfig, Maintenance
from .mqtt import MqttPublisher
from .p1 import P1Ingest, build_source
from .p1 import P1Ingest, build_source, is_enabled
from . import web
OPTIONS_PATH = "/data/options.json"
@@ -96,7 +96,7 @@ class Controller:
# until it opts in.
self.p1 = P1Ingest(phases=int(opts.get("meter_phases", 1)),
max_age_s=float(opts.get("meter_max_age_s", 30)))
self.p1_enabled = str(opts.get("meter_source", "off")) not in ("off", "")
self.p1_enabled = is_enabled(opts)
# live state
self.auto = bool(store.data.get("auto", opts.get("auto_start", False)))
@@ -324,16 +324,26 @@ class Controller:
await asyncio.sleep(1)
def publish(self) -> None:
self.mqtt.publish({
values = {
"setpoint": self.target,
"grid": self.grid,
"battery": self.batt,
"soc": self.soc,
"phase": self.maint.phase,
"status": "running" if self.auto else "stopped",
}
# ⚠️ ONLY when P1 ingestion is actually running. The ESP32's stale-input
# watchdog subscribes to sensor.p1_sample_age_s and forces the layer-1
# failsafe once it reaches max_age_s. With meter_source off there is no
# ingester feeding it, so published_age_s would be time-since-startup
# climbing without bound - i.e. every existing install would cross the
# threshold within 30 s and pin its inverter at 0 W forever. Publishing
# nothing leaves the entity non-existent, which is the status quo and
# what has_state() in the firmware is checking for.
if self.p1_enabled:
# Recomputed here, once a second, on purpose - see P1Ingest.
"p1_age": round(self.p1.published_age_s, 1),
})
values["p1_age"] = round(self.p1.published_age_s, 1)
self.mqtt.publish(values)
async def shutdown(self) -> None:
"""Deterministic wind-down. Do not skip this."""
@@ -501,6 +511,7 @@ async def amain() -> None:
broker.get("port", 1883) if broker else 1883,
broker.get("username") if broker else None,
broker.get("password") if broker else None,
omit=() if is_enabled(opts) else ("p1_age",),
)
except Exception as err: # noqa: BLE001
_LOG.warning("MQTT unavailable (%s) - continuing without status entities", err)
goodwe_controller/app/mqtt.py
+8 -1
View File
@@ -60,7 +60,12 @@ AVAILABILITY = f"{BASE}/availability"
class MqttPublisher:
def __init__(self, host, port, username=None, password=None):
def __init__(self, host, port, username=None, password=None, omit=()):
# `omit` drops sensor keys from discovery entirely. ⚠️ Announcing a
# sensor that nothing will ever publish to is not harmless here:
# p1_sample_age_s is a watchdog input, and an entity that exists but is
# never fed is a worse signal than one that does not exist at all.
self.omit = set(omit)
self.enabled = mqtt is not None and bool(host)
self.client = None
if not self.enabled:
@@ -94,6 +99,8 @@ class MqttPublisher:
def _announce(self) -> None:
for key, object_id, name, unit, dev_class, state_class, icon in SENSORS:
if key in self.omit:
continue
cfg = {
"name": name,
"object_id": object_id,
goodwe_controller/app/p1.py
+45 -9
View File
@@ -213,8 +213,16 @@ class QuarterAverager:
halves credited to the two blocks, never attributed wholly to either.
"""
def __init__(self, phases: int = 1):
def __init__(self, phases: int = 1, max_hold_s: float = 30.0):
self.phases = phases
# ⚠️ How long one sample may be held forward before the series is
# treated as a gap rather than a plateau. Without this the meter can die
# while importing 5 kW, come back ten minutes later, and the hold-forward
# credits 5 kW x 600 s to the capacity-tariff accumulator - a fabricated
# peak, on a permanent record, from data that was never measured. Set
# from meter_max_age_s: the point past which the reading is not trusted
# for control is the point past which it must not be billed either.
self.max_hold_s = float(max_hold_s)
self._block: int | None = None # epoch seconds of the block start
self._acc = 0.0 # W*s of offtake in the open block
self._pp_acc = [0.0] * phases
@@ -273,16 +281,22 @@ class QuarterAverager:
return closed
cursor = self._last_t
# Beyond this instant the held value stops being evidence of anything.
# The stretch from here to `t` is walked so the block boundaries are
# still crossed correctly, but nothing is accumulated and `_elapsed`
# does not grow - which is what makes a closed block, always divided by
# the full 900 s, actually get dragged down by the missing coverage.
hold_end = self._last_t + self.max_hold_s
while True:
end = self._block + QUARTER_S
stop = min(t, end)
dt = stop - cursor
if dt > 0:
self._acc += max(self._last_net, 0.0) * dt
covered = max(0.0, min(stop, hold_end) - cursor)
if covered > 0:
self._acc += max(self._last_net, 0.0) * covered
if self._last_pp is not None:
for i, v in enumerate(self._last_pp[: self.phases]):
self._pp_acc[i] += max(v, 0.0) * dt
self._elapsed += dt
self._pp_acc[i] += max(v, 0.0) * covered
self._elapsed += covered
cursor = stop
if stop < end:
break
@@ -321,7 +335,9 @@ class P1Ingest:
def __init__(self, phases: int = 1, max_age_s: float = 30.0):
self.phases = phases
self.max_age_s = float(max_age_s)
self.averager = QuarterAverager(phases)
# The same threshold governs control and billing: a reading too old to
# steer by is too old to bill by. See QuarterAverager.max_hold_s.
self.averager = QuarterAverager(phases, max_hold_s=self.max_age_s)
self.blocks: list[QuarterBlock] = []
self.samples = 0
self.parse_errors = 0
@@ -477,9 +493,17 @@ class HaDsmrSource:
continue
payload = json.loads(msg.data)
if payload.get("id") == 2 and payload.get("type") == "result":
# ⚠️ Prime the cache, but do NOT build a sample from it.
# get_states returns whatever HA currently holds, which
# after a Core restart is a RestoreEntity value of unknown
# age. Stamping that with ingest_ts=now resets the age to
# zero and reports a fresh meter that may have been dead for
# an hour - a synthetic sample hiding the outage from the
# watchdog that exists to catch it. The cache is what lets
# the FIRST real state_changed build a complete sample; the
# age stays honest until one arrives.
for obj in payload.get("result") or []:
self._absorb(obj.get("entity_id"), obj.get("state"))
self._schedule()
elif payload.get("type") == "event":
data = (payload.get("event") or {}).get("data") or {}
if data.get("entity_id") not in self.ids:
@@ -647,6 +671,18 @@ class MqttP1Source:
# --------------------------------------------------------------------------- #
# selection
# --------------------------------------------------------------------------- #
def is_enabled(opts: dict) -> bool:
"""Whether P1 ingestion is switched on at all.
⚠️ One definition, because three places depend on it and they MUST agree:
where the grid reading comes from, whether the ingest task is started, and
whether sensor.p1_sample_age_s is announced over MQTT discovery. An age
sensor announced with no ingester behind it is a watchdog input nobody is
feeding, and the ESP32 trips on it.
"""
return str(opts.get("meter_source", "off") or "off").strip() not in ("off", "")
def build_source(opts: dict, ingest: P1Ingest, session, broker: dict | None):
"""Return the transport named by `meter_source`, or None if disabled.
@@ -654,7 +690,7 @@ def build_source(opts: dict, ingest: P1Ingest, session, broker: dict | None):
changing transport is a config edit, never a code path.
"""
source = str(opts.get("meter_source", "off") or "off").strip()
if source in ("off", ""):
if not is_enabled(opts):
return None
if source == SOURCE_HA:
return HaDsmrSource(session, ingest, {