diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 752a174..db8153e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,9 @@ jobs: - name: Cover step/stop pairing stays within the ETS Function (issue #11) run: python tests/test_cover_pairing.py + - name: RM/Rückmeldung shutter feedback classifies as status, not light (issue #12) + run: python tests/test_rm_status.py + - name: Console script is installed run: | python -c "import importlib.metadata as m; print('entry points:', [e.name for e in m.entry_points(group='console_scripts') if e.name == 'nickol-knx-mcp'])" diff --git a/nickol_knx_mcp/analyze.py b/nickol_knx_mcp/analyze.py index b54d7f1..9879c55 100644 --- a/nickol_knx_mcp/analyze.py +++ b/nickol_knx_mcp/analyze.py @@ -14,7 +14,7 @@ import re from collections import defaultdict from typing import Any, Optional -from .project import LoadedProject, GARecord, STATUS_KEYWORDS +from .project import LoadedProject, GARecord, name_is_status from .pairing import (find_status, function_status_pairs, base_tokens, positional_status, self_reporting) from .intent import INTENT_FUNCTIONAL, INTENT_RESERVE, INTENT_SCRATCH @@ -161,8 +161,7 @@ def _function_role_status(project: LoadedProject) -> list[dict[str, Any]]: def _is_status_ga(ga: GARecord) -> bool: if ga.kind == "status": return True - low = ga.name.lower() - return any(k in low for k in STATUS_KEYWORDS) + return name_is_status(ga.name) # Central / group-macro command names ("Общее освещение - Все группы", "Все diff --git a/nickol_knx_mcp/project.py b/nickol_knx_mcp/project.py index c454539..93abd15 100644 --- a/nickol_knx_mcp/project.py +++ b/nickol_knx_mcp/project.py @@ -31,6 +31,27 @@ COMMAND_KEYWORDS = [ "soll", "вкл", "выкл", "упр", "команд", "задан", ] +# Standalone status abbreviations that sit as a whole trailing token ("… RM"). +# The substring keywords above carry "rm "/"rm_" (a following delimiter) and so +# MISS a name that *ends* in the bare token, and base_tokens() drops "rm"/"fb" +# as stopwords, so token-overlap pairing can't see them either — a DPT-5.001 +# shutter feedback named "… RM" then falls to the lighting default (issue #12, +# field data by Kris1166). Region conventions vary; RM/FB are the German-market +# standard for Rückmeldung / Feedback. +_STATUS_ABBREV = frozenset({"rm", "fb"}) + + +def name_is_status(name: str) -> bool: + """True if a GA name marks it a status/feedback object — via the substring + keywords OR a standalone status abbreviation token (RM/FB) that both the + substring form and the tokenizer miss.""" + low = (name or "").lower() + if any(k in low for k in STATUS_KEYWORDS): + return True + for ch in "/-_.,()[]:": + low = low.replace(ch, " ") + return any(t in _STATUS_ABBREV for t in low.split()) + # Domain terms by name (used to decide a GA's functional domain as ONE signal, # combined with the DPT and the group-range context — see _classify_category). # Checked in this priority order (most specific first; lighting is the generic @@ -256,7 +277,7 @@ def _split_three_level(address: str) -> tuple[Optional[int], Optional[int], Opti def _override_kind_by_name(name: str, kind: str) -> str: """Refine command/status using name keywords (helps when DPT is generic).""" low = name.lower() - if any(k in low for k in STATUS_KEYWORDS): + if name_is_status(name): return "status" if any(k in low for k in COMMAND_KEYWORDS): # only upgrade unknown -> command; never overwrite explicit sensor @@ -356,29 +377,55 @@ def build_loaded_from_raw(raw: KNXProject, path: str) -> LoadedProject: ) gas[rec.address] = rec - # Authoritative shutter classification from ETS Function roles. A GA whose - # Function role is a shutter role (MoveUpDown / StopStepUpDown / slat) IS a - # shutter object — even when its bare DPT is domain-agnostic (1.007 step, - # 1.010 start/stop) and its name carries no shutter keyword. The Function - # role outranks DPT and name (explain_ga's own hierarchy), so it rescues the - # step/stop GAs a name-only classifier leaves 'unknown' (issue #11, field - # data by Kris1166). Never demotes: only promotes a non-shutter GA. + # Authoritative shutter classification from ETS Function MEMBERSHIP. + # A Function that carries a shutter role (MoveUpDown / StopStepUpDown / slat) + # on ANY of its members is a shutter Function — so every GA inside it is a + # shutter object, even one with a blank role, a domain-agnostic bare DPT + # (1.007 step, 1.010 start/stop) and no shutter keyword in its name: + # * the step/stop a name-only classifier leaves 'unknown' (issue #11), and + # * a DPT-5.001 position feedback named only "… RM" (Rückmeldung) that + # otherwise falls to the lighting brightness default (issue #12). + # The Function outranks DPT and name (explain_ga's hierarchy). Never demotes; + # only promotes a non-shutter GA whose DPT is plausibly a shutter object + # (1.x control / 5.x position) — never a 9.x temperature / 13.x energy GA. + # An ETS Function is a free grouping, not proof of a single domain (an + # integrator can drop lighting, shutters and a scene recall into one "Floor 1" + # Function). So promote a member to shutter only on POSITIVE shutter evidence, + # not blanket membership (LLM-council review of the issue #12 fix): + # * the member is itself a step/stop control (DPT 1.007 / 1.010), or a + # DPT-5.001 POSITION STATUS (a feedback — never a 5.001 command, which could + # be a scene recall / dimming value), AND + # * the Function actually carries an up/down MOVE (DPT 1.008) — the sibling + # that makes it a real shutter, not just a shutter-role substring, AND + # * the member's name carries no EXPLICIT non-shutter domain (defense in depth). + # This rescues the issue #11 step/stop and the issue #12 "… RM" position feedback + # while leaving a scene recall / dimmer feedback / boolean lock in a mixed + # Function untouched. + def _promote_to_shutter(rec: Optional[GARecord], has_move: bool) -> None: + if rec is None or rec.category == "shutter" or not has_move: + return + is_step = rec.dpt_main == 1 and rec.dpt_sub in (7, 10) + is_pos_status = rec.dpt_main == 5 and rec.dpt_sub == 1 and rec.kind == "status" + if not (is_step or is_pos_status): + return + dom = _domain_from_text(rec.name) + if dom and dom != "shutter": + return + rec.category = "shutter" + if rec.ha_platform in ("light", "switch", "unknown"): + rec.ha_platform = "cover" + for fn in (raw.get("functions", {}) or {}).values(): - for a_key, ref in (fn.get("group_addresses", {}) or {}).items(): - role = (ref.get("role") or "").lower() - if not any(t in role for t in _SHUTTER_ROLE_TOKENS): - continue - rec = gas.get(ref.get("address") or a_key) - if rec is None or rec.category == "shutter": - continue - # Guard: a shutter role only promotes a plausibly-shutter DPT — a 1-bit - # control (move/step/stop) or a 5.x position — never retype a 9.x - # temperature / 13.x energy GA on a role substring like "updown". - if rec.dpt_main not in (1, 5): - continue - rec.category = "shutter" - if rec.ha_platform in ("light", "switch", "unknown"): - rec.ha_platform = "cover" + members = fn.get("group_addresses", {}) or {} + recs = [gas.get(ref.get("address") or a_key) for a_key, ref in members.items()] + has_shutter_role = any( + any(t in (ref.get("role") or "").lower() for t in _SHUTTER_ROLE_TOKENS) + for ref in members.values()) + has_move = any(r is not None and r.dpt_main == 1 and r.dpt_sub == 8 for r in recs) + if not has_shutter_role: + continue + for rec in recs: + _promote_to_shutter(rec, has_move) return LoadedProject( path=path, diff --git a/tests/test_rm_status.py b/tests/test_rm_status.py new file mode 100644 index 0000000..a30525b --- /dev/null +++ b/tests/test_rm_status.py @@ -0,0 +1,123 @@ +"""Regression: a DPT-5.001 shutter position feedback named "… RM" (Rückmeldung, +the German-market status abbreviation) must be recognised as the cover's position +STATUS — not misclassified as a standalone lighting/light entity (issue #12, field +data by Kris1166). + +Two layers were broken and are both covered here: + * classification — the RM GA sits in a shutter ETS Function (blank role) but a + bare DPT 5.001 with no shutter keyword fell to the lighting default; Function + membership now promotes it to `shutter`; + * status recognition — "RM"/"FB" is a whole trailing token the substring + keywords ("rm "/"rm_") and the stopword tokenizer both dropped, so it read as + a command; `name_is_status` now recognises it, so it wires as the cover's + `position_state_address` (a status), never a `position_address` (a command). + +Negative guard: a genuine LIGHT brightness feedback named "… RM" that is NOT in a +shutter Function must stay `light` — the promotion is Function-scoped, not a +blanket "RM ⇒ shutter". +""" +from nickol_knx_mcp.project import build_loaded_from_raw +from nickol_knx_mcp.generate_ha import generate_ha_yaml +from nickol_knx_mcp.analyze import detect_missing_status +import yaml + + +def _ga(addr, name, dm, ds): + return {"name": name, "identifier": f"G{addr}", "raw_address": 0, "address": addr, + "project_uid": None, "dpt": {"main": dm, "sub": ds}, "data_secure": False, + "communication_object_ids": [], "description": "", "comment": ""} + + +def _fn(name, *addr_roles): + return {"name": name, "group_addresses": {a: {"address": a, "role": r} for a, r in addr_roles}} + + +def _build(): + gas, fns = {}, {} + # Window shutter: RM status IS in the per-channel function (blank role); move + # carries MoveUpDown. This is the 26/29 case that was misclassified as light. + gas["2/1/26"] = _ga("2/1/26", "Zone18 Fenster Bewegen", 1, 8) + gas["2/2/26"] = _ga("2/2/26", "Zone18 Fenster Schritt/Stop", 1, 7) + gas["2/3/26"] = _ga("2/3/26", "Zone18 Fenster RM", 5, 1) + fns["FN-Fenster-26"] = _fn("Fenster", ("2/1/26", "MoveUpDown"), + ("2/2/26", "StopStepUpDown"), ("2/3/26", "")) + # Awning: today it is "accidentally" rescued by the "Markise" keyword — it must + # still resolve to the cover's position status, now for the RIGHT reason. + gas["2/1/50"] = _ga("2/1/50", "MarkiseZone1 Bewegen", 1, 8) + gas["2/2/50"] = _ga("2/2/50", "MarkiseZone1 Schritt/Stop", 1, 17) + gas["2/3/50"] = _ga("2/3/50", "MarkiseZone1 RM", 5, 1) + fns["FN-Markise-1"] = _fn("Markise Z1", ("2/1/50", ""), ("2/2/50", ""), ("2/3/50", "")) + # NEGATIVE: a real light with a brightness feedback named "RM", NOT in a + # shutter function — must stay a light, RM is its brightness state. + gas["1/0/0"] = _ga("1/0/0", "Küche Licht schalten", 1, 1) + gas["1/1/0"] = _ga("1/1/0", "Küche Licht Helligkeit", 5, 1) + gas["1/3/0"] = _ga("1/3/0", "Küche Licht RM", 5, 1) + # NEGATIVE 2 (gate-1 audit): a blank-role light GA that shares a heterogeneous + # shutter Function must NOT be promoted — its explicit light domain wins. + gas["2/1/60"] = _ga("2/1/60", "Zone20 Rollladen Bewegen", 1, 8) + gas["2/2/60"] = _ga("2/2/60", "Living Room Light On", 1, 1) # stray light in a shutter fn + fns["FN-Mixed-60"] = _fn("Mixed", ("2/1/60", "MoveUpDown"), ("2/2/60", "")) + # NEGATIVE 3 (LLM-council): a DPT-5.001 scene-recall COMMAND (blank role, neutral + # name) sharing a shutter Function must NOT become a cover — it is not a position + # STATUS, so promotion must skip it even though the Function is a shutter one. + gas["2/1/70"] = _ga("2/1/70", "Zone30 Rollladen Bewegen", 1, 8) + gas["2/2/70"] = _ga("2/2/70", "Zone30 Rollladen Schritt/Stop", 1, 7) + gas["2/4/70"] = _ga("2/4/70", "Zone30 Szene Abruf", 5, 1) # 5.001 COMMAND, not status + fns["FN-Scene-70"] = _fn("Rollladen70", ("2/1/70", "MoveUpDown"), + ("2/2/70", "StopStepUpDown"), ("2/4/70", "")) + + raw = {"info": {"group_address_style": "ThreeLevel", "schema_version": "21"}, + "group_addresses": gas, "communication_objects": {}, "devices": {}, + "functions": fns, "topology": {}, + "group_ranges": {"2": {"address_start": 4096, "name": "Beschattung", + "group_ranges": {}}}} + return build_loaded_from_raw(raw, "rm-status.knxproj") + + +def main(): + p = _build() + + # 1. Classification: each shutter RM GA is category=shutter, kind=status. + for a in ("2/3/26", "2/3/50"): + g = p.gas[a] + assert g.category == "shutter", f"{a}: category {g.category!r}, expected shutter" + assert g.kind == "status", f"{a}: kind {g.kind!r}, expected status" + # negative: the LIGHT's RM stays lighting (not promoted — not a shutter function) + assert p.gas["1/3/0"].category == "lighting", p.gas["1/3/0"].category + assert p.gas["1/3/0"].kind == "status", "a light's RM is still a status" + # negative 2: a stray light inside a heterogeneous shutter function stays lighting + assert p.gas["2/2/60"].category == "lighting", \ + f"stray light in a shutter fn was over-promoted: {p.gas['2/2/60'].category}" + # negative 3: a 5.001 scene-recall COMMAND in a shutter function is NOT promoted + # (only a 5.001 position STATUS is), while the step/stop 2/2/70 still is. + assert p.gas["2/4/70"].category != "shutter", \ + f"a 5.001 scene-recall command was over-promoted to shutter: {p.gas['2/4/70'].category}" + assert p.gas["2/2/70"].category == "shutter", "the step/stop in the same fn must promote" + + doc = yaml.safe_load(generate_ha_yaml(p)["yaml"]) + covers = {c["move_long_address"]: c for c in doc.get("knx", {}).get("cover", []) or []} + lights = {l.get("address"): l for l in doc.get("knx", {}).get("light", []) or []} + + # 2. Each cover gets its RM GA as position_state — and the RM GA is NOT a light. + assert covers["2/1/26"].get("position_state_address") == "2/3/26", covers["2/1/26"] + assert covers["2/1/50"].get("position_state_address") == "2/3/50", covers["2/1/50"] + light_addrs = {a for l in lights.values() for a in + (l.get("address"), l.get("state_address"), l.get("brightness_address"), + l.get("brightness_state_address"))} + assert "2/3/26" not in light_addrs, "the shutter RM was wired as a light" + assert "2/3/50" not in light_addrs, "the awning RM was wired as a light" + + # 3. negative: the real light still exists and uses its RM as brightness state. + assert "1/0/0" in lights, "the genuine light disappeared" + + # 4. no false 'missing status' for the covers whose RM status now pairs. + miss = {f["address"] for f in detect_missing_status(p) + if f.get("code") == "missing_status_address"} + assert "2/1/26" not in miss and "2/1/50" not in miss, f"false missing-status: {miss}" + + print("test_rm_status: OK — '… RM' shutter feedback classifies as shutter/status and " + "wires as the cover's position_state (issue #12); a light's RM stays a light.") + + +if __name__ == "__main__": + main()