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

TEL-04 review: unshadow the helper, validate config at startup, and record

Browse Source 
what the rig proved about the age sensor

Review findings 1, 3, 5 and 6. Finding 2 is deliberately untouched - it is
its own ticket.

3. `built` was rebound at test_p1.py:816 by `built = build_source(...)`,
   silently disarming the build() wrapper for anything appended below it.
   Renamed to `sel`. Reproduced the reviewer's failure before fixing:
   appending a check that calls built() after that line gives
   `TypeError: 'HaSignedSource' object is not callable` and aborts at 163 of
   180; with the rename the same probe reaches 180 and passes.

5. DOCS.md now states the "length must equal meter_phases" constraint that
   config.yaml already carried, plus what leaving the list empty actually
   costs: on the surveyed reading the phases carry 2769 W of import while the
   connection nets 187 W, so the tariff quantity is understated ~15x.

6. build_source now checks the ha_signed wiring once at startup instead of
   once per telegram: a blank p1_net_entity, or a phase list whose length
   disagrees with meter_phases, logs an error and disables ingestion. Both
   otherwise fail in the single way indistinguishable from a healthy source
   nobody has fed yet - no samples, a climbing age, the watchdog holding the
   battery at 0 W, and nothing in the log.

1. THE AGE SENSOR. Measured on the ENV-01 rig against the real HomeWizard
   integration, meter frozen via hwsim's `?fault=freeze` seam (cleared in a
   finally:, rig verified restored):

     - websocket state_changed for the meter over 70 s : 0
     - last_reported advanced (REST serialiser)        : no
     - last_reported advanced (websocket serialiser)   : no
     - subscribe_events(state_reported)                : rejected,
       "Event filter is required for event state_reported"

   So Home Assistant exposes NO arrival signal for a repeated reading, and
   the proposed fix - stamp from last_reported via subscribe_entities - is
   not available. subscribe_entities listens only to EVENT_STATE_CHANGED, and
   as_compressed_state carries no last_reported at all.

   The age is therefore "time since the value changed", which on ha_dsmr is
   mostly harmless (a telegram moves several entities) and on ha_signed is
   not: one entity means a healthy meter under a flat load is
   indistinguishable from a dead one. Recorded loudly in DOCS.md, in the
   HaSignedSource docstring and in the CHANGELOG, with the measured 42.2 s
   and 97.0 s gaps from our own capture.

   meter_max_age_s is deliberately NOT widened. The two conditions produce an
   identical signal, so a larger number does not separate them - it only
   chooses which of the two errors you get, and it would disarm the watchdog
   for a genuinely dead meter as well. The honest fix is an arrival stamp the
   meter itself provides.

test_p1.py: 174 -> 179 checks, all green. Other three suites unchanged.

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-25 17:34:37 +02:00
co-authored by Claude Opus 5
parent 632be44f6c
commit e663e10245
4 changed files with 129 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
goodwe_controller/DOCS.md
+41 -8
View File
@@ -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