fix: profile access:'RO' now withholds the control (#374)

v1.6.0 added the VPP registers to the MOD profile as 'RO' and created five
writable controls anyway - two selects, two numbers, and control_authority
arriving 8 seconds later through the deferred path. One of them is a -100..+100%
slider on register 30409, which #373 measured climbing toward its setpoint and
importing 912 W from the grid with allow_grid_charge off. That is precisely the
exposure #373 said would not ship.

Two mistakes, both mine, both avoidable from what I already knew.

The first: `access` was never read anywhere outside profiles/. The exploration
for #371-373 told me so in as many words, and I wrote it into the #371 reply -
"profile membership is the only gate number.py and select.py consult, so an RO
entry would still create the entity" - and then relied on the flag in #373
regardless.

The second: I believed the is_wit gate confined the VPP controls. It returns
only INSIDE the WIT branch; non-WIT profiles fall through to the generic loop
that creates a control for any WRITABLE_REGISTERS entry whose register is in the
profile. The gate confines the WIT-specific classes and nothing else, so adding
the registers to MOD was sufficient on its own.

`access` now means what everyone assumes. is_read_only_register() in const.py is
consulted by both generic loops and by the deferred VPP path in select.py - that
last one matters, because gating only the setup loop fixes four of the five and
leaves control_authority to arrive seconds later.

Of 517 control/profile pairs, six change: the five above, plus SPE register 117,
which the profile already documents as "firmware-determined, writes may be
rejected". That is the same defect in miniature and removing the control is
correct.

The stale-control cleanup now treats read-only as stale too. Membership alone
would have left the five entities behind as unavailable on every install that
already has them, since their registers are still in the profile.

On the tests. Two of them passed while this shipped, and both were the wrong
shape rather than merely thin:

  test_mod_vpp_registers_are_read_only   asserted the flag was 'RO'. It was.
                                         Nothing read it, so the assertion was
                                         true and meaningless.
  test_the_wit_gate_still_confines_...   asserted the gate kept its shape. It
                                         did. The gate never confined the path
                                         that created these entities.

Both tested a declaration, or a mechanism believed to enforce it, instead of the
outcome. Replaced with tests on what the user gets. The model-based one is
explicitly paired with a source-level check, because _controls_created_for()
applies the read-only filter itself and would otherwise pass whatever the
platforms actually do - a model agreeing with itself, which is how this class of
error survives.

Verified by stashing only number.py and select.py: the two load-bearing tests
fail against the shipped code and pass with the fix.

Confirmed from the reporter's diagnostics that the rest of v1.6.0 is sound - the
peak-shaving values read 7.5/7.5/50/7.5/100 kW/%/kW/% against his portal, so the
#372 mappings and scaling are right on hardware, 30474 reads -20 so the signed
handling works, and vpp_control_authority_available is True, which is independent
confirmation of the capability #373 reported.

Co-Authored-By: Claude <noreply@anthropic.com>

Author: 0xAHA

RELEASENOTES.md

@@ -4,6 +4,34 @@
 
 ---
 
+## v1.6.1 (pre-release)
+
+Issues: #374
+
+> **Fixes a defect in the v1.6.0 pre-release.** If you installed v1.6.0 on a MOD or MID
+> TL3-XH, please take this one — v1.5.5 remains the stable release and was never affected.
+
+- **Five writable VPP controls appeared on MOD TL3-XH in v1.6.0 and should not have.**
+  Control Authority, Remote Power Control, Remote Duration, Remote Charge/Discharge Power
+  and VPP AC Charge Enable were created as operable entities, including a −100…+100 %
+  power slider. v1.6.0 marked those registers read-only in the profile, but nothing read
+  that flag.
+
+  They are removed on upgrade. Nothing was written to them and they were all at their safe
+  values; the exposure is what is being fixed.
+
+- **A profile marking a register read-only now withholds the control.** Previously `access`
+  was documentation only. This also removes one pre-existing control — SPE Grid Compliance
+  Region (register 117), which the profile already described as firmware-determined with
+  writes rejected.
+
+- The four VPP diagnostic sensors, the peak-shaving sensors and the Grid Charge Stopped SOC
+  control from v1.6.0 are unaffected and stay.
+
+Reported by @KevlarD-67, with the mechanism traced to the exact line.
+
+---
+
 ## v1.6.0 (pre-release)
 
 Issues: #371, #372, #373 — all on MOD/MID TL3-XH

custom_components/growatt_modbus/__init__.py

@@ -21,6 +21,7 @@
     REGISTER_MAPS,
     WRITABLE_REGISTERS,
     DEVICE_TYPE_INVERTER,
+    is_read_only_register,
 )
 from .coordinator import GrowattConfigEntry, GrowattModbusCoordinator
 from .device_profiles import get_profile
