diff --git a/CHANGELOG.md b/CHANGELOG.md index 03f6824..998651d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,16 @@ All notable changes to `ebus-sdk` are recorded here. Format follows [Keep a Chan ### Added +- `DeviceTreeBuilder.add_root_capabilities()`: the tree's root can carry its own capabilities. `add()` only ever creates children, so a root's own surfaces (an enclosure's aggregate metering, its state, its controls) had no declarative expression and had to be hand-rolled beside the builder, which left one model with two construction styles and put the root outside every guarantee the builder gives (idempotence, ordered teardown, model cleanup). The root already exists, so this materializes onto it rather than constructing anything; the model group defaults to the root's device id, matching how `add()` keys a child. `root_capabilities()` reads back what has accumulated. ([#67](https://github.com/electrification-bus/python-sdk/issues/67)) + +- `DeviceTreeBuilder.extend()`: a device that already exists can grow a capability. A capability set is not always known when a device is first published, since a storage system commissioned at runtime gives an enclosure shed and forecast surfaces it did not have at boot, and `add()` short-circuits an already-built spec, so the builder modeled devices appearing and disappearing but not a device growing. The workaround was unsafe rather than merely absent: `Device.add_node` and `Node.add_property` both replace wholesale, so re-declaring a live device through `build_from_declarations` dropped the previous node's properties from `$description` while leaving their retained topics on the broker, and only `delete_node` clears those. The result was a tree whose description and whose broker state disagreed, persisting across restarts. `extend()` materializes inside one `state_transition()` and folds the new model keys into the same bookkeeping `remove()` uses. ([#68](https://github.com/electrification-bus/python-sdk/issues/68)) + - `node_id` on `build_from_declarations` and `DeviceTreeBuilder`: a callable mapping a capability to the Homie node id it materializes onto, defaulting to the capability itself. The node id was hardcoded to the capability name, which is right until one device carries two instances of the same capability (two lugs, two meters), at which point the second silently lands on the first one's node. `node_type` and `node_name` were already callables, so the id was the one part of a node a caller could not choose. Renaming is all it does: the declaration's vocabulary stays `capability`, the model group still comes from the spec, and the returned map is still keyed by the declared capability, so a caller who ignores it sees no change. Pairs with `PropertySpec.model_group` from 0.21.0, which separates the same two instances in the model the way this separates them on the wire; using one without the other moves the collision rather than removing it. ([#47](https://github.com/electrification-bus/python-sdk/issues/47)) +### Changed + +- Materializing declarations is now idempotent, at three levels. An existing Homie **node** is reused rather than replaced (`Device.add_node` is a wholesale `self._nodes.update(...)`, which is the mechanism behind the description-versus-broker divergence above); an existing Homie **property** is reused rather than re-added (`Node.add_property` replaces and republishes with `force=True`); and a materialization that would create nothing **does not open a state transition at all**. That last one matters most: an empty transition still emits `init` then `ready`, and that edge forces every controller on the bus to resync, so a re-declaration that changes nothing must not cost one. Together these make `build_from_declarations` safe to call twice, which is what lets a re-fired incremental lifecycle be a genuine no-op rather than a quieter republish. + ### Fixed - `build_from_declarations` and `DeviceTreeBuilder` now REUSE an observable property the model already holds instead of replacing it. `_materialize` guarded the model group with `has_group` and then, two lines later, added the property unconditionally, and `GroupedPropertyDict.add_property` is a wholesale `self._properties[property_id] = property`. So a producer handing over a model it had already populated got that property swapped for a fresh one, losing its value and, worse, every callback and `entity_setter` attached to it: `$description` kept advertising `settable: true` while the actuator behind it was gone, and an arriving `/set` did nothing. Nor was it self-healing, since the builder path seeds only a static `initial_value` and `Property.set_value` fires callbacks only on an actual change, so a value written once at group creation never republished. This is the exact case `DeviceTreeBuilder` documents as the reason it accepts a model rather than creating one, which made the gap a documented guarantee the code did not provide. The builder now records only properties it actually created, so removing a device deletes what it added and leaves what the producer owned. A spec whose python type disagrees with the property already in the model raises rather than binding a Homie twin to a mismatched observable. ([#66](https://github.com/electrification-bus/python-sdk/issues/66)) diff --git a/doc/building-a-proxy.md b/doc/building-a-proxy.md index 2897ada..eb7213e 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. +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. ## Lifecycle and state diff --git a/src/ebus_sdk/declaration.py b/src/ebus_sdk/declaration.py index 9ec82e0..f1a3fc2 100644 --- a/src/ebus_sdk/declaration.py +++ b/src/ebus_sdk/declaration.py @@ -18,6 +18,7 @@ from __future__ import annotations +from contextlib import nullcontext from dataclasses import dataclass from functools import partial from typing import Any, Callable, Iterable, Optional, Sequence, Union @@ -259,22 +260,29 @@ def _materialize( declared: dict[tuple, PropertySpec] = {} model_keys: list = [] created_groups: list = [] - with device.state_transition(): + # Open a state transition only if there is something to announce. An empty + # one still emits init -> ready, and that edge forces every controller on the + # bus to resync, so a re-declaration that changes nothing must not cost one. + needs_transition = _needs_materializing(device, model, grouped, node_id, default_group) + with device.state_transition() if needs_transition else nullcontext(): for capability, cap_specs in grouped.items(): # A node exists to carry published properties. If every spec on this # capability is internal, creating one would announce an empty node. published = [spec for spec in cap_specs if not spec.internal_only] - node = ( - device.add_node_from_dict( + # Reuse an existing node. Device.add_node is a wholesale + # `self._nodes.update(...)`, so re-declaring would drop the previous + # node's properties from $description while LEAVING their retained + # topics on the broker (only delete_node clears those), producing a + # tree whose description and whose broker state disagree. + node = None + if published: + node = device.get_node(node_id(capability)) or device.add_node_from_dict( { "id": node_id(capability), "name": node_name(capability), "type": node_type(capability), } ) - if published - else None - ) for spec in cap_specs: group = _group_for(spec, default_group) if not model.has_group(group): @@ -315,6 +323,14 @@ def _materialize( prop_dict["round_to"] = spec.round_to if not spec.retained: prop_dict["retained"] = False + # Reuse likewise: Node.add_property replaces wholesale and + # republishes with force=True, so re-declaring an unchanged + # property would re-announce it. A datatype that actually changed + # is caught on the model side above, which raises. + existing_prop = node.get_property(spec.prop_id) + if existing_prop is not None: + homie_props[(capability, spec.prop_id)] = existing_prop + continue homie_prop = node.add_property_from_dict(prop_dict) bind_property_to_homie(model, group, spec.model_key, homie_prop) # Inbound/control path for a settable property with a translator: @@ -327,6 +343,31 @@ def _materialize( return _Materialized(homie_props, declared, model_keys, created_groups) +def _needs_materializing( + device: Device, + model: GroupedPropertyDict, + grouped: dict, + node_id: Callable[[str], str], + default_group: Optional[str], +) -> bool: + """True when any spec still has something to create on the device or the model. + + Answered BEFORE the transition opens, because the question is whether to open + one at all: an init -> ready edge that announces nothing is a cost paid by + every controller on the bus. + """ + for capability, cap_specs in grouped.items(): + for spec in cap_specs: + if model.get(_group_for(spec, default_group), spec.model_key) is None: + return True + if spec.internal_only: + continue + node = device.get_node(node_id(capability)) + if node is None or node.get_property(spec.prop_id) is None: + return True + return False + + def _seed( model: GroupedPropertyDict, declared: dict, @@ -482,6 +523,10 @@ def __init__( self._model_keys: dict = {} self._created_groups: dict = {} self._deferred: list = [] + # The root is not a DeviceSpec, so its bookkeeping lives beside the + # per-spec maps rather than inside them. Nothing removes a root. + self._root_props: dict = {} + self._root_model_keys: list = [] def add(self, spec: DeviceSpec) -> Optional[Device]: """Materialize `spec` as a device, or defer it while its id is unknown. @@ -582,6 +627,82 @@ def remove(self, spec: DeviceSpec) -> None: self._devices.pop(gone, None) self._homie_props.pop(gone, None) + def add_root_capabilities(self, specs: Iterable[PropertySpec], *, model_group: Optional[str] = None) -> dict: + """Materialize capabilities onto the tree's ROOT device. + + `add()` only ever creates children, so a root's own capabilities (an + enclosure's aggregate metering, its state, its control surfaces) had no + declarative expression and had to be hand-rolled beside the builder: one + model, two construction styles, and the root outside every guarantee the + builder gives. + + The root already exists, so this materializes onto it rather than + constructing anything. `model_group` defaults to the root's device id, + matching how `add()` keys a child's group. Idempotent, so a re-fired + lifecycle re-declares nothing. + + Returns `{(capability, prop_id): homie.Property}` for the root, and the + map accumulates across calls, so `add_root_capabilities` twice returns + everything the root has. + """ + group = model_group or self._root.id() + built = _materialize( + self._root, + self._model, + specs, + node_type=self._node_type, + node_name=self._node_name, + node_id=self._node_id, + default_group=group, + ) + _seed(self._model, built.declared, default_group=group) + self._root_props.update(built.homie_props) + self._root_model_keys.extend(built.model_keys) + return dict(self._root_props) + + def root_capabilities(self) -> dict: + """`{(capability, prop_id): homie.Property}` materialized onto the root so far.""" + return dict(self._root_props) + + def extend(self, spec: DeviceSpec, specs: Iterable[PropertySpec]) -> dict: + """Give a device this builder already built additional capabilities. + + A device's capability set is not always known when it is first published: + a storage system is commissioned and the enclosure gains shed and + forecast surfaces it did not have at boot. `add()` short-circuits an + already-built spec, so the builder modeled devices appearing and + disappearing but not a device GROWING. + + Materializes inside one `state_transition()`, so the device announces + once, and folds the new model keys into the same bookkeeping `remove()` + uses. Idempotent: extending with a capability already present is a no-op + rather than a republish, because incremental lifecycles re-fire. + + 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) + if device is None: + raise KeyError( + f"{spec.device_class}: not built, so there is nothing to extend. " + "add() it first (a deferred device has no tree yet)." + ) + group = spec.resolve_model_group(device.id()) + built = _materialize( + device, + self._model, + specs, + node_type=self._node_type, + node_name=self._node_name, + node_id=self._node_id, + 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]) + 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) diff --git a/tests/test_device_tree.py b/tests/test_device_tree.py index 28d8366..1c883c2 100644 --- a/tests/test_device_tree.py +++ b/tests/test_device_tree.py @@ -301,3 +301,128 @@ def test_specs_are_compared_by_identity(root): assert a != b # identical fields, still two declarations builder.add(a) assert builder.device_for(b) is None + + +# --- Root capabilities (GH #67) --------------------------------------------- + + +def test_the_root_can_carry_its_own_capabilities(root, mock_paho): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + + props = builder.add_root_capabilities([PropertySpec("meter", "active-power", PropertyDatatype.FLOAT, Unit.WATT)]) + + # Materialized onto the root itself, not as a child of it. + assert root.get_node("meter") is not None + assert root.children_ids() == [] + assert set(props) == {("meter", "active-power")} + # Keyed by the root's device id, matching how add() keys a child's group. + model.set_value("enclosure-1", "active-power", 4200.0) + assert props[("meter", "active-power")].value() == 4200.0 + + +def test_root_capabilities_accumulate_and_are_idempotent(root, mock_paho): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + builder.add_root_capabilities([PropertySpec("meter", "active-power", PropertyDatatype.FLOAT)]) + builder.add_root_capabilities([PropertySpec("info", "vendor-name", PropertyDatatype.STRING)]) + + assert set(builder.root_capabilities()) == {("meter", "active-power"), ("info", "vendor-name")} + + quiet = len(mock_paho.publish.call_args_list) + builder.add_root_capabilities([PropertySpec("meter", "active-power", PropertyDatatype.FLOAT)]) + # Re-declaring publishes nothing: a re-fired lifecycle must not re-announce. + assert len(mock_paho.publish.call_args_list) == quiet + + +def test_root_capabilities_coexist_with_children(root, mock_paho): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + builder.add_root_capabilities([PropertySpec("meter", "active-power", PropertyDatatype.FLOAT)]) + builder.add(DeviceSpec("circuit", INFO, device_id="c-1")) + + assert root.get_node("meter") is not None + assert root.children_ids() == ["c-1"] + assert "meter" in root.description()["nodes"] + assert "c-1" in root.description()["children"] + + +def test_root_capabilities_can_name_their_own_model_group(root): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + builder.add_root_capabilities([PropertySpec("meter", "active-power", PropertyDatatype.FLOAT)], model_group="panel") + assert "panel" in model.groups() + assert "enclosure-1" not in model.groups() + + +# --- Growing an existing device (GH #68) ------------------------------------ + + +def test_extend_gives_a_built_device_a_new_capability(root, mock_paho): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id="bess-1") + builder.add(spec) + assert builder.device_for(spec).get_node("shed") is None + + props = builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)]) + + device = builder.device_for(spec) + assert device.get_node("shed") is not None + # The returned map is everything the device has, not only the addition. + assert set(props) == {("info", "serial-number"), ("shed", "shed-state")} + model.set_value("bess-1", "shed-state", "SHED") + assert props[("shed", "shed-state")].value() == "SHED" + + +def test_extend_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)]) + + quiet = len(mock_paho.publish.call_args_list) + builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)]) + assert len(mock_paho.publish.call_args_list) == quiet + + +def test_extend_does_not_drop_the_devices_existing_properties(root, mock_paho): + """The hazard the workaround had: re-declaring dropped properties from + $description while leaving their retained topics on the broker.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id="bess-1") + builder.add(spec) + model.set_value("bess-1", "serial-number", "B-1") + + builder.extend(spec, [PropertySpec("info", "vendor-name", PropertyDatatype.STRING)]) + + device = builder.device_for(spec) + described = device.description()["nodes"]["info"]["properties"] + assert set(described) == {"serial-number", "vendor-name"} + # The pre-existing property kept its live value rather than being replaced. + assert model.value("bess-1", "serial-number") == "B-1" + + +def test_extend_folds_into_the_bookkeeping_remove_uses(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(spec) + # Both the original and the extended model entries are gone. + assert model.get("bess-1", "serial-number") is None + assert model.get("bess-1", "shed-state") is None + assert "bess-1" not in model.groups() + + +def test_extend_refuses_a_device_that_is_not_built(root): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("bess", INFO, device_id=lambda: None) + builder.add(spec) # deferred, so no tree to extend + with pytest.raises(KeyError, match="not built"): + builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)])