checks / unit (pull_request) Successful in 4m58s
MQTT discovery announced ONE device and hung every entity off one unique_id prefix, so at two inverters B's per-unit telemetry had nowhere to go. It has somewhere now, and not one entity that already exists moved. ⚠️ THE ASYMMETRY IS DELIBERATE. The first roster entry's per-unit rows are announced on THE DEVICE IT ALREADY HAS; every later entry gets its own device, via_device the controller. "One device per inverter" is tidier and is the change that renames every entity on the live install - and because HA keeps the id it composed at FIRST discovery, a rename produces a NEW entity beside a DEAD one: the history stays behind and every dashboard, template and automation naming it goes on pointing at a sensor that quietly stopped updating, including the three live watchdog automations. The composed ids come out symmetric anyway (sensor.goodwe_inverter_a_battery_power / ..._inverter_b_...), so the tidy design's only real benefit is bought here without its cost. * four per-unit rows - setpoint, battery power, SoC, SAFETY-02 sign latch - announced only from a roster of two. At N=1 nothing is announced and nothing is published: no second device, no renamed entity, no added topic. * grid and p1_sample_age_s stay SITE-scoped and are announced ONCE, at every roster length 1-6. One P1, one grid connection, and p1_sample_age_s is a watchdog input - a second copy is a second answer to "is the meter fresh?". * inverter.roster_names() is the one answer to what the units are called, read by build_roster() and by the publisher, which must know the roster before a Controller exists because announcing happens on the CONNACK. * DECISIONS/TEL-07-second-mqtt-device.md records the two designs side by side. VERIFIED BY DIFFING THE ANNOUNCED PAYLOADS, not by reading the code. The release/1.0 baseline in test_mqtt.py was generated by running that commit's module, not transcribed. Roster of one: 9 topics, 0 missing, 0 changed, 0 added. Roster of two: 0 missing, 0 changed, 8 added. MEASURED, NOT ASSERTED. Sixteen mutations through tools/mutation_run.sh; twelve are in the file as a running table, each named with the property it breaks. Three of those properties exist because the table found their ABSENCE - dropping the unit from the state topic, moving `grid` into the per-unit set, and `units=()` in main()'s publisher call were all GREEN on the first run, as was a roster fallback naming two nameless entries `inverter-a`. Part 9 enumerates what the file still cannot see, with sizes. ⚠️ INVERTER B DOES NOT PHYSICALLY EXIST (~2026-09-12). Every roster of two here is SIMULATED, from the datasheet and serial in NOTES.md §1.1b. Nothing about B has been measured and no entity described here has been seen on a broker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
386 lines
19 KiB
Python
386 lines
19 KiB
Python
"""Optional MQTT discovery, so the controller's state appears as real HA entities.
|
|
|
|
Optional on purpose: the add-on runs headless without a broker and simply
|
|
publishes nothing. Nothing in the control path depends on this - if MQTT breaks,
|
|
the battery keeps being controlled correctly and only the dashboard goes stale.
|
|
That separation is deliberate; observability must never be able to take down
|
|
control.
|
|
"""
|
|
|
|
import json
|
|
import logging
|
|
|
|
try:
|
|
import paho.mqtt.client as mqtt
|
|
except ImportError: # pragma: no cover - container always has it
|
|
mqtt = None
|
|
|
|
_LOG = logging.getLogger("goodwe.mqtt")
|
|
|
|
# ⚠️ The DEVICE name is half of every entity_id. Home Assistant composes
|
|
# entity_id from device name + entity name, so "GoodWe RS485 Controller" plus
|
|
# "GoodWe battery power" yields
|
|
# sensor.goodwe_rs485_controller_goodwe_battery_power. object_id in the
|
|
# discovery payload did NOT override it (tested on HA 2026.8). So the device is
|
|
# named "GoodWe" and the entities are named without repeating it - that is what
|
|
# makes the ids short, predictable, and identical on every install.
|
|
DEVICE = {
|
|
"identifiers": ["goodwe_rs485_controller"],
|
|
"name": "GoodWe",
|
|
"manufacturer": "GoodWe (via RS485 meter emulation)",
|
|
"model": "ES/BP series",
|
|
}
|
|
|
|
# (key, object_id, name, unit, device_class, state_class, icon)
|
|
#
|
|
# ⚠️ object_id is what pins the entity_id. Without it Home Assistant derives the
|
|
# id from the DEVICE name plus the entity name and produces
|
|
# `sensor.goodwe_rs485_controller_goodwe_battery_power` - unpredictable, ugly,
|
|
# and different if anyone renames the device. Dashboards and documentation need
|
|
# these ids to be stable across every install, so they are declared, not derived.
|
|
SENSORS = [
|
|
("setpoint", "goodwe_setpoint", "Setpoint", "W", "power", "measurement", None),
|
|
("grid", "goodwe_grid_power", "Grid power", "W", "power", "measurement", None),
|
|
("battery", "goodwe_battery_power", "Battery power", "W", "power", "measurement", None),
|
|
("soc", "goodwe_battery_soc", "Battery SoC", "%", "battery", "measurement", None),
|
|
# SAFETY-31. `sum(|Bi|) - |sum(Bi)|` over MEASURED per-inverter power: the
|
|
# power going round between the inverters, which the grid sensor above
|
|
# CANNOT SHOW because it nets to zero there. The two are published side by
|
|
# side on purpose - "grid 0 W, circulating 6000 W" is the whole phenomenon
|
|
# in two rows of a dashboard, and no arithmetic on the grid figure alone can
|
|
# ever produce the second one.
|
|
#
|
|
# ⚠️ Published as 0 at the shipped one-inverter roster, not omitted, and
|
|
# that is a deliberate difference from `p1_age` below. An absent entity is
|
|
# right for a watchdog input nothing is feeding; this is a quantity whose
|
|
# value is KNOWN at N=1 - identically zero, for every reading - and a
|
|
# dashboard row reading 0 W is the honest report of it.
|
|
("circulation", "goodwe_circulating_power", "Circulating power", "W", "power", "measurement", "mdi:sync-alert"),
|
|
("phase", "goodwe_maintenance_phase", "Maintenance phase", None, None, None, "mdi:battery-sync"),
|
|
("status", "goodwe_controller_status", "Controller status", None, None, None, "mdi:heart-pulse"),
|
|
# ⚠️ THE LIVE ENTITY IS `sensor.goodwe_p1_sample_age`, NOT what this
|
|
# object_id spells. An earlier version of this comment said the id "must be
|
|
# exactly sensor.p1_sample_age_s" - that name has never existed on any HA,
|
|
# and believing it is how a safety layer gets disarmed by someone tidying up.
|
|
#
|
|
# On HA 2026.8 `object_id` in a discovery payload does NOT determine the
|
|
# entity_id. HA composes it from the device name plus the entity name:
|
|
# "GoodWe RS485 Controller" + "P1 sample age" -> sensor.goodwe_p1_sample_age.
|
|
# Verified against the live system 2026-08-26; the firmware subscribes to
|
|
# that composed name (goodwe-master-stage4-failsafe.yaml, homeassistant
|
|
# sensor p1_age_s), and its own comment records that sensor.p1_sample_age_s
|
|
# never existed.
|
|
#
|
|
# So: do NOT "fix" the firmware to match the object_id below, and do not
|
|
# rename the device or this entity's friendly name - either changes the
|
|
# composed id and silently disarms the ingest guard, because has_state()
|
|
# simply stays false forever and nothing reports it. The firmware calls this
|
|
# out as the failure that "FAILS SILENTLY AND LOOKS CORRECT".
|
|
#
|
|
# The value: seconds since the newest accepted P1 telegram, republished
|
|
# every second so a meter frozen at a constant value still shows a climbing
|
|
# age - the false-trip this entity exists to remove.
|
|
#
|
|
# ⚠️ Separately, and not fixed here: the firmware's ingest guard tests this
|
|
# sensor's VALUE rather than its arrival, so a dead publisher freezes the
|
|
# number and reads permanently fresh. Filed as TEL-06.
|
|
("p1_age", "p1_sample_age_s", "P1 sample age", "s", "duration", "measurement", None),
|
|
]
|
|
|
|
BASE = "goodwe_ctl"
|
|
AVAILABILITY = f"{BASE}/availability"
|
|
|
|
# --- TEL-07: a SECOND device, for inverter B, and NOT one per inverter -------
|
|
#
|
|
# ⚠️ READ THE DEVICE-NAME NOTE ABOVE BEFORE CHANGING ANYTHING HERE. The device
|
|
# name is half of every entity_id, so re-scoping `DEVICE` renames every entity
|
|
# the live install already has - and HA keeps the id it composed at FIRST
|
|
# discovery, so what a rename actually produces is a NEW entity beside a dead
|
|
# one: the history stays behind on the old id, and every dashboard, template
|
|
# and automation that names it (including `goodwe_master_watchdog_held_*` and
|
|
# `goodwe_master_parked_at_0_w_*`) keeps pointing at the entity that stopped
|
|
# updating. That is silent. So:
|
|
#
|
|
# `DEVICE` IS NOT TOUCHED AND ITS ENTITIES ARE NOT TOUCHED. It is the SITE
|
|
# device - the meter's view and the controller's own state - and at the
|
|
# shipped roster of one it is also, in effect, inverter A's.
|
|
#
|
|
# THE FIRST ROSTER ENTRY GETS NO NEW DEVICE. Its per-unit rows are announced
|
|
# ON THE DEVICE IT ALREADY HAS, under new `unique_id`s and new names. Nothing
|
|
# existing is renamed, so nothing existing loses its history.
|
|
#
|
|
# EVERY LATER ENTRY GETS ITS OWN DEVICE (`via_device` the site one).
|
|
#
|
|
# ⚠️ THE ASYMMETRY IS THE POINT, AND IT WILL LOOK LIKE AN OVERSIGHT. "One
|
|
# device per inverter", with A as `inverter-a` and B as `inverter-b`, is tidy,
|
|
# symmetrical, and is the change that destroys the live system's recorded
|
|
# history. DO NOT "CLEAN THIS UP". The composed entity_ids come out symmetric
|
|
# anyway - `sensor.goodwe_inverter_a_battery_power` and
|
|
# `sensor.goodwe_inverter_b_battery_power` - which is the whole benefit of the
|
|
# tidy version, without the cost.
|
|
#
|
|
# WHAT IS PER-UNIT AND WHAT IS SITE, and why the split is where it is:
|
|
#
|
|
# PER-UNIT setpoint (this unit's share of the site target, the number its
|
|
# own heartbeat re-emits), battery power (`inv.batt`, this unit's
|
|
# own meter reading), SoC (`inv.soc`, THIS pack - the site row is
|
|
# the EMPTIEST pack, which at N>=2 may be the other one), and the
|
|
# SAFETY-02 sign latch, which is per actuator by construction.
|
|
#
|
|
# SITE `grid` and `p1_age`. There is ONE P1 meter and ONE grid
|
|
# connection; announcing them per inverter would invent two of a
|
|
# thing there is one of. `p1_sample_age_s` is additionally a
|
|
# WATCHDOG INPUT (see `omit` below and the firmware note above), so
|
|
# a second copy of it is a second copy of a safety signal - two
|
|
# answers to "is the meter fresh?", one of which is always the one
|
|
# nobody is looking at. Also site: circulating power (it is
|
|
# `sum(|Bi|) - |sum(Bi)|`, a property of the SET), the controller
|
|
# status, the maintenance phase, and the capacity-tariff peak.
|
|
#
|
|
# NOT ANNOUNCED, AND SAID OUT LOUD RATHER THAN LEFT LOOKING FINISHED: the
|
|
# card also lists "inverter AC power" and per-unit HEARTBEATS as per-unit.
|
|
# Neither has a source in this add-on today - nothing reads an AC-power
|
|
# register per unit, and `inv.last_beat` is a monotonic loop clock with no
|
|
# wall-clock age published anywhere. Announcing them would create entities
|
|
# nothing ever feeds, which is exactly what the `omit` note below refuses to
|
|
# do for `p1_sample_age_s`. They are gaps, not omissions.
|
|
UNIT_SENSORS = [
|
|
("setpoint", "setpoint", "Setpoint", "W", "power", "measurement", None),
|
|
("battery", "battery_power", "Battery power", "W", "power", "measurement", None),
|
|
("soc", "battery_soc", "Battery SoC", "%", "battery", "measurement", None),
|
|
("sign_latch", "sign_latch", "Sign latch", None, None, None, "mdi:flash-alert"),
|
|
]
|
|
|
|
# ⚠️ A ROSTER OF ONE ANNOUNCES NOTHING NEW - not a device, not an entity, not a
|
|
# topic. Every install running today is a roster of one, and "the upgrade added
|
|
# four entities nobody asked for" is a change to a live system's entity list
|
|
# for no information: at N=1 the site rows ARE this unit's rows, exactly.
|
|
PER_UNIT_MIN_ROSTER = 2
|
|
|
|
|
|
def _slug(text) -> str:
|
|
"""Home Assistant's own object_id spelling: lowercase, `_`, no runs.
|
|
|
|
⚠️ ASCII-ONLY ON PURPOSE. `str.isalnum()` is true for 'é' and for '٤', and
|
|
HA transliterates those rather than keeping them, so treating them as
|
|
alphanumeric here would produce an id this module claims and HA does not.
|
|
They become separators, which is conservative in the right direction: two
|
|
names that differ ONLY outside ASCII collide, and `unit_slugs()` below is
|
|
what stops a collision becoming two entities with one `unique_id`.
|
|
"""
|
|
out = []
|
|
for ch in str(text).strip().lower():
|
|
out.append(ch if (ch.isascii() and ch.isalnum()) else "_")
|
|
slug = "".join(out)
|
|
while "__" in slug:
|
|
slug = slug.replace("__", "_")
|
|
return slug.strip("_")
|
|
|
|
|
|
def _title(name) -> str:
|
|
"""`inverter-b` -> `Inverter B`. The half of the entity_id HA composes."""
|
|
words = str(name).replace("-", " ").replace("_", " ").split()
|
|
return " ".join(w[:1].upper() + w[1:] for w in words) or str(name)
|
|
|
|
|
|
def unit_slugs(units) -> list:
|
|
"""One id-safe slug per roster entry, in roster order, ALL DISTINCT.
|
|
|
|
⚠️ THE DE-COLLISION IS NOT DECORATION. `validate.py` rule 9(c) refuses two
|
|
entries with the SAME name; it does not refuse `inverter-b` beside
|
|
`Inverter B`, and those two slug to one string. Two entities published with
|
|
one `unique_id` is not an error in HA - the second one is DROPPED, so one
|
|
inverter's telemetry silently stops existing. The suffix is the roster
|
|
INDEX, so it is stable for a given roster and does not depend on what the
|
|
other unit is called.
|
|
"""
|
|
slugs, seen = [], set()
|
|
for i, name in enumerate(units):
|
|
slug = _slug(name) or f"inverter_{i + 1}"
|
|
while slug in seen:
|
|
slug = f"{slug}_{i + 1}"
|
|
seen.add(slug)
|
|
slugs.append(slug)
|
|
return slugs
|
|
|
|
|
|
def announced_units(units) -> list:
|
|
"""The roster, in order - or EMPTY below `PER_UNIT_MIN_ROSTER`.
|
|
|
|
⚠️ ONE ANSWER TO "WHICH UNITS GET PER-UNIT ENTITIES", read by the
|
|
announcement here AND by `Controller.publish()` in main.py. Two spellings of
|
|
the same predicate is how an entity gets announced that nothing publishes
|
|
to, or published to on a topic nothing announced - and at N=1 the second
|
|
would be an added topic, which AC 4 forbids outright.
|
|
"""
|
|
names = [str(u) for u in units]
|
|
if len(names) < PER_UNIT_MIN_ROSTER:
|
|
return []
|
|
return names
|
|
|
|
|
|
def unit_state_key(slug: str, key: str) -> str:
|
|
"""The `publish()` key - i.e. the state topic under `BASE` - for one unit.
|
|
|
|
Written once, here, because main.py builds it and this module announces it;
|
|
a second transcription is a topic announced and never fed.
|
|
"""
|
|
return f"unit/{slug}/{key}"
|
|
|
|
|
|
def unit_device(name, slug: str) -> dict:
|
|
"""B's OWN device. ⚠️ Never returned for the first roster entry."""
|
|
return {
|
|
"identifiers": [f"{DEVICE['identifiers'][0]}_{slug}"],
|
|
"name": f"{DEVICE['name']} {_title(name)}",
|
|
"manufacturer": DEVICE["manufacturer"],
|
|
"model": DEVICE["model"],
|
|
# Hangs B under the controller in HA's device tree rather than beside
|
|
# it. Cosmetic, and the one line that says in the UI what the asymmetry
|
|
# above means: there is one controller, and it drives both.
|
|
"via_device": DEVICE["identifiers"][0],
|
|
}
|
|
|
|
|
|
def _payload(name, object_id, unique_id, state_topic, device,
|
|
unit, dev_class, state_class, icon) -> dict:
|
|
"""One discovery payload.
|
|
|
|
⚠️ KEY ORDER IS PART OF THE OUTPUT. `json.dumps` writes keys in insertion
|
|
order, so this dict's construction order IS the bytes on the wire, and AC 1
|
|
is a BYTE comparison against what `release/1.0` publishes. Do not reorder.
|
|
"""
|
|
cfg = {
|
|
"name": name,
|
|
"object_id": object_id,
|
|
"unique_id": unique_id,
|
|
"state_topic": state_topic,
|
|
"availability_topic": AVAILABILITY,
|
|
"device": device,
|
|
}
|
|
if unit:
|
|
cfg["unit_of_measurement"] = unit
|
|
if dev_class:
|
|
cfg["device_class"] = dev_class
|
|
if state_class:
|
|
cfg["state_class"] = state_class
|
|
if icon:
|
|
cfg["icon"] = icon
|
|
return cfg
|
|
|
|
|
|
def discovery_payloads(units=(), omit=()) -> list:
|
|
"""Every `(topic, payload)` this add-on announces, in announcement order.
|
|
|
|
Split out of `_announce()` so that the acceptance criterion can be graded as
|
|
a DIFF OF THE ANNOUNCED PAYLOADS against `release/1.0`, rather than as a
|
|
reading of the code. `_announce()` publishes exactly this list and nothing
|
|
else, which is itself checked.
|
|
"""
|
|
omit = set(omit)
|
|
out = []
|
|
for key, object_id, name, unit, dev_class, state_class, icon in SENSORS:
|
|
if key in omit:
|
|
continue
|
|
out.append((
|
|
f"homeassistant/sensor/{BASE}_{key}/config",
|
|
_payload(name, object_id, f"{BASE}_{key}", f"{BASE}/{key}",
|
|
DEVICE, unit, dev_class, state_class, icon)))
|
|
names = announced_units(units)
|
|
slugs = unit_slugs(names)
|
|
site_slug = _slug(DEVICE["name"])
|
|
for i, (name, slug) in enumerate(zip(names, slugs)):
|
|
# ⚠️ `i == 0` IS THE ASYMMETRY, IN ONE LINE. The first roster entry
|
|
# keeps the device it already has and carries its name in the ENTITY
|
|
# name instead; every later one gets its own device and does not.
|
|
# Either way HA composes device-name + entity-name into the same
|
|
# string, which is why `object_id` below can be written once for both.
|
|
device = DEVICE if i == 0 else unit_device(name, slug)
|
|
prefix = f"{_title(name)} " if i == 0 else ""
|
|
for key, oid, label, unit, dev_class, state_class, icon in UNIT_SENSORS:
|
|
unique_id = f"{BASE}_unit_{slug}_{key}"
|
|
out.append((
|
|
f"homeassistant/sensor/{unique_id}/config",
|
|
_payload(prefix + label,
|
|
f"{site_slug}_{slug}_{oid}",
|
|
unique_id,
|
|
f"{BASE}/{unit_state_key(slug, key)}",
|
|
device, unit, dev_class, state_class, icon)))
|
|
return out
|
|
|
|
|
|
class MqttPublisher:
|
|
def __init__(self, host, port, username=None, password=None, omit=(),
|
|
units=()):
|
|
# `omit` drops sensor keys from discovery entirely. ⚠️ Announcing a
|
|
# sensor that nothing will ever publish to is not harmless here:
|
|
# p1_sample_age_s is a watchdog input, and an entity that exists but is
|
|
# never fed is a worse signal than one that does not exist at all.
|
|
self.omit = set(omit)
|
|
# TEL-07. The roster's NAMES, in roster order - `inverter.roster_names()`,
|
|
# not a second reading of the option. Defaults to empty, which
|
|
# `announced_units()` treats exactly as a roster of one: nothing
|
|
# per-unit is announced, so every caller that has not been taught about
|
|
# the roster keeps publishing what it published before.
|
|
self.units = [str(u) for u in units]
|
|
self.enabled = mqtt is not None and bool(host)
|
|
self.client = None
|
|
if not self.enabled:
|
|
_LOG.info("MQTT not configured - status entities will not be published")
|
|
return
|
|
# paho-mqtt 2.x requires a callback API version; 1.x has no such
|
|
# argument and Alpine ships 1.x. Support both rather than pinning, so
|
|
# the container can use the distro package instead of compiling.
|
|
try:
|
|
self.client = mqtt.Client(mqtt.CallbackAPIVersion.VERSION2,
|
|
client_id="goodwe_rs485_controller")
|
|
except AttributeError:
|
|
self.client = mqtt.Client(client_id="goodwe_rs485_controller")
|
|
if username:
|
|
self.client.username_pw_set(username, password or "")
|
|
self.client.will_set(AVAILABILITY, "offline", retain=True)
|
|
# ⚠️ Announce from on_connect, never straight after connect(). paho
|
|
# processes the CONNACK on its network thread, so a publish issued
|
|
# immediately after connect() is made while still disconnected - and
|
|
# paho DROPS QoS-0 publishes when disconnected, silently. The result is
|
|
# an add-on that logs "MQTT connected" and creates no entities at all.
|
|
# As a bonus, this also re-announces after every reconnect.
|
|
self.client.on_connect = lambda *_args, **_kw: self._announce()
|
|
try:
|
|
self.client.connect(host, int(port), keepalive=60)
|
|
self.client.loop_start()
|
|
_LOG.info("MQTT connected to %s:%s", host, port)
|
|
except OSError as err:
|
|
_LOG.warning("MQTT connect failed (%s) - continuing without it", err)
|
|
self.enabled = False
|
|
|
|
def _announce(self) -> None:
|
|
# ⚠️ NO PAYLOAD IS BUILT HERE. `discovery_payloads()` is the whole
|
|
# announcement, so the check that diffs it against `release/1.0` is
|
|
# diffing what actually goes on the wire - and a payload built here
|
|
# instead would be one this add-on publishes and no check has ever seen.
|
|
for topic, cfg in discovery_payloads(self.units, self.omit):
|
|
self.client.publish(topic, json.dumps(cfg), retain=True)
|
|
self.client.publish(AVAILABILITY, "online", retain=True)
|
|
|
|
def publish(self, values: dict) -> None:
|
|
if not self.enabled or self.client is None:
|
|
return
|
|
try:
|
|
for key, value in values.items():
|
|
if value is None:
|
|
continue
|
|
self.client.publish(f"{BASE}/{key}", str(value))
|
|
except OSError as err: # pragma: no cover
|
|
_LOG.debug("MQTT publish failed: %s", err)
|
|
|
|
def close(self) -> None:
|
|
if not self.enabled or self.client is None:
|
|
return
|
|
try:
|
|
self.client.publish(AVAILABILITY, "offline", retain=True)
|
|
self.client.loop_stop()
|
|
self.client.disconnect()
|
|
except OSError: # pragma: no cover
|
|
pass
|