diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index cadab2e..80443af 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -20,6 +20,20 @@ across the three sources. Still defaults to `off`; an existing install is unaffected until it opts in. +⚠️ **`sensor.p1_sample_age_s` is published on `ha_signed`, but must not yet be +thresholded by the ESP32 stale-input watchdog.** On the HA WebSocket paths the +age is stamped from `state_changed`, so it measures time since the value +*changed*, not since the meter *reported* - and Home Assistant exposes no +arrival signal for a repeated reading (no `state_changed`, no `last_reported` +movement on either serialiser, and `state_reported` is not subscribable over +the WebSocket). Measured on the ENV-01 rig against the real HomeWizard +integration. `ha_dsmr` mostly escapes it because a telegram moves several +entities at once; `ha_signed` has one, so a healthy meter under a flat load is +indistinguishable from a dead one. Our own capture has the house meter going +42.2 s and 97.0 s between changes. Raising `meter_max_age_s` does not fix that, +it only chooses which error you get; the fix is an arrival stamp from the meter +itself and is a separate ticket. Full detail in DOCS.md. + ## 0.3.0 **SAFETY-04.** The control law's integrator is now an explicit accumulator, diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 7252dce..403f4a5 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -81,7 +81,16 @@ than in YAML nobody reviews underneath a safety input. | `p1_phase_import_entities` | `[]` | L1..L3, in order. Needed for the capacity-tariff peak on a three-phase connection | | `p1_phase_export_entities` | `[]` | L1..L3, in order | | `p1_net_entity` | | `ha_signed` only. The **signed** net-power sensor: `+` import, `-` export | -| `p1_phase_net_entities` | `[]` | `ha_signed` only. L1..L3, in order, each signed the same way. Needed for the capacity-tariff peak on a three-phase connection | +| `p1_phase_net_entities` | `[]` | `ha_signed` only. L1..L3, in order, each signed the same way. Needed for the capacity-tariff peak on a three-phase connection. **The list length must equal `meter_phases`** | + +Both per-phase lists are checked against `meter_phases` **once at startup**: a +list of the wrong length disables P1 ingestion with an error in the log, rather +than letting every telegram fail its phase-count check one at a time. Leaving +the list empty is fine and is not an error — you simply get no per-phase +figures, and therefore no capacity-tariff peak. On a three-phase connection +that is a much bigger omission than it looks: on a surveyed reading here the +phases carried 2769 W of import while the connection netted 187 W, so the +billed quantity is understated roughly fifteenfold if the phases are missing. #### How long a dead meter takes to reach 0 W @@ -139,13 +148,37 @@ The entity is only created when `meter_source` is not `off`. With P1 ingestion disabled there is nothing feeding it, and an age sensor climbing with no ingester behind it would trip the firmware watchdog on a system that is working fine. -> **Known limit, `mqtt_p1` only.** The age measures *arrival*, not change. On the -> two HA WebSocket paths (`ha_dsmr`, `ha_signed`) that is exactly right: a frozen -> meter emits no `state_changed`, so nothing arrives and the age climbs. On the -> MQTT path a bridge that is stuck republishing its last telegram keeps arriving, -> so the age stays near zero and a frozen meter still looks fresh. Detecting -> *that* needs a change-detector rather than an arrival-detector, and it is not -> in this version. Prefer an HA WebSocket source where both are available. +> **Known limit, `mqtt_p1`.** The age measures *arrival*. On the MQTT path a +> bridge that is stuck republishing its last telegram keeps arriving, so the age +> stays near zero and a frozen meter still looks fresh. Detecting *that* needs a +> change-detector rather than an arrival-detector, and it is not in this version. + +> ⚠️ **Known limit, `ha_signed` — do not drive a watchdog off this age yet.** +> On the HA WebSocket paths the age is stamped when a `state_changed` arrives, +> which means it measures *time since the value last changed*, not time since +> the meter last reported. Home Assistant offers nothing better: a repeated +> reading produces no `state_changed`, does **not** advance `last_reported` on +> either the REST or the WebSocket serialiser, and `state_reported` cannot be +> subscribed to over the WebSocket at all (`Event filter is required for event +> state_reported`). All three measured on the ENV-01 rig against the real +> HomeWizard integration with the meter frozen: 0 `state_changed` in 70 s and no +> timestamp movement anywhere. +> +> `ha_dsmr` mostly escapes this because a DSMR telegram updates several entities +> and something in the set almost always moves. **`ha_signed` has exactly one +> entity, so a healthy meter under a flat load is indistinguishable from a dead +> one.** This is not hypothetical: in our own captures +> (`sim/scenarios/ha-p1_meter_active_power-2026-08-20.json`) the real house meter +> went **42.2 s and 97.0 s** between changes, and 23 Aug peaks at 29.1 s — all +> past the default `meter_max_age_s` of 30. +> +> So `sensor.p1_sample_age_s` on `ha_signed` is safe to *read*, and it is +> correct whenever the value is moving, but it must not yet be thresholded by +> the ESP32 stale-input watchdog: a quiet house would trip the battery to 0 W. +> Raising `meter_max_age_s` is **not** the fix — the two conditions produce an +> identical signal, so a bigger number only chooses which of the two errors you +> get. The real fix is an arrival stamp the meter itself provides, i.e. reading +> the HomeWizard local API directly rather than through an HA entity. ### Control diff --git a/goodwe_controller/app/p1.py b/goodwe_controller/app/p1.py index f562930..46b52ad 100644 --- a/goodwe_controller/app/p1.py +++ b/goodwe_controller/app/p1.py @@ -722,6 +722,25 @@ class HaSignedSource(HaDsmrSource): ingest timestamping, meter_max_age_s, the clock-recomputed age sensor, and `unavailable` treated as a missing reading rather than 0 W. + ⚠️ THE AGE ON THIS TRANSPORT MEASURES TIME SINCE THE VALUE CHANGED, not time + since the meter reported, and on one entity those are very different things. + Home Assistant offers no arrival signal for a repeated reading: it emits no + `state_changed`, it does not advance `last_reported` on either serialiser, + and `state_reported` cannot be subscribed to over the websocket at all + ("Event filter is required for event state_reported"). All three measured on + the ENV-01 rig against the real HomeWizard integration with the meter frozen + - 0 state_changed in 70 s, no timestamp movement anywhere. + + `ha_dsmr` mostly escapes it because a DSMR telegram moves several entities at + once. This transport has ONE, so a healthy meter under a flat load looks + exactly like a dead one - and our own capture has the real meter going 42.2 s + and 97.0 s between changes, both past the default max_age_s of 30. Hence + DOCS.md: sensor.p1_sample_age_s is correct while the value moves and must not + yet be thresholded by the firmware watchdog on this transport. Raising + meter_max_age_s does not fix it, it only chooses which of the two errors you + get. The fix is an arrival stamp from the meter itself - reading the + HomeWizard local API rather than an HA entity - which is a separate ticket. + ⚠️ The debounce is inherited but does nothing useful here, and that is fine: one telegram is one entity, so there is no burst of per-entity events to coalesce and no window in which a new reading sits beside a stale one. It @@ -811,10 +830,27 @@ def build_source(opts: dict, ingest: P1Ingest, session, broker: dict | None): "phase_export": opts.get("p1_phase_export_entities") or [], }) if source == SOURCE_HA_SIGNED: - return HaSignedSource(session, ingest, { - "net": opts.get("p1_net_entity", ""), - "phase_net": opts.get("p1_phase_net_entities") or [], - }) + net = str(opts.get("p1_net_entity", "") or "").strip() + phase_net = [str(e).strip() for e in + (opts.get("p1_phase_net_entities") or []) if str(e).strip()] + # ⚠️ Both of these are checked ONCE here rather than per telegram. A + # misconfigured source otherwise fails silently in the only way that + # looks exactly like a healthy one that has not been sent anything yet: + # no samples, a climbing age, and the firmware watchdog holding the + # battery at 0 W with nothing in the log saying why. + if not net: + _LOG.error("meter_source %s needs p1_net_entity - P1 ingestion " + "disabled (sensor.p1_sample_age_s would otherwise be " + "announced with nothing feeding it)", SOURCE_HA_SIGNED) + return None + if phase_net and len(phase_net) != ingest.phases: + _LOG.error("p1_phase_net_entities has %d entities but meter_phases " + "is %d - P1 ingestion disabled. Every telegram would be " + "rejected on the phase-count check.", + len(phase_net), ingest.phases) + return None + return HaSignedSource(session, ingest, + {"net": net, "phase_net": phase_net}) if source == SOURCE_MQTT: broker = broker or {} return MqttP1Source(ingest, str(opts.get("meter_mqtt_topic", "")), diff --git a/goodwe_controller/test_p1.py b/goodwe_controller/test_p1.py index 96221f0..c5ad246 100644 --- a/goodwe_controller/test_p1.py +++ b/goodwe_controller/test_p1.py @@ -813,13 +813,18 @@ check("sensor.p1_sample_age_s is a live number on this transport too", print("ha_signed: selection by config") check("ha_signed is enabled", is_enabled({"meter_source": SOURCE_HA_SIGNED}) is True) -built = build_source({"meter_source": SOURCE_HA_SIGNED, - "p1_net_entity": "sensor.p1_meter_active_power"}, - P1Ingest(), None, None) +# ⚠️ `sel`, not `built` - that name is the build() wrapper defined at the top of +# this file, and rebinding it here silently disarms every check appended below +# this line. Caught in review: an added check went `TypeError: 'HaSignedSource' +# object is not callable` and aborted the suite, which is the exact failure the +# wrapper exists to prevent, reintroduced by a name collision. +sel = build_source({"meter_source": SOURCE_HA_SIGNED, + "p1_net_entity": "sensor.p1_meter_active_power"}, + P1Ingest(), None, None) check("meter_source ha_signed selects the signed transport", - isinstance(built, HaSignedSource)) + isinstance(sel, HaSignedSource)) check("...wired to p1_net_entity, and subscribed to exactly that one entity", - built.ids == {"sensor.p1_meter_active_power"}) + sel.ids == {"sensor.p1_meter_active_power"}) # ⚠️ The three modes must not bleed into each other: ha_dsmr must keep ignoring # p1_net_entity, or a half-configured install silently reads the wrong sensor. plain = build_source({"meter_source": SOURCE_HA, @@ -833,6 +838,30 @@ check("meter_source off still selects nothing", check("an unrecognised meter_source selects nothing rather than guessing", build_source({"meter_source": "ha_signd"}, P1Ingest(), None, None) is None) +# ⚠️ Caught once at startup, not once per telegram. A source that is wired up +# wrong otherwise fails in the one way indistinguishable from a healthy source +# nobody has sent anything to yet: no samples, a climbing age, the watchdog +# holding the battery at 0 W, and nothing in the log saying why. +check("a blank p1_net_entity is refused rather than silently never receiving", + build_source({"meter_source": SOURCE_HA_SIGNED, "p1_net_entity": ""}, + P1Ingest(), None, None) is None) +check("...and whitespace does not sneak past it", + build_source({"meter_source": SOURCE_HA_SIGNED, "p1_net_entity": " "}, + P1Ingest(), None, None) is None) +check("a phase list that disagrees with meter_phases is refused at startup", + build_source({"meter_source": SOURCE_HA_SIGNED, "p1_net_entity": "sensor.n", + "p1_phase_net_entities": ["sensor.a", "sensor.b"]}, + P1Ingest(phases=3), None, None) is None) +check("a phase list that agrees with meter_phases is accepted", + isinstance(build_source( + {"meter_source": SOURCE_HA_SIGNED, "p1_net_entity": "sensor.n", + "p1_phase_net_entities": ["sensor.a", "sensor.b", "sensor.c"]}, + P1Ingest(phases=3), None, None), HaSignedSource)) +check("no phase list at all is still fine - per-phase billing is optional", + isinstance(build_source( + {"meter_source": SOURCE_HA_SIGNED, "p1_net_entity": "sensor.n"}, + P1Ingest(phases=3), None, None), HaSignedSource)) + # --------------------------------------------------------------------------- # print("the age sensor must not exist when P1 is off") # ⚠️ This is a fleet-wide regression guard, not a nicety. The ESP32 watchdog