Files
goodwe-addon/goodwe_controller/DECISIONS/TEL-11-pv-production.md
T
glenn schrooyenandClaude Opus 5.5 7559d21eda
checks / unit (pull_request) Successful in 8m39s
TEL-14: PV sign declared per entity (pv_invert_entities)
Some PV entities (kWh meters, ESPHome clamps in the PV circuit) read positive
for consumption and negative for production. `pv_invert_entities` lists the
ones to flip; the flip is applied per member inside pv.total(), before the sum,
so the MQTT `pv` row and the status page carry the corrected figure. Default []
is byte-identical to 0.12.0.

Shape measured in supervisor_shape_probe.py section H (Supervisor 2026.08.0):
a parallel list validates the 0.12.0 blob unchanged and renders as its own
translated repeatable field; dicts on pv_entities refuse 0.12.0's stored
strings; a prefix is invisible syntax; a global bool cannot describe a mixed
roster. Status page marks flipped entities and cautions per entity below
-50 W, saying ADD or REMOVE. Telemetry only: control documents unchanged.

Record: DECISIONS/TEL-14-pv-sign.md (reverses TEL-11 section 2).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
2026-09-25 00:25:24 +02:00

20 KiB
Raw Blame History

TEL-11 — the PV production channel

What was decided, what it was decided against, and what the evidence was.

0. The problem, in OPT-10's numbers

Over sixteen days at the reference site the pack reached 90 % SoC on two of them, and 20.5 kWh of surplus left the site while the pack still had room — 12.6 kWh of it with the battery not charging at all. That single measurement is upstream of most of what else was found that week: the nightly floor, SAFETY-37's misdiagnosis, the SAFETY-57 deadlock, a 2 h 47 min saturation freeze.

It cannot be attributed, because PV production is recorded nowhere in this system. Three causes need three different fixes:

cause fix
there was no irradiance none — the site behaved correctly
production was there, charge power was limited a limit, somewhere
production was there, the pack had room, the optimiser declined the only software defect

Guessing between them is how an optimiser gets tuned to solve a cloud problem. This card adds the channel and does not do the attribution. Ship it, let it record a week, OPT-10 picks it up with data.

1. It is an HA entity, not a register — and that is a product decision

The RS485 bus this add-on speaks carries the battery inverter it drives. On nearly every site the PV inverter is a different box from a different vendor, reporting into Home Assistant through its own integration; it is not on this bus and there is no reason it would be. So the signal comes from an entity: pv_entity takes whatever sensor.* the site's PV integration provides, any brand HA can read.

Reading a register would have been cheaper on one house and wrong for a fleet — the trade product-not-one-house names.

2. The sign convention: positive is production, and there is no pv_invert

Reversed by TEL-14 (flag and negative caution): DECISIONS/TEL-14-pv-sign.md.

The convention is derived, not assumed

Two sign rules already ship, and both are recorded in DOCS.md's Sources table rather than inferred here:

  • meter_entity — positive must mean importing (meter_invert flips it);
  • batt_entity — + = discharging (batt_invert flips it).

Both are "positive = power arriving at the house's AC bus". Production arrives there too, so positive = producing is the continuation of the shipped convention rather than a fresh choice.

Why an invert flag would be over-building

