diff --git a/custom_components/localthings/entity.py b/custom_components/localthings/entity.py index 17962bf..8a8fc3d 100644 --- a/custom_components/localthings/entity.py +++ b/custom_components/localthings/entity.py @@ -8,7 +8,7 @@ from homeassistant.helpers.device_registry import DeviceInfo from homeassistant.const import EntityCategory from .registry.adapter import _key -from .registry.discovery import BoundEntity +from .registry.discovery import BoundEntity, _snake_to_title from .const import DOMAIN from .coordinator import LocalThingsCoordinator @@ -44,7 +44,7 @@ def _derive_name(state_key: str) -> str: instance number with a space: "door_cooler_open1" → "Door Cooler Open 1". """ name = re.sub(r'(\d+)$', lambda m: f' {m.group()}' if int(m.group()) > 0 else '', state_key) - return name.replace('_', ' ').title().strip() + return _snake_to_title(name).strip() class LocalThingsEntity(CoordinatorEntity[LocalThingsCoordinator]): @@ -57,7 +57,15 @@ class LocalThingsEntity(CoordinatorEntity[LocalThingsCoordinator]): self._bound = bound self._state_key = _key(bound) self._attr_unique_id = f"{DOMAIN}_{coordinator.device_serial}_{self._state_key}" - self._attr_name = bound.desc.name if bound.desc.name is not None else _derive_name(self._state_key) + if bound.desc.name is not None: + self._attr_name = bound.desc.name + elif bound.instance_name: + # A device-given instance name (e.g. an ice maker's "Cubed + # Ice") takes the place of the href-derived instance label, + # keeping the same entity-specific suffix (issue #27). + self._attr_name = f"{bound.instance_name} {_derive_name(bound.desc.key)}".strip() + else: + self._attr_name = _derive_name(self._state_key) self._attr_translation_key = bound.desc.translation_key self._attr_icon = bound.desc.icon raw_cat = bound.desc.entity_category diff --git a/custom_components/localthings/observe.py b/custom_components/localthings/observe.py index acda4fe..e3c5aad 100644 --- a/custom_components/localthings/observe.py +++ b/custom_components/localthings/observe.py @@ -69,6 +69,7 @@ class ObserveManager: self.last_mode_change_wall = time.time() self._settle_until: dict[str, float] = {} self._settle_lock = threading.Lock() + self._cache_lock = threading.Lock() self.subscribed_hrefs: set[str] = set() self._notified: set[str] = set() self._last_notify_ts: float | None = None @@ -92,11 +93,35 @@ class ObserveManager: return True def apply(self, href: str, rep: dict, source: str) -> bool: - """Gate a StateCache.apply_rep call through the write-settle guard.""" + """Gate a StateCache.apply_rep call through the write-settle guard. + + Merges the incoming rep onto whatever's already cached for this + href rather than handing it to StateCache.apply_rep verbatim -- + apply_rep does a full replace, and Samsung devices don't always + repeat every field on every update (issue #27: a /mode/vs/0 + OBSERVE notify -- and, on at least one Bespoke fridge, even the + /device/0 sweep entry for that href -- can carry just `modes`, + omitting `supportedOptions`/`supportedModes` entirely). A full + replace would silently wipe fields a select entity's + exists_fn/options_field gates on the moment one partial update + comes through, even though nothing about the device's actual + supported options changed. + + `apply()` is the sole path StateCache mutations flow through in + this component (poll, sweep, and OBSERVE notify all funnel here), + so `_cache_lock` serializes the read-then-write across those + threads (DTLS reader for notifies, executor threads for poll/ + sweep) -- without it, two concurrent updates for the same href + could each read the same prior rep and the second writer would + silently lose the first's fields, reintroducing the exact bug + this merge fixes. + """ if self._is_settling(href): self.log.debug("dropping %s update for %s (settling)", source, href) return False - return self.cache.apply_rep(href, rep, source=source) + with self._cache_lock: + merged = {**(self.cache.get(href) or {}), **rep} + return self.cache.apply_rep(href, merged, source=source) def on_notification(self, href: str, payload: bytes) -> None: """Wired as DtlsCoapSession.on_notification. Runs on the DTLS diff --git a/custom_components/localthings/registry/capabilities/fridge.py b/custom_components/localthings/registry/capabilities/fridge.py index b6d6dd1..2f2589c 100644 --- a/custom_components/localthings/registry/capabilities/fridge.py +++ b/custom_components/localthings/registry/capabilities/fridge.py @@ -100,6 +100,12 @@ ICEMAKER_NIGHTTIME = Capability( # Icemaker (generic — covers /icemaker/one/vs/0, /icemaker/two/vs/0) # /icemaker/status/vs/0 is kept as exact-href cap and binds first. # /icemaker/nighttime/vs/0 is excluded by match_fn (lacks iceMaker.state). +# +# Entity names are derived from x.com.samsung.da.iceMaker.name ("CUBED_ICE", +# "ICE_BITES") via name_field, not the href's "one"/"two" segment -- these two +# ice makers are independent on/off toggles that can both be enabled at once +# (issue #27), so they stay separate entities rather than a single ice-type +# select, but users still want them labeled with the device's own names. # --------------------------------------------------------------------------- def _icemaker_write(field): @@ -112,6 +118,7 @@ ICEMAKER_GENERIC = Capability( href=None, href_prefix='/icemaker/', match_fn=lambda rep, resources: 'x.com.samsung.da.iceMaker.state' in rep, + name_field='x.com.samsung.da.iceMaker.name', poll_tier='warm', entities=( SensorDesc(key='making_status', diff --git a/custom_components/localthings/registry/capability.py b/custom_components/localthings/registry/capability.py index 05921f3..2c6b2d3 100644 --- a/custom_components/localthings/registry/capability.py +++ b/custom_components/localthings/registry/capability.py @@ -15,6 +15,11 @@ class Capability: rt_filter: Optional[str] = None # bind only if rt_filter in rep.get('rt', ()) href_prefix: Optional[str] = None # pattern caps only: bind only if href starts with this strip_prefix_in_key: bool = False # strip href_prefix segs before building key_override + # Rep field holding this instance's device-given name (e.g. an ice + # maker's "CUBED_ICE"/"ICE_BITES"), normalized and used as the display + # name prefix in place of the href-derived instance label. Does not + # affect key_override/unique_id -- only what's shown in the UI. + name_field: Optional[str] = None match_fn: Optional[Callable[[dict, dict], bool]] = None # match_fn(rep, resources) -> bool # Rare optional hook — only operational-state-style resources use this. on_observation: Optional[Callable[[dict, dict], None]] = None diff --git a/custom_components/localthings/registry/discovery.py b/custom_components/localthings/registry/discovery.py index 61e27e3..891c2ae 100644 --- a/custom_components/localthings/registry/discovery.py +++ b/custom_components/localthings/registry/discovery.py @@ -27,6 +27,25 @@ class BoundEntity: desc: SamsungEntityDescription instance: str = '' key_override: Optional[str] = None + instance_name: Optional[str] = None + + +def _snake_to_title(s: str) -> str: + """'CUBED_ICE'/'cubed_ice' -> 'Cubed Ice'. Shared with entity.py's + _derive_name, which applies the same transform to a state key.""" + return s.replace('_', ' ').title() + + +def _instance_name(cap: Capability, rep: dict) -> Optional[str]: + """Normalize `cap.name_field`'s raw value ("CUBED_ICE" -> "Cubed Ice") + for use as a display-name prefix, or None if the cap doesn't declare + one or the device didn't report it.""" + if not cap.name_field: + return None + raw = rep.get(cap.name_field) + if not isinstance(raw, str) or not raw: + return None + return _snake_to_title(raw) def instance_suffix(href: str) -> str: @@ -37,6 +56,18 @@ def instance_suffix(href: str) -> str: return '' +def _bind(cap: Capability, href: str, inst: str, inst_name: Optional[str], + key_prefix: Optional[str] = None) -> list[BoundEntity]: + """Build one BoundEntity per entity on `cap`, sharing the instance/ + key-prefix/instance-name computed once by the caller.""" + return [ + BoundEntity(href=href, capability=cap, desc=desc, instance=inst, + key_override=f'{key_prefix}_{desc.key}' if key_prefix else None, + instance_name=inst_name) + for desc in cap.entities + ] + + def discover( resources: dict[str, dict], registry: dict[str, list[Capability]], @@ -58,9 +89,7 @@ def discover( if cap.match_fn is not None and not cap.match_fn(rep, resources): continue inst = instance_suffix(href) - for desc in cap.entities: - out.append(BoundEntity(href=href, capability=cap, - desc=desc, instance=inst)) + out.extend(_bind(cap, href, inst, _instance_name(cap, rep))) matched = True if matched: @@ -78,10 +107,7 @@ def discover( # Auto-derive key prefix from href segments (skip digits and 'vs') src = href[len(cap.href_prefix):] if (cap.strip_prefix_in_key and cap.href_prefix) else href segs = [s for s in src.strip('/').split('/') if s and not s.isdigit() and s != 'vs'] - for desc in cap.entities: - key_override = '_'.join(segs) + '_' + desc.key - out.append(BoundEntity(href=href, capability=cap, desc=desc, - instance=inst, key_override=key_override)) + out.extend(_bind(cap, href, inst, _instance_name(cap, rep), '_'.join(segs))) matched = True break diff --git a/tests/localthings/test_observe.py b/tests/localthings/test_observe.py index a7e1aff..7d3fe9a 100644 --- a/tests/localthings/test_observe.py +++ b/tests/localthings/test_observe.py @@ -34,6 +34,28 @@ def test_apply_writes_through_when_not_settling(): assert mgr.cache.get('/oven/vs/0') == {'a': 1} +def test_apply_merges_partial_update_onto_prior_rep(): + """Regression test for issue #27: a Bespoke fridge's /mode/vs/0 notify + (and even a later sweep entry for that href) can carry only `modes`, + omitting `supportedOptions` entirely. A full replace would wipe + `supportedOptions` from the cache the moment that partial update + arrives, even though the device's supported options didn't change -- + which is exactly what made the flex-zone select disappear.""" + mgr = _manager() + full = { + 'x.com.samsung.da.modes': ['CVN_CONVERTIBLE_ZONE', 'CV_FDR_MEAT'], + 'x.com.samsung.da.supportedOptions': ['CV_FDR_WINE', 'CV_FDR_MEAT'], + } + mgr.apply('/mode/vs/0', full, source='poll') + + partial = {'x.com.samsung.da.modes': ['CVN_CONVERTIBLE_ZONE', 'WATERFILTER_ENABLE']} + mgr.apply('/mode/vs/0', partial, source='observe') + + cached = mgr.cache.get('/mode/vs/0') + assert cached['x.com.samsung.da.modes'] == ['CVN_CONVERTIBLE_ZONE', 'WATERFILTER_ENABLE'] + assert cached['x.com.samsung.da.supportedOptions'] == ['CV_FDR_WINE', 'CV_FDR_MEAT'] + + def test_apply_drops_update_during_settle_window(): mgr = _manager() mgr.cache.apply_rep('/oven/vs/0', {'a': 1}, source='seed') diff --git a/tests/test_discovery.py b/tests/test_discovery.py index 7b5a8d9..59702e2 100644 --- a/tests/test_discovery.py +++ b/tests/test_discovery.py @@ -158,6 +158,46 @@ def test_discover_match_fn_decline_is_not_logged_as_gap(): assert seen == [] +def test_discover_pattern_cap_reads_name_field(): + """A pattern cap's name_field normalizes a device-given instance name + (e.g. an ice maker's "CUBED_ICE") for use in the entity's display name, + independent of the href-derived key (issue #27).""" + cap = Capability( + href=None, + href_prefix='/icemaker/', + name_field='x.com.samsung.da.iceMaker.name', + entities=(BinarySensorDesc(key='enabled', field='x.com.samsung.da.iceMaker.state'),), + ) + resources = { + '/icemaker/one/vs/0': { + 'x.com.samsung.da.iceMaker.state': 'On', + 'x.com.samsung.da.iceMaker.name': 'CUBED_ICE', + }, + '/icemaker/two/vs/0': { + 'x.com.samsung.da.iceMaker.state': 'On', + 'x.com.samsung.da.iceMaker.name': 'ICE_BITES', + }, + } + bound = discover(resources, {}, pattern_caps=[cap]) + names = {b.href: b.instance_name for b in bound} + assert names == { + '/icemaker/one/vs/0': 'Cubed Ice', + '/icemaker/two/vs/0': 'Ice Bites', + } + + +def test_discover_pattern_cap_name_field_absent_leaves_instance_name_none(): + cap = Capability( + href=None, + href_prefix='/icemaker/', + name_field='x.com.samsung.da.iceMaker.name', + entities=(BinarySensorDesc(key='enabled', field='x.com.samsung.da.iceMaker.state'),), + ) + resources = {'/icemaker/one/vs/0': {'x.com.samsung.da.iceMaker.state': 'On'}} + bound = discover(resources, {}, pattern_caps=[cap]) + assert bound[0].instance_name is None + + def test_discover_rt_filter_gates_binding(): """Cap with rt_filter must not bind a rep whose rt list does not match.""" oven_mode_cap = Capability( diff --git a/tests/test_entity_naming.py b/tests/test_entity_naming.py new file mode 100644 index 0000000..1b9bacf --- /dev/null +++ b/tests/test_entity_naming.py @@ -0,0 +1,45 @@ +"""Tests for LocalThingsEntity's display-name derivation +(custom_components/localthings/entity.py) -- the explicit-name, +device-given-instance-name, and href-derived fallbacks. +""" +from custom_components.localthings.entity import LocalThingsEntity +from custom_components.localthings.registry.capability import Capability +from custom_components.localthings.registry.discovery import BoundEntity +from custom_components.localthings.registry.entities import BinarySensorDesc + + +class _FakeCoordinator: + device_serial = 'TEST-SERIAL' + + def __init__(self, last_resources=None): + self.last_resources = last_resources or {} + + +def _make_entity(desc, href='/x/vs/0', key_override=None, instance='', instance_name=None): + capability = Capability(href=href, entities=(desc,)) + bound = BoundEntity(href=href, capability=capability, desc=desc, + instance=instance, key_override=key_override, + instance_name=instance_name) + return LocalThingsEntity(_FakeCoordinator(), bound) + + +def test_explicit_name_wins_over_everything(): + desc = BinarySensorDesc(key='enabled', name='Explicit Name') + entity = _make_entity(desc, instance_name='Cubed Ice') + assert entity._attr_name == 'Explicit Name' + + +def test_instance_name_prefixes_the_derived_suffix(): + """Issue #27: an ice maker's device-given name ("Cubed Ice") replaces + the href-derived instance label ("Icemaker One") as the name prefix, + keeping the same entity-specific suffix.""" + desc = BinarySensorDesc(key='enabled') + entity = _make_entity(desc, key_override='icemaker_one_enabled', + instance_name='Cubed Ice') + assert entity._attr_name == 'Cubed Ice Enabled' + + +def test_no_instance_name_falls_back_to_derived_state_key(): + desc = BinarySensorDesc(key='enabled') + entity = _make_entity(desc, key_override='icemaker_one_enabled') + assert entity._attr_name == 'Icemaker One Enabled'