@@ -466,7 +467,13 @@ def _cleanup_unsupported_vpp_entities() -> None:
     # SOC-limit cleanup.
     if profile_is_known and holding:
         for control_name, control_config in WRITABLE_REGISTERS.items():
-            if control_config.get("register") in holding:
+            _reg = control_config.get("register")
+            # Stale for either reason: the profile no longer maps the register at all, or
+            # it maps it read-only. The second case is why v1.6.0's five VPP controls on
+            # MOD would otherwise survive the v1.6.1 fix — their registers are still in
+            # the profile, just marked RO, so a membership test alone leaves them behind
+            # as unavailable (#374).
+            if _reg in holding and not is_read_only_register(holding.get(_reg)):
                 continue
             for _domain in ("number", "select", "time"):
                 stale_eid = entity_registry.async_get_entity_id(

custom_components/growatt_modbus/const.py

@@ -1107,6 +1107,25 @@ def get_entity_category(sensor_key: str) -> str | None:
 }
 
 
+def is_read_only_register(register_def) -> bool:
+    """True when a profile marks this register read-only.
+
+    `access` was documentation that nothing read until v1.6.1. v1.6.0 added the VPP
+    registers to the MOD profile as 'RO' on the assumption the flag would stop controls
+    being created for them, and the generic loops in number.py and select.py created five
+    writable controls anyway — including the power setpoint that was measured importing
+    from the grid to reach its target (#374).
+
+    Absent or unrecognised means writable, so nothing that works today changes: a profile
+    has to say 'RO'/'R' explicitly to withhold a control. Of 517 control/profile pairs,
+    six are affected — the five above and SPE register 117, which documents itself as
+    "firmware-determined, writes may be rejected" and is the same defect in miniature.
+    """
+    if not isinstance(register_def, dict):
+        return False
+    return str(register_def.get("access", "")).strip().upper() in ("RO", "R")
+
+
 def resolve_block_size(value) -> int:
     """Resolve a stored max_block_size option to an integer.
 

custom_components/growatt_modbus/manifest.json

@@ -12,5 +12,5 @@
     "pymodbus>=3.0.0",
     "pyserial>=3.4"
   ],
-  "version": "1.6.0"
+  "version": "1.6.1"
 }

custom_components/growatt_modbus/number.py

@@ -15,6 +15,7 @@
     WRITABLE_REGISTERS,
     CONF_REGISTER_MAP,
     get_device_type_for_control,
+    is_read_only_register,
 )
 from .coordinator import GrowattModbusCoordinator
 from .entity import GrowattEntity
@@ -109,6 +110,21 @@ async def async_setup_entry(
         if register_num not in holding_registers:
             continue  # Skip if register not in this profile
 
+        # A profile marking the register read-only means "this model has the address but
+        # will not accept a write" — so do not offer a control for it (#374).
+        #
+        # Until v1.6.1 `access` was documentation that nothing read. v1.6.0 added the VPP
+        # registers to the MOD profile as 'RO' expecting that to be enough, and this loop
+        # created five writable controls anyway — including the power setpoint measured
+        # importing from the grid to reach its target. The flag now means what everyone
+        # already assumed it meant.
+        if is_read_only_register(holding_registers.get(register_num)):
+            _LOGGER.debug(
+                "Skipping %s: register %d is read-only on this profile",
+                control_name, register_num,
+            )
+            continue
+
         # Profile-specific filter: only_profiles restricts to named maps; not_profiles excludes them
         _only = control_config.get('only_profiles')
         if _only and register_map_name not in _only:

custom_components/growatt_modbus/select.py

@@ -15,6 +15,7 @@
     WRITABLE_REGISTERS,
     CONF_REGISTER_MAP,
     get_device_type_for_control,
+    is_read_only_register,
     DEVICE_TYPE_BATTERY,
     MOD_TOU_PERIODS,
 )
@@ -90,6 +91,16 @@ async def async_setup_entry(
         if register_num not in holding_registers:
             continue  # Skip if register not in this profile
 
+        # A profile marking the register read-only means "this model has the address but
+        # will not accept a write" — so do not offer a control for it (#374). See
+        # is_read_only_register() for what v1.6.0 shipped by assuming this already worked.
+        if is_read_only_register(holding_registers.get(register_num)):
+            _LOGGER.debug(
+                "Skipping %s: register %d is read-only on this profile",
+                control_name, register_num,
+            )
+            continue
+
         # Profile-specific filter: only_profiles restricts to named maps; not_profiles excludes them
         _only = control_config.get('only_profiles')
         if _only and register_map_name not in _only:
@@ -151,6 +162,16 @@ async def async_setup_entry(
         cfg = WRITABLE_REGISTERS[ctrl]
         if cfg["register"] not in holding_registers:
             continue
+        # Read-only on this profile, so there is nothing to defer — it is not "skipped
+        # pending data", it is withheld deliberately (#374).
+        #
+        # This check has to be here as well as in the loop above, or the two paths
+        # disagree: on the reported MOD install control_authority was skipped at setup
+        # for want of live data, then added 8 seconds later by this path once the first
+        # poll confirmed 30100 answers. Gating only the setup loop would have fixed four
+        # of the five controls and left this one.
+        if is_read_only_register(holding_registers.get(cfg["register"])):
+            continue
         # Only defer if it was actually skipped above (flag was False at setup time)
         already_created = any(
             getattr(e, "_control_name", None) == ctrl for e in entities

tests/test_control_registers.py

@@ -126,33 +126,138 @@ def test_only_3312_is_writable_in_the_peak_shaving_cluster():
 
 
 @pytest.mark.parametrize("addr", [30100, 30407, 30408, 30409, 30410, 30474])
-def test_mod_vpp_registers_are_read_only(addr):
-    """#373 established that this family supports remote power control, and also that the
-    power value is a target rather than a cap: at 100% it climbed toward the setpoint and
-    imported from the grid with allow_grid_charge off. Until that is guarded, these are
-    exposed for visibility only."""
+def test_mod_vpp_registers_are_marked_read_only(addr):
+    """The declaration. On its own this proves nothing about what the user sees — see the
+    test below, which is the one that matters."""
     reg = _holding("MOD_6000_15000TL3_XH").get(addr)
     assert reg is not None, f"holding {addr} is not mapped on MOD-XH"
-    assert str(reg.get("access", "")).upper() in ("RO", "R"), (
+    assert _const.is_read_only_register(reg), (
         f"holding {addr} is writable on MOD-XH. Commanding VPP power on this family needs "
         f"a guard against importing from the grid to reach the setpoint (#373)."
     )
 
 
-def test_the_wit_gate_still_confines_writable_vpp_controls():
-    """`access: 'RO'` in the profile does not by itself stop a control being created —
-    number.py and select.py key off WRITABLE_REGISTERS plus profile membership, not the
-    access flag. What actually keeps MOD read-only is the WIT-only name check.
+# ---------------------------------------------------------------------------
+# No control may exist for a register its profile marks read-only (#374)
+# ---------------------------------------------------------------------------
+#
+# This replaces two tests that passed while the defect shipped, which is worth recording
+# because both were the wrong shape rather than merely incomplete:
+#
+#   test_mod_vpp_registers_are_read_only    asserted the `access` flag was 'RO'. It was.
+#                                           Nothing read the flag, so the assertion was
+#                                           true and meaningless.
+#   test_the_wit_gate_still_confines_...    asserted the WIT gate still had its shape. It
+#                                           did. But the gate only returns *inside* the
+#                                           WIT branch — non-WIT profiles fall through to
+#                                           the generic loop it was believed to confine.
+#
+# Both tested a declaration, or a mechanism believed to enforce it, rather than the
+# outcome. v1.6.0 created five writable VPP controls on MOD with both passing, including a
+# -100..+100% power slider on the register measured importing from the grid to reach its
+# setpoint. The tests below assert the outcome instead.
+
+
+PLATFORMS = ("number.py", "select.py")
 
-    So if that gate becomes a capability probe, MOD gains writable VPP controls the moment
-    its hardware answers — including the power setpoint that #373 measured importing from
-    the grid to reach its target. That is the change to make deliberately, with a guard,
-    not to arrive at by refactoring.
+
+def test_both_control_platforms_consult_the_read_only_flag():
+    """The load-bearing test. Everything below models the loops; this checks the loops.
+
+    `_controls_created_for` applies the read-only filter itself, so on its own it would
+    pass whatever number.py and select.py actually do — a model of the code agreeing with
+    itself. That is the shape of mistake that let v1.6.0 ship: a test asserting a
+    declaration rather than an outcome.
+    """
+    component = Path(__file__).parent.parent / "custom_components" / "growatt_modbus"
+    for platform in PLATFORMS:
+        src = (component / platform).read_text(encoding="utf-8")
+        assert "is_read_only_register" in src, (
+            f"{platform} never consults the read-only flag, so a profile marking a "
+            f"register RO does not stop the control being created — which is exactly "
+            f"what shipped in v1.6.0 (#374)"
+        )
+
+
+def _controls_created_for(map_key: str) -> set[str]:
+    """Control names the generic loops would create for a profile.
+
+    Mirrors the filters in number.py/select.py: profile membership, the read-only flag,
+    only_profiles/not_profiles. Bespoke classes and the live-confirmation skips are not
+    modelled — they can only ever remove entities from this set, never add one, so a
+    control absent here cannot appear in Home Assistant.
+
+    This is a model, not the code. Pair it with
+    test_both_control_platforms_consult_the_read_only_flag, which checks the real thing.
+    """
+    holding = _holding(map_key)
+    created = set()
+    for name, cfg in WRITABLE_REGISTERS.items():
+        addr = cfg.get("register")
+        if addr not in holding:
+            continue
+        if _const.is_read_only_register(holding.get(addr)):
+            continue
+        only = cfg.get("only_profiles")
+        if only and map_key not in only:
+            continue
+        not_p = cfg.get("not_profiles")
+        if not_p and map_key in not_p:
+            continue
+        created.add(name)
+    return created
+
+
+@pytest.mark.parametrize("map_key", sorted(REGISTER_MAPS))
+def test_no_control_is_created_for_a_read_only_register(map_key):
+    """The general rule. A profile marking a register read-only is a statement that the
+    hardware will not accept a write, and the only way to honour it is not to offer the
+    control."""
+    holding = _holding(map_key)
+    offending = sorted(
+        name for name in _controls_created_for(map_key)
+        if _const.is_read_only_register(holding.get(WRITABLE_REGISTERS[name]["register"]))
+    )
+    assert not offending, f"{map_key} would create controls for read-only registers: {offending}"
+
+
+@pytest.mark.parametrize("addr", [30100, 30407, 30408, 30409, 30410])
+def test_mod_creates_no_vpp_control(addr):
+    """The specific case, stated as the user-visible outcome rather than a flag.
+
+    Each of these had a control in v1.6.0: two selects, two numbers, and control_authority
+    arriving 8 seconds later through the deferred-registration path.
+    """
+    created = _controls_created_for("MOD_6000_15000TL3_XH")
+    named = {
+        name for name, cfg in WRITABLE_REGISTERS.items() if cfg.get("register") == addr
+    }
+    leaked = sorted(named & created)
+    assert not leaked, (
+        f"MOD-XH would create {leaked} for register {addr}. #373 defers writable VPP "
+        f"controls on this family until commanding power is bounded against available PV."
+    )
+
+
+def test_the_deferred_path_honours_read_only_too():
+    """Gating only the setup loop fixes four of the five.
+
+    control_authority is skipped at setup for want of live data, then added by the
+    deferred listener once the first poll confirms 30100 answers — which is exactly what
+    the reported log shows happening 8 seconds in. The two paths have to agree.
     """
     src = (Path(__file__).parent.parent / "custom_components" / "growatt_modbus"
            / "select.py").read_text(encoding="utf-8")
