diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b2ea89..0282526 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to `ebus-sdk` are recorded here. Format follows [Keep a Chan ## [Unreleased] +### Added + +- `DeviceTreeBuilder.remove_capabilities()`: the inverse of `extend()`. A capability that becomes relevant at runtime can stop being relevant, and without this its node stayed advertised in `$description` with retained topics behind it. `Device.delete_node()` already clears those and re-announces, so what this closes is the bookkeeping: reaching around the builder to call it left `model_keys` and `created_groups` describing properties that no longer existed, and a later `remove()` working from that stale record. Idempotent like `extend()`, and named for capabilities rather than nodes because that is the declarative vocabulary. ([#78](https://github.com/electrification-bus/python-sdk/issues/78)) + +### Changed + +- `DeviceTreeBuilder` keys its bookkeeping on the **resolved device id** rather than on `DeviceSpec` object identity. A producer deriving its spec set from a manifest re-derives equal-but-distinct objects on every pass, and identity keying made each pass a new device; the alternative was an unstated obligation to hold a `device_id -> DeviceSpec` map for the process lifetime and never re-derive, which is precisely what a declarative API exists to avoid. `add()`, `remove()`, `extend()`, `device_for()` and `homie_properties()` now all answer for any spec naming the same device. `add()` remains idempotent on the DEVICE rather than on the declaration: a differing capability set on an already-built id returns the existing device unchanged rather than applying the difference, since `add()` mutating a live tree is not what its name suggests; `extend()` is how a built device grows. Deferred specs stay keyed by identity, having no id yet by definition. ([#74](https://github.com/electrification-bus/python-sdk/issues/74)) + ### Fixed - `conditionally_settable` was inert: `_materialize` never read it. The half that looked right is that the property did come out not-settable; the half that bit is that the `entity_setter` was registered only when `settable` was true, so the caller's later `set_settable(True)` opened a `/set` topic with no translator behind it. The property then advertised that it accepts commands and silently discarded them, which is the exact failure the field was introduced to avoid, one step further along. It is the only route the API offers for per-instance settability decided at runtime, and it was the route that did not work. The translator is now wired at build time even though the property starts not-settable. The test that shipped with the feature asserted only the not-settable half, which is why this survived review: a test written from the design rationale checks the rationale rather than the feature. ([#72](https://github.com/electrification-bus/python-sdk/issues/72)) diff --git a/doc/building-a-proxy.md b/doc/building-a-proxy.md index eb7213e..c34ed63 100644 --- a/doc/building-a-proxy.md +++ b/doc/building-a-proxy.md @@ -196,6 +196,8 @@ Four things the tree builder does that the single-device one has no need to: - **`add()` is idempotent.** Incremental lifecycles re-fire, and a second `add()` of a built spec returns the same `Device` without republishing anything. - **`remove()` is depth-first**, grandchild before parent, derived from the live tree rather than an ordering you maintain, so nothing ever observes an orphaned child. It also deletes the model entries it added, and any group it created that is now empty. +Specs are matched by the device id they resolve to, not by object identity, so you can re-derive your spec set from a manifest on every pass and `add()` / `extend()` / `remove()` keep answering for the same device. `add()` is idempotent on the device rather than the declaration: a differing capability set on a built id returns the existing device, and `extend(spec, specs)` / `remove_capabilities(spec, capabilities)` are how a built device grows and shrinks. + The root is not only a parent: `builder.add_root_capabilities(specs)` materializes capabilities onto the root device itself, keyed in the model by the root's device id, and `builder.extend(spec, specs)` gives an already-built device a capability it did not have at boot. Both are idempotent, and a call that would create nothing does not open a state transition at all, so a re-fired lifecycle costs no `init` to `ready` edge (an empty one still forces every controller on the bus to resync). Each `add()` announces its own device and makes the parent republish its `$description`. To collapse a burst of adds into one parent announcement, wrap them in the parent's `state_transition()`. Use `on_created` for per-child side effects rather than post-processing the returned tree. diff --git a/src/ebus_sdk/declaration.py b/src/ebus_sdk/declaration.py index 1b91f90..6c89c62 100644 --- a/src/ebus_sdk/declaration.py +++ b/src/ebus_sdk/declaration.py @@ -210,7 +210,7 @@ class _Materialized: homie_props: dict declared: dict - model_keys: list + model_keys: list # (capability, group, model_key), so a node's share is identifiable created_groups: list @@ -297,7 +297,7 @@ def _materialize( model.add_property(group, ObservableProperty(id=spec.model_key, type=py_type)) # Only what this call created, so a later remove() deletes what # it added and leaves anything the producer owned first. - model_keys.append((group, spec.model_key)) + model_keys.append((capability, group, spec.model_key)) elif existing.type() is not py_type: raise ValueError( f"{capability}/{spec.prop_id}: the model already holds " @@ -561,19 +561,29 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: the same `Device` without touching the tree, because incremental lifecycles re-fire and a second add must not republish or duplicate. """ - existing = self._devices.get(spec) - if existing is not None: - return existing + # Keyed on the RESOLVED DEVICE ID, not on this spec object. A producer + # deriving its spec set from a manifest re-derives equal-but-distinct + # objects on every pass, and identity keying silently made each pass a + # new device; the alternative was an unstated obligation to hold a + # device_id -> DeviceSpec map for the process lifetime and never + # re-derive, which defeats the point of a declarative API. A spec whose + # id is already built returns that device unchanged: to give a built + # device more capabilities, use extend(). + device_id = spec.resolve_device_id() + if device_id is not None: + existing = self._devices.get(device_id) + if existing is not None: + return existing if spec.parent is None: parent_device: Optional[Device] = self._root else: - parent_device = self._devices.get(spec.parent) or self.add(spec.parent) + parent_id = spec.parent.resolve_device_id() + parent_device = (self._devices.get(parent_id) if parent_id else None) or self.add(spec.parent) if parent_device is None: self._defer(spec) # the parent is itself waiting on an id return None - device_id = spec.resolve_device_id() if device_id is None: self._defer(spec) return None @@ -590,10 +600,10 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: # dispatch synchronously) must not leave a live device the builder has no # record of: device_for() would return None and remove() would be a # silent no-op, stranding retained topics. - self._devices[spec] = device - self._homie_props[spec] = {} - self._model_keys[spec] = [] - self._created_groups[spec] = [] + self._devices[device_id] = device + self._homie_props[device_id] = {} + self._model_keys[device_id] = [] + self._created_groups[device_id] = [] group = spec.resolve_model_group(device_id) built = _materialize( @@ -607,9 +617,9 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: ) _seed(self._model, built.declared, default_group=group) - self._homie_props[spec].update(built.homie_props) - self._model_keys[spec].extend(built.model_keys) - self._created_groups[spec].extend(built.created_groups) + self._homie_props[device_id].update(built.homie_props) + self._model_keys[device_id].extend(built.model_keys) + self._created_groups[device_id].extend(built.created_groups) if spec in self._deferred: self._deferred.remove(spec) if spec.on_created is not None: @@ -649,12 +659,13 @@ def remove(self, spec: DeviceSpec) -> None: # deliberately torn down. self._deferred = [s for s in self._deferred if not _descends_from(s, spec)] - device = self._devices.get(spec) + device_id = spec.resolve_device_id() + device = self._devices.get(device_id) if device_id is not None else None if device is None: return # never built (or already removed); the queue is now clean doomed = {id(d) for d in _descendants(device)} - removed = [s for s, d in self._devices.items() if id(d) in doomed] + removed = [k for k, d in self._devices.items() if id(d) in doomed] device.delete() for gone in removed: # Bookkeeping is dropped whatever the model does, so a teardown can @@ -664,7 +675,7 @@ def remove(self, spec: DeviceSpec) -> None: # arrive after the group is gone, since delete_group removes it # before firing and dispatch is synchronous. try: - for group, model_key in self._model_keys.get(gone, []): + for _capability, group, model_key in self._model_keys.get(gone, []): if self._model.has_group(group) and self._model.get(group, model_key) is not None: self._model.delete_property(group, model_key) for group in self._created_groups.get(gone, []): @@ -706,7 +717,7 @@ def add_root_capabilities(self, specs: Iterable[PropertySpec], *, model_group: O ) _seed(self._model, built.declared, default_group=group) self._root_props.update(built.homie_props) - self._root_model_keys.extend(built.model_keys) + self._root_model_keys.extend(built.model_keys) # (capability, group, model_key) return dict(self._root_props) def root_capabilities(self) -> dict: @@ -730,7 +741,8 @@ def extend(self, spec: DeviceSpec, specs: Iterable[PropertySpec]) -> dict: Raises `KeyError` for a spec that is not built. Use `add()` first; a deferred device has no tree to extend. """ - device = self._devices.get(spec) + device_id = spec.resolve_device_id() + device = self._devices.get(device_id) if device_id is not None else None if device is None: raise KeyError( f"{spec.device_class}: not built, so there is nothing to extend. " @@ -747,14 +759,64 @@ def extend(self, spec: DeviceSpec, specs: Iterable[PropertySpec]) -> dict: default_group=group, ) _seed(self._model, built.declared, default_group=group) - self._homie_props[spec].update(built.homie_props) - self._model_keys[spec].extend(built.model_keys) - self._created_groups[spec].extend(built.created_groups) - return dict(self._homie_props[spec]) + self._homie_props[device_id].update(built.homie_props) + self._model_keys[device_id].extend(built.model_keys) + self._created_groups[device_id].extend(built.created_groups) + return dict(self._homie_props[device_id]) + + def remove_capabilities(self, spec: DeviceSpec, capabilities: Iterable[str]) -> None: + """Take capabilities away from a built device: the inverse of `extend()`. + + A capability that becomes relevant at runtime can stop being relevant, + and without this its node stayed advertised in `$description` with + retained topics behind it. `Device.delete_node()` already clears those + and re-announces, so the gap this closes is the bookkeeping: reaching + around the builder to call it left `model_keys` and `created_groups` + describing properties that no longer exist, and a later `remove()` + working from that stale record. + + Idempotent, like `extend()`: a capability the device does not have is + skipped rather than an error, because incremental lifecycles re-fire. + Named for capabilities rather than nodes because that is the declarative + vocabulary; the node id is resolved through the builder's `node_id`. + + Raises `KeyError` for a spec that is not built. + """ + device_id = spec.resolve_device_id() + device = self._devices.get(device_id) if device_id is not None else None + if device is None: + raise KeyError(f"{spec.device_class}: not built, so there is nothing to remove from. add() it first.") + + for capability in capabilities: + if device.get_node(self._node_id(capability)) is None: + continue # already gone, or never had it + device.delete_node(self._node_id(capability)) + + doomed = [entry for entry in self._model_keys[device_id] if entry[0] == capability] + for _capability, group, model_key in doomed: + if self._model.has_group(group) and self._model.get(group, model_key) is not None: + self._model.delete_property(group, model_key) + self._model_keys[device_id] = [entry for entry in self._model_keys[device_id] if entry[0] != capability] + self._homie_props[device_id] = { + key: prop for key, prop in self._homie_props[device_id].items() if key[0] != capability + } + # A group this builder created and that is now empty goes with it. + for group in {entry[1] for entry in doomed}: + if ( + group in self._created_groups[device_id] + and self._model.has_group(group) + and not self._model.items(group) + ): + self._model.delete_group(group) + self._created_groups[device_id] = [g for g in self._created_groups[device_id] if g != group] def device_for(self, spec: DeviceSpec) -> Optional[Device]: - """The live `Device` for `spec`, or None if it is deferred or removed.""" - return self._devices.get(spec) + """The live `Device` for `spec`, or None if it is deferred or removed. + + Resolved by device id, so any spec naming the same device answers. + """ + device_id = spec.resolve_device_id() + return self._devices.get(device_id) if device_id is not None else None def homie_properties(self, spec: DeviceSpec) -> dict: """`{(capability, prop_id): homie.Property}` for `spec`, as the single-device builder returns. @@ -762,7 +824,8 @@ def homie_properties(self, spec: DeviceSpec) -> dict: Empty for a spec that is not built. An `internal_only` property has no Homie twin and so is absent, exactly as in `build_from_declarations`. """ - return self._homie_props.get(spec, {}) + device_id = spec.resolve_device_id() + return dict(self._homie_props.get(device_id, {})) if device_id is not None else {} def deferred(self) -> list: """The specs waiting on an id, in the order they were first attempted.""" diff --git a/tests/test_device_tree.py b/tests/test_device_tree.py index c568da2..bf2914c 100644 --- a/tests/test_device_tree.py +++ b/tests/test_device_tree.py @@ -295,14 +295,40 @@ def test_homie_properties_returns_the_twins_like_the_single_device_builder(root) assert builder.homie_properties(DeviceSpec("pv", INFO, device_id="pv-1")) == {} -def test_specs_are_compared_by_identity(root): +def test_a_respec_of_the_same_device_id_is_the_same_device(root): + """A producer re-deriving its spec set from a manifest makes equal-but-distinct + objects on every pass; the builder keys on the device those specs name (GH #74).""" model = GroupedPropertyDict() builder = DeviceTreeBuilder(root, model) a = DeviceSpec("circuit", INFO, device_id="c-1") b = DeviceSpec("circuit", INFO, device_id="c-1") - assert a != b # identical fields, still two declarations - builder.add(a) - assert builder.device_for(b) is None + assert a != b # the dataclass itself is still identity-compared + + device = builder.add(a) + assert builder.add(b) is device, "re-deriving the spec set must not build a second device" + assert builder.device_for(b) is device + assert builder.homie_properties(b) == builder.homie_properties(a) + assert root.children_ids() == ["c-1"] + + +def test_a_respec_can_drive_removal(root): + """The obligation #74 removes: a caller no longer has to hold the original object.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + builder.add(DeviceSpec("circuit", INFO, device_id="c-1")) + builder.remove(DeviceSpec("circuit", INFO, device_id="c-1")) + assert root.children_ids() == [] + + +def test_a_differing_spec_on_a_built_id_returns_the_existing_device(root): + """Documented: add() is idempotent on the device, it does not apply a new + capability set. extend() is how a built device grows.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + device = builder.add(DeviceSpec("circuit", INFO, device_id="c-1")) + same = builder.add(DeviceSpec("circuit", INFO + METER, device_id="c-1")) + assert same is device + assert device.get_node("meter") is None # not applied; use extend() # --- Root capabilities (GH #67) --------------------------------------------- @@ -526,3 +552,98 @@ def reenter(*args, **kwargs): assert all(d is device for d in seen if d is not None) assert root.children_ids() == ["c-1"] + + +# --- Taking a capability away again (GH #78) -------------------------------- + + +def test_remove_capabilities_is_the_inverse_of_extend(root, mock_paho): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id="bess-1") + builder.add(spec) + builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)]) + device = builder.device_for(spec) + assert device.get_node("shed") is not None + + builder.remove_capabilities(spec, ["shed"]) + + # Gone from the tree, from $description, and from the model. + assert device.get_node("shed") is None + assert "shed" not in device.description()["nodes"] + assert model.get("bess-1", "shed-state") is None + # What it did not touch is untouched. + assert device.get_node("info") is not None + assert model.get("bess-1", "serial-number") is not None + + +def test_remove_capabilities_clears_the_retained_topics(root, mock_paho): + """The point of going through the builder: delete_node clears them, and + reaching around it would leave the bookkeeping describing what is gone.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id="bess-1") + builder.add(spec) + builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)]) + model.set_value("bess-1", "shed-state", "SHED") + + before = len(mock_paho.publish.call_args_list) + builder.remove_capabilities(spec, ["shed"]) + after = [c for c in mock_paho.publish.call_args_list[before:] if c.args] + + retractions = [c for c in after if str(c.args[0]).endswith("/shed/shed-state") and c.args[1] == ""] + assert retractions, "the retained value topic was left on the broker" + + +def test_remove_capabilities_is_idempotent(root, mock_paho): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id="bess-1") + builder.add(spec) + builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)]) + builder.remove_capabilities(spec, ["shed"]) + + quiet = len(mock_paho.publish.call_args_list) + builder.remove_capabilities(spec, ["shed", "never-had-this"]) + assert len(mock_paho.publish.call_args_list) == quiet + + +def test_a_capability_can_be_added_removed_and_added_again(root, mock_paho): + """The lifecycle the issue describes: relevant, then not, then relevant again.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id="bess-1") + builder.add(spec) + shed = [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)] + + builder.extend(spec, shed) + builder.remove_capabilities(spec, ["shed"]) + builder.extend(spec, shed) + + device = builder.device_for(spec) + assert device.get_node("shed") is not None + model.set_value("bess-1", "shed-state", "SHED") + assert builder.homie_properties(spec)[("shed", "shed-state")].value() == "SHED" + + +def test_remove_after_remove_capabilities_stays_consistent(root, mock_paho): + """The stale-bookkeeping hazard: remove() must not work from a record of + properties that no longer exist.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id="bess-1") + builder.add(spec) + builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)]) + builder.remove_capabilities(spec, ["shed"]) + + builder.remove(spec) + assert builder.device_for(spec) is None + assert "bess-1" not in model.groups() + assert root.children_ids() == [] + + +def test_remove_capabilities_refuses_an_unbuilt_device(root): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + with pytest.raises(KeyError, match="not built"): + builder.remove_capabilities(DeviceSpec("bess", INFO, device_id="nope"), ["shed"])