From 29ca9b5157cd9f80969844818f6fae9418aedbbb Mon Sep 17 00:00:00 2001 From: admin Date: Thu, 24 Sep 2026 23:24:50 +0200 Subject: [PATCH] DOC-1: operator docs name the real P1 entity and a roster that starts - DOCS.md / config.yaml: the watchdog's P1 age entity is sensor.goodwe_p1_sample_age (device "GoodWe" + "P1 sample age", as app/mqtt.py announces and the live install recorded), not sensor.p1_sample_age_s. - DOCS.md: the inverters: example and per-entry table carry the required bms_soc_floor (and capacity_kwh); config.yaml's schema comment says four required keys, not three. - Field guide: marked v0.1.0/outdated; slew_w row no longer recommends the 1000 W that SAFETY-39 now refuses. HTML regenerated; the PDF is NOT rebuilt (no Chrome here). README/DOCS pointers fixed. - test_docs.py: the documented roster must pass app/validate.py and the schema's per-entry keys; the documented entity id must equal the id composed from app/mqtt.py; the wrong id may appear in no shipped doc. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- FIELD-GUIDE.md | 7 +- README.md | 6 +- docs/build_guide.py | 4 +- docs/field-guide.html | 4 +- goodwe_controller/DOCS.md | 23 ++-- goodwe_controller/config.yaml | 11 +- goodwe_controller/test_docs.py | 210 +++++++++++++++++++++++++++++++++ 7 files changed, 243 insertions(+), 22 deletions(-) create mode 100644 goodwe_controller/test_docs.py diff --git a/FIELD-GUIDE.md b/FIELD-GUIDE.md index 2f0f486..1d71b4b 100644 --- a/FIELD-GUIDE.md +++ b/FIELD-GUIDE.md @@ -4,6 +4,10 @@ The guide technicians carry is the PDF: **`docs/GoodWe-RS485-Field-Guide.pdf`** — 10 pages, A4, print it double-sided. +⚠️ **Outdated: it is v0.1.0.** Its `slew_w` row is corrected in `build_guide.py` +and `field-guide.html`, but the PDF has not been rebuilt (see below). Where the +guide disagrees with `goodwe_controller/DOCS.md`, DOCS.md wins. + It covers, in the order the work is done: pre-visit checks, hardware, firmware, add-on install and configuration, how to read the Web UI, the six commissioning gates, the e-stop (and the statement a site without one must sign), proving a @@ -18,7 +22,8 @@ copy cannot drift from the PDF someone is holding in a cellar. python3 docs/build_guide.py # regenerates field-guide.html and the PDF ``` -Requires Chrome or Chromium (used headless for print-to-PDF). Screenshots live +Requires Chrome or Chromium (used headless for print-to-PDF); `CHROME` in the +script is a Windows path, so edit it on any other machine. Screenshots live in `docs/img/` and are base64-embedded, so `field-guide.html` is standalone. ⚠️ **Every section must stay under the A4 printable height** (1039 px at 96 dpi diff --git a/README.md b/README.md index cd500c7..037f702 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ goodwe_controller/ the add-on returns, run against its own release image firmware/goodwe-master.yaml ESPHome config for the T-CAN485 estop/rs485_log.py optional RS485 e-stop / bus witness -docs/GoodWe-RS485-Field-Guide.pdf the guide techs carry (10pp, A4) +docs/GoodWe-RS485-Field-Guide.pdf the guide techs carry (10pp, A4; outdated) docs/build_guide.py its single source - rebuilds the HTML and PDF FIELD-GUIDE.md how to edit and rebuild the guide docs/RELEASE.md how a change reaches the battery - `master` is @@ -79,8 +79,8 @@ machine itself dying: 2. **Wind-down before firmware updates** — 0 W written before the update starts. 3. **RS485 e-stop (optional)** — writes 0 W after 30 s of total bus silence. -Sites sold without the e-stop must have the acknowledgement in `FIELD-GUIDE.md` -§10 signed. +Sites sold without the e-stop must have the client acknowledgement in the field +guide (Step 7b) signed. ## Development diff --git a/docs/build_guide.py b/docs/build_guide.py index 82f80a8..f006f71 100644 --- a/docs/build_guide.py +++ b/docs/build_guide.py @@ -95,7 +95,7 @@ figcaption { font-size:9pt; color:#5b6169; margin-top:1.2mm; }

