Reload HomeKit security system on stale valid values#172752
Reload HomeKit security system on stale valid values#172752mattias-arrelid wants to merge 4 commits into
Conversation
Reconfiguring the alarm panel so an area exposes different arm modes
changes the entity's supported_features. The HomeKit accessory freezes its
valid security states from supported_features at build time and is only
rebuilt while the entity stays available. A panel reconfiguration typically
takes the entity through unavailable, so the reload guard misses the
change and the valid-values set goes stale; a later state then maps to a
value it no longer allows and raises ValueError.
Before pushing a value, check it against the entity's current
supported_features:
- value already valid for the characteristic: push it.
- value not valid yet, but the current features support it: the features
grew since build, so reload to rebuild the accessory.
- value not valid and not supported now either: the entity is reporting a
state it never advertised, so reloading would rebuild the same set and
loop. Log and skip instead.
Timeline that triggers it:
1. Accessory is built. The area exposes e.g. away only, so the valid
current states are {disarmed, triggered, away}; armed_home (0) is not
among them.
2. The panel is reconfigured to also allow arm-home; the entity goes
unavailable while the integration reloads.
3. The entity returns advertising ARM_HOME. The reload guard in
async_update_event_state_callback only compares attributes when both
old and new states are available, so the unavailable -> available
transition is skipped and the accessory keeps its stale valid values.
4. The entity reports armed_home -> mapped to 0 -> set_value(0) raised
"SecuritySystemCurrentState: value=0 is an invalid value."
home-assistant
Bot
added
bugfix
cla-signed
has-tests
integration: homekit
Top 100
|
Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves HomeKit SecuritySystem accessory robustness when an alarm control panel’s supported_features changes at runtime by reloading (or skipping) updates when a HomeKit characteristic’s frozen ValidValues set becomes stale.
Changes:
- Factor out supported-state derivation into
_supported_states()and introduceDEFAULT_SUPPORTED_FEATURES. - Add
_set_or_reload()to avoidValueErrorwhen pushing states not present in the characteristic’s frozenValidValues, reloading when appropriate. - Add tests covering reload vs skip behavior when
supported_featuresgrows or a state is unadvertised.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
homeassistant/components/homekit/type_security_systems.py |
Adds runtime staleness detection for ValidValues and reload/skip logic based on current supported_features. |
tests/components/homekit/test_type_security_systems.py |
Adds test cases to verify reload vs skip behavior for supported feature changes and invalid states. |
| if value in char.properties.get(PROP_VALID_VALUES, {}).values(): | ||
| char.set_value(value) | ||
| return True | ||
| if value in currently_supported: | ||
| self.async_reload() | ||
| else: | ||
| _LOGGER.debug( | ||
| "%s: Skipping unsupported security state %d; not in %s", | ||
| self.entity_id, | ||
| value, | ||
| currently_supported, | ||
| ) | ||
| return False |
There was a problem hiding this comment.
pyhap always stores ValidValues as a dict (the existing code here already calls .values() on it), so this should be safe as written.
| acc.run() | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert 0 not in acc.char_current_state.properties["ValidValues"].values() |
There was a problem hiding this comment.
pyhap always stores ValidValues as a dict (the existing code here already calls .values() on it), so this should be safe as written.
| current_supported, target_supported = _supported_states( | ||
| new_state.attributes.get( | ||
| ATTR_SUPPORTED_FEATURES, DEFAULT_SUPPORTED_FEATURES | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Done in 6272c83 — async_update_state now resolves the current/target HomeKit states first and returns early when neither maps, so _supported_states() is only computed when a value is actually being pushed.
Compute current/target HomeKit states first and bail before deriving the supported sets when neither maps, so async_update_state allocates nothing on the (common) path where the state has no HomeKit equivalent.
| default_current_states = current_char.properties.get("ValidValues") | ||
| default_target_services = target_char.properties.get("ValidValues") |
There was a problem hiding this comment.
Good catch — done in 8f7a734, the build-time lookups now use PROP_VALID_VALUES too so the module no longer mixes the constant with the literal.
| def _supported_states(supported_features: int) -> tuple[list[int], list[int]]: | ||
| """Return the supported (current_states, target_services) for the features. |
There was a problem hiding this comment.
target_supported_services (and HASS_TO_HOMEKIT_SERVICES / default_target_services) is the existing naming in this module; the extracted helper just keeps it. Renaming only the helper would make it inconsistent with the surrounding original code, and renaming everything is out of scope for this fix — leaving as-is for consistency.
| stay in sync. | ||
| """ | ||
| current_supported_states = [HK_ALARM_DISARMED, HK_ALARM_TRIGGERED] | ||
| target_supported_services = [HK_ALARM_DISARMED] |
There was a problem hiding this comment.
Same as above — keeping the existing module naming (*_services) for consistency; out of scope to rename here.
| current_supported_states.append(HK_ALARM_NIGHT_ARMED) | ||
| target_supported_services.append(HK_ALARM_NIGHT_ARMED) | ||
|
|
||
| return current_supported_states, target_supported_services |
There was a problem hiding this comment.
Same as above — keeping the existing module naming (*_services) for consistency; out of scope to rename here.
| acc.async_update_state(grew) | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert mock_reload.called |
There was a problem hiding this comment.
Agreed, tightened to call_count == 1 in 8f7a734 — a loop is exactly what the skip path guards against, so asserting a single reload is a better regression test.
Use the PROP_VALID_VALUES constant for the build-time lookups too, so the module no longer mixes the constant with the "ValidValues" literal. Tighten the reload test to assert exactly one reload, catching an accidental loop.
| if value in char.properties.get(PROP_VALID_VALUES, {}).values(): | ||
| char.set_value(value) | ||
| return True | ||
| if value in currently_supported: | ||
| self.async_reload() | ||
| else: | ||
| _LOGGER.debug( | ||
| "%s: Skipping unsupported security state %d; not in %s", | ||
| self.entity_id, | ||
| value, | ||
| currently_supported, | ||
| ) | ||
| return False |
There was a problem hiding this comment.
As noted on the earlier thread: pyhap always stores ValidValues as a dict (its own valid_value_or_raise calls .values(), and the build-time code here does too), so the list shape doesn't occur.
| def _supported_states(supported_features: int) -> tuple[list[int], list[int]]: | ||
| """Return the supported (current_states, target_services) for the features. | ||
|
|
||
| Mirrors how HomeKit's valid values are derived from the alarm entity's | ||
| supported_features, so the accessory build and the runtime staleness check | ||
| stay in sync. | ||
| """ | ||
| current_supported_states = [HK_ALARM_DISARMED, HK_ALARM_TRIGGERED] | ||
| target_supported_services = [HK_ALARM_DISARMED] |
There was a problem hiding this comment.
As on the other naming threads — *_services is the existing module naming; keeping it consistent and out of scope to rename here.
| - value not valid and not supported now either: the entity is reporting | ||
| a state it doesn't advertise -> reloading wouldn't help (and would | ||
| loop), so log once and skip. | ||
|
|
||
| Returns True if the value was pushed, False otherwise. | ||
| """ |
There was a problem hiding this comment.
Good catch — fixed in 4e680ed by dropping "once" from the docstring. The skip is debug-level and rare, so a per-value log-once guard isn't worth the extra state; the wording was just wrong.
| else: | ||
| _LOGGER.debug( | ||
| "%s: Skipping unsupported security state %d; not in %s", | ||
| self.entity_id, | ||
| value, | ||
| currently_supported, | ||
| ) |
There was a problem hiding this comment.
Same as above — docstring corrected in 4e680ed.
The skip path logs on every unsupported state, not once; drop the misleading word from the docstring.
| ) | ||
|
|
||
|
|
||
| def _supported_states(supported_features: int) -> tuple[list[int], list[int]]: |
There was a problem hiding this comment.
As on the earlier naming threads — *_services is the existing module naming; keeping it consistent, renaming is out of scope here.
| stay in sync. | ||
| """ | ||
| current_supported_states = [HK_ALARM_DISARMED, HK_ALARM_TRIGGERED] | ||
| target_supported_services = [HK_ALARM_DISARMED] |
There was a problem hiding this comment.
As on the earlier naming threads — *_services is the existing module naming; keeping it consistent, renaming is out of scope here.
|
|
||
| if supported_features & AlarmControlPanelEntityFeature.ARM_HOME: | ||
| current_supported_states.append(HK_ALARM_STAY_ARMED) | ||
| target_supported_services.append(HK_ALARM_STAY_ARMED) |
There was a problem hiding this comment.
As on the earlier naming threads — *_services is the existing module naming; keeping it consistent, renaming is out of scope here.
| | AlarmControlPanelEntityFeature.ARM_VACATION | ||
| ): | ||
| current_supported_states.append(HK_ALARM_AWAY_ARMED) | ||
| target_supported_services.append(HK_ALARM_AWAY_ARMED) |
There was a problem hiding this comment.
As on the earlier naming threads — *_services is the existing module naming; keeping it consistent, renaming is out of scope here.
| target_supported_services.append(HK_ALARM_NIGHT_ARMED) | ||
|
|
||
| return current_supported_states, target_supported_services |
There was a problem hiding this comment.
As on the earlier naming threads — *_services is the existing module naming; keeping it consistent, renaming is out of scope here.
| entity_id = "alarm_control_panel.test" | ||
|
|
||
| attrs = {"supported_features": AlarmControlPanelEntityFeature.ARM_HOME} | ||
| hass.states.async_set(entity_id, AlarmControlPanelState.DISARMED, attributes=attrs) |
There was a problem hiding this comment.
ruff check (incl. E501) passes on this file — the line is 87 chars, under the 88 limit, so no wrapping needed.
| acc.run() | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert 0 not in acc.char_current_state.properties["ValidValues"].values() |
There was a problem hiding this comment.
The test file already references "ValidValues" as a literal in several existing assertions; keeping these consistent with that. The constant matters in the production lookup (which can drift with pyhap); asserting on the well-known HAP property name by literal in tests is fine.
|
|
||
| assert not mock_reload.called | ||
| # State left unchanged; no ValueError raised. | ||
| assert 0 not in acc.char_current_state.properties["ValidValues"].values() |
There was a problem hiding this comment.
Same as above — consistent with the existing "ValidValues" literals already used in this test file.
Proposed change
The HomeKit
SecuritySystemaccessory freezes its validSecuritySystemCurrentState/SecuritySystemTargetStatevalues from the alarm entity'ssupported_featuresat build time.async_update_statethen maps the live state and callsset_value.ATTR_SUPPORTED_FEATURESis inRELOAD_ON_CHANGE_ATTRS, so the accessory is normally rebuilt when features change. But the reload check inasync_update_event_state_callbackonly compares attributes when both the old and new states are available. Asupported_featureschange arriving across an unavailable/unknown boundary (e.g. reconfiguring the panel so an area exposes different arm modes, which takes the entity through unavailable while the integration reloads) is skipped, leaving the frozen valid-values set stale. The next push then maps to a value that set no longer contains and raises:Before pushing a value, this now checks it against the entity's current
supported_features:Timeline that triggers it:
{disarmed, triggered, away};armed_home(0) is not among them.ARM_HOME. The reload guard inasync_update_event_state_callbackonly compares attributes when both old and new states are available, so theunavailable -> availabletransition is skipped and the accessory keeps its stale valid values.armed_home-> mapped to0->set_value(0)raised the error above.This addresses the runtime
set_valueValueErrorthat propagates out ofasync_update_stateand breaks the state-change dispatch. It does not touch the separate, harmless build-time log from pyhap'soverride_properties(ikalchev/HAP-python#473, #491), nor does it make an integration that permanently under-declaressupported_featureswork in HomeKit — for that case the accessory now skips the unsupported state instead of raising or reload-looping.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: