pv_entity — one optional key that records the site's PV production so
OPT-10's 20.5 kWh of unstored surplus can be attributed. It does not do the
attribution: ship it, let it record a week, OPT-10 picks it up with data.
Three causes produce that number and need three different fixes — no irradiance
(nothing to fix), a charge-power limit, or the optimiser declining to charge
(the only software defect). None of them is separable from a system that records
production nowhere.
The design constraint, honoured
An HA entity, not a register. This add-on's RS485 bus carries the battery
inverter it drives; a household's PV inverter is normally a different box from a
different vendor reporting into HA through its own integration. pv_entity
takes whatever sensor.* the site's PV integration provides — any brand HA can
read, no assumption that it is on the bus.
The three questions the card asked
1. The sign convention — positive is production, and no pv_invert
Derived rather than assumed, out of the two conventions already shipped
(DOCS.md, Sources): meter_entity positive = importing, batt_entity
positive = discharging. Both are "power arriving at the house's AC bus",
and production arrives there too.
An invert flag would be over-building, for two reasons that do not apply to
the keys it would sit beside:
those quantities are bipolar and read by the control path — inv.batt
feeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power
sensor, so a reversed sensor hands the site a plausible number it acts on. pv_w reaches no decision at all;
production is unipolar, so the error is self-announcing: a reversed sensor
reads −4 kW at noon in the first day of the history this card creates. A
flag would let an installer hide it.
No negative-value alarm either — a PV inverter's own standby draw makes a small
negative reading at night ordinary, and any threshold would be a number nobody
measured. Details in DECISIONS/TEL-11-pv-production.md §2.
2. Absence is never zero — and dropped is not absent
publish()skips a None. That is right for circulation and exactly
wrong here: a skipped row leaves HA holding the last number forever, and the
recorder draws a flat line through the outage — indistinguishable from a
measurement, and it manufactures the "there was no surplus" conclusion this
card exists to test.
So the absence is published, with HA's own payload:
homeassistant/components/mqtt/const.py: PAYLOAD_NONE = "None". mqtt/sensor.py: if payload == PAYLOAD_NONE: self._attr_native_value = None
— tested before the numeric branch.
(home-assistant/core dev, retrieved 2026-09-24; the shipped MQTT-sensor docs
say the same in the state_topic entry.)
The entity goes to unknown and the recorder stores a gap. Three states,
not two:
state
frame
wire
page
no source
no pv key
no topic, no entity
no row
no reading
pv: None
goodwe_ctl/pv = None
PV — + a caution naming the reason
zero watts
pv: 0.0
goodwe_ctl/pv = 0.0
PV 0 W
ABSENT_AS_UNKNOWN is a one-key set and the suite asserts circulation: None is still dropped — widening the rule silently would be a behaviour
change to another card's row. A failed read also clears the last good value.
3. It must not become a control input — graded twice
In the suite (test_pv.py part 6): 40 cycles of the real Controller.cycle(), commands bit-identical across unset / no reading / 0 W / 8 kW; inputs_ok unmoved; and a computed census (33 modules under app/) showing the material is mentioned in exactly three — pv.py, main.py, mqtt.py. The scanner's own red direction is exercised on every run.
Across trees (tools/tel11_pv_trace.py, 41 stimulus cells):
control release/1.0 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 set 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
telemetry release/1.0 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 set 4e6d1cb289a957223ad0205f732a6fec6110ce1f2032c98ad60694e67625274f
OK 1/3 UNSET: control AND telemetry byte-identical to the baseline tree.
OK 2/3 SET: the control document is byte-identical with the channel on.
OK 3/3 CONTROL OF THE CONTROL: the telemetry document DOES move (42 cells).
⚠️The control clause is deliberately NOT safety56_behaviour_trace.py's.
That card changed wording, so its --compare refuses unless the sentences
differ. This card's claim is that with the option unset nothing differs, so
"the documents are identical" is the result and cannot also be the control.
What is proved instead: the two app/ fingerprints differ (else refused),
and exactly one side reacts to pv_entity (else refused).
Note clause 1 is stronger than "the control path is untouched": unset is
identical in controlandtelemetry — announcement and status rows
included.
Genuinely optional, and not a second price_now_entity
price_now_entity/price_avg_entity are legal only while price_provider is off and the add-on refuses to start otherwise. Avoided structurally:
validate.py does not mention pv_entity. It cannot conflict with anything,
because nothing else reads it.
the row lives in a new OPTIONAL_SENSORS list and discovery_payloads()'s new include argument defaults to empty — so test_mqtt.py's fixtures, tools/rehearse_roster.py and anything written next year announce exactly
what they announced before. (p1_age solves the same problem the other way
round, with omit, because it is announced by default.) tools/rehearsal/announced-reference.txt therefore needs no new row.
unset ⇒ no state call at all, no entity, no topic, no status row, no page
row. test_pv.py asserts the empty call list.
set and failing ⇒ ok=False, warn=True, a caution. status_payload()
builds NOT READY out of rows that are neither, so a telemetry channel can
never put a battery site into NOT READY.
the read rides in the existing gather with the per-unit reads, so its
worst case overlaps I/O that already happens. A hung PV sensor cannot add hass.state()'s 10 s timeout to the coroutine that feeds the ESP32 heartbeat.
The unit is read off the entity
mW, W, kW, MW, GW, TW — HA's own UnitOfPower, every factor an
exact power of ten. No unit, or a unit outside that table (including BTU/h,
which is in HA's enum), is refused by name and the page says which. read_inputs()'s abs(raw) < 50 kW-or-W guess is safe for a quarter-hour
demand forecast and is not safe here: 30 W of dusk is an ordinary reading
and that heuristic would publish it as 30 kW.
⚠️ The card's central mutation was a SURVIVOR on the first battery
self.grid -= self.pv_w in read_inputs() — exactly the defect the card
forbids — came back green at +106 B, on disk, purged, genuinely running.
The fixture drove a 6 kW export over 12 cycles, which puts the loop against its rail: a command moving slew_w every cycle produces the same
ramp for −6000 W and −14 000 W of apparent error. MUTATION.md habit 23,
with a saturated actuator in place of a coinciding value — the habit's list is
all expressions, so this is now its second recorded instance, in that file.
The fixture drives a 600 W import the loop settles on against a responsive
meter, asserts the resting target is well inside the rail, and asks habit 23's
own question as a check: shift the meter by the same 8 kW and confirm the
commands really would have moved.
Evidence
31/31 suites green in the CI image, checks.yml's exact command, purged
tree, :ro, one file at a time.
Ten mutations, no survivors, each on its own cp -a of the tree — habit
18's revert hazard closed by construction rather than by a verified revert.
Byte deltas, sha256s before/after, and which named checks went red: DECISIONS/TEL-11-pv-production.md §7.
⚠️ Two things for the reviewer to decide, not me
This is a runtime change behind an unchanged version, and tools/predeploy_check.py will refuse a deploy.app/, config.yaml and translations/ are all in its RUNTIME tuple and config.yaml still reads 0.10.1, so the gate reports a NO-OP deploy until a version is cut.
That is PKG's call and this card does not take it.
No CHANGELOG.md section.test_version_gate.py 11b refuses a newest ## heading that is not config.yaml's version:, so opening one means
taking the same packaging decision. The operator account is in DOCS.md; the
release note belongs to the next PKG ticket, which should say that the next
version adds one optional key, announces no new entity on an install
that does not set it, and changes no control behaviour.
Also in this diff, and worth a glance
test_translations.py's "the two halves of config.yaml hold the same 72
keys" was 75 before this card. Verified out of git: 72 at ac98548
(TARIFF-15, where the sentence was written), then 73 (TARIFF-17), 74
(TARIFF-16), 75 (SAFETY-10) — three cards added an option each and none looked
at the sentence, because nothing asserts it. It now reads the count off the
tree, so it cannot rot.
## What this is
`pv_entity` — one optional key that records the site's **PV production** so
OPT-10's 20.5 kWh of unstored surplus can be **attributed**. It does not do the
attribution: ship it, let it record a week, OPT-10 picks it up with data.
Three causes produce that number and need three different fixes — no irradiance
(nothing to fix), a charge-power limit, or the optimiser declining to charge
(the only software defect). None of them is separable from a system that records
production **nowhere**.
## The design constraint, honoured
**An HA entity, not a register.** This add-on's RS485 bus carries the *battery*
inverter it drives; a household's PV inverter is normally a different box from a
different vendor reporting into HA through its own integration. `pv_entity`
takes whatever `sensor.*` the site's PV integration provides — any brand HA can
read, no assumption that it is on the bus.
## The three questions the card asked
### 1. The sign convention — **positive is production**, and no `pv_invert`
Derived rather than assumed, out of the two conventions already shipped
(`DOCS.md`, *Sources*): `meter_entity` positive = **importing**, `batt_entity`
positive = **discharging**. Both are *"power arriving at the house's AC bus"*,
and production arrives there too.
An invert flag would be **over-building**, for two reasons that do not apply to
the keys it would sit beside:
* those quantities are **bipolar and read by the control path** — `inv.batt`
feeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power
sensor, so a reversed sensor hands the site a plausible number it **acts** on.
`pv_w` reaches no decision at all;
* production is **unipolar**, so the error is self-announcing: a reversed sensor
reads **−4 kW at noon** in the first day of the history this card creates. A
flag would let an installer hide it.
No negative-value alarm either — a PV inverter's own standby draw makes a small
negative reading at night ordinary, and any threshold would be a number nobody
measured. Details in `DECISIONS/TEL-11-pv-production.md` §2.
### 2. Absence is never zero — and *dropped* is not *absent*
`publish()` **skips** a `None`. That is right for `circulation` and **exactly
wrong here**: a skipped row leaves HA holding the last number forever, and the
recorder draws a **flat line through the outage** — indistinguishable from a
measurement, and it manufactures the *"there was no surplus"* conclusion this
card exists to **test**.
So the absence is published, with HA's own payload:
> `homeassistant/components/mqtt/const.py`: `PAYLOAD_NONE = "None"`.
> `mqtt/sensor.py`: `if payload == PAYLOAD_NONE: self._attr_native_value = None`
> — tested **before** the numeric branch.
> (home-assistant/core `dev`, retrieved 2026-09-24; the shipped MQTT-sensor docs
> say the same in the `state_topic` entry.)
The entity goes to **`unknown`** and the recorder stores a **gap**. Three states,
not two:
| state | frame | wire | page |
|---|---|---|---|
| no source | no `pv` key | no topic, no entity | no row |
| no reading | `pv: None` | `goodwe_ctl/pv` = `None` | `PV —` + a caution naming the reason |
| zero watts | `pv: 0.0` | `goodwe_ctl/pv` = `0.0` | `PV 0 W` |
`ABSENT_AS_UNKNOWN` is a **one-key** set and the suite asserts `circulation:
None` is **still dropped** — widening the rule silently would be a behaviour
change to another card's row. A failed read also **clears** the last good value.
### 3. It must not become a control input — graded twice
**In the suite** (`test_pv.py` part 6): 40 cycles of the real
`Controller.cycle()`, commands **bit-identical** across `unset` / `no reading` /
`0 W` / `8 kW`; `inputs_ok` unmoved; and a **computed** census (33 modules under
`app/`) showing the material is mentioned in exactly three — `pv.py`, `main.py`,
`mqtt.py`. The scanner's own red direction is exercised on every run.
**Across trees** (`tools/tel11_pv_trace.py`, 41 stimulus cells):
```
control release/1.0 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 set 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
telemetry release/1.0 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 set 4e6d1cb289a957223ad0205f732a6fec6110ce1f2032c98ad60694e67625274f
OK 1/3 UNSET: control AND telemetry byte-identical to the baseline tree.
OK 2/3 SET: the control document is byte-identical with the channel on.
OK 3/3 CONTROL OF THE CONTROL: the telemetry document DOES move (42 cells).
```
⚠️ **The control clause is deliberately NOT `safety56_behaviour_trace.py`'s.**
That card changed wording, so its `--compare` refuses unless the *sentences*
differ. This card's claim is that with the option unset **nothing** differs, so
"the documents are identical" is the **result** and cannot also be the control.
What is proved instead: the two `app/` fingerprints **differ** (else refused),
and **exactly one** side reacts to `pv_entity` (else refused).
Note clause 1 is stronger than "the control path is untouched": **unset** is
identical in `control` **and** `telemetry` — announcement and status rows
included.
## Genuinely optional, and not a second `price_now_entity`
`price_now_entity`/`price_avg_entity` are legal only while `price_provider` is
`off` and the add-on **refuses to start** otherwise. Avoided **structurally**:
* `validate.py` does not mention `pv_entity`. It cannot conflict with anything,
because nothing else reads it.
* the row lives in a new `OPTIONAL_SENSORS` list and `discovery_payloads()`'s new
`include` argument **defaults to empty** — so `test_mqtt.py`'s fixtures,
`tools/rehearse_roster.py` and anything written next year announce exactly
what they announced before. (`p1_age` solves the same problem the other way
round, with `omit`, because it is announced *by default*.)
`tools/rehearsal/announced-reference.txt` therefore needs **no** new row.
* unset ⇒ **no state call at all**, no entity, no topic, no status row, no page
row. `test_pv.py` asserts the empty call list.
* set and failing ⇒ `ok=False, warn=True`, a **caution**. `status_payload()`
builds `NOT READY` out of rows that are neither, so a telemetry channel can
never put a battery site into `NOT READY`.
* the read rides in the **existing gather** with the per-unit reads, so its
worst case overlaps I/O that already happens. A hung PV sensor cannot add
`hass.state()`'s 10 s timeout to the coroutine that feeds the ESP32 heartbeat.
## The unit is read off the entity
`mW`, `W`, `kW`, `MW`, `GW`, `TW` — HA's own `UnitOfPower`, every factor an
exact power of ten. No unit, or a unit outside that table (including `BTU/h`,
which **is** in HA's enum), is **refused by name** and the page says which.
`read_inputs()`'s `abs(raw) < 50` kW-or-W guess is safe for a quarter-hour
demand forecast and is **not** safe here: 30 W of dusk is an ordinary reading
and that heuristic would publish it as 30 kW.
## ⚠️ The card's central mutation was a SURVIVOR on the first battery
`self.grid -= self.pv_w` in `read_inputs()` — exactly the defect the card
forbids — came back **green** at **+106 B**, on disk, purged, genuinely running.
The fixture drove a **6 kW export over 12 cycles**, which puts the loop
**against its rail**: a command moving `slew_w` every cycle produces the **same
ramp** for −6000 W and −14 000 W of apparent error. MUTATION.md **habit 23**,
with a *saturated actuator* in place of a coinciding value — the habit's list is
all *expressions*, so this is now its second recorded instance, in that file.
The fixture drives a **600 W import the loop settles on** against a responsive
meter, asserts the resting target is well inside the rail, and asks habit 23's
own question **as a check**: shift the meter by the same 8 kW and confirm the
commands really would have moved.
## Evidence
* **31/31** suites green in the CI image, `checks.yml`'s exact command, purged
tree, `:ro`, one file at a time.
* **Ten mutations, no survivors**, each on its own `cp -a` of the tree — habit
18's revert hazard closed by construction rather than by a verified revert.
Byte deltas, sha256s before/after, and which named checks went red:
`DECISIONS/TEL-11-pv-production.md` §7.
## ⚠️ Two things for the reviewer to decide, not me
1. **This is a runtime change behind an unchanged version, and
`tools/predeploy_check.py` will refuse a deploy.** `app/`, `config.yaml` and
`translations/` are all in its `RUNTIME` tuple and `config.yaml` still reads
`0.10.1`, so the gate reports a **NO-OP** deploy until a version is cut.
That is PKG's call and this card does not take it.
2. **No `CHANGELOG.md` section.** `test_version_gate.py` 11b refuses a newest
`## ` heading that is not `config.yaml`'s `version:`, so opening one means
taking the same packaging decision. The operator account is in `DOCS.md`; the
release note belongs to the next PKG ticket, which should say that the next
version adds **one optional key**, announces **no new entity** on an install
that does not set it, and changes **no control behaviour**.
## Also in this diff, and worth a glance
`test_translations.py`'s *"the two halves of config.yaml hold the same **72**
keys"* was **75** before this card. Verified out of git: 72 at `ac98548`
(TARIFF-15, where the sentence was written), then 73 (TARIFF-17), 74
(TARIFF-16), 75 (SAFETY-10) — three cards added an option each and none looked
at the sentence, because nothing asserts it. It now reads the count off the
tree, so it cannot rot.
## Out of scope, untouched
OPT-10's attribution itself; SAFETY-58, SAFETY-59, TEST-25, CI-10; the 710/790
disagreement; `CHANGELOG.md`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
OPT-10 measured that this site's pack reaches 90 % SoC on 2 days of 16 and that
20.5 kWh of surplus left the site while the pack had room - 12.6 kWh of it with
the battery not charging at all - and could not say WHY, because PV production
is recorded NOWHERE. Three causes need three different fixes (no irradiance:
nothing to fix; a charge-power limit; the optimiser declining to charge, which
is the only software defect) and guessing between them is how an optimiser gets
tuned to solve a cloud problem. This card adds the channel. It does not do the
attribution.
AN HA ENTITY, NOT A REGISTER, AND THAT IS A PRODUCT DECISION. This add-on's
RS485 bus carries the BATTERY inverter it drives; a household's PV inverter is
normally a different box from a different vendor reporting into Home Assistant
through its own integration. `pv_entity` takes whatever `sensor.*` the site's
own PV integration provides, any brand HA can read.
POSITIVE IS PRODUCTION, AND THERE IS NO `pv_invert`. The convention is derived
rather than assumed: `meter_entity` is positive when IMPORTING and `batt_entity`
is positive when DISCHARGING (DOCS.md, Sources), both of which are "power
arriving at the house's AC bus", and production arrives there too. Those two
keys carry invert flags because they are BIPOLAR and are READ BY THE CONTROL
PATH - `inv.batt` feeds SAFETY-30's per-unit sign check and SAFETY-31's
circulating-power sensor, so a reversed sensor hands the site a plausible number
it acts on. Production is unipolar and is read by nothing: a reversed sensor
announces itself as -4 kW at noon in the very history this card creates, and a
flag would only let an installer hide it. No negative-value alarm either - a PV
inverter's own standby draw makes a small negative reading at night ordinary, so
any threshold would be a number nobody measured.
ABSENCE IS NEVER ZERO, AND "DROPPED" IS NOT ABSENT. `publish()` SKIPS a None,
which is right for `circulation` and exactly wrong here: a skipped row leaves
Home Assistant holding the last number forever and the recorder draws a FLAT
LINE through the outage, which is indistinguishable from a measurement and
manufactures the "there was no surplus" conclusion this card exists to TEST. So
the absence is published, with HA's own payload: `mqtt/const.py` declares
`PAYLOAD_NONE = "None"` and `mqtt/sensor.py` tests it BEFORE the numeric branch,
so the entity's state becomes `unknown` and the recorder stores a GAP. Read out
of home-assistant/core `dev` on 2026-09-24 and quoted at the constant.
`ABSENT_AS_UNKNOWN` is a one-key set and the suite asserts `circulation: None`
is STILL dropped.
THE UNIT IS READ OFF THE ENTITY. `mW`, `W`, `kW`, `MW`, `GW`, `TW` - HA's own
`UnitOfPower`, every factor an exact power of ten. A sensor with no unit, or a
unit outside that table, is REFUSED by name and the status page says which.
`read_inputs()`'s `abs(raw) < 50` kW-or-W guess is safe for a quarter-hour
demand forecast and is NOT safe here: 30 W of dusk is an ordinary reading and
that heuristic would publish it as 30 kW.
GENUINELY OPTIONAL, AND NOT A SECOND `price_now_entity`. validate.py does not
mention the key - it cannot conflict with anything, because nothing else reads
it. The MQTT row lives in a new `OPTIONAL_SENSORS` list whose `include` argument
DEFAULTS TO EMPTY, so every caller that has never heard of `pv_entity` -
test_mqtt.py's fixtures, tools/rehearse_roster.py, anything written next year -
announces exactly what it announced before. Unset: no state call at all, no
entity, no topic, no status row, no page row. Set and failing: a CAUTION
(`ok=False, warn=True`), never a `bad` row, so a telemetry channel can never put
a battery site into NOT READY.
IT MUST NOT BECOME A CONTROL INPUT, AND THAT IS GRADED TWICE. `test_pv.py`
part 6 drives 40 cycles of the real `cycle()` and asserts the commands are
bit-identical across unset / no reading / 0 W / 8 kW, with a COMPUTED census
(33 modules under app/) showing the material is mentioned in exactly three:
pv.py, main.py, mqtt.py. `tools/tel11_pv_trace.py` is the cross-tree half -
`safety56_behaviour_trace.py`'s pattern with a DIFFERENT control clause, because
this card's claim is that nothing differs rather than that only sentences do: it
REFUSES unless the two `app/` fingerprints differ and unless exactly one side
reacts to the option. Measured on 41 stimulus cells:
control release/1.0 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 set 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
telemetry release/1.0 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 set 4e6d1cb289a957223ad0205f732a6fec6110ce1f2032c98ad60694e67625274f
UNSET is byte-identical in BOTH documents, which is stronger than "the control
path is untouched": an install that does not set the option sees no change of
any kind, announcement and status rows included.
⚠️ AND THE CARD'S CENTRAL MUTATION WAS A SURVIVOR ON THE FIRST BATTERY.
`self.grid -= self.pv_w` in `read_inputs()` - +106 B, on disk, purged, genuinely
running - came back GREEN, because the fixture drove a 6 kW export over 12
cycles, which puts the loop AGAINST ITS RAIL: a command moving `slew_w` every
cycle produces the same ramp for -6000 W and for -14 000 W of apparent error.
MUTATION.md habit 23 with a saturated ACTUATOR in place of a coinciding value,
and it is in that file now as the habit's second instance. The fixture now
drives a 600 W import the loop SETTLES on, against a responsive meter, and asks
habit 23's own question as its own check: shift the METER by the same 8 kW and
confirm the commands really would have moved.
Evidence: 31/31 suites green in the CI image, checks.yml's exact command, purged
tree, `:ro`. Ten mutations, NO survivors, each on its own `cp -a` of the tree so
habit 18's revert hazard is closed by construction - table in
`DECISIONS/TEL-11-pv-production.md` §7 with byte deltas and sha256s.
⚠️ THIS IS A RUNTIME CHANGE BEHIND AN UNCHANGED VERSION, AND
`tools/predeploy_check.py` WILL REFUSE IT. `app/`, `config.yaml` and
`translations/` are all in its `RUNTIME` tuple and `config.yaml` still reads
0.10.1, so the gate reports a NO-OP deploy until a version is cut. That is PKG's
call and this card does not take it.
No CHANGELOG section: `test_version_gate.py` 11b refuses a newest `## ` heading
that is not config.yaml's `version:`, and opening one would mean taking that
same packaging decision. The operator account is in DOCS.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
Review 1/8 — the green is this PR's own, and here is how that was established
CI-10 says a green context on this forge may belong to another PR, because statuses are keyed by head SHA. So the status was not taken at face value. Traced:
Head 07ba3433ed2d9aa8945a8e36cea42994efb618c5 and base 9f6b9f14cd260a4b84e1561d96a76f9368d6e23e were read off the remote, not off a local branch.
The only status on that SHA is checks / unit (pull_request) → success, pointing at run 825 / job 1950.
Run 825: path = checks.yml@refs/pull/113/head, event = pull_request, run_attempt = 1, head_sha = 07ba343…. Attempt 1, so this is not a replayed stored payload.
The job log carries the payload explicitly:
PRNUM: 113
BASEREF: release/1.0
base release/1.0 9f6b9f14cd260a4b84e1561d96a76f9368d6e23e
head refs/pull/113/head 07ba3433ed2d9aa8945a8e36cea42994efb618c5
GRADED: refs/pull/113/head merged into release/1.0
That is the exact check CI-10 asks for: the payload names PR 113, with base release/1.0, at the base SHA this PR actually targets. It is not PR #102's PRNUM: 111 failure mode.
It is a real execution, not CI-9's laundered skipped: six steps all success, a real runner (goodwe-rig), 8m6s wall clock, and the suite step reporting 31 test file(s) to run with per-file durations (test_loop.py (93s), test_watchdog.py (67s), test_peakguard.py (67s)). A skipped status has no run, no job and no target_url; this has all three.
No old run was rerun at any point during this review.
Independently reproduced locally: the full suite in the same CI image (goodwe-addon-checks:1), same command shape, purged tree, :ro mount, one file at a time — 31/31 PASS, exit 0.
## Review 1/8 — the green is this PR's own, and here is how that was established
CI-10 says a green context on this forge may belong to another PR, because statuses are keyed by head SHA. So the status was not taken at face value. Traced:
- Head `07ba3433ed2d9aa8945a8e36cea42994efb618c5` and base `9f6b9f14cd260a4b84e1561d96a76f9368d6e23e` were read off the **remote**, not off a local branch.
- The only status on that SHA is `checks / unit (pull_request)` → `success`, pointing at **run 825 / job 1950**.
- Run 825: `path = checks.yml@refs/pull/113/head`, `event = pull_request`, `run_attempt = 1`, `head_sha = 07ba343…`. Attempt 1, so this is not a replayed stored payload.
- The **job log** carries the payload explicitly:
```
PRNUM: 113
BASEREF: release/1.0
base release/1.0 9f6b9f14cd260a4b84e1561d96a76f9368d6e23e
head refs/pull/113/head 07ba3433ed2d9aa8945a8e36cea42994efb618c5
GRADED: refs/pull/113/head merged into release/1.0
```
That is the exact check CI-10 asks for: the payload names **PR 113**, with base **release/1.0**, at the base SHA this PR actually targets. It is not PR #102's `PRNUM: 111` failure mode.
- It is a **real execution**, not CI-9's laundered `skipped`: six steps all `success`, a real runner (`goodwe-rig`), 8m6s wall clock, and the suite step reporting `31 test file(s) to run` with per-file durations (`test_loop.py (93s)`, `test_watchdog.py (67s)`, `test_peakguard.py (67s)`). A `skipped` status has no run, no job and no `target_url`; this has all three.
- No old run was rerun at any point during this review.
**Independently reproduced locally**: the full suite in the same CI image (`goodwe-addon-checks:1`), same command shape, purged tree, `:ro` mount, one file at a time — **31/31 PASS, exit 0**.
Review 2/8 — ⭐ the control path is genuinely untouched, and the substituted control clause is sound
This is the whole ticket, so the tool's own output was not taken as evidence. Both trees were re-run from scratch in the CI image (:ro, PYTHONDONTWRITEBYTECODE=1, __pycache__ purged), with tel11_pv_trace.py copied into a clean release/1.0 checkout at 9f6b9f1.
The hashes reproduce exactly, including the one the card leads with:
control release/1.0 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 set 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
telemetry release/1.0 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 set 4e6d1cb289a957223ad0205f732a6fec6110ce1f2032c98ad60694e67625274f
OK 1/3, OK 2/3, OK 3/3 (42 cells) rc=0
The reasoning about SAFETY-56's clause is right
SAFETY-56 changed wording, so "the two sides must differ" was a valid control for that card: identity there would have meant the instrument saw nothing. TEL-11's headline claim is identity, so identity is the result and cannot also be the control without being circular. Declining to reuse that clause is correct, not a convenience.
And its replacement refusals actually refuse — each was fired deliberately
forced condition
result
same tree compared with itself
REFUSED: … SAME app/ fingerprint … one tree twice — rc=2
two different trees, neither reacting to pv_entity
REFUSED: set differs from unset on NEITHER side — rc=2
control moves but telemetry frozen (forged doc)
REFUSED 3/3: the telemetry document did NOT move … — rc=2
None of the three is decorative.
The clause that matters was counter-tested with a real planted leak
A control-path leak (self.grid = self.grid - self.pv_w in read_inputs()) was planted in a copy of the head tree, purged, and traced. Clause 2/3 went red:
FAIL 2/3 CONTROL MOVED at 'EXPORT-6K/noon' when `pv_entity` was set
… 26 cells …
rc=1
26 of 41 cells — matching DECISIONS §7's documented figure exactly — and the none/zero/bad-unit schedules correctly did not move, because subtracting nothing and subtracting zero change nothing. Clause 1/3 stayed green throughout, which is right: the leak only fires when the option is set.
So the identity in 2/3 is an identity the instrument could have broken, and did break when given something to find. That is the difference between this and a vacuous green.
Supporting, independently checked: hass.state() is called from exactly one place in the whole package (_read_pv), so the trace fixture's blanket state() double cannot contaminate any other read; and the computed census holds — pv_entity|pv_w|pv_why|optional_keys|ABSENT_AS_UNKNOWN appear in main.py, mqtt.py, pv.py only, across 33 modules.
## Review 2/8 — ⭐ the control path is genuinely untouched, and the substituted control clause is sound
This is the whole ticket, so the tool's own output was not taken as evidence. Both trees were re-run from scratch in the CI image (`:ro`, `PYTHONDONTWRITEBYTECODE=1`, `__pycache__` purged), with `tel11_pv_trace.py` copied into a clean `release/1.0` checkout at `9f6b9f1`.
**The hashes reproduce exactly, including the one the card leads with:**
```
control release/1.0 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
control TEL-11 set 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
telemetry release/1.0 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
telemetry TEL-11 set 4e6d1cb289a957223ad0205f732a6fec6110ce1f2032c98ad60694e67625274f
OK 1/3, OK 2/3, OK 3/3 (42 cells) rc=0
```
### The reasoning about SAFETY-56's clause is right
SAFETY-56 changed *wording*, so "the two sides must differ" was a valid control **for that card**: identity there would have meant the instrument saw nothing. TEL-11's headline claim **is** identity, so identity is the *result* and cannot also be the control without being circular. Declining to reuse that clause is correct, not a convenience.
### And its replacement refusals actually refuse — each was fired deliberately
| forced condition | result |
|---|---|
| same tree compared with itself | `REFUSED: … SAME app/ fingerprint … one tree twice` — **rc=2** |
| two different trees, neither reacting to `pv_entity` | `REFUSED: set differs from unset on NEITHER side` — **rc=2** |
| control moves but telemetry frozen (forged doc) | `REFUSED 3/3: the telemetry document did NOT move …` — **rc=2** |
None of the three is decorative.
### The clause that matters was counter-tested with a real planted leak
A control-path leak (`self.grid = self.grid - self.pv_w` in `read_inputs()`) was planted in a copy of the head tree, purged, and traced. Clause 2/3 **went red**:
```
FAIL 2/3 CONTROL MOVED at 'EXPORT-6K/noon' when `pv_entity` was set
… 26 cells …
rc=1
```
**26 of 41 cells** — matching `DECISIONS §7`'s documented figure exactly — and the `none`/`zero`/`bad-unit` schedules correctly did *not* move, because subtracting nothing and subtracting zero change nothing. Clause 1/3 stayed green throughout, which is right: the leak only fires when the option is set.
So the identity in 2/3 is an identity the instrument **could** have broken, and did break when given something to find. That is the difference between this and a vacuous green.
Supporting, independently checked: `hass.state()` is called from exactly one place in the whole package (`_read_pv`), so the trace fixture's blanket `state()` double cannot contaminate any other read; and the computed census holds — `pv_entity|pv_w|pv_why|optional_keys|ABSENT_AS_UNKNOWN` appear in **`main.py`, `mqtt.py`, `pv.py` only**, across 33 modules.
Review 3/8 — ⭐ M7 is genuinely red now, the replacement fixture discriminates, and the habit-23 framing is right
M7 is red
The leak was planted on a purged copy of the head tree and test_pv.py run in the CI image:
FAIL ⭐ THE COMMANDS ARE BIT-IDENTICAL ACROSS unset / no reading / 0 W / 8 kW …
FAIL ⭐ PRECONDITION (habit 23): the same stimulus with the METER shifted by that 8 kW …
2 FAILED rc=1
Not a survivor. And note which two went red — see below, because the second one is the interesting half.
The replacement fixture actually discriminates
The concern with the first fixture was that a 6 kW export parks the loop on its rail, where −6000 W and −14000 W of apparent error produce the same slew_w ramp. The new fixture's numbers show it is genuinely off the rail:
unset : [370.0, 510.0, 570.0, 590.0, 610.0] -> settled at 610 W
8000 W : [370.0, 510.0, 570.0, 590.0, 610.0] -> settled at 610 W
meter shifted by the same 8 kW -> settled at -5000 W
It settles at 610 W with max_w at 5000 — comfortably inside the rail, and the suite asserts that as its own precondition (abs(target) > 100 and < 0.5 * max_w) rather than leaving it to be read off. The shifted-meter probe rests 5610 W away. So "the commands did not move" is a fact about the code, not about a saturated actuator.
A nice property worth recording: under M7 the habit-23 precondition is itself an exact detector. The mutant makes the 8 kW-PV run see a meter of 600 − 8000 W, which is precisely the shifted control run — so _h_shift.writes != _w_big becomes false and the precondition fails. The check written to guard the identity turns out to catch the mutation on its own.
The habit-23 framing is right
Habit 23 is "a mutation that ran and could not change the answer on the data it was given" — distinct from stale bytecode, and invisible to all three things a mutation report is required to state, because all three are about the file and this is about the stimulus. M7 fits that exactly: purged, on disk, non-zero delta, genuinely executed, and still unable to move the answer.
Calling it the second instance is fair. SAFETY-57's 0.0 or x survivor is the first; the zero-byte _release/_grade order swap recorded beside it is a habit-22 finding, not a no-op mutant. So this is instance two of the habit itself.
And the generalisation earns its place rather than just restating the case. Habit 23's existing list — or, and, truthiness, default arguments, getattr fallbacks — is all expressions with a coinciding value. The new text's claim is that a rail, a clamp, a slew limit, a deadband and a latched alarm are the same hazard in the plant: the place where the controller's output stops being a function of its input. That is a real extension of the habit's domain, and the warning that "this repository's control fixtures park there by default, because the dramatic stimulus is the saturating one" is the actionable part. The restated precondition — not "does it command something" but "does it rest somewhere the mutation could move it from" — is the correct repair.
## Review 3/8 — ⭐ M7 is genuinely red now, the replacement fixture discriminates, and the habit-23 framing is right
### M7 is red
The leak was planted on a purged copy of the head tree and `test_pv.py` run in the CI image:
```
FAIL ⭐ THE COMMANDS ARE BIT-IDENTICAL ACROSS unset / no reading / 0 W / 8 kW …
FAIL ⭐ PRECONDITION (habit 23): the same stimulus with the METER shifted by that 8 kW …
2 FAILED rc=1
```
Not a survivor. And note *which* two went red — see below, because the second one is the interesting half.
### The replacement fixture actually discriminates
The concern with the first fixture was that a 6 kW export parks the loop on its rail, where −6000 W and −14000 W of apparent error produce the same `slew_w` ramp. The new fixture's numbers show it is genuinely off the rail:
```
unset : [370.0, 510.0, 570.0, 590.0, 610.0] -> settled at 610 W
8000 W : [370.0, 510.0, 570.0, 590.0, 610.0] -> settled at 610 W
meter shifted by the same 8 kW -> settled at -5000 W
```
It **settles** at 610 W with `max_w` at 5000 — comfortably inside the rail, and the suite asserts that as its own precondition (`abs(target) > 100 and < 0.5 * max_w`) rather than leaving it to be read off. The shifted-meter probe rests 5610 W away. So "the commands did not move" is a fact about the code, not about a saturated actuator.
A nice property worth recording: under M7 the habit-23 precondition is itself an **exact** detector. The mutant makes the 8 kW-PV run see a meter of 600 − 8000 W, which is precisely the shifted control run — so `_h_shift.writes != _w_big` becomes false and the precondition fails. The check written to guard the identity turns out to catch the mutation on its own.
### The habit-23 framing is right
Habit 23 is "a mutation that **ran** and could not **change the answer** on the data it was given" — distinct from stale bytecode, and invisible to all three things a mutation report is required to state, because all three are about the file and this is about the stimulus. M7 fits that exactly: purged, on disk, non-zero delta, genuinely executed, and still unable to move the answer.
Calling it the **second instance** is fair. SAFETY-57's `0.0 or x` survivor is the first; the zero-byte `_release`/`_grade` order swap recorded beside it is a habit-**22** finding, not a no-op mutant. So this is instance two of the habit itself.
And the generalisation earns its place rather than just restating the case. Habit 23's existing list — `or`, `and`, truthiness, default arguments, `getattr` fallbacks — is **all expressions with a coinciding value**. The new text's claim is that a rail, a clamp, a slew limit, a deadband and a latched alarm are the same hazard **in the plant**: the place where the controller's output stops being a function of its input. That is a real extension of the habit's domain, and the warning that "this repository's control fixtures park there by default, because the dramatic stimulus is the saturating one" is the actionable part. The restated precondition — *not* "does it command something" but "does it rest somewhere the mutation could move it from" — is the correct repair.
Review 4/8 — ⭐ absence is distinguishable from zero: all three states verified on the wire
Confirmed by running test_pv.py part 3 in the CI image and reading the payload bytes, not the prose:
no source : 'pv' in frame = False
no reading: None -> wire ('goodwe_ctl/pv', 'None', False)
zero watts: 0.0 -> wire ('goodwe_ctl/pv', '0.0', False)
Three genuinely distinct states:
state
frame
wire
no source
no pv key at all
nothing published, no entity announced
no reading
pv: None
goodwe_ctl/pv = None
zero watts
pv: 0.0
goodwe_ctl/pv = 0.0
The two that share a topic are different bytes, which is the acceptance criterion. "None" is HA's own PAYLOAD_NONE, taken from the module constant rather than spelled as a literal at the call site, and the suite asserts that too.
The reasoning behind this is the strongest argument in the card and it holds up: publish() skipping a None would leave HA holding the last number forever, and the recorder would draw a flat line through the outage. Read back a week later that is indistinguishable from a real measurement — and it manufactures exactly the "there was no surplus" conclusion this card exists to test. Dropping a row is not publishing an absence.
Two guards against this becoming a wider behaviour change, both confirmed:
ABSENT_AS_UNKNOWN is a one-key frozenset, and the suite asserts circulation: None is still dropped. Widening the rule silently would be a behaviour change to another card's row. M3 in the battery is the mutation that widens it, and it is red on that named check.
A failed read clears the last good value — _read_pv assigns on every call including the failing ones. M5 is the mutation that makes a value outlive its sensor, red on 2 named checks. Verified in the part 2 output: the cycle after a 4100 W reading disappears publishes an absence, not 4100 W again.
checks() renders the absence as an em dash on the page, not as 0 W, which closes the same gap on the operator surface.
## Review 4/8 — ⭐ absence is distinguishable from zero: all three states verified on the wire
Confirmed by running `test_pv.py` part 3 in the CI image and reading the payload bytes, not the prose:
```
no source : 'pv' in frame = False
no reading: None -> wire ('goodwe_ctl/pv', 'None', False)
zero watts: 0.0 -> wire ('goodwe_ctl/pv', '0.0', False)
```
Three genuinely distinct states:
| state | frame | wire |
|---|---|---|
| no source | no `pv` key at all | nothing published, no entity announced |
| no reading | `pv: None` | `goodwe_ctl/pv` = `None` |
| zero watts | `pv: 0.0` | `goodwe_ctl/pv` = `0.0` |
The two that share a topic are **different bytes**, which is the acceptance criterion. `"None"` is HA's own `PAYLOAD_NONE`, taken from the module constant rather than spelled as a literal at the call site, and the suite asserts that too.
The reasoning behind this is the strongest argument in the card and it holds up: `publish()` skipping a `None` would leave HA holding the last number **forever**, and the recorder would draw a flat line through the outage. Read back a week later that is indistinguishable from a real measurement — and it manufactures exactly the "there was no surplus" conclusion this card exists to *test*. Dropping a row is not publishing an absence.
Two guards against this becoming a wider behaviour change, both confirmed:
- `ABSENT_AS_UNKNOWN` is a **one-key** frozenset, and the suite asserts `circulation: None` is **still dropped**. Widening the rule silently would be a behaviour change to another card's row. M3 in the battery is the mutation that widens it, and it is red on that named check.
- A failed read **clears** the last good value — `_read_pv` assigns on every call including the failing ones. M5 is the mutation that makes a value outlive its sensor, red on 2 named checks. Verified in the part 2 output: the cycle after a 4100 W reading disappears publishes an absence, not 4100 W again.
`checks()` renders the absence as an em dash on the page, not as `0 W`, which closes the same gap on the operator surface.
Review 5/8 — the sign convention is derived, not asserted; both refusals judged sound
The derivation checks out against the shipped tree
The card claims positive = production follows from two conventions already shipped. Verified in DOCS.mdon release/1.0, not on this branch:
line 135: | `meter_entity` | yes | Net grid power. **Positive must mean importing** |
line 138: | `batt_entity` | yes | Battery power — again the ESP32's read, `+` = discharging |
Both are "positive = power arriving at the house's AC bus", and production arrives there too. So the convention is genuinely read off what the product already does rather than picked and justified afterwards. That is the right way round.
No pv_invert — sound
The distinction the card draws is the load-bearing one, and it is correct: meter_entity and batt_entity are bipolar and read by the control path (inv.batt feeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power sensor), so a reversed sensor there hands the site a plausible number it acts on — the flag is what stops a control defect. pv_w reaches no decision at all, which this review verified independently (comment 2/8).
On the worry that a missing flag is cheap to add later but a wrong sign in a week of history is not — I don't think that lands here, for two reasons:
The error is not silent and not lossy. Production is unipolar, so a reversed sensor reads −4 kW at noon: glaring in the history, on the status row (PV production: -4200 W from …), and on the page. And it is recoverable — a negated series is still the measurement, invertible after the fact. A week of reversed PV is an annoyance, not a destroyed dataset.
The flag would make it worse, not better. An invert flag is a place for an installer to "fix" a reading by eye and produce a plausible positive series that is silently wrong — which is the failure mode you cannot detect a week later. The card's own words: "a flag would let an installer hide it." For the one channel whose entire purpose is to be believed later, self-announcing beats configurable.
The trace tool's stimulus set includes a negative PV schedule, so the negative path is actually exercised rather than assumed.
No negative-value alarm — sound, with one residual for a later card
The argument is right on its own terms: a PV inverter's own standby draw makes a small negative reading at night ordinary, and any threshold would be a number nobody has measured. Inventing a constant here to look thorough would be the worse error.
Residual, not blocking and not in scope for this card: detection currently depends on a human looking at the page or the history. There is a threshold-free formulation available later — a daily maximum that is negative needs no measured constant to be obviously wrong — which would catch a reversed sensor on day one rather than whenever somebody looks. Worth a line in whatever picks up OPT-10's attribution; it is not something to hold this PR for, and I'd rather it be designed against a week of real data than guessed at now.
## Review 5/8 — the sign convention is derived, not asserted; both refusals judged sound
### The derivation checks out against the shipped tree
The card claims positive = production follows from two conventions already shipped. Verified in `DOCS.md` **on `release/1.0`**, not on this branch:
```
line 135: | `meter_entity` | yes | Net grid power. **Positive must mean importing** |
line 138: | `batt_entity` | yes | Battery power — again the ESP32's read, `+` = discharging |
```
Both are "positive = power arriving at the house's AC bus", and production arrives there too. So the convention is genuinely read off what the product already does rather than picked and justified afterwards. That is the right way round.
### No `pv_invert` — sound
The distinction the card draws is the load-bearing one, and it is correct: `meter_entity` and `batt_entity` are **bipolar and read by the control path** (`inv.batt` feeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power sensor), so a reversed sensor there hands the site a plausible number it **acts on** — the flag is what stops a control defect. `pv_w` reaches no decision at all, which this review verified independently (comment 2/8).
On the worry that a missing flag is cheap to add later but a wrong sign in a week of history is not — I don't think that lands here, for two reasons:
1. **The error is not silent and not lossy.** Production is unipolar, so a reversed sensor reads −4 kW at noon: glaring in the history, on the status row (`PV production: -4200 W from …`), and on the page. And it is *recoverable* — a negated series is still the measurement, invertible after the fact. A week of reversed PV is an annoyance, not a destroyed dataset.
2. **The flag would make it worse, not better.** An invert flag is a place for an installer to "fix" a reading by eye and produce a *plausible* positive series that is silently wrong — which is the failure mode you cannot detect a week later. The card's own words: "a flag would let an installer hide it." For the one channel whose entire purpose is to be believed later, self-announcing beats configurable.
The trace tool's stimulus set includes a `negative` PV schedule, so the negative path is actually exercised rather than assumed.
### No negative-value alarm — sound, with one residual for a later card
The argument is right on its own terms: a PV inverter's own standby draw makes a small negative reading at night ordinary, and any threshold would be a number nobody has measured. Inventing a constant here to look thorough would be the worse error.
Residual, **not blocking and not in scope for this card**: detection currently depends on a human looking at the page or the history. There is a threshold-free formulation available later — a *daily maximum* that is negative needs no measured constant to be obviously wrong — which would catch a reversed sensor on day one rather than whenever somebody looks. Worth a line in whatever picks up OPT-10's attribution; it is not something to hold this PR for, and I'd rather it be designed against a week of real data than guessed at now.
Review 6/8 — genuinely optional, and it is not a second price_now_entity
The trap named in the brief is real: price_now_entity/price_avg_entity are legal only while price_provider is off, and the add-on refuses to start otherwise — an "optional" key with a companion rule that can brick a boot. Confirmed this card did not create another one:
validate.py does not mention pv_entity at all (grepped; no match). There is no companion rule to get wrong, and nothing else reads the key, so it cannot conflict with anything. The avoidance is structural rather than a rule someone remembered to leave out.
The schema entry is a bare pv_entity: str?.
Unset behaves exactly as today, and this is the part I checked hardest because it is where "optional" usually leaks:
Byte-identical, verified across trees. Trace clause 1/3 compares release/1.0-unset against TEL-11-unset on both documents — control and telemetry, so announcement bytes, status rows, page rows and published frames are all included. Identical. That claim is stronger than "the control path is untouched", and it is the one an existing install cares about.
No read at all.read_inputs() appends the coroutine only if self.pv_entity, so at the shipped default the gather is the same list it has always been — not a coroutine, not an HTTP call, not a log line. test_pv.py asserts the empty state-call list. M10 (gate removed) is red on 2 named checks plus 2 crashes.
No warning spam, no degraded row. Status rows: unset: 10; set: 11. No "PV not configured" nag — deliberately unlike the peak_forecast_entity caution directly above it, and the reasoning for the difference is right: that one nags because an unset forecast leaves a money guard without an input; this is pure instrumentation.
No NOT READY path. A set-but-failing sensor is ok=False, warn=True — a caution. status_payload() builds NOT READY from rows that are neither, so a telemetry channel cannot put a battery site into NOT READY. Asserted by name; M9 is the mutation that makes it a bad row and it is red.
No re-announcement of existing entities.test_mqtt.py: with the option set it is exactly one new topic, appended last, with every preceding config byte-identical and in the same order — checked at a roster of two as well, so no per-unit entity's position shifts. A re-order here would be a zero-byte edit that re-announces every entity on a live install; the suite pins the ordered list precisely because of that.
The include=() default is the right direction of the two available. p1_age uses omit because it is announced by default and withdrawn; pv does not exist unless asked for. Every check written before this key existed keeps grading the announcement that most installs will receive, and tools/rehearsal/announced-reference.txt correctly needs no new row.
One predicate, not two: mqtt.optional_keys() gates the announcement andController.publish(). Two spellings is how you get an entity nothing feeds, or a topic nothing declared — the suite asserts they are the same expression, and M8 (optional_keys() returns ("pv",) unconditionally) is red on 3 named checks.
## Review 6/8 — genuinely optional, and it is not a second `price_now_entity`
The trap named in the brief is real: `price_now_entity`/`price_avg_entity` are legal only while `price_provider` is `off`, and the add-on **refuses to start** otherwise — an "optional" key with a companion rule that can brick a boot. Confirmed this card did not create another one:
- **`validate.py` does not mention `pv_entity` at all** (grepped; no match). There is no companion rule to get wrong, and nothing else reads the key, so it cannot conflict with anything. The avoidance is structural rather than a rule someone remembered to leave out.
- The schema entry is a bare `pv_entity: str?`.
Unset behaves exactly as today, and this is the part I checked hardest because it is where "optional" usually leaks:
- **Byte-identical, verified across trees.** Trace clause 1/3 compares `release/1.0`-unset against `TEL-11`-unset on **both** documents — control *and* telemetry, so announcement bytes, status rows, page rows and published frames are all included. Identical. That claim is stronger than "the control path is untouched", and it is the one an existing install cares about.
- **No read at all.** `read_inputs()` appends the coroutine only `if self.pv_entity`, so at the shipped default the gather is the same list it has always been — not a coroutine, not an HTTP call, not a log line. `test_pv.py` asserts the empty state-call list. M10 (gate removed) is red on 2 named checks plus 2 crashes.
- **No warning spam, no degraded row.** Status rows: `unset: 10; set: 11`. No "PV not configured" nag — deliberately unlike the `peak_forecast_entity` caution directly above it, and the reasoning for the difference is right: that one nags because an unset forecast leaves a *money guard* without an input; this is pure instrumentation.
- **No `NOT READY` path.** A set-but-failing sensor is `ok=False, warn=True` — a caution. `status_payload()` builds `NOT READY` from rows that are neither, so a telemetry channel cannot put a battery site into `NOT READY`. Asserted by name; M9 is the mutation that makes it a `bad` row and it is red.
- **No re-announcement of existing entities.** `test_mqtt.py`: with the option set it is *exactly one new topic*, appended last, with every preceding config byte-identical and in the same order — checked at a roster of two as well, so no per-unit entity's position shifts. A re-order here would be a zero-byte edit that re-announces every entity on a live install; the suite pins the ordered list precisely because of that.
The `include=()` default is the right direction of the two available. `p1_age` uses `omit` because it is announced *by default* and withdrawn; `pv` does not exist unless asked for. Every check written before this key existed keeps grading the announcement that most installs will receive, and `tools/rehearsal/announced-reference.txt` correctly needs no new row.
One predicate, not two: `mqtt.optional_keys()` gates the announcement *and* `Controller.publish()`. Two spellings is how you get an entity nothing feeds, or a topic nothing declared — the suite asserts they are the same expression, and M8 (`optional_keys()` returns `("pv",)` unconditionally) is red on 3 named checks.
Review 7/8 — the three corrections, each confirmed against the tree
1. test_translations.py's key count — the correction is real, and the numbers are right
Run on release/1.0 and on this head, same check:
release/1.0 : "… the two halves of config.yaml hold the same 72 keys, once each" ← hardcoded
TEL-11 : "… the two halves of config.yaml hold the same 76 keys, once each" ← computed
The base tree asserts a sentence saying 72 while actually holding 75 — 76 minus this card's own pv_entity. So the stated history (72 at ac98548, then TARIFF-17, TARIFF-16, SAFETY-10 adding one each) is arithmetically consistent, and the sentence had been false for three cards without a single run going red, because nothing asserted it. len(ORDER) now derives it. A number inside a check's own message is a claim like any other (habit 22), and this one had rotted exactly that way.
2. The suite is 31/31, not 30/30
Confirmed by file count: release/1.0 has 30test_*.py, this head has 31. The CI job log agrees (31 test file(s) to run), and so does my own run in the CI image (31 PASS, exit 0).
3. ⚠️predeploy_check.py will refuse a deploy of this — flagged, not worked around
Confirmed on both sides:
RUNTIME contains goodwe_controller/app/, goodwe_controller/translations/andgoodwe_controller/config.yaml — this PR touches all three.
config.yaml still reads version: "0.10.1", unchanged in this diff.
So the gate will report a NO-OP deploy until a version is cut. The important part for review: tools/predeploy_check.py is not in this diff — it is not in the 13 changed files, the RUNTIME tuple is untouched, and no exemption was added for pv_entity. The PR body raises it as the first of two things explicitly handed to the reviewer rather than decided unilaterally. That is the correct handling: the gate is a real signal about a runtime change shipping behind an unchanged version, and silencing it would have been the defect.
Same for CHANGELOG.md: untouched, because 11b refuses a newest ## heading that is not config.yaml's version:, so opening one takes the packaging decision. Correctly left to PKG.
This PR is therefore mergeable but not deployable. Merging it does not put anything on the owner's house; a PKG version cut is required before a deploy, and that is a separate, explicit authorisation.
## Review 7/8 — the three corrections, each confirmed against the tree
### 1. `test_translations.py`'s key count — the correction is real, and the numbers are right
Run on **`release/1.0`** and on this head, same check:
```
release/1.0 : "… the two halves of config.yaml hold the same 72 keys, once each" ← hardcoded
TEL-11 : "… the two halves of config.yaml hold the same 76 keys, once each" ← computed
```
The base tree asserts a sentence saying **72** while actually holding **75** — 76 minus this card's own `pv_entity`. So the stated history (72 at `ac98548`, then TARIFF-17, TARIFF-16, SAFETY-10 adding one each) is arithmetically consistent, and the sentence had been false for three cards without a single run going red, because nothing asserted it. `len(ORDER)` now derives it. A number inside a check's own message is a claim like any other (habit 22), and this one had rotted exactly that way.
### 2. The suite is 31/31, not 30/30
Confirmed by file count: `release/1.0` has **30** `test_*.py`, this head has **31**. The CI job log agrees (`31 test file(s) to run`), and so does my own run in the CI image (31 PASS, exit 0).
### 3. ⚠️ `predeploy_check.py` will refuse a deploy of this — flagged, not worked around
Confirmed on both sides:
- `RUNTIME` contains `goodwe_controller/app/`, `goodwe_controller/translations/` **and** `goodwe_controller/config.yaml` — this PR touches all three.
- `config.yaml` still reads `version: "0.10.1"`, unchanged in this diff.
So the gate will report a **NO-OP deploy** until a version is cut. The important part for review: **`tools/predeploy_check.py` is not in this diff** — it is not in the 13 changed files, the `RUNTIME` tuple is untouched, and no exemption was added for `pv_entity`. The PR body raises it as the first of two things explicitly handed to the reviewer rather than decided unilaterally. That is the correct handling: the gate is a real signal about a runtime change shipping behind an unchanged version, and silencing it would have been the defect.
Same for `CHANGELOG.md`: untouched, because 11b refuses a newest `## ` heading that is not `config.yaml`'s `version:`, so opening one takes the packaging decision. Correctly left to PKG.
**This PR is therefore mergeable but not deployable.** Merging it does not put anything on the owner's house; a PKG version cut is required before a deploy, and that is a separate, explicit authorisation.
Review 8/8 — scope, one residual for a later card, and the verdict
Scope: 1937 insertions, and they earn their place
lines
verdict
app/pv.py + main.py + mqtt.py
341
the actual runtime change
test_pv.py
643
7 parts, incl. a computed 33-module census with its own red direction exercised
tools/tel11_pv_trace.py
390
the instrument
DECISIONS/ + DOCS.md + MUTATION.md
470
reasoning, operator docs, the habit
config/translations/test_mqtt
93
the key and its announcement
The runtime change is 341 lines for a channel that has to work on any brand HA can read, refuse six unit cases by name, and distinguish three states. That is proportionate.
The 390-line trace tool is the one I'd have challenged on sight, and it does earn its place — because the card's central claim is a negative ("nothing differs"), and a negative cannot be graded by reading a diff. I planted a real control-path leak and the tool caught it on 26 of 41 cells; I forced all three of its refusals and all three fired. It is a working instrument, not ceremony. It also has a stated blind-spot list with shapes (habit 13), including the honest one: it is an identity over a fixed stimulus, so a divergence needing an input the set never produces is invisible by construction — which is why the stimulus is printed with the documents.
One residual — non-blocking, for whoever next touches this
_read_pv() rides in the same asyncio.gather() as the per-unit control reads. That is the right call for latency, and the reasoning in the comment is correct: gathered, its worst case overlaps I/O that already happens, so a hung PV sensor cannot add hass.state()'s 10 s timeout to the coroutine feeding the ESP32 heartbeat.
The narrow gap is exceptions, not latency. Every other member of that gather goes through hass.number(), which cannot raise. _read_pv calls pv.watts() on whatever resp.json() returned, and watts() does state.get(...) — an AttributeError if the body were ever valid JSON but not an object. gather() has no return_exceptions=True, so that would abort the control input read for the cycle, and run_control() is an unsupervised task.
I am deliberately not calling this a defect: it requires the Supervisor Core API to return a 200 with a non-object body for /states/<id>, which is not a thing HA does, and hass.state() already catches every realistic failure (404, timeout, transport, HTTP error) and returns None. It is worth a return_exceptions=True or a defensive isinstance(state, dict) next time that file is open — the cost is one line, and it would make "a telemetry read cannot affect control" true by construction rather than by the JSON body's good behaviour.
Verdict: sound — merging
Everything the card claims was checked against the tree rather than taken from the description, and nothing was found false:
✅ green traced to PR 113 / base release/1.0, attempt 1, a real 8m6s execution
✅ control hashes reproduced; leak counter-test red on 26/41; all three refusals fire
✅ M7 genuinely red; fixture settles at 610 W well inside a 5000 W rail; habit-23 framing correct
✅ three states distinct on the wire; circulation: None still dropped
✅ sign convention derived from shipped DOCS.md; both refusals judged sound
✅ unset byte-identical in control and telemetry; validate.py untouched — no second price_now_entity
✅ 31/31 in the CI image locally, exit 0; key count 72→76 correction real
✅ predeploy NO-OP flagged, not worked around; CHANGELOG.md correctly left to PKG
⚠️Merging this does not deploy it.config.yaml stays at 0.10.1, so predeploy_check.py reports a NO-OP until PKG cuts a version. Nothing reaches the owner's house from this merge.
## Review 8/8 — scope, one residual for a later card, and the verdict
### Scope: 1937 insertions, and they earn their place
| | lines | verdict |
|---|---|---|
| `app/pv.py` + `main.py` + `mqtt.py` | **341** | the actual runtime change |
| `test_pv.py` | 643 | 7 parts, incl. a computed 33-module census with its own red direction exercised |
| `tools/tel11_pv_trace.py` | 390 | the instrument |
| `DECISIONS/` + `DOCS.md` + `MUTATION.md` | 470 | reasoning, operator docs, the habit |
| config/translations/test_mqtt | 93 | the key and its announcement |
The runtime change is 341 lines for a channel that has to work on any brand HA can read, refuse six unit cases by name, and distinguish three states. That is proportionate.
The 390-line trace tool is the one I'd have challenged on sight, and it does earn its place — because the card's central claim is a **negative** ("nothing differs"), and a negative cannot be graded by reading a diff. I planted a real control-path leak and the tool caught it on 26 of 41 cells; I forced all three of its refusals and all three fired. It is a working instrument, not ceremony. It also has a stated blind-spot list with shapes (habit 13), including the honest one: it is an identity over a *fixed* stimulus, so a divergence needing an input the set never produces is invisible by construction — which is why the stimulus is printed with the documents.
### One residual — non-blocking, for whoever next touches this
`_read_pv()` rides in the same `asyncio.gather()` as the per-unit control reads. That is the right call for **latency**, and the reasoning in the comment is correct: gathered, its worst case overlaps I/O that already happens, so a hung PV sensor cannot add `hass.state()`'s 10 s timeout to the coroutine feeding the ESP32 heartbeat.
The narrow gap is **exceptions**, not latency. Every other member of that gather goes through `hass.number()`, which cannot raise. `_read_pv` calls `pv.watts()` on whatever `resp.json()` returned, and `watts()` does `state.get(...)` — an `AttributeError` if the body were ever valid JSON but not an object. `gather()` has no `return_exceptions=True`, so that would abort the **control** input read for the cycle, and `run_control()` is an unsupervised task.
I am deliberately not calling this a defect: it requires the Supervisor Core API to return a 200 with a non-object body for `/states/<id>`, which is not a thing HA does, and `hass.state()` already catches every realistic failure (404, timeout, transport, HTTP error) and returns `None`. It is worth a `return_exceptions=True` or a defensive `isinstance(state, dict)` next time that file is open — the cost is one line, and it would make "a telemetry read cannot affect control" true by construction rather than by the JSON body's good behaviour.
### Verdict: sound — merging
Everything the card claims was checked against the tree rather than taken from the description, and nothing was found false:
- ✅ green traced to **PR 113 / base release/1.0**, attempt 1, a real 8m6s execution
- ✅ control hashes reproduced; leak counter-test red on 26/41; all three refusals fire
- ✅ M7 genuinely red; fixture settles at 610 W well inside a 5000 W rail; habit-23 framing correct
- ✅ three states distinct on the wire; `circulation: None` still dropped
- ✅ sign convention derived from shipped `DOCS.md`; both refusals judged sound
- ✅ unset byte-identical in control *and* telemetry; `validate.py` untouched — no second `price_now_entity`
- ✅ 31/31 in the CI image locally, exit 0; key count 72→76 correction real
- ✅ predeploy NO-OP flagged, not worked around; `CHANGELOG.md` correctly left to PKG
⚠️ **Merging this does not deploy it.** `config.yaml` stays at `0.10.1`, so `predeploy_check.py` reports a NO-OP until PKG cuts a version. Nothing reaches the owner's house from this merge.
admin
merged commit 94da37fc72 into release/1.02026-09-24 15:55:30 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
What this is
pv_entity— one optional key that records the site's PV production soOPT-10's 20.5 kWh of unstored surplus can be attributed. It does not do the
attribution: ship it, let it record a week, OPT-10 picks it up with data.
Three causes produce that number and need three different fixes — no irradiance
(nothing to fix), a charge-power limit, or the optimiser declining to charge
(the only software defect). None of them is separable from a system that records
production nowhere.
The design constraint, honoured
An HA entity, not a register. This add-on's RS485 bus carries the battery
inverter it drives; a household's PV inverter is normally a different box from a
different vendor reporting into HA through its own integration.
pv_entitytakes whatever
sensor.*the site's PV integration provides — any brand HA canread, no assumption that it is on the bus.
The three questions the card asked
1. The sign convention — positive is production, and no
pv_invertDerived rather than assumed, out of the two conventions already shipped
(
DOCS.md, Sources):meter_entitypositive = importing,batt_entitypositive = discharging. Both are "power arriving at the house's AC bus",
and production arrives there too.
An invert flag would be over-building, for two reasons that do not apply to
the keys it would sit beside:
inv.battfeeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power
sensor, so a reversed sensor hands the site a plausible number it acts on.
pv_wreaches no decision at all;reads −4 kW at noon in the first day of the history this card creates. A
flag would let an installer hide it.
No negative-value alarm either — a PV inverter's own standby draw makes a small
negative reading at night ordinary, and any threshold would be a number nobody
measured. Details in
DECISIONS/TEL-11-pv-production.md§2.2. Absence is never zero — and dropped is not absent
publish()skips aNone. That is right forcirculationand exactlywrong here: a skipped row leaves HA holding the last number forever, and the
recorder draws a flat line through the outage — indistinguishable from a
measurement, and it manufactures the "there was no surplus" conclusion this
card exists to test.
So the absence is published, with HA's own payload:
The entity goes to
unknownand the recorder stores a gap. Three states,not two:
pvkeypv: Nonegoodwe_ctl/pv=NonePV —+ a caution naming the reasonpv: 0.0goodwe_ctl/pv=0.0PV 0 WABSENT_AS_UNKNOWNis a one-key set and the suite assertscirculation: Noneis still dropped — widening the rule silently would be a behaviourchange to another card's row. A failed read also clears the last good value.
3. It must not become a control input — graded twice
In the suite (
test_pv.pypart 6): 40 cycles of the realController.cycle(), commands bit-identical acrossunset/no reading/0 W/8 kW;inputs_okunmoved; and a computed census (33 modules underapp/) showing the material is mentioned in exactly three —pv.py,main.py,mqtt.py. The scanner's own red direction is exercised on every run.Across trees (
tools/tel11_pv_trace.py, 41 stimulus cells):⚠️ The control clause is deliberately NOT
safety56_behaviour_trace.py's.That card changed wording, so its
--comparerefuses unless the sentencesdiffer. This card's claim is that with the option unset nothing differs, so
"the documents are identical" is the result and cannot also be the control.
What is proved instead: the two
app/fingerprints differ (else refused),and exactly one side reacts to
pv_entity(else refused).Note clause 1 is stronger than "the control path is untouched": unset is
identical in
controlandtelemetry— announcement and status rowsincluded.
Genuinely optional, and not a second
price_now_entityprice_now_entity/price_avg_entityare legal only whileprice_providerisoffand the add-on refuses to start otherwise. Avoided structurally:validate.pydoes not mentionpv_entity. It cannot conflict with anything,because nothing else reads it.
OPTIONAL_SENSORSlist anddiscovery_payloads()'s newincludeargument defaults to empty — sotest_mqtt.py's fixtures,tools/rehearse_roster.pyand anything written next year announce exactlywhat they announced before. (
p1_agesolves the same problem the other wayround, with
omit, because it is announced by default.)tools/rehearsal/announced-reference.txttherefore needs no new row.row.
test_pv.pyasserts the empty call list.ok=False, warn=True, a caution.status_payload()builds
NOT READYout of rows that are neither, so a telemetry channel cannever put a battery site into
NOT READY.worst case overlaps I/O that already happens. A hung PV sensor cannot add
hass.state()'s 10 s timeout to the coroutine that feeds the ESP32 heartbeat.The unit is read off the entity
mW,W,kW,MW,GW,TW— HA's ownUnitOfPower, every factor anexact power of ten. No unit, or a unit outside that table (including
BTU/h,which is in HA's enum), is refused by name and the page says which.
read_inputs()'sabs(raw) < 50kW-or-W guess is safe for a quarter-hourdemand forecast and is not safe here: 30 W of dusk is an ordinary reading
and that heuristic would publish it as 30 kW.
⚠️ The card's central mutation was a SURVIVOR on the first battery
self.grid -= self.pv_winread_inputs()— exactly the defect the cardforbids — came back green at +106 B, on disk, purged, genuinely running.
The fixture drove a 6 kW export over 12 cycles, which puts the loop
against its rail: a command moving
slew_wevery cycle produces the sameramp for −6000 W and −14 000 W of apparent error. MUTATION.md habit 23,
with a saturated actuator in place of a coinciding value — the habit's list is
all expressions, so this is now its second recorded instance, in that file.
The fixture drives a 600 W import the loop settles on against a responsive
meter, asserts the resting target is well inside the rail, and asks habit 23's
own question as a check: shift the meter by the same 8 kW and confirm the
commands really would have moved.
Evidence
checks.yml's exact command, purgedtree,
:ro, one file at a time.cp -aof the tree — habit18's revert hazard closed by construction rather than by a verified revert.
Byte deltas, sha256s before/after, and which named checks went red:
DECISIONS/TEL-11-pv-production.md§7.⚠️ Two things for the reviewer to decide, not me
tools/predeploy_check.pywill refuse a deploy.app/,config.yamlandtranslations/are all in itsRUNTIMEtuple andconfig.yamlstill reads0.10.1, so the gate reports a NO-OP deploy until a version is cut.That is PKG's call and this card does not take it.
CHANGELOG.mdsection.test_version_gate.py11b refuses a newest##heading that is notconfig.yaml'sversion:, so opening one meanstaking the same packaging decision. The operator account is in
DOCS.md; therelease note belongs to the next PKG ticket, which should say that the next
version adds one optional key, announces no new entity on an install
that does not set it, and changes no control behaviour.
Also in this diff, and worth a glance
test_translations.py's "the two halves of config.yaml hold the same 72keys" was 75 before this card. Verified out of git: 72 at
ac98548(TARIFF-15, where the sentence was written), then 73 (TARIFF-17), 74
(TARIFF-16), 75 (SAFETY-10) — three cards added an option each and none looked
at the sentence, because nothing asserts it. It now reads the count off the
tree, so it cannot rot.
Out of scope, untouched
OPT-10's attribution itself; SAFETY-58, SAFETY-59, TEST-25, CI-10; the 710/790
disagreement;
CHANGELOG.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
OPT-10 measured that this site's pack reaches 90 % SoC on 2 days of 16 and that 20.5 kWh of surplus left the site while the pack had room - 12.6 kWh of it with the battery not charging at all - and could not say WHY, because PV production is recorded NOWHERE. Three causes need three different fixes (no irradiance: nothing to fix; a charge-power limit; the optimiser declining to charge, which is the only software defect) and guessing between them is how an optimiser gets tuned to solve a cloud problem. This card adds the channel. It does not do the attribution. AN HA ENTITY, NOT A REGISTER, AND THAT IS A PRODUCT DECISION. This add-on's RS485 bus carries the BATTERY inverter it drives; a household's PV inverter is normally a different box from a different vendor reporting into Home Assistant through its own integration. `pv_entity` takes whatever `sensor.*` the site's own PV integration provides, any brand HA can read. POSITIVE IS PRODUCTION, AND THERE IS NO `pv_invert`. The convention is derived rather than assumed: `meter_entity` is positive when IMPORTING and `batt_entity` is positive when DISCHARGING (DOCS.md, Sources), both of which are "power arriving at the house's AC bus", and production arrives there too. Those two keys carry invert flags because they are BIPOLAR and are READ BY THE CONTROL PATH - `inv.batt` feeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power sensor, so a reversed sensor hands the site a plausible number it acts on. Production is unipolar and is read by nothing: a reversed sensor announces itself as -4 kW at noon in the very history this card creates, and a flag would only let an installer hide it. No negative-value alarm either - a PV inverter's own standby draw makes a small negative reading at night ordinary, so any threshold would be a number nobody measured. ABSENCE IS NEVER ZERO, AND "DROPPED" IS NOT ABSENT. `publish()` SKIPS a None, which is right for `circulation` and exactly wrong here: a skipped row leaves Home Assistant holding the last number forever and the recorder draws a FLAT LINE through the outage, which is indistinguishable from a measurement and manufactures the "there was no surplus" conclusion this card exists to TEST. So the absence is published, with HA's own payload: `mqtt/const.py` declares `PAYLOAD_NONE = "None"` and `mqtt/sensor.py` tests it BEFORE the numeric branch, so the entity's state becomes `unknown` and the recorder stores a GAP. Read out of home-assistant/core `dev` on 2026-09-24 and quoted at the constant. `ABSENT_AS_UNKNOWN` is a one-key set and the suite asserts `circulation: None` is STILL dropped. THE UNIT IS READ OFF THE ENTITY. `mW`, `W`, `kW`, `MW`, `GW`, `TW` - HA's own `UnitOfPower`, every factor an exact power of ten. A sensor with no unit, or a unit outside that table, is REFUSED by name and the status page says which. `read_inputs()`'s `abs(raw) < 50` kW-or-W guess is safe for a quarter-hour demand forecast and is NOT safe here: 30 W of dusk is an ordinary reading and that heuristic would publish it as 30 kW. GENUINELY OPTIONAL, AND NOT A SECOND `price_now_entity`. validate.py does not mention the key - it cannot conflict with anything, because nothing else reads it. The MQTT row lives in a new `OPTIONAL_SENSORS` list whose `include` argument DEFAULTS TO EMPTY, so every caller that has never heard of `pv_entity` - test_mqtt.py's fixtures, tools/rehearse_roster.py, anything written next year - announces exactly what it announced before. Unset: no state call at all, no entity, no topic, no status row, no page row. Set and failing: a CAUTION (`ok=False, warn=True`), never a `bad` row, so a telemetry channel can never put a battery site into NOT READY. IT MUST NOT BECOME A CONTROL INPUT, AND THAT IS GRADED TWICE. `test_pv.py` part 6 drives 40 cycles of the real `cycle()` and asserts the commands are bit-identical across unset / no reading / 0 W / 8 kW, with a COMPUTED census (33 modules under app/) showing the material is mentioned in exactly three: pv.py, main.py, mqtt.py. `tools/tel11_pv_trace.py` is the cross-tree half - `safety56_behaviour_trace.py`'s pattern with a DIFFERENT control clause, because this card's claim is that nothing differs rather than that only sentences do: it REFUSES unless the two `app/` fingerprints differ and unless exactly one side reacts to the option. Measured on 41 stimulus cells: control release/1.0 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f control TEL-11 unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f control TEL-11 set 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f telemetry release/1.0 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164 telemetry TEL-11 unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164 telemetry TEL-11 set 4e6d1cb289a957223ad0205f732a6fec6110ce1f2032c98ad60694e67625274f UNSET is byte-identical in BOTH documents, which is stronger than "the control path is untouched": an install that does not set the option sees no change of any kind, announcement and status rows included. ⚠️ AND THE CARD'S CENTRAL MUTATION WAS A SURVIVOR ON THE FIRST BATTERY. `self.grid -= self.pv_w` in `read_inputs()` - +106 B, on disk, purged, genuinely running - came back GREEN, because the fixture drove a 6 kW export over 12 cycles, which puts the loop AGAINST ITS RAIL: a command moving `slew_w` every cycle produces the same ramp for -6000 W and for -14 000 W of apparent error. MUTATION.md habit 23 with a saturated ACTUATOR in place of a coinciding value, and it is in that file now as the habit's second instance. The fixture now drives a 600 W import the loop SETTLES on, against a responsive meter, and asks habit 23's own question as its own check: shift the METER by the same 8 kW and confirm the commands really would have moved. Evidence: 31/31 suites green in the CI image, checks.yml's exact command, purged tree, `:ro`. Ten mutations, NO survivors, each on its own `cp -a` of the tree so habit 18's revert hazard is closed by construction - table in `DECISIONS/TEL-11-pv-production.md` §7 with byte deltas and sha256s. ⚠️ THIS IS A RUNTIME CHANGE BEHIND AN UNCHANGED VERSION, AND `tools/predeploy_check.py` WILL REFUSE IT. `app/`, `config.yaml` and `translations/` are all in its `RUNTIME` tuple and `config.yaml` still reads 0.10.1, so the gate reports a NO-OP deploy until a version is cut. That is PKG's call and this card does not take it. No CHANGELOG section: `test_version_gate.py` 11b refuses a newest `## ` heading that is not config.yaml's `version:`, and opening one would mean taking that same packaging decision. The operator account is in DOCS.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjUReview 1/8 — the green is this PR's own, and here is how that was established
CI-10 says a green context on this forge may belong to another PR, because statuses are keyed by head SHA. So the status was not taken at face value. Traced:
07ba3433ed2d9aa8945a8e36cea42994efb618c5and base9f6b9f14cd260a4b84e1561d96a76f9368d6e23ewere read off the remote, not off a local branch.checks / unit (pull_request)→success, pointing at run 825 / job 1950.path = checks.yml@refs/pull/113/head,event = pull_request,run_attempt = 1,head_sha = 07ba343…. Attempt 1, so this is not a replayed stored payload.That is the exact check CI-10 asks for: the payload names PR 113, with base release/1.0, at the base SHA this PR actually targets. It is not PR #102's
PRNUM: 111failure mode.skipped: six steps allsuccess, a real runner (goodwe-rig), 8m6s wall clock, and the suite step reporting31 test file(s) to runwith per-file durations (test_loop.py (93s),test_watchdog.py (67s),test_peakguard.py (67s)). Askippedstatus has no run, no job and notarget_url; this has all three.Independently reproduced locally: the full suite in the same CI image (
goodwe-addon-checks:1), same command shape, purged tree,:romount, one file at a time — 31/31 PASS, exit 0.Review 2/8 — ⭐ the control path is genuinely untouched, and the substituted control clause is sound
This is the whole ticket, so the tool's own output was not taken as evidence. Both trees were re-run from scratch in the CI image (
:ro,PYTHONDONTWRITEBYTECODE=1,__pycache__purged), withtel11_pv_trace.pycopied into a cleanrelease/1.0checkout at9f6b9f1.The hashes reproduce exactly, including the one the card leads with:
The reasoning about SAFETY-56's clause is right
SAFETY-56 changed wording, so "the two sides must differ" was a valid control for that card: identity there would have meant the instrument saw nothing. TEL-11's headline claim is identity, so identity is the result and cannot also be the control without being circular. Declining to reuse that clause is correct, not a convenience.
And its replacement refusals actually refuse — each was fired deliberately
REFUSED: … SAME app/ fingerprint … one tree twice— rc=2pv_entityREFUSED: set differs from unset on NEITHER side— rc=2REFUSED 3/3: the telemetry document did NOT move …— rc=2None of the three is decorative.
The clause that matters was counter-tested with a real planted leak
A control-path leak (
self.grid = self.grid - self.pv_winread_inputs()) was planted in a copy of the head tree, purged, and traced. Clause 2/3 went red:26 of 41 cells — matching
DECISIONS §7's documented figure exactly — and thenone/zero/bad-unitschedules correctly did not move, because subtracting nothing and subtracting zero change nothing. Clause 1/3 stayed green throughout, which is right: the leak only fires when the option is set.So the identity in 2/3 is an identity the instrument could have broken, and did break when given something to find. That is the difference between this and a vacuous green.
Supporting, independently checked:
hass.state()is called from exactly one place in the whole package (_read_pv), so the trace fixture's blanketstate()double cannot contaminate any other read; and the computed census holds —pv_entity|pv_w|pv_why|optional_keys|ABSENT_AS_UNKNOWNappear inmain.py,mqtt.py,pv.pyonly, across 33 modules.Review 3/8 — ⭐ M7 is genuinely red now, the replacement fixture discriminates, and the habit-23 framing is right
M7 is red
The leak was planted on a purged copy of the head tree and
test_pv.pyrun in the CI image:Not a survivor. And note which two went red — see below, because the second one is the interesting half.
The replacement fixture actually discriminates
The concern with the first fixture was that a 6 kW export parks the loop on its rail, where −6000 W and −14000 W of apparent error produce the same
slew_wramp. The new fixture's numbers show it is genuinely off the rail:It settles at 610 W with
max_wat 5000 — comfortably inside the rail, and the suite asserts that as its own precondition (abs(target) > 100 and < 0.5 * max_w) rather than leaving it to be read off. The shifted-meter probe rests 5610 W away. So "the commands did not move" is a fact about the code, not about a saturated actuator.A nice property worth recording: under M7 the habit-23 precondition is itself an exact detector. The mutant makes the 8 kW-PV run see a meter of 600 − 8000 W, which is precisely the shifted control run — so
_h_shift.writes != _w_bigbecomes false and the precondition fails. The check written to guard the identity turns out to catch the mutation on its own.The habit-23 framing is right
Habit 23 is "a mutation that ran and could not change the answer on the data it was given" — distinct from stale bytecode, and invisible to all three things a mutation report is required to state, because all three are about the file and this is about the stimulus. M7 fits that exactly: purged, on disk, non-zero delta, genuinely executed, and still unable to move the answer.
Calling it the second instance is fair. SAFETY-57's
0.0 or xsurvivor is the first; the zero-byte_release/_gradeorder swap recorded beside it is a habit-22 finding, not a no-op mutant. So this is instance two of the habit itself.And the generalisation earns its place rather than just restating the case. Habit 23's existing list —
or,and, truthiness, default arguments,getattrfallbacks — is all expressions with a coinciding value. The new text's claim is that a rail, a clamp, a slew limit, a deadband and a latched alarm are the same hazard in the plant: the place where the controller's output stops being a function of its input. That is a real extension of the habit's domain, and the warning that "this repository's control fixtures park there by default, because the dramatic stimulus is the saturating one" is the actionable part. The restated precondition — not "does it command something" but "does it rest somewhere the mutation could move it from" — is the correct repair.Review 4/8 — ⭐ absence is distinguishable from zero: all three states verified on the wire
Confirmed by running
test_pv.pypart 3 in the CI image and reading the payload bytes, not the prose:Three genuinely distinct states:
pvkey at allpv: Nonegoodwe_ctl/pv=Nonepv: 0.0goodwe_ctl/pv=0.0The two that share a topic are different bytes, which is the acceptance criterion.
"None"is HA's ownPAYLOAD_NONE, taken from the module constant rather than spelled as a literal at the call site, and the suite asserts that too.The reasoning behind this is the strongest argument in the card and it holds up:
publish()skipping aNonewould leave HA holding the last number forever, and the recorder would draw a flat line through the outage. Read back a week later that is indistinguishable from a real measurement — and it manufactures exactly the "there was no surplus" conclusion this card exists to test. Dropping a row is not publishing an absence.Two guards against this becoming a wider behaviour change, both confirmed:
ABSENT_AS_UNKNOWNis a one-key frozenset, and the suite assertscirculation: Noneis still dropped. Widening the rule silently would be a behaviour change to another card's row. M3 in the battery is the mutation that widens it, and it is red on that named check._read_pvassigns on every call including the failing ones. M5 is the mutation that makes a value outlive its sensor, red on 2 named checks. Verified in the part 2 output: the cycle after a 4100 W reading disappears publishes an absence, not 4100 W again.checks()renders the absence as an em dash on the page, not as0 W, which closes the same gap on the operator surface.Review 5/8 — the sign convention is derived, not asserted; both refusals judged sound
The derivation checks out against the shipped tree
The card claims positive = production follows from two conventions already shipped. Verified in
DOCS.mdonrelease/1.0, not on this branch:Both are "positive = power arriving at the house's AC bus", and production arrives there too. So the convention is genuinely read off what the product already does rather than picked and justified afterwards. That is the right way round.
No
pv_invert— soundThe distinction the card draws is the load-bearing one, and it is correct:
meter_entityandbatt_entityare bipolar and read by the control path (inv.battfeeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power sensor), so a reversed sensor there hands the site a plausible number it acts on — the flag is what stops a control defect.pv_wreaches no decision at all, which this review verified independently (comment 2/8).On the worry that a missing flag is cheap to add later but a wrong sign in a week of history is not — I don't think that lands here, for two reasons:
PV production: -4200 W from …), and on the page. And it is recoverable — a negated series is still the measurement, invertible after the fact. A week of reversed PV is an annoyance, not a destroyed dataset.The trace tool's stimulus set includes a
negativePV schedule, so the negative path is actually exercised rather than assumed.No negative-value alarm — sound, with one residual for a later card
The argument is right on its own terms: a PV inverter's own standby draw makes a small negative reading at night ordinary, and any threshold would be a number nobody has measured. Inventing a constant here to look thorough would be the worse error.
Residual, not blocking and not in scope for this card: detection currently depends on a human looking at the page or the history. There is a threshold-free formulation available later — a daily maximum that is negative needs no measured constant to be obviously wrong — which would catch a reversed sensor on day one rather than whenever somebody looks. Worth a line in whatever picks up OPT-10's attribution; it is not something to hold this PR for, and I'd rather it be designed against a week of real data than guessed at now.
Review 6/8 — genuinely optional, and it is not a second
price_now_entityThe trap named in the brief is real:
price_now_entity/price_avg_entityare legal only whileprice_providerisoff, and the add-on refuses to start otherwise — an "optional" key with a companion rule that can brick a boot. Confirmed this card did not create another one:validate.pydoes not mentionpv_entityat all (grepped; no match). There is no companion rule to get wrong, and nothing else reads the key, so it cannot conflict with anything. The avoidance is structural rather than a rule someone remembered to leave out.pv_entity: str?.Unset behaves exactly as today, and this is the part I checked hardest because it is where "optional" usually leaks:
release/1.0-unset againstTEL-11-unset on both documents — control and telemetry, so announcement bytes, status rows, page rows and published frames are all included. Identical. That claim is stronger than "the control path is untouched", and it is the one an existing install cares about.read_inputs()appends the coroutine onlyif self.pv_entity, so at the shipped default the gather is the same list it has always been — not a coroutine, not an HTTP call, not a log line.test_pv.pyasserts the empty state-call list. M10 (gate removed) is red on 2 named checks plus 2 crashes.unset: 10; set: 11. No "PV not configured" nag — deliberately unlike thepeak_forecast_entitycaution directly above it, and the reasoning for the difference is right: that one nags because an unset forecast leaves a money guard without an input; this is pure instrumentation.NOT READYpath. A set-but-failing sensor isok=False, warn=True— a caution.status_payload()buildsNOT READYfrom rows that are neither, so a telemetry channel cannot put a battery site intoNOT READY. Asserted by name; M9 is the mutation that makes it abadrow and it is red.test_mqtt.py: with the option set it is exactly one new topic, appended last, with every preceding config byte-identical and in the same order — checked at a roster of two as well, so no per-unit entity's position shifts. A re-order here would be a zero-byte edit that re-announces every entity on a live install; the suite pins the ordered list precisely because of that.The
include=()default is the right direction of the two available.p1_ageusesomitbecause it is announced by default and withdrawn;pvdoes not exist unless asked for. Every check written before this key existed keeps grading the announcement that most installs will receive, andtools/rehearsal/announced-reference.txtcorrectly needs no new row.One predicate, not two:
mqtt.optional_keys()gates the announcement andController.publish(). Two spellings is how you get an entity nothing feeds, or a topic nothing declared — the suite asserts they are the same expression, and M8 (optional_keys()returns("pv",)unconditionally) is red on 3 named checks.Review 7/8 — the three corrections, each confirmed against the tree
1.
test_translations.py's key count — the correction is real, and the numbers are rightRun on
release/1.0and on this head, same check:The base tree asserts a sentence saying 72 while actually holding 75 — 76 minus this card's own
pv_entity. So the stated history (72 atac98548, then TARIFF-17, TARIFF-16, SAFETY-10 adding one each) is arithmetically consistent, and the sentence had been false for three cards without a single run going red, because nothing asserted it.len(ORDER)now derives it. A number inside a check's own message is a claim like any other (habit 22), and this one had rotted exactly that way.2. The suite is 31/31, not 30/30
Confirmed by file count:
release/1.0has 30test_*.py, this head has 31. The CI job log agrees (31 test file(s) to run), and so does my own run in the CI image (31 PASS, exit 0).3. ⚠️
predeploy_check.pywill refuse a deploy of this — flagged, not worked aroundConfirmed on both sides:
RUNTIMEcontainsgoodwe_controller/app/,goodwe_controller/translations/andgoodwe_controller/config.yaml— this PR touches all three.config.yamlstill readsversion: "0.10.1", unchanged in this diff.So the gate will report a NO-OP deploy until a version is cut. The important part for review:
tools/predeploy_check.pyis not in this diff — it is not in the 13 changed files, theRUNTIMEtuple is untouched, and no exemption was added forpv_entity. The PR body raises it as the first of two things explicitly handed to the reviewer rather than decided unilaterally. That is the correct handling: the gate is a real signal about a runtime change shipping behind an unchanged version, and silencing it would have been the defect.Same for
CHANGELOG.md: untouched, because 11b refuses a newest##heading that is notconfig.yaml'sversion:, so opening one takes the packaging decision. Correctly left to PKG.This PR is therefore mergeable but not deployable. Merging it does not put anything on the owner's house; a PKG version cut is required before a deploy, and that is a separate, explicit authorisation.
Review 8/8 — scope, one residual for a later card, and the verdict
Scope: 1937 insertions, and they earn their place
app/pv.py+main.py+mqtt.pytest_pv.pytools/tel11_pv_trace.pyDECISIONS/+DOCS.md+MUTATION.mdThe runtime change is 341 lines for a channel that has to work on any brand HA can read, refuse six unit cases by name, and distinguish three states. That is proportionate.
The 390-line trace tool is the one I'd have challenged on sight, and it does earn its place — because the card's central claim is a negative ("nothing differs"), and a negative cannot be graded by reading a diff. I planted a real control-path leak and the tool caught it on 26 of 41 cells; I forced all three of its refusals and all three fired. It is a working instrument, not ceremony. It also has a stated blind-spot list with shapes (habit 13), including the honest one: it is an identity over a fixed stimulus, so a divergence needing an input the set never produces is invisible by construction — which is why the stimulus is printed with the documents.
One residual — non-blocking, for whoever next touches this
_read_pv()rides in the sameasyncio.gather()as the per-unit control reads. That is the right call for latency, and the reasoning in the comment is correct: gathered, its worst case overlaps I/O that already happens, so a hung PV sensor cannot addhass.state()'s 10 s timeout to the coroutine feeding the ESP32 heartbeat.The narrow gap is exceptions, not latency. Every other member of that gather goes through
hass.number(), which cannot raise._read_pvcallspv.watts()on whateverresp.json()returned, andwatts()doesstate.get(...)— anAttributeErrorif the body were ever valid JSON but not an object.gather()has noreturn_exceptions=True, so that would abort the control input read for the cycle, andrun_control()is an unsupervised task.I am deliberately not calling this a defect: it requires the Supervisor Core API to return a 200 with a non-object body for
/states/<id>, which is not a thing HA does, andhass.state()already catches every realistic failure (404, timeout, transport, HTTP error) and returnsNone. It is worth areturn_exceptions=Trueor a defensiveisinstance(state, dict)next time that file is open — the cost is one line, and it would make "a telemetry read cannot affect control" true by construction rather than by the JSON body's good behaviour.Verdict: sound — merging
Everything the card claims was checked against the tree rather than taken from the description, and nothing was found false:
circulation: Nonestill droppedDOCS.md; both refusals judged soundvalidate.pyuntouched — no secondprice_now_entityCHANGELOG.mdcorrectly left to PKG⚠️ Merging this does not deploy it.
config.yamlstays at0.10.1, sopredeploy_check.pyreports a NO-OP until PKG cuts a version. Nothing reaches the owner's house from this merge.