-    assert "is_wit = str(register_map_name).upper() in" in src, (
-        "the WIT gate in select.py has changed shape. If it is now a capability probe, "
-        "MOD-XH will gain writable VPP controls — #373 defers those until commanding "
-        "power is bounded against available PV."
+    deferred = src[src.index("deferred_vpp: list"):src.index("if deferred_vpp:")]
+    assert "is_read_only_register" in deferred, (
+        "the deferred VPP registration path does not check the read-only flag, so a "
+        "control withheld at setup will be added a few seconds later anyway"
     )
+
+
+def test_mod_still_creates_the_controls_it_should():
+    """The counterweight. It would be easy to fix #374 by withholding too much."""
+    created = _controls_created_for("MOD_6000_15000TL3_XH")
+    for expected in ("batt_first_charge_power_rate", "batt_first_charge_stopped_soc",
+                     "grid_first_discharge_stopped_soc", "grid_charge_stopped_soc"):
+        assert expected in created, f"MOD-XH lost the {expected} control"

tests/test_stale_entity_cleanup.py

@@ -76,15 +76,32 @@ def test_controls_are_cleaned_up_by_profile_too():
 
 
 def test_control_cleanup_checks_the_register_not_the_name():
-    """Controls are gated on `control_config['register'] in holding_registers`, so the
-    cleanup has to test the same thing. Matching on the control name instead would keep
-    entities whose register was removed but whose name still exists in const.py — which is
-    exactly the 1090/1092 case, since both names are still defined there for other
-    families."""
+    """Controls are gated on the register, so the cleanup has to test the same thing.
+    Matching on the control name instead would keep entities whose register was removed
+    but whose name still exists in const.py — exactly the 1090/1092 case, since both names
+    remain defined there for other families."""
     body = _setup_entry_source()
-    assert re.search(r"control_config\.get\(\s*[\"']register[\"']\s*\)\s+in\s+holding", body), (
-        "stale-control cleanup does not test the control's register against the profile's "
-        "holding registers"
+    assert re.search(r"control_config\.get\(\s*[\"']register[\"']\s*\)", body), (
+        "stale-control cleanup does not look up the control's register"
+    )
+    assert re.search(r"\bin\s+holding\b", body), (
+        "stale-control cleanup does not test the register against the profile's holding "
+        "registers"
+    )
+
+
+def test_control_cleanup_also_removes_now_read_only_controls():
+    """A register can stop backing a control without leaving the profile.
+
+    v1.6.0 shipped five writable VPP controls on MOD because the profile's `access: 'RO'`
+    was never read. v1.6.1 makes the loops honour it — but those registers are still in
+    the profile, so a membership test alone would leave the five entities behind as
+    `unavailable` on every install that already has them (#374).
+    """
+    body = _setup_entry_source()
+    assert "is_read_only_register" in body, (
+        "stale-control cleanup does not consider read-only registers, so controls "
+        "withdrawn by marking the register RO will linger in the registry"
     )