From fec99b907bb71de11bba3201ee46dfbaeb2e43ee Mon Sep 17 00:00:00 2001 From: Codex Date: Thu, 3 Sep 2026 18:48:23 +0300 Subject: [PATCH] feat: record vacuum trails under the route that produced them User-Visible: no Issue: #162 --- custom_components/houseplan/trails.py | 114 +++++++++++++++++++++++--- scripts/mutation-gate.mjs | 27 ++++++ tests_backend/test_trail_recorder.py | 77 +++++++++++++++++ tests_backend/test_trails.py | 71 ++++++++++++++++ 4 files changed, 277 insertions(+), 12 deletions(-) diff --git a/custom_components/houseplan/trails.py b/custom_components/houseplan/trails.py index fbf8dae4..5c009aa6 100755 --- a/custom_components/houseplan/trails.py +++ b/custom_components/houseplan/trails.py @@ -21,6 +21,7 @@ from homeassistant.helpers.event import async_call_later, async_track_state_chan from homeassistant.helpers.storage import Store from .const import DOMAIN +from .vacuum_routes import effective_routes _LOGGER = logging.getLogger(__name__) @@ -31,14 +32,31 @@ FIRE_THROTTLE_S = 2.0 # event-bus updates for live cards MOVING_STATES = {"cleaning", "returning", "on"} -def can_resume_trail_run(run: Any, map_id: str, now: float) -> bool: +def same_run_identity(run: Any, route_id: str, map_id: str) -> bool: + """Whether a stored run and a fresh point belong to the same cleanup. + + Route identity wins when both sides have one: two maps of one robot may + share a map id only across different sources, and a route change (new + source, map or target space) is a new run by definition (#162). A run + written before #162 carries no route id, so it is still matched by map id + and keeps resuming exactly as it used to. + """ + if not isinstance(run, dict): + return False + stored_route = run.get("route_id") + if route_id and isinstance(stored_route, str) and stored_route: + return stored_route == route_id + return run.get("map_id") == map_id + + +def can_resume_trail_run(run: Any, map_id: str, now: float, route_id: str = "") -> bool: """Whether an ended current run may be reopened for this point. Store timestamps are untrusted persisted data. Only finite JSON-number timestamps and a non-negative inclusive grace interval are accepted; malformed values and wall-clock rollback fail closed into a new run. """ - if not isinstance(run, dict) or run.get("map_id") != map_id: + if not same_run_identity(run, route_id, map_id): return False ended = run.get("ended") if ( @@ -84,18 +102,27 @@ class TrailBook: def __init__(self, data: dict[str, Any] | None = None) -> None: self.data: dict[str, Any] = data if isinstance(data, dict) else {} - def on_point(self, marker: str, map_id: str, x: float, y: float, now: float) -> bool: + def on_point( + self, marker: str, map_id: str, x: float, y: float, now: float, + route_id: str = "", source: str = "", + ) -> bool: rec = self.data.setdefault(marker, {}) cur = rec.get("current") - resumed = bool(cur and can_resume_trail_run(cur, map_id, now)) + resumed = bool(cur and can_resume_trail_run(cur, map_id, now, route_id)) if resumed: cur["ended"] = None - if not cur or cur.get("ended") is not None or cur.get("map_id") != map_id: + if not cur or cur.get("ended") is not None or not same_run_identity(cur, route_id, map_id): # a new run begins: the old one becomes "previous" (and the one # before it is forgotten — we keep exactly two, per the owner) if cur: rec["previous"] = cur cur = {"map_id": map_id, "started": now, "ended": None, "points": []} + # #162: the run remembers WHICH route wrote it, so the card can put + # it on the right floor without re-deriving the routing itself. + if route_id: + cur["route_id"] = route_id + if source: + cur["source"] = source rec["current"] = cur pts: list[list[float]] = cur["points"] if pts and pts[-1][0] == x and pts[-1][1] == y: @@ -122,11 +149,33 @@ class TrailBook: """Forget every stored run of one plan marker.""" return self.data.pop(marker, None) is not None + def drop_unknown_routes(self, marker: str, route_ids: set[str]) -> bool: + """Forget runs whose route no longer exists (deleted or re-targeted). + + Only runs that name a route are touched. A pre-#162 run names none and + is adopted by the card instead — deleting it here would destroy history + the user can still legitimately see. + """ + rec = self.data.get(marker) + if not isinstance(rec, dict): + return False + changed = False + for slot in ("current", "previous"): + run = rec.get(slot) + if not isinstance(run, dict): + continue + stored = run.get("route_id") + if isinstance(stored, str) and stored and stored not in route_ids: + rec.pop(slot) + changed = True + return changed + class TrailRecorder: """HA wiring: watch the tracked entities, feed the book, persist, notify.""" def __init__(self, hass: HomeAssistant, rt: Any) -> None: + self.routes_by_marker: dict[str, list[dict[str, Any]]] = {} self.hass = hass self.rt = rt self.store = Store(hass, 1, f"{DOMAIN}.trails") @@ -166,22 +215,35 @@ class TrailRecorder: cfg = stored.get("config") or {} pairs: dict[str, list[tuple[str, str]]] = {} health_pairs: set[tuple[str, str]] = set() + routes_by_marker: dict[str, list[dict[str, Any]]] = {} for m in cfg.get("markers") or []: if m.get("removed") is True: continue v = m.get("vacuum") or {} src = v.get("source") - if not src or v.get("live") is False: + if v.get("live") is False: continue marker_id = str(m.get("id")) - health_pairs.add((marker_id, str(src))) + # #162: a multi-floor robot may publish each map through its own + # camera, so every route source is watched — not only the root + # discovery source, which is now just one of them. + routes = effective_routes(marker_id, v, str(m.get("space") or ""), src) + routes_by_marker[marker_id] = routes + sources = {str(route["source"]) for route in routes if route.get("source")} + if src: + sources.add(str(src)) + if not sources: + continue vac = self._vacuum_entity(m) - if vac: - # HP-1540-03: append, never overwrite — every floor's - # marker records its own copy of the run - pairs.setdefault(src, []).append((marker_id, vac)) + for source in sorted(sources): + health_pairs.add((marker_id, source)) + if vac: + # HP-1540-03: append, never overwrite — every floor's + # marker records its own copy of the run + pairs.setdefault(source, []).append((marker_id, vac)) self._refresh_source_health(health_pairs) self.pairs = pairs + self.routes_by_marker = routes_by_marker self._resubscribe() # A run already in progress (HA restarted mid-cleanup, or the user # just finished calibrating) must start recording NOW, not at the @@ -261,6 +323,17 @@ class TrailRecorder: for marker in config.get("markers") or [] if marker.get("id") is not None and marker.get("removed") is not True } + # #162: a route that vanished (deleted, or re-targeted to another + # space, which is a new identity) takes its own runs with it, while the + # marker and its other routes stay untouched. + for marker in config.get("markers") or []: + marker_id = str(marker.get("id")) + if marker.get("removed") is True or marker_id not in self.book.data: + continue + vacuum = marker.get("vacuum") or {} + routes = effective_routes( + marker_id, vacuum, str(marker.get("space") or ""), vacuum.get("source")) + self.book.drop_unknown_routes(marker_id, {str(r.get("id")) for r in routes}) orphan_ids = set(self.book.data) - live_marker_ids if not orphan_ids: return 0 @@ -381,9 +454,26 @@ class TrailRecorder: except (TypeError, ValueError): continue map_id = resolve_map_id(attrs, st_vac.attributes) - changed |= self.book.on_point(marker, map_id, x, y, now) + route_id = self._route_id(marker, src, map_id) + changed |= self.book.on_point( + marker, map_id, x, y, now, route_id=route_id, + source=src if route_id else "", + ) return changed + def _route_id(self, marker: str, source: str, map_id: str) -> str: + """The route this point belongs to, or "" when routing cannot say. + + Validation keeps (source, map_id) unique inside one marker, so at most + one route can match. No match means the map is unmapped: the point is + still recorded — raw history is valuable — but it is not filed under a + route that does not own it. + """ + for route in getattr(self, "routes_by_marker", {}).get(marker) or (): + if route.get("source") == source and route.get("map_id") == map_id: + return str(route.get("id") or "") + return "" + @callback def _on_state(self, event: Any) -> None: eid = event.data.get("entity_id") diff --git a/scripts/mutation-gate.mjs b/scripts/mutation-gate.mjs index 8fd13ba2..76d164cc 100644 --- a/scripts/mutation-gate.mjs +++ b/scripts/mutation-gate.mjs @@ -315,6 +315,33 @@ const MUTANT_DEFINITIONS = [ replace: ' const previousAllowed = !!previousRoute\n && !!active && active.space === input.renderSpace', }], }, + { + id: 'vacuum-run-forgets-its-route', + guard: 'node scripts/backend-test-guard.mjs ' + + 'route_change_starts_a_new_run_even_on_the_same_map_id ' + + 'tests_backend/test_trails.py', + because: 'a stored run must remember which route wrote it, or two maps that share a map ' + + 'id across different cameras collapse into one run on the wrong floor (#162, M-F)', + patches: [{ + file: 'custom_components/houseplan/trails.py', + find: ' if route_id:\n cur["route_id"] = route_id', + replace: ' if False:\n cur["route_id"] = route_id', + }], + }, + { + id: 'vacuum-retargeted-route-keeps-its-old-trails', + guard: 'node scripts/backend-test-guard.mjs ' + + 'drop_unknown_routes_touches_only_runs_that_name_a_route ' + + 'tests_backend/test_trails.py', + because: 'a route that was deleted or re-targeted to another space must take its runs ' + + 'with it; keeping them replays an old cleanup on a floor it never happened on ' + + '(#162, M-E server half)', + patches: [{ + file: 'custom_components/houseplan/trails.py', + find: ' if isinstance(stored, str) and stored and stored not in route_ids:', + replace: ' if False:', + }], + }, { id: 'area-snapshot-cleanup-ignores-authority', guard: 'npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs ' diff --git a/tests_backend/test_trail_recorder.py b/tests_backend/test_trail_recorder.py index 7943b8ab..55354423 100644 --- a/tests_backend/test_trail_recorder.py +++ b/tests_backend/test_trail_recorder.py @@ -729,3 +729,80 @@ def test_overlapping_refreshes_leave_one_subscription_teardown_zero(): _run_isolated(scenario()) finally: trails.async_track_state_change_event = old_track + + +def test_refresh_watches_every_route_source_not_only_the_root(monkeypatch): + """#162: карты одного робота могут идти через разные камеры.""" + import asyncio + + tracked = [] + old_track = trails.async_track_state_change_event + trails.async_track_state_change_event = lambda hass, ents, cb: ( + tracked.append(sorted(ents)) or (lambda: None)) + try: + markers = [{ + "id": "m1", "binding": "entity:vacuum.x50", "space": "floor1", + "vacuum": { + "source": "camera.floor1", + "map_routes": [ + {"id": "vr1", "source": "camera.floor1", "map_id": "a", "space": "floor1"}, + {"id": "vr2", "source": "camera.floor2", "map_id": "b", "space": "floor2"}, + ], + }, + }] + + class CS: + async def async_load(self): + return {"config": {"markers": markers}} + + class RT: + config_store = CS() + + hass = Hass({ + "vacuum.x50": S("docked", {}), + "camera.floor1": S("idle", {}), + "camera.floor2": S("idle", {}), + }) + rec = trails.TrailRecorder(hass, RT()) + _run_isolated(rec.async_refresh()) + assert sorted(rec.pairs) == ["camera.floor1", "camera.floor2"] + assert rec.pairs["camera.floor2"] == [("m1", "vacuum.x50")] + assert tracked == [["camera.floor1", "camera.floor2", "vacuum.x50"]] + assert [r["id"] for r in rec.routes_by_marker["m1"]] == ["vr1", "vr2"] + finally: + trails.async_track_state_change_event = old_track + + +def test_sample_files_the_point_under_its_route(): + states = { + "vacuum.x50": S("cleaning", {}), + "camera.floor2": S("idle", {"vacuum_position": {"x": 5, "y": 6}, "map_name": "b"}), + } + rec = trails.TrailRecorder(Hass(states), None) + rec.pairs = {"camera.floor2": [("m1", "vacuum.x50")]} + rec.routes_by_marker = {"m1": [ + {"id": "vr1", "source": "camera.floor1", "map_id": "b", "space": "floor1"}, + {"id": "vr2", "source": "camera.floor2", "map_id": "b", "space": "floor2"}, + ]} + rec._on_state(E("camera.floor2")) + run = rec.book.data["m1"]["current"] + assert run["route_id"] == "vr2", "источник, а не только id карты, выбирает маршрут" + assert run["source"] == "camera.floor2" + assert run["points"] == [[5.0, 6.0]] + + +def test_sample_without_a_matching_route_records_legacy_shaped_run(): + states = { + "vacuum.x50": S("cleaning", {}), + "camera.map": S("idle", {"vacuum_position": {"x": 1, "y": 2}, "map_name": "неизвестная"}), + } + rec = trails.TrailRecorder(Hass(states), None) + rec.pairs = {"camera.map": [("m1", "vacuum.x50")]} + rec.routes_by_marker = {"m1": [ + {"id": "vr1", "source": "camera.map", "map_id": "b", "space": "floor1"}, + ]} + rec._on_state(E("camera.map")) + run = rec.book.data["m1"]["current"] + assert "route_id" not in run, "чужой маршрут прогону не приписывается" + assert run["map_id"] == "неизвестная" + assert run["points"] == [[1.0, 2.0]] diff --git a/tests_backend/test_trails.py b/tests_backend/test_trails.py index f5ea9f9c..15656d12 100644 --- a/tests_backend/test_trails.py +++ b/tests_backend/test_trails.py @@ -143,3 +143,74 @@ def test_junk_store_data_tolerated(): assert b.data == {} b.on_point("m", "0", 1, 1, 1.0) assert b.data["m"]["current"]["points"] == [[1, 1]] + + +# --- #162: маршруты карт и пространств --------------------------------------- + +def test_route_change_starts_a_new_run_even_on_the_same_map_id(): + b = TrailBook() + b.on_point("m", "default", 1.0, 1.0, 100.0, route_id="vr_a", source="camera.a") + b.on_point("m", "default", 2.0, 2.0, 101.0, route_id="vr_b", source="camera.b") + rec = b.data["m"] + assert rec["previous"]["route_id"] == "vr_a" + assert rec["current"]["route_id"] == "vr_b" + assert rec["current"]["source"] == "camera.b" + assert rec["current"]["points"] == [[2.0, 2.0]] + + +def test_same_route_keeps_one_run(): + b = TrailBook() + b.on_point("m", "default", 1.0, 1.0, 100.0, route_id="vr_a", source="camera.a") + b.on_point("m", "default", 2.0, 2.0, 101.0, route_id="vr_a", source="camera.a") + assert "previous" not in b.data["m"] + assert b.data["m"]["current"]["points"] == [[1.0, 1.0], [2.0, 2.0]] + + +def test_run_without_route_stays_legacy_shaped(): + b = TrailBook() + b.on_point("m", "0", 1.0, 2.0, 100.0) + run = b.data["m"]["current"] + assert "route_id" not in run and "source" not in run + assert set(run) == {"map_id", "started", "ended", "points"} + + +def test_legacy_run_resumes_by_map_id_as_before(): + b = TrailBook() + b.on_point("m", "0", 1.0, 2.0, 100.0) + b.end_run("m", 200.0) + assert b.on_point("m", "0", 3.0, 4.0, 300.0) + assert "previous" not in b.data["m"] + assert b.data["m"]["current"]["points"] == [[1.0, 2.0], [3.0, 4.0]] + + +def test_route_run_does_not_resume_into_another_route(): + b = TrailBook() + b.on_point("m", "default", 1.0, 2.0, 100.0, route_id="vr_a") + b.end_run("m", 200.0) + b.on_point("m", "default", 3.0, 4.0, 300.0, route_id="vr_b") + assert b.data["m"]["previous"]["route_id"] == "vr_a" + assert b.data["m"]["current"]["route_id"] == "vr_b" + + +def test_drop_unknown_routes_touches_only_runs_that_name_a_route(): + b = TrailBook() + b.on_point("m", "m1", 1.0, 1.0, 100.0, route_id="vr_gone") + b.on_point("m", "m2", 2.0, 2.0, 101.0, route_id="vr_live") + assert b.drop_unknown_routes("m", {"vr_live"}) is True + assert "previous" not in b.data["m"] + assert b.data["m"]["current"]["route_id"] == "vr_live" + assert b.drop_unknown_routes("m", {"vr_live"}) is False + legacy = TrailBook() + legacy.on_point("m", "m1", 1.0, 1.0, 100.0) + assert legacy.drop_unknown_routes("m", set()) is False, "легаси-прогон не трогаем" + assert legacy.data["m"]["current"]["points"] == [[1.0, 1.0]] + assert TrailBook().drop_unknown_routes("нет такого", {"vr"}) is False + + +def test_same_run_identity_contract(): + same = ns["same_run_identity"] + assert same({"route_id": "vr_a", "map_id": "x"}, "vr_a", "y") is True + assert same({"route_id": "vr_a", "map_id": "x"}, "vr_b", "x") is False + assert same({"map_id": "x"}, "vr_a", "x") is True, "легаси-прогон опознаётся картой" + assert same({"map_id": "x"}, "", "y") is False + assert same(None, "vr", "x") is False