diff --git a/custom_components/localthings/entity.py b/custom_components/localthings/entity.py index e8c0b9b..cbdbca1 100644 --- a/custom_components/localthings/entity.py +++ b/custom_components/localthings/entity.py @@ -8,6 +8,7 @@ from homeassistant.helpers.device_registry import DeviceInfo from homeassistant.const import EntityCategory from .registry.adapter import _key +from .registry.batch import is_stub_rep from .registry.discovery import BoundEntity, _snake_to_title from .const import DOMAIN @@ -21,9 +22,17 @@ def _is_included(bound: BoundEntity, coordinator: 'LocalThingsCoordinator') -> b require that field to be present in the resource rep so that optional fields on shared resources don't create phantom entities. - An empty rep ({}) means /device/0 returned a stub for this resource — - the resource exists on the device but data hasn't been fetched yet. - In that case we include the entity so it can be populated by sub-polls. + A stub rep (is_stub_rep — /device/0's "resource exists, no data fetched + yet" marker) is included anyway so it can be populated by sub-polls. A + genuinely empty {} rep is included too by this default gate -- whether + empty means "not populated yet" or "permanently unsupported" needs + per-field domain knowledge this generic gate doesn't have: /alarms/vs/0's + {} is fridge.py's documented *normal* no-alarm state (see + _active_alarm_codes), not an absence signal, and it's far from the only + resource like that. Only a capability whose author has actually verified + a field is genuinely never populated on unsupported hardware opts into + stricter gating with its own is_stub_rep-based exists_fn (see + common.ENERGY_METER, issue #127) -- this default stays permissive. """ rep = coordinator.last_resources.get(bound.href) if rep is None: @@ -31,7 +40,7 @@ def _is_included(bound: BoundEntity, coordinator: 'LocalThingsCoordinator') -> b if bound.desc.exists_fn is not None: return bound.desc.exists_fn(rep, coordinator.last_resources) if bound.desc.field: - if not rep: # stub — resource known to exist, data not yet fetched + if not rep or is_stub_rep(rep): return True return bound.desc.field in rep return True # rep_fn or no-field entities (ButtonDesc) are always included diff --git a/custom_components/localthings/registry/batch.py b/custom_components/localthings/registry/batch.py index 6a90c2e..c7fb3ad 100644 --- a/custom_components/localthings/registry/batch.py +++ b/custom_components/localthings/registry/batch.py @@ -2,8 +2,25 @@ from __future__ import annotations +def is_stub_rep(rep: dict) -> bool: + """True for the device's "resource exists, no data fetched yet" marker -- + an echoed {"href": "..."} with no other fields. + + Distinct from a genuinely empty {} rep, which is the device's confirmed + (if empty) answer -- e.g. an unsupported resource on this model that will + never populate. Conflating the two used to make every field-gated entity + on a permanently-empty resource look like a not-yet-fetched stub forever, + creating phantom always-"unknown" entities (issue #127).""" + return isinstance(rep, dict) and set(rep.keys()) == {'href'} + + def parse_device0_batch(device0: list) -> dict[str, dict]: - """Extract {href: rep} from a /device/0 CBOR list response.""" + """Extract {href: rep} from a /device/0 CBOR list response. + + A stub rep is passed through unchanged rather than collapsed to {} -- + downstream code (entity._is_included, capability exists_fns) uses + is_stub_rep to tell "not fetched yet" apart from a confirmed-empty {}. + """ out = {} for entry in device0[1:]: # skip [0] (device-level rep) if not isinstance(entry, dict): @@ -12,8 +29,6 @@ def parse_device0_batch(device0: list) -> dict[str, dict]: rep = entry.get('rep') if not href: continue - # rep == {"href": "..."} is a stub (resource present, no current data). - # Include it as {} so capabilities still bind and the entity exists. if isinstance(rep, dict): - out[href] = {} if set(rep.keys()) == {'href'} else rep + out[href] = rep return out diff --git a/custom_components/localthings/registry/capabilities/common.py b/custom_components/localthings/registry/capabilities/common.py index cd03a91..ac60523 100644 --- a/custom_components/localthings/registry/capabilities/common.py +++ b/custom_components/localthings/registry/capabilities/common.py @@ -12,6 +12,7 @@ against live device dumps: """ from datetime import datetime, timezone +from ..batch import is_stub_rep from ..capability import Capability from ..entities import ( BinarySensorDesc, ButtonDesc, SelectDesc, SensorDesc, SwitchDesc, @@ -380,32 +381,35 @@ _DEAD_INSTANTANEOUS_POWER = '-500' ENERGY_METER = Capability( href='/energy/consumption/vs/0', entities=( - # `not rep` keeps the empty-{} stub carve-out (see entity._is_included): + # `is_stub_rep(rep)` keeps the stub carve-out (see entity._is_included): # an explicit exists_fn otherwise bypasses it, which would drop the - # entity when /device/0 returns a not-yet-fetched stub. On a populated - # rep, hide power only for the dead sentinel or an absent field. + # entity when /device/0 returns a not-yet-fetched stub. A genuinely + # empty {} rep is NOT a stub -- it's the device's confirmed (if empty) + # answer, so it falls through to the normal field/sentinel checks like + # any populated rep. On a populated rep, hide power only for the dead + # sentinel or an absent field. SensorDesc(key='power_watts', field='x.com.samsung.da.instantaneousPower', device_class='power', state_class='measurement', unit='W', value_fn=clamp_power, - exists_fn=lambda rep, resources: not rep or ( + exists_fn=lambda rep, resources: is_stub_rep(rep) or ( rep.get('x.com.samsung.da.instantaneousPower') not in (None, _DEAD_INSTANTANEOUS_POWER))), SensorDesc(key='energy_kwh', field='x.com.samsung.da.cumulativePower', device_class='energy', state_class='total_increasing', unit='kWh', value_fn=wh_to_kwh, exists_fn=lambda rep, resources: ( - not rep or 'x.com.samsung.da.cumulativePower' in rep)), + is_stub_rep(rep) or 'x.com.samsung.da.cumulativePower' in rep)), # cumulativeConsumption is a second, independently-varying running # total alongside cumulativePower -- some fridges (issue #26) report - # both. Self-gates off where only cumulativePower is present. `not - # rep or` keeps the same empty-{} stub carve-out as power_watts/ + # both. Self-gates off where only cumulativePower is present. The + # `is_stub_rep(rep) or` keeps the same stub carve-out as power_watts/ # energy_kwh above -- without it, an exists_fn permanently drops the # entity if setup happens to land on a not-yet-fetched stub. SensorDesc(key='power_energy_kwh', field='x.com.samsung.da.cumulativeConsumption', device_class='energy', state_class='total_increasing', unit='kWh', value_fn=wh_to_kwh, exists_fn=lambda rep, resources: ( - not rep or 'x.com.samsung.da.cumulativeConsumption' in rep)), + is_stub_rep(rep) or 'x.com.samsung.da.cumulativeConsumption' in rep)), # AI Energy Mode's lifetime savings estimate vs. an unoptimized # baseline -- present on some models (e.g. TP1X_REF_21K, issue #21/ # #27) and absent on others (issue #20/#26), unlike cumulativePower. @@ -413,7 +417,7 @@ ENERGY_METER = Capability( device_class='energy', state_class='total_increasing', unit='kWh', value_fn=wh_to_kwh, exists_fn=lambda rep, resources: ( - not rep or 'x.com.samsung.da.cumulativeSavedPower' in rep)), + is_stub_rep(rep) or 'x.com.samsung.da.cumulativeSavedPower' in rep)), # Monthly billing-cycle totals -- the completed prior month and the # in-progress current month. Not ever-increasing (each resets at # month boundary), so no state_class. @@ -421,12 +425,12 @@ ENERGY_METER = Capability( device_class='energy', unit='kWh', value_fn=wh_to_kwh, exists_fn=lambda rep, resources: ( - not rep or 'x.com.samsung.da.monthlyConsumption' in rep)), + is_stub_rep(rep) or 'x.com.samsung.da.monthlyConsumption' in rep)), SensorDesc(key='energy_this_month_kwh', field='x.com.samsung.da.thismonthlyConsumption', device_class='energy', unit='kWh', value_fn=wh_to_kwh, exists_fn=lambda rep, resources: ( - not rep or 'x.com.samsung.da.thismonthlyConsumption' in rep)), + is_stub_rep(rep) or 'x.com.samsung.da.thismonthlyConsumption' in rep)), ), ) @@ -499,7 +503,7 @@ AI_ENERGY_LEVEL = Capability( href='/energy/ailevel/vs/0', poll_tier='cold', entities=( - # No `not rep` stub carve-out on either side, unlike most exists_fn + # No is_stub_rep carve-out on either side, unlike most exists_fn # gates in this file -- entity creation only ever runs once, against # whichever snapshot happens to be current the moment platforms are # set up (see entity._is_included / __init__.py's @@ -562,7 +566,7 @@ SELF_CHECK = Capability( icon='mdi:alert-circle-outline', entity_category='diagnostic', exists_fn=lambda rep, resources: ( - not rep or 'x.com.samsung.da.error' in rep), + is_stub_rep(rep) or 'x.com.samsung.da.error' in rep), value_fn=lambda v: (', '.join(v) if v else None) if isinstance(v, list) else v), ButtonDesc(key='selfcheck_start', field='', payload='Start', icon='mdi:play-circle-outline', diff --git a/custom_components/localthings/registry/capabilities/cooktop.py b/custom_components/localthings/registry/capabilities/cooktop.py index b76138f..96f1cfd 100644 --- a/custom_components/localthings/registry/capabilities/cooktop.py +++ b/custom_components/localthings/registry/capabilities/cooktop.py @@ -9,6 +9,7 @@ cooktop must not be remotely ignited by an automation. import re +from ..batch import is_stub_rep from ..capability import Capability from ..entities import BinarySensorDesc, SensorDesc @@ -97,7 +98,7 @@ COOKTOP_MODE = Capability( options, f'OperationState{slot}' ), exists_fn=lambda rep, resources, slot=slot: ( - not rep or _option_value( + is_stub_rep(rep) or _option_value( rep.get('x.com.samsung.da.options'), f'OperationState{slot}', ) is not None diff --git a/custom_components/localthings/registry/capabilities/range_hood.py b/custom_components/localthings/registry/capabilities/range_hood.py index 0b14f34..59aa9b7 100644 --- a/custom_components/localthings/registry/capabilities/range_hood.py +++ b/custom_components/localthings/registry/capabilities/range_hood.py @@ -9,6 +9,7 @@ independent fields. from datetime import datetime, timezone +from ..batch import is_stub_rep from ..capability import Capability from ..entities import ( BinarySensorDesc, @@ -104,7 +105,7 @@ HOOD_FAN = Capability( # #137) -- this board has no auto-ventilation mode, unlike the # standalone range hood this capability was written for. exists_fn=lambda rep, resources: ( - not rep or 'x.com.samsung.da.hood.autoOperation' in rep), + is_stub_rep(rep) or 'x.com.samsung.da.hood.autoOperation' in rep), value_fn=lambda value: str(value).lower() == 'on', ), ), diff --git a/tests/fixtures/golden/air_purifier.json b/tests/fixtures/golden/air_purifier.json index 7e049c5..05e3410 100644 --- a/tests/fixtures/golden/air_purifier.json +++ b/tests/fixtures/golden/air_purifier.json @@ -7,18 +7,12 @@ "diagnosis_status", "display_light", "dust", - "energy_kwh", - "energy_last_month_kwh", - "energy_saved_kwh", - "energy_this_month_kwh", "fan_direction", "filter_progress", "fine_dust", "odor", "operating_mode", - "power_energy_kwh", "power_switch", - "power_watts", "super_fine_dust" ] } diff --git a/tests/fixtures/golden/refrigerator_artik051_dongle_ref.json b/tests/fixtures/golden/refrigerator_artik051_dongle_ref.json index 6174968..6c50fe9 100644 --- a/tests/fixtures/golden/refrigerator_artik051_dongle_ref.json +++ b/tests/fixtures/golden/refrigerator_artik051_dongle_ref.json @@ -5,16 +5,10 @@ "defrost_delay", "diagnosis_status", "door_onedoorfreezer_open", - "energy_kwh", - "energy_last_month_kwh", - "energy_saved_kwh", - "energy_this_month_kwh", "firmware_update", "freezer_setpoint", "freezer_temperature", "ice_maker_enabled", - "power_energy_kwh", - "power_watts", "rapid_freezing", "rapid_fridge", "sabbath_mode" diff --git a/tests/fixtures/golden/refrigerator_artik051_dongle_ref_cooler.json b/tests/fixtures/golden/refrigerator_artik051_dongle_ref_cooler.json index 984569c..44d2d96 100644 --- a/tests/fixtures/golden/refrigerator_artik051_dongle_ref_cooler.json +++ b/tests/fixtures/golden/refrigerator_artik051_dongle_ref_cooler.json @@ -8,14 +8,8 @@ "diagnosis_status", "door_cooler_open", "door_onedoorfreezer_open", - "energy_kwh", - "energy_last_month_kwh", - "energy_saved_kwh", - "energy_this_month_kwh", "firmware_update", "ice_maker_enabled", - "power_energy_kwh", - "power_watts", "rapid_freezing", "rapid_fridge", "sabbath_mode" diff --git a/tests/test_batch.py b/tests/test_batch.py new file mode 100644 index 0000000..d707662 --- /dev/null +++ b/tests/test_batch.py @@ -0,0 +1,56 @@ +"""Tests for registry.batch — the /device/0 sweep parser and its stub marker. + +issue #127: a device whose /energy/consumption/vs/0 is permanently +unsupported reports a genuinely empty {} rep for it. The previous parser +collapsed /device/0's own {"href": "..."} "not fetched yet" marker to that +same {} shape, so downstream exists_fn checks couldn't tell "confirmed +empty" apart from "haven't polled it yet" and created phantom always- +"unknown" entities either way. is_stub_rep/parse_device0_batch now keep the +two shapes distinct. +""" +from custom_components.localthings.registry.batch import is_stub_rep, parse_device0_batch + + +class TestIsStubRep: + def test_true_for_bare_href_marker(self): + assert is_stub_rep({'href': '/energy/consumption/vs/0'}) is True + + def test_false_for_genuinely_empty_rep(self): + assert is_stub_rep({}) is False + + def test_false_for_populated_rep(self): + assert is_stub_rep({'x.com.samsung.da.cumulativePower': '58900'}) is False + + def test_false_for_href_plus_data(self): + """A real, populated rep may legitimately echo 'href' alongside + actual fields -- only a rep with *no other keys* is the stub.""" + assert is_stub_rep({'href': '/x/0', 'value': True}) is False + + +class TestParseDevice0Batch: + def test_stub_rep_kept_distinct_from_genuine_empty(self): + device0 = [ + {}, + {'href': '/energy/consumption/vs/0', 'rep': {'href': '/energy/consumption/vs/0'}}, + {'href': '/sabbath/vs/0', 'rep': {}}, + ] + resources = parse_device0_batch(device0) + assert is_stub_rep(resources['/energy/consumption/vs/0']) is True + assert is_stub_rep(resources['/sabbath/vs/0']) is False + assert resources['/sabbath/vs/0'] == {} + + def test_populated_rep_passes_through_unchanged(self): + device0 = [ + {}, + {'href': '/door/cooler/0', 'rep': {'openState': 'Close'}}, + ] + resources = parse_device0_batch(device0) + assert resources['/door/cooler/0'] == {'openState': 'Close'} + + def test_skips_entries_without_href(self): + device0 = [{}, {'rep': {'a': 1}}] + assert parse_device0_batch(device0) == {} + + def test_skips_non_dict_rep(self): + device0 = [{}, {'href': '/x/0', 'rep': 'not-a-dict'}] + assert parse_device0_batch(device0) == {} diff --git a/tests/test_common_capabilities.py b/tests/test_common_capabilities.py index 8f7bcad..2050a70 100644 --- a/tests/test_common_capabilities.py +++ b/tests/test_common_capabilities.py @@ -253,13 +253,24 @@ class TestEnergyMeter: kwh = next(e for e in common.ENERGY_METER.entities if e.key == 'energy_kwh') assert kwh.exists_fn({'x.com.samsung.da.cumulativePower': '58900'}, {}) is True - def test_both_entities_included_on_empty_stub(self): - """An empty {} rep means the resource exists but data isn't fetched yet - (see entity._is_included) -- include both so sub-polls populate them.""" + def test_both_entities_included_on_true_stub(self): + """A true stub -- /device/0's {"href": "..."} "not fetched yet" + marker (see registry.batch.is_stub_rep) -- means the resource exists + but data isn't fetched yet; include both so sub-polls populate them.""" pw = next(e for e in common.ENERGY_METER.entities if e.key == 'power_watts') kwh = next(e for e in common.ENERGY_METER.entities if e.key == 'energy_kwh') - assert pw.exists_fn({}, {}) is True - assert kwh.exists_fn({}, {}) is True + stub = {'href': '/energy/consumption/vs/0'} + assert pw.exists_fn(stub, {}) is True + assert kwh.exists_fn(stub, {}) is True + + def test_both_entities_hidden_on_genuinely_empty_rep(self): + """A real {} rep (no 'href' key) is the device's confirmed -- if + empty -- answer, not a stub, so a model that never populates this + resource doesn't get a phantom always-"unknown" entity (issue #127).""" + pw = next(e for e in common.ENERGY_METER.entities if e.key == 'power_watts') + kwh = next(e for e in common.ENERGY_METER.entities if e.key == 'energy_kwh') + assert pw.exists_fn({}, {}) is False + assert kwh.exists_fn({}, {}) is False def test_power_watts_hidden_when_field_absent_in_populated_rep(self): """A populated rep that lacks instantaneousPower must not spawn a @@ -388,13 +399,20 @@ class TestAiEnergyLevelStubDoesNotDecideThePlatform: if a stub carve-out let one of them win at setup time while the other wins once real data lands, flatten() would feed the already-instantiated entity a value shaped for the other platform (e.g. a bool into a Select - expecting a string option). Neither side gets a `not rep` carve-out, so - an unfetched stub can never win entity creation for either platform -- + expecting a string option). Neither side gets an is_stub_rep carve-out, + so an unfetched stub can never win entity creation for either platform -- the entity simply doesn't appear until a reload happens with real data, same as any other exists_fn-gated entity in this codebase that's unlucky on first-poll timing, instead of appearing as the wrong widget type.""" - def test_neither_widget_exists_on_empty_stub_rep(self): + def test_neither_widget_exists_on_true_stub_rep(self): + switch = _ai_energy_level_desc('SwitchDesc') + select = _ai_energy_level_desc('SelectDesc') + stub = {'href': '/energy/ailevel/vs/0'} + assert switch.exists_fn(stub, {}) is False + assert select.exists_fn(stub, {}) is False + + def test_neither_widget_exists_on_genuinely_empty_rep(self): switch = _ai_energy_level_desc('SwitchDesc') select = _ai_energy_level_desc('SelectDesc') assert switch.exists_fn({}, {}) is False @@ -416,11 +434,17 @@ class TestSelfCheckError: desc = self._desc() assert desc.exists_fn({'x.com.samsung.da.status': 'Ready'}, {}) is False - def test_exists_for_empty_stub_rep(self): - """An empty {} rep is /device/0's not-yet-fetched-stub carve-out -- - must be included-for-now, same as ENERGY_METER's fields.""" + def test_exists_for_true_stub_rep(self): + """A true stub ({"href": "..."}) is /device/0's not-yet-fetched + marker -- must be included-for-now, same as ENERGY_METER's fields.""" desc = self._desc() - assert desc.exists_fn({}, {}) is True + assert desc.exists_fn({'href': '/selfcheck/vs/0'}, {}) is True + + def test_hidden_for_genuinely_empty_rep(self): + """A real {} rep is the device's confirmed empty answer, not a stub -- + must NOT be force-included (issue #127's phantom-entity pattern).""" + desc = self._desc() + assert desc.exists_fn({}, {}) is False def test_value_joins_list(self): desc = self._desc() diff --git a/tests/test_entity.py b/tests/test_entity.py new file mode 100644 index 0000000..1e0538b --- /dev/null +++ b/tests/test_entity.py @@ -0,0 +1,79 @@ +"""Tests for entity._is_included -- the one-time entity-creation gate run +per platform at async_setup_entry (see sensor.py etc.), separate from +adapter.flatten()'s per-poll state values. issue #127 review: this default +gate has to stay permissive on a genuinely empty {} rep, not just a true +is_stub_rep stub -- a resource like /alarms/vs/0 is validly empty in its +normal, no-alarm state (fridge._active_alarm_codes), so excluding the +entity there would silently drop working sensors on first-poll timing, +not just fix phantom ones. +""" +from custom_components.localthings.entity import _is_included +from custom_components.localthings.registry.capability import Capability +from custom_components.localthings.registry.discovery import BoundEntity +from custom_components.localthings.registry.entities import SensorDesc + + +class _FakeCoordinator: + def __init__(self, last_resources): + self.last_resources = last_resources + + +def _bound(desc, href): + capability = Capability(href=href, entities=(desc,)) + return BoundEntity(href=href, capability=capability, desc=desc) + + +class TestDefaultFieldGate: + """No explicit exists_fn -- the field-presence default in entity.py.""" + + def _bound(self): + return _bound(SensorDesc(key='x', field='x.com.samsung.da.value'), '/x/vs/0') + + def test_included_when_field_present(self): + bound = self._bound() + coord = _FakeCoordinator({'/x/vs/0': {'x.com.samsung.da.value': '1'}}) + assert _is_included(bound, coord) is True + + def test_excluded_when_field_absent_from_populated_rep(self): + bound = self._bound() + coord = _FakeCoordinator({'/x/vs/0': {'x.com.samsung.da.other': '1'}}) + assert _is_included(bound, coord) is False + + def test_included_on_true_stub(self): + bound = self._bound() + coord = _FakeCoordinator({'/x/vs/0': {'href': '/x/vs/0'}}) + assert _is_included(bound, coord) is True + + def test_included_on_genuinely_empty_rep(self): + """The default gate stays permissive on a real {} -- see + /alarms/vs/0, whose empty rep is the normal no-alarm state, not a + signal the hardware is unsupported. Only a capability-specific + exists_fn (e.g. common.ENERGY_METER) opts into excluding on + confirmed-empty, after verifying that's actually safe for its field.""" + bound = self._bound() + coord = _FakeCoordinator({'/x/vs/0': {}}) + assert _is_included(bound, coord) is True + + def test_excluded_when_href_missing_from_resources(self): + bound = self._bound() + coord = _FakeCoordinator({}) + assert _is_included(bound, coord) is False + + +class TestExplicitExistsFnGate: + def test_explicit_exists_fn_overrides_default_field_gate(self): + desc = SensorDesc(key='x', field='x.com.samsung.da.value', + exists_fn=lambda rep, resources: rep.get('flag') is True) + bound = _bound(desc, '/x/vs/0') + coord = _FakeCoordinator({'/x/vs/0': {'flag': True}}) + assert _is_included(bound, coord) is True + coord = _FakeCoordinator({'/x/vs/0': {'flag': False, 'x.com.samsung.da.value': '1'}}) + assert _is_included(bound, coord) is False + + +class TestNoFieldEntities: + def test_rep_fn_entity_always_included(self): + desc = SensorDesc(key='x', rep_fn=lambda rep: rep.get('x.com.samsung.da.value')) + bound = _bound(desc, '/x/vs/0') + coord = _FakeCoordinator({'/x/vs/0': {}}) + assert _is_included(bound, coord) is True