The question is not "could a sensor be reversed" — it could. It is what a flag would buy, and the answer is nothing, for two reasons that do not apply to the two keys it would sit beside:

  1. Those two quantities are read by the control path; this one is read by nothing. inv.batt feeds SAFETY-30's per-unit sign check and SAFETY-31's circulating-power sensor (_read_unit()'s own docstring says so), so a reversed battery sensor hands the site a plausible number it then acts on — a latch that trips when it should not, or does not trip when it should. pv_w reaches no decision at all (§4), so a reversed PV sensor cannot mislead one.
  2. Production is unipolar, so the error is self-announcing. Battery power and grid power are legitimately negative half the time, which is exactly why a flipped sign is invisible in them. A PV array does not consume: a reversed sensor reads −4 kW at noon, in the first day of the very history this card exists to create. An invert flag would let an installer hide that by flipping it to something that looks right, and a second place for the sign to be wrong is a second thing to get wrong.

So: the convention is documented in config.yaml, both translations, DOCS.md and app/pv.py, the value is published exactly as the sensor reports it, and there is no flag. test_pv.py part 1 pins -8 W through unchanged.

And no negative-value alarm either

The tempting middle path — warn when the reading is negative — needs a threshold, because a PV inverter's own standby draw makes a small negative reading at night entirely ordinary. Any threshold here would be a number nobody measured, in a card whose whole subject is not fabricating numbers. The published row is the detector.

3. Absence is never zero, and it is not "dropped" either

This is the constraint that shaped the publication, and the obvious implementation fails it.

MqttPublisher.publish() skips a None value. That is right for circulation — its note in main.py says a missing per-unit reading should leave the last figure standing rather than publish a zero that reads as health — and it is exactly wrong here: a skipped row leaves Home Assistant holding the last number it was told, forever, and the recorder then draws a flat line at that value through the outage. Read back a week later that is indistinguishable from a real measurement, and if the last value happened to be small it manufactures precisely the "there was no surplus to store" conclusion the card exists to test.

So TEL-11 publishes the absence, using Home Assistant's own mechanism:

homeassistant/components/mqtt/const.py: PAYLOAD_NONE = "None". homeassistant/components/mqtt/sensor.py, in the state-message handler: if payload == PAYLOAD_NONE: self._attr_native_value = None; return — and it is tested before the numeric branch, so it holds for a row carrying device_class: power. (github.com/home-assistant/core, branch dev, retrieved 2026-09-24. The shipped MQTT-sensor documentation says the same thing in its state_topic entry: "a 'None' value will set the sensor to an unknown state".)

The entity therefore goes to unknown and the recorder stores a gap. mqtt.ABSENT_AS_UNKNOWN is a one-key set and test_pv.py part 3 asserts both that the pv row gets this treatment and that circulation still does not — widening the rule silently would be a behaviour change to another card's row.

⚠️ What was NOT built, and why. A per-entity availability block (availability_mode: all, an extra retained topic) would also produce a distinguishable state — unavailable rather than unknown — but it needs a second topic, a second retained message, a change to _payload()'s byte-order-sensitive key construction, and it survives restarts only if the retention is right. The payload route is one branch in publish() and one constant, and it is graded as bytes on the topic rather than as a claim about HA's availability handling.

Three states, not two

state frame wire page
no source (pv_entity unset) 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

A failed read also clears the last good value (_read_pv() assigns on every call), so a reading never outlives its own sensor. test_pv.py part 2 drives a good read followed by a dead one, because a fixture that answers the same thing forever cannot reach that failure — MUTATION.md habit 9's shape.

4. It must not become a control input, and that is graded twice

In the suite

test_pv.py part 6 drives twelve cycles of the real Controller.cycle() against a 6 kW export and asserts the commands written to the inverter are bit-identical across unset / no reading / 0 W / 8 kW of production, with a precondition check that the stimulus commands something at all (habit 1). inputs_ok is asserted not to move either: a broken PV integration is not a missing input, because nothing computes with it.

A computed census (habit 12 — the scope is derived from the tree, not typed) then reports which of the 33 modules under app/ mention the material in any spelling, comments included. It is three: pv.py, main.py, mqtt.py. The scanner's own red direction is exercised on every run.

Across trees, by hash

tools/safety56_behaviour_trace.py is the pattern; tools/tel11_pv_trace.py is this card's version of it, and its control clause had to be different, which is worth stating because the difference is the interesting part.

SAFETY-56 changed wording and had to show behaviour did not move, so its --compare refuses unless the sentences halves differ — otherwise the two runs might be one tree twice. TEL-11's headline claim is the opposite shape: with the option unset, nothing differs. "The documents are identical" is the result, so it cannot also be the control. The two things proved instead are:

  • the two runs came from different trees (app/*.py sha256 + byte count, compared, and an identity is refused);
  • exactly one side reacts to pv_entity — that side is the HEAD, the other is a tree that does not have the option. Neither, or both, is refused.

It drives 41 stimulus cells (5 grid worlds × 8 PV schedules, plus a run stopped part-way) and prints two documents per setting: control — every command written, every self.target, inputs_ok, the loop's three fields, the arbiter's resolution, the maintenance phase, tick by tick, with no TEL-11 material in it — and telemetry, which is everything the card is allowed to move.

Measured, in the CI image, on purged trees:

  HEAD is B; baseline is A (it does not react to `pv_entity`).
  tree      A 73296909170ec789237d6e704cce9ab0d9f83b3df1495a14d2497285c3041a12
  tree      B d5011b6a18e2e3803dc09cac0214b0378fa4c7388fc0ad8ad89b832a5c92af48
  control   A unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
  control   B unset 846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
  control   B set   846a10a5b89e321086d535825925721e2f791012e8405948d4fd38d3f9d0be5f
  telemetry A unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
  telemetry B unset a443a6ceaffe072b44bcd5f2ad946c1ed6c9e01b69e278eca631f7f4d7892164
  telemetry B set   4e6d1cb289a957223ad0205f732a6fec6110ce1f2032c98ad60694e67625274f
OK  1/3  UNSET: control AND telemetry are byte-identical to the baseline tree.
OK  2/3  SET: the control document is byte-identical with the channel switched on.
OK  3/3  CONTROL OF THE CONTROL: the telemetry document DOES move when the
         option is set (42 stimulus cells differ).

Note what clause 1 claims and clause 2 does not: unset is identical in control and telemetry, which is stronger than "the control path is untouched" — an install that does not set the option sees no change of any kind, including its announcement and its status rows. Set is identical in control only, which is the whole of "instrumentation, not an input".

5. 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. That is the trap this card was told not to rebuild, and it is avoided structurally rather than by intention:

  • validate.py does not mention pv_entity at all. It cannot conflict with anything, because nothing else reads it.
  • the MQTT row lives in OPTIONAL_SENSORS, not SENSORS, and discovery_payloads()'s new include argument defaults to empty. So a caller that has never heard of pv_entity — every fixture in test_mqtt.py, tools/rehearse_roster.py, anything written next year — announces exactly what it announced before. p1_age solves the same problem the other way round, with omit, because it is announced by default and withdrawn when the meter is off; getting that direction wrong here would have left most installs with an entity nothing feeds, which is the hazard omit's own note names.
  • the read is gated on the option, so an unset install makes no state call at all — not a read that fails quietly.
  • there is no status row when it is unset. peak_forecast_entity nags when it is unset because an unset forecast leaves a money guard without an input; this is instrumentation, and a permanent caution on every install that never asked for a PV channel is the warning spam the card forbids.
  • when it is set and the read fails, the row is ok=False, warn=True — a caution. status_payload() builds NOT READY out of rows that are neither ok nor warn, so a telemetry channel can never put a battery site into NOT READY.

6. The unit is read off the entity, and an unreadable one is refused

read_inputs() guesses kW-or-W for peak_forecast_entity with abs(raw) < 50, and that is safe there: nobody's quarter-hour demand forecast is 50 W. It is not safe here — 30 W of real production at dusk is an ordinary reading, and that heuristic would publish it as 30 kW.

So pv.watts() reads the entity's own unit_of_measurement and converts against Home Assistant's own UnitOfPower table (mW, W, kW, MW, GW, TW; 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. The refusal is loud; a guess would be silent, and a factor of 1000 in the one channel that exists to be believed a week later is the worse failure.

⚠️ BTU/h is in HA's enum and is deliberately not in the table: its factor is a measured conversion rather than a decimal shift, no PV integration publishes in it, and a fabricated constant is what this repository's whole process exists to prevent. It lands in the refusal, by name, and test_pv.py part 1 pins that.

7. The mutation battery

Every check this card adds was driven by the mutation that defeats the claim it names. Method: cp -a of the tree per mutation into a unique directory, the edit applied there, __pycache__ purged, the suites run in the CI image with the tree bind-mounted :ro, one file at a time. The shipped tree is never edited, so habit 18's revert hazard — SAFETY-37's harness killed between the edit and the cp that put the file back, and two harnesses sharing one backup path — is closed by construction rather than by a verified revert. Each row below records the byte delta, the file's sha256 before and after, and that the mutated line was read back off disk before the run started.

# mutation file bytes sha256 (first 16) before → after red on
M1 the pv row moved into SENSORS, i.e. announced on every install mqtt.py −79 B 328d249b9d698ef6 → 532d3954c023bf3d 3 named in test_mqtt.py, 1 in test_pv.py
M2 publish() drops a None pv row again, so HA keeps the last number mqtt.py −98 B 328d249b9d698ef6 → dd54be458f446475 1 named (the acceptance criterion itself)
M3 ABSENT_AS_UNKNOWN widened past the one key it was written for mqtt.py +23 B 328d249b9d698ef6 → 8fe571ebff3dacf4 1 named (circulation must still be dropped)
M4 a dead sensor reported as 0 W instead of an absence pv.py −1 B 55ba7471e9995436 → 9eecc34152b7ac12 4 named
M5 _read_pv() assigns only on a good read — a value outlives its sensor main.py +46 B c4d2cab34e50b5fd → 189802aa69ce166d 2 named
M6 the unit is read and then ignored — a kW sensor published as watts pv.py −18 B 55ba7471e9995436 → ab19ef66c52f9379 2 named
M7 self.grid = self.grid − self.pv_w in read_inputs() — PV wired into the control path main.py +106 B c4d2cab34e50b5fd → c38216edb7c5c8db 2 named, plus tel11_pv_trace.py clause 2/3 on 26 of the 41 stimulus cells
M8 optional_keys() returns ("pv",) unconditionally mqtt.py −52 B 328d249b9d698ef6 → 86ff23efe3088ad8 3 named
M9 the PV caution made a bad row, i.e. a dead sensor is a NOT READY main.py +1 B c4d2cab34e50b5fd → 45c74cab89943564 1 named
M10 the read gate removed — an unset option is read once a second forever main.py −10 B c4d2cab34e50b5fd → b67baa8aba3019e8 2 named, plus two crashes

on_disk = True for all ten: the mutated line was grepped back out of the file after the edit and before the run (MUTATION.md reporting rule 2), and no mutation is size-preserving, so rule 1's +0 B caveat does not arise.

⚠️ M10's two extra reds are CRASHES, not named checks, and that is recorded rather than tidied away. Every Hass double in test_inverter.py and test_telemetry.py predates this read and has no state() method, so an unconditional PV read is an AttributeError rather than a failed assertion — habit 20's shape, a red with no name attached. Fixing those fixtures would be a change to what their checks report and belongs to whoever next touches them; this card's own fixture has state() and grades the property by name.

⚠️ M7's trace limb is where the two halves meet. The mutant moves the control document on 26 of 41 cells — every cell whose PV schedule produces a READING. The five none cells and the five zero cells do not move, which is correct and is worth seeing: subtracting nothing, and subtracting zero, change nothing. A battery that only drove a dead sensor would have reported this mutation as a survivor for the third time.

The clean tree those before hashes belong to: app/main.py c4d2cab34e50b5fd, app/mqtt.py 328d249b9d698ef6, app/pv.py 55ba7471e9995436.

⚠️ M7 was a SURVIVOR on the first battery, and the fixture was the reason

The card's central mutation — self.grid = self.grid - self.pv_w in read_inputs(), i.e. exactly the defect the whole card forbids — came back green on a six-file run at +106 B, on disk, purged, genuinely running.

The property did not hold; the fixture could not reach it. test_pv.py part 6 drove a 6 kW export over 12 cycles, which puts the loop against its rail: the command moves slew_w every cycle and −6000 W and −14 000 W of apparent error produce the same ramp, tick for tick. MUTATION.md habit 23, with a saturated actuator in place of a coinciding value — and the habit's list of no-op-prone constructs is all expressions, so this is worth its own entry, which it now has.

The fixture now drives a 600 W import the loop settles on, with a responsive meter (grid = base − batt) so the error actually closes, asserts the resting target is well inside the rail, 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. On the first fixture that question answers no, in one line. Every number in the table above is from the battery re-run against the fixed tree.

⚠️ A red count is a property of the suite as much as of the mutation. These name this tree at this commit; a later card that adds or re-bases a check moves the number with the mutation untouched. Re-measure rather than inherit.

8. What none of this covers, with shapes

  • Home Assistant itself. That None renders as unknown and that the recorder keeps a gap is read out of HA's source and quoted in §3; no HA runs in this suite. What is graded here is the bytes this add-on publishes.
  • Whether the operator's sensor tells the truth. A sensor wired backwards, or reporting DC where AC was meant, or a string instead of the array, is published faithfully. §2 is the argument for why that is the right failure mode; it is not a claim that it cannot happen.
  • A roster of more than one. The row is a site row — there is one PV array as far as this add-on is concerned — so nothing per-unit exists to separate, and every fixture is the shipped roster of one.
  • The attribution. This card records production. Whether the 20.5 kWh was cloud, a limit or the optimiser is OPT-10's, and it needs a week of this channel's history before it can be asked.
  • tools/, the firmware and the ingress HTML are outside part 6's census.
  • The identity is only as wide as the stimulus. tel11_pv_trace.py prints its 41 cells with the documents for exactly that reason.

9. No CHANGELOG.md entry

test_version_gate.py 11b refuses a newest ## heading that is not config.yaml's version:, and opening an ## Unreleased section would mean taking a packaging decision this card does not get to take. The operator account is in DOCS.md; the release note belongs to the next PKG ticket, which should say that 0.10.1 → next adds one optional key, announces no new entity on an install that does not set it, and changes no control behaviour.