GoodWe RS485 Controller

-
Field installation & commissioning guide — v0.1.0
+
Field installation & commissioning guide — v0.1.0, outdated: where it disagrees with DOCS.md, DOCS.md wins
Installer copy
@@ -503,7 +503,7 @@ figcaption { font-size:9pt; color:#5b6169; margin-top:1.2mm; } - + diff --git a/docs/field-guide.html b/docs/field-guide.html index ddbb264..77570b2 100644 --- a/docs/field-guide.html +++ b/docs/field-guide.html @@ -60,7 +60,7 @@ figcaption { font-size:9pt; color:#5b6169; margin-top:1.2mm; }

GoodWe RS485 Controller

-
Field installation & commissioning guide — v0.1.0
+
Field installation & commissioning guide — v0.1.0, outdated: where it disagrees with DOCS.md, DOCS.md wins
Installer copy
@@ -468,7 +468,7 @@ figcaption { font-size:9pt; color:#5b6169; margin-top:1.2mm; }
SettingValueReason
gain0.6Inverter needs 3–6 s to settle (1.4 s dead time, 94 % by 3.3 s) against a ~5 s cycle. At the limit — do not raise
slew_w1000Most of a correction in the first cycle without running far ahead of the hardware
slew_w00 follows the rail (max_w / 9). A faster fixed value is refused at startup (SAFETY-39) — see DOCS.md
deadband_w15Measured residual: mean 15.4 W, max 27 W. At 10 W the command never rests
saturation_w / cycles500 / 3Divergence = inverter at a limit → magnitude may fall, never rise. The 3-cycle term is essential: tested instantly it fires on every large correction
step_w10Register is 1 W but response lands on a ~17.6 W ladder. Finer is meaningless
- + diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 0ca2edb..837742b 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -4,8 +4,8 @@ Drives a GoodWe ES/BP battery inverter over its RS485 meter bus: holds net grid exchange at zero, and runs a monthly battery maintenance cycle so the BMS can balance cells and recalibrate its coulomb counter. -Installers: read `FIELD-GUIDE.md` in the repository. It is not optional reading — -it contains the commissioning gates and the failure modes. +Installers: the printed field guide (`docs/GoodWe-RS485-Field-Guide.pdf`) is +v0.1.0 and outdated; where it disagrees with this page, this page wins. ## Before you start @@ -165,12 +165,14 @@ inverters: soc_entity: sensor.goodwe_a_soc batt_entity: sensor.goodwe_a_battery_power max_w: 5000 + bms_soc_floor: 10 capacity_kwh: 15.36 - name: inverter-b setpoint_entity: number.goodwe_b_setpoint_w soc_entity: sensor.goodwe_b_soc batt_entity: sensor.goodwe_b_battery_power max_w: 4600 + bms_soc_floor: 10 capacity_kwh: 20.48 ``` @@ -190,16 +192,18 @@ inverter's own **AC** power. If it is DC the AC-side ceiling is nearer 4324 W. | `name` | yes | Short identifier. It namespaces this unit's SAFETY-02 latch, heartbeat and write bookkeeping in the add-on's state file, so **two units may not share one** | | `setpoint_entity` | yes | This inverter's own ESPHome `number.*_goodwe_setpoint_w` | | `max_w` | yes | The supervised cap on what may be written to **this** inverter. The site's ceiling is the **sum** | +| `bms_soc_floor` | yes | This pack's own BMS cut-out percent (SAFETY-32). See [`bms_soc_floor`](#tariff-all-optional) | | `soc_entity` | | This pack's state of charge | | `batt_entity` | | This unit's battery power, `+` = discharging | | `batt_invert` | | Flip if needed | +| `capacity_kwh` | | This pack's usable capacity. State it on every entry or on none — see [OPT-09](#how-the-site-target-is-divided-between-the-packs-opt-09) | ⚠️ **Nothing is inherited.** An entry that leaves out `soc_entity` has **no SoC sensor** — it does *not* fall back to the flat one. That is deliberate: every per-device value became per-instance in 0.5.x precisely so that no inverter is ever graded against a sibling's numbers, and a silent fallback would rebuild -that in YAML. `name`, `setpoint_entity` and `max_w` are required of every entry -for the same reason: there is no rating to borrow. +that in YAML. `name`, `setpoint_entity`, `max_w` and `bms_soc_floor` are required +of every entry for the same reason: there is no rating or cut-out to borrow. ⚠️ **Clear the flat `setpoint_entity` when you declare a roster,** or the add-on refuses to start and says so. It is the single-inverter form and it is not read @@ -600,7 +604,7 @@ than in YAML nobody reviews underneath a safety input. > `ha_signed`.** It reads the same meter and produces the same numbers, but it > polls the meter directly instead of watching a Home Assistant entity — and > that is the difference between an age sensor a watchdog can threshold and one -> it cannot. See "`sensor.p1_sample_age_s`" below; `ha_signed` remains for +> it cannot. See "`sensor.goodwe_p1_sample_age`" below; `ha_signed` remains for > installs where the meter is only reachable through Home Assistant. > ⚠️ **`ha_dsmr` and `mqtt_p1` have never processed a telegram from real @@ -755,7 +759,7 @@ connection-level reading is still accepted on its own. **Why this mode exists:** every HTTP response is an *arrival*. The meter answered, now, with its current reading — whether or not the number moved. That -is the signal `sensor.p1_sample_age_s` needs and the one Home Assistant cannot +is the signal `sensor.goodwe_p1_sample_age` needs and the one Home Assistant cannot give it at all (see the note below). It is also simply fewer moving parts: the five-second cadence is the meter's own, rather than an integration's polling of it re-published as a state change. @@ -792,12 +796,13 @@ as `… 4 rejected, measurement unchanged for 312 s`. On a house drawing real power that figure stays in the seconds; minutes of it while the load is clearly not near zero is the meter to go and look at. -#### `sensor.p1_sample_age_s` +#### `sensor.goodwe_p1_sample_age` Published over MQTT discovery whenever a broker is available: **seconds since the newest accepted telegram**, refreshed every second rather than only when a telegram lands. The ESP32's stale-input watchdog subscribes to this exact entity -id, so do not rename it. +id; do not rename the `GoodWe` device or this entity, since HA composes the id +from both (see [What appears in Home Assistant at two inverters](#what-appears-in-home-assistant-at-two-inverters)). The reason it is recomputed against the clock is that Home Assistant only pushes a state when the state *changes*. A meter sitting at a genuinely constant reading @@ -833,7 +838,7 @@ behind it would trip the firmware watchdog on a system that is working fine. > went **42.2 s and 97.0 s** between changes, and 23 Aug peaks at 29.1 s — all > past the default `meter_max_age_s` of 30. > -> So `sensor.p1_sample_age_s` on `ha_signed` is safe to *read*, and it is +> So `sensor.goodwe_p1_sample_age` on `ha_signed` is safe to *read*, and it is > correct whenever the value is moving, but it must not yet be thresholded by > the ESP32 stale-input watchdog: a quiet house would trip the battery to 0 W. > Raising `meter_max_age_s` is **not** the fix — the two conditions produce an diff --git a/goodwe_controller/config.yaml b/goodwe_controller/config.yaml index 2e301b3..d3a9493 100644 --- a/goodwe_controller/config.yaml +++ b/goodwe_controller/config.yaml @@ -93,7 +93,7 @@ options: # against a sibling's numbers; an entry silently inheriting `soc_entity` would # rebuild that defect in YAML. `name`, `setpoint_entity`, `max_w` and # `bms_soc_floor` are therefore REQUIRED of every entry (the schema below enforces it, and - # validate.py rule 9 enforces it again for the callers the Supervisor never + # validate.py rules 8 and 9 enforce it again for the callers the Supervisor never # sees). An omitted `soc_entity`/`batt_entity` means THIS UNIT HAS NO SUCH # SENSOR - `inputs_ok` already stops the site for that - and never means # "use the other one's". @@ -224,7 +224,7 @@ options: # ha_signed reads ONE signed HA entity (+ import / - export), which is what a # HomeWizard P1 publishes and what ha_dsmr cannot consume. # homewizard_local skips Home Assistant entirely and polls the meter's own - # local API. ⚠️ It is the ONLY mode whose sensor.p1_sample_age_s measures when + # local API. ⚠️ It is the ONLY mode whose sensor.goodwe_p1_sample_age measures when # the meter REPORTED rather than when the value last CHANGED - HA emits # nothing at all for a repeated reading - so it is the only mode a firmware # watchdog may threshold. See DOCS.md. @@ -916,12 +916,13 @@ schema: # batteries, whereas an absent capacity has an honest answer - no opinion # about the split, every unit weighs 1.0, which is what shipped. # - # ⚠️ THREE KEYS ARE REQUIRED PER ENTRY AND THAT IS DELIBERATE. An entry with + # ⚠️ FOUR KEYS ARE REQUIRED PER ENTRY AND THAT IS DELIBERATE. An entry with # no `name` has no Store namespace and would share another unit's SAFETY-02 # latch; one with no `setpoint_entity` is a unit nothing can command; one with # no `max_w` would have to borrow a rating, which is the sibling-grading - # defect DEV-02 removed. The optional pair is the sensors, whose absence - # honestly means "this unit reports none". + # defect DEV-02 removed; `bms_soc_floor` is SAFETY-32's (options block above). + # The optional keys are the sensors, whose absence honestly means "this unit + # reports none", and `capacity_kwh`. # # ⚠️ THIS BLOCK IS DECLARED BEFORE `max_w:` ON PURPOSE. `max_w` now appears # TWICE under `schema:` - once nested here, once flat below - and a reader diff --git a/goodwe_controller/test_docs.py b/goodwe_controller/test_docs.py new file mode 100644 index 0000000..43fe729 --- /dev/null +++ b/goodwe_controller/test_docs.py @@ -0,0 +1,210 @@ +"""Runnable check that the operator docs say what the code does. `python3 test_docs.py` + +DOC-1. Two things the docs got wrong and an installer would have copied: + +1. The `inverters:` example in DOCS.md must be ACCEPTED by the shipped + validator (`app/validate.py`) and carry every key config.yaml's schema + requires per entry. It omitted `bms_soc_floor` and would not start. +2. The P1 age entity the ESP32 watchdog subscribes to must be documented under + the id Home Assistant composes from what `app/mqtt.py` announces, and the + old wrong id must appear in no shipped doc. + +Each part also runs its own mutation (habit 1 of MUTATION.md): the spoilt input +must turn it red, or the green above it means nothing. +""" + +import os +import re +import sys + +from app import mqtt +from app.validate import validate_startup_config + +HERE = os.path.dirname(os.path.abspath(__file__)) +REPO = os.path.dirname(HERE) + +fails = [] + + +def check(name, cond): + print(f" {'ok ' if cond else 'FAIL'} {name}") + if not cond: + fails.append(name) + + +def read(*parts): + with open(os.path.join(*parts), encoding="utf-8") as fh: + return fh.read() + + +DOCS = read(HERE, "DOCS.md") +CFG = read(HERE, "config.yaml") + + +# --------------------------------------------------------------------------- # +# A reader for the two YAML shapes used here. No PyYAML in the checks image +# (test_defaults_pin.py says why), so this is by hand, and part 1a checks it +# actually found something. +def scalar(raw): + raw = raw.split("#", 1)[0].strip() + if raw[:1] in "\"'" and raw[-1:] == raw[:1] and len(raw) >= 2: + return raw[1:-1] + if raw in ("true", "false"): + return raw == "true" + if raw == "[]": + return [] + for cast in (int, float): + try: + return cast(raw) + except ValueError: + pass + return raw + + +def parse_example(text): + """Top-level `key: value` lines plus one list of flat mappings.""" + out, cur_list, cur = {}, None, None + for line in text.splitlines(): + if not line.split("#", 1)[0].strip(): + continue + m = re.match(r"^([a-z0-9_]+):\s*(.*)$", line) + if m: + if m.group(2).split("#", 1)[0].strip(): + out[m.group(1)] = scalar(m.group(2)) + cur_list = None + else: + cur_list = out[m.group(1)] = [] + continue + m = re.match(r"^\s+-\s+([a-z0-9_]+):\s*(.*)$", line) + if m and cur_list is not None: + cur = {m.group(1): scalar(m.group(2))} + cur_list.append(cur) + continue + m = re.match(r"^\s+([a-z0-9_]+):\s*(.*)$", line) + if m and cur is not None: + cur[m.group(1)] = scalar(m.group(2)) + continue + raise ValueError(f"unparsed line in the DOCS.md example: {line!r}") + return out + + +# --------------------------------------------------------------------------- # +print("DOC-1 - the docs say what the code does\n") +print("1. the `inverters:` example in DOCS.md starts") + +section = DOCS[DOCS.index("### More than one inverter (`inverters`)"):] +section = section[:section.index("\n### ", 1)] +block = re.search(r"```yaml\n(.*?)```", section, re.S).group(1) +example = parse_example(block) +entries = example.get("inverters") or [] + +# The options a fresh install is given, from config.yaml itself: the example is +# pasted on top of these, exactly as an installer does. +opts_block = CFG[CFG.index("\noptions:"):CFG.index("\nschema:")] +shipped = {m.group(1): scalar(m.group(2)) + for m in re.finditer(r"^ ([a-z0-9_]+): *(.*)$", opts_block, re.M)} + +# config.yaml's per-entry schema: required keys are the ones without `?`. +schema_block = CFG[CFG.index("\nschema:"):] +inv_schema = re.search(r"^ inverters:\n((?: .*\n)+)", schema_block, re.M).group(1) +schema_keys = dict(re.findall(r"^ -? ?\s*([a-z0-9_]+): *(\S+)", inv_schema, re.M)) +required = sorted(k for k, t in schema_keys.items() if not t.endswith("?")) + +check(f"1a. the reader found the example: {len(entries)} entries, " + f"{len(shipped)} shipped options, schema keys {sorted(schema_keys)}", + len(entries) == 2 and all(len(e) >= 4 for e in entries) + and len(shipped) > 50 and "bms_soc_floor" in required) + +missing = [(e.get("name"), k) for e in entries for k in required if k not in e] +check(f"1b. every entry carries every key the schema requires {required} " + f"(missing: {missing})", not missing) + +unknown = [(e.get("name"), k) for e in entries for k in e if k not in schema_keys] +check(f"1c. no entry uses a key the schema does not declare (found: {unknown})", + not unknown) + + +def with_example(ex): + opts = dict(shipped) + opts.update(ex) + return opts + + +problems = validate_startup_config(with_example(example)) +for p in problems: + print(f" refused: {p[:160]}") +check("1d. the shipped validator accepts the example as documented", problems == []) + +# Precondition: the example clears the flat setpoint_entity, so without its +# roster there is no actuator. If this passed, 1d graded the flat keys, not the +# roster. +flat_only = {k: v for k, v in example.items() if k != "inverters"} +check("1e. ...and it is the ROSTER that 1d graded (the example without it is " + "refused)", validate_startup_config(with_example(flat_only)) != []) + +# The mutation this card is about: the key the docs used to leave out. +spoilt = dict(example, inverters=[ + {k: v for k, v in e.items() if k != "bms_soc_floor"} for e in entries]) +sp = validate_startup_config(with_example(spoilt)) +check("1f. MUTATION: the example with `bms_soc_floor` removed is refused, " + "and the refusal names it", + sp != [] and any("bms_soc_floor" in p for p in sp)) + +# --------------------------------------------------------------------------- # +print("\n2. the P1 age entity is documented under its real id") + + +def compose(device_name, entity_name): + """HA's entity_id from device name + entity name (mqtt.py's note on + DEVICE). Written here, not borrowed from mqtt._slug, so a change to that + helper cannot move both sides of the comparison.""" + s = re.sub(r"[^a-z0-9]+", "_", f"{device_name} {entity_name}".lower()) + return "sensor." + s.strip("_") + + +p1_rows = [r for r in mqtt.SENSORS if r[0] == "p1_age"] +check("2a. mqtt.py announces exactly one `p1_age` sensor", len(p1_rows) == 1) +real_id = compose(mqtt.DEVICE["name"], p1_rows[0][2]) if p1_rows else "" +print(f" composed from mqtt.py: {real_id}") + +# Anchor outside mqtt.py: the id recorded from the live install. +recorded = read(HERE, "tools", "rehearsal", "announced-reference.txt") +check("2b. ...and that composes to the id recorded from the live install " + "(tools/rehearsal/announced-reference.txt)", + re.search(rf"^{re.escape(real_id)}\s+goodwe_ctl_p1_age\b", recorded, re.M) + is not None) + +m = re.search(r"^#### `(sensor\.[a-z0-9_]+)`\n\n(?:.*\n){0,6}?.*stale-input watchdog", + DOCS, re.M) +documented = m.group(1) if m else None +check(f"2c. DOCS.md's watchdog-entity section is headed with that id " + f"(found {documented!r})", documented == real_id) + +# The wrong id, anywhere an installer reads. CHANGELOG.md and DECISIONS/ are +# history and keep what was said at the time. +SHIPPED_DOCS = [ + os.path.join(HERE, "DOCS.md"), os.path.join(HERE, "config.yaml"), + os.path.join(REPO, "README.md"), os.path.join(REPO, "FIELD-GUIDE.md"), + os.path.join(REPO, "docs", "build_guide.py"), + os.path.join(REPO, "docs", "field-guide.html"), +] +WRONG = "p1_sample_age_s" +hits = [] +for path in SHIPPED_DOCS: + for n, line in enumerate(read(path).splitlines(), 1): + if WRONG in line: + hits.append(f"{os.path.relpath(path, REPO)}:{n}") +check(f"2d. `{WRONG}` appears in no shipped doc (hits: {hits})", not hits) + +# MUTATION: the old heading must fail 2c's comparison. +bad = DOCS.replace(f"#### `{real_id}`", f"#### `sensor.{WRONG}`") +mb = re.search(r"^#### `(sensor\.[a-z0-9_]+)`\n\n(?:.*\n){0,6}?.*stale-input watchdog", + bad, re.M) +check("2e. MUTATION: the old heading is found and does not match", + mb is not None and mb.group(1) != real_id) + +print() +if fails: + print(f"FAILED: {len(fails)}") + sys.exit(1) +print("all ok")
SettingValueReason
gain0.6Inverter needs 3–6 s to settle (1.4 s dead time, 94 % by 3.3 s) against a ~5 s cycle. At the limit — do not raise
slew_w1000Most of a correction in the first cycle without running far ahead of the hardware
slew_w00 follows the rail (max_w / 9). A faster fixed value is refused at startup (SAFETY-39) — see DOCS.md
deadband_w15Measured residual: mean 15.4 W, max 27 W. At 10 W the command never rests
saturation_w / cycles500 / 3Divergence = inverter at a limit → magnitude may fall, never rise. The 3-cycle term is essential: tested instantly it fires on every large correction
step_w10Register is 1 W but response lands on a ~17.6 W ladder. Finer is meaningless