Add quirks v2 siren entity based on ZCLEnumMetadata#668
Conversation
Co-authored-by: puddly <32534428+puddly@users.noreply.github.com>
|
see comment: #667 (comment) |
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Comment-only since this is a draft. Reviewed alongside the sibling PR #667 (SwitchMetadata route) which I'll cover separately.
How the two PRs compare
#667 and this PR explore the same question — how does a v2 quirk emit an HA siren entity? — from two directions:
- #667 adds only
ConfigurableAttributeSiren(SwitchMetadata→sirenplatform). Two-state, on/off, no tones. - #668 adds
ConfigurableAttributeSirenandEnumSiren(ZCLEnumMetadata→sirenplatform with tones), plus refactors the existingSirenintoBaseSiren/BaseZclSiren/AdvancedSiren/BasicSiren. It supersedes #667.
For the immediate motivator (the smoke-sensor-with-siren in zigpy/zha-device-handlers#4637), this PR is the more useful design: the device has multiple tones and a HA siren entity with available_tones is a better fit than a select. The EnumSiren route also keeps the door open for translation reuse with select when that lands.
The weakness of the enum-on-siren overload is the order dependency (see inline). The PR body says "Entry 0 from enum will be the off-state" but the code uses declaration order (next(iter(enum)), entries[1]), not value order. With well-formed enums (Off = 0 first) the two coincide, but an author who writes Alert = 1, Off = 0 will silently get "Alert" treated as off. Worth either documenting + asserting, or sorting by .value and treating 0 as off explicitly.
Given the choice between the two, I'd land this one (#668) and close #667 as superseded — it covers strictly more cases and the SwitchMetadata path here is the same as #667.
Blocking issues
- CI is red — the 5 new tests (
test_siren_configurable_attribute*,test_siren_enum) all fail withAttributeError: type object 'EntityPlatform' has no attribute 'SIREN'. The zigpy dep addingEntityPlatform.SIREN = "siren"isn't merged/released yet (sibling #667's body acknowledges this; #668's body doesn't). Block until zigpy ships and the floor here is bumped. - dmulcahey's API-shape concern on #667 carries over:
.switch(..., entity_platform=EntityPlatform.SIREN)and.enum(..., entity_platform=EntityPlatform.SIREN)are unintuitive QuirkBuilder shapes. A dedicated.siren(...)builder method (or.siren_from_switch/.siren_from_enum) reads better. Not a hard blocker but the API surface ships to quirks authors and is harder to change later.
Minor
_attr_available_tones: dict[int, str]is declared onBaseSirenwithout a default;ConfigurableAttributeSiren.__init__andEnumSiren.__init__both set it, butAdvancedSiren/BasicSirenset it aftersuper().__init__. Readinginfo_objectbetweensuper().__init__and the assignment wouldAttributeError— currently safe becauseinfo_objectisfunctools.cached_propertyevaluated lazily, but worth a class-level default (= {}) for robustness.- The
Siren→AdvancedSirenrename: I confirmed HA Core (homeassistant/components/zha/siren.py) only importsSirenEntityFeature, notSiren, andunique_idis class-name-agnostic — so the rename is safe externally. Worth mentioning in the PR body for reviewers' peace of mind. - Mypy regresses by 2 errors (
_init_from_quirks_metadataLSP violations onConfigurableAttributeSirenandEnumSiren), mirroring the existing pattern inselect.py/switch.py. Acceptable as repo-wide style consistency, not new debt.
| value = self._cluster_handler.cluster.get(self._attribute_name) | ||
| if value is None: | ||
| return False | ||
| off_value = next(iter(self._enum)).value |
There was a problem hiding this comment.
Order-dependency bug. next(iter(self._enum)) returns the first declared member, not the member with value 0. If a quirk author writes
class SirenMode(t.enum8):
Alert = 0x01
Off = 0x00
Alarm = 0x02then off_value = 1, and is_on returns False when the device is actually alerting. Same hazard in async_turn_off (line 355) and async_turn_on's default-tone (line 347 — entries[1] picks the second declared entry, not value 1).
The PR body says "Entry 0 from enum will be the off-state" which could mean either index 0 or value 0; the code chose index 0 (declaration order). Suggest either:
- pick by value:
off_value = 0;default_tone = sorted(self._enum, key=lambda m: m.value)[1], and documentvalue 0 = off; or - keep declaration order but validate at
_init_from_quirks_metadata:assert next(iter(self._enum)).value == 0, "first enum member must be the off entry with value 0"and document that in the QuirkBuilder docstring.
I'd lean toward the value-based variant — it's harder to get wrong from the quirk side.
| # All entries except index 0 (off) are exposed as tones | ||
| entries = list(self._enum) | ||
| self._attr_available_tones = { | ||
| entry.value: entry.name.replace("_", " ") for entry in entries[1:] |
There was a problem hiding this comment.
entry.name.replace("_", " ") is the only humanization step. For enum members like FIRE_PANIC the result is "FIRE PANIC" (all caps), inconsistent with the existing AdvancedSiren._attr_available_tones ("Fire Panic", title-cased) on line 418. If available_tones is rendered in the HA UI for tone selection, the casing will look off depending on whether the enum uses SCREAMING_SNAKE_CASE or Pascal_Snake_Case. Consider entry.name.replace("_", " ").title() or, better, requiring the enum author to provide friendly names via translations once those land. (See PR body's Future section — same place this'll need an update.)
| ) -> None: | ||
| """Turn on siren. Uses tone if provided, otherwise the second enum entry.""" | ||
| entries = list(self._enum) | ||
| if tone is not None and tone in self._attr_available_tones: |
There was a problem hiding this comment.
Silent fallthrough: tone is not None and tone in self._attr_available_tones is False both when tone is invalid (caller error — typo, wrong value) and when tone == 0 (the off entry's value; not in available_tones because tones excludes index 0). In both cases the entity fires the default tone instead of erroring. For a siren that's surprising — async_turn_on(tone=999) should probably raise ValueError, not start blaring at the default tone.
Minor — the HA siren service schema may already validate tone before this is reached, in which case ignore.
| self.maybe_emit_state_changed_event() | ||
|
|
||
|
|
||
| class ConfigurableAttributeSiren(BaseSiren): |
There was a problem hiding this comment.
Mostly duplicating ConfigurableAttributeSwitch (inverter, force-inverted, on_value/off_value, _init_from_quirks_metadata, async_update) — but only the on/off plumbing, none of the siren-specific behavior (timed-off via _off_listener, available_tones, strobe/level). A device whose siren state lives in a single attribute is realistic, but the entity then can't time-out via the cluster's IasWd.Warning mechanism the way AdvancedSiren/BasicSiren do. Fine as-is for the smoke-sensor case in zigpy/zha-device-handlers#4637, just worth noting in the docstring that this entity has no built-in duration handling.
| (Platform.SENSOR, ZCLSensorMetadata): sensor.Sensor, | ||
| (Platform.SELECT, ZCLEnumMetadata): select.ZCLEnumSelectEntity, | ||
| (Platform.NUMBER, NumberMetadata): number.NumberConfigurationEntity, | ||
| (Platform.SIREN, SwitchMetadata): siren.ConfigurableAttributeSiren, |
There was a problem hiding this comment.
Worth confirming whether ZHA needs Platform.SIREN to appear alongside Platform.SWITCH for SwitchMetadata (currently both map, dispatched by entity_platform). The lookup at L312 does Platform(entity_metadata.entity_platform.value) then (platform, metadata_type) — so a quirk with entity_platform=EntityPlatform.SIREN lands on siren.ConfigurableAttributeSiren, and the default EntityPlatform.SWITCH lands on switch.ConfigurableAttributeSwitch. Good. Just make sure the registry has no platform that both SwitchMetadata paths can match against and feature_priority doesn't trip the new SIREN feature group accidentally for non-siren switches.
3 participants
DRAFT.
Based on:
SwitchMetadata#667Proposed change
This allows quirks v2 entities to provide
entity_platform=EntityPlatform.SIRENfor.enum()entities to create a siren entity on the HA side, instead of a select entity.Entry 0 from enum will be the off-state.
Entry 1 will be the default tone if enabled without one explicitly selected (similar to ZCL siren that hardcodes it).
Remaining entries will also be exposed as additional tones for the siren entity in HA.
Additional information
This came up as zigpy/zha-device-handlers#4637 added a select entity with "Stop" and two different tones for a smoke sensor with a siren.
ZCLEnumMetadataalready works good enough to also allow us to create siren entities with it, so that's what this PR implements. IMO, having entry 0 in the enum be the OFF state for the siren entity is fine.Creating separate
SirenMetadatawould confuse things more (for the few devices that will ever use that..), as we'd potentially need to support on/off attribute writes with no tones, but also multiple tones, and maybe even custom commands for siren entities at some point? (which we don't, yet)Reusing existing
SwitchMetadataandZCLEnumMetadataworks well enough and is somewhat consistent with the existing implementation of.enum(entity_platform=EntityPlatform.SENSOR). I don't think we should overcomplicate sirens at this point.Future
When we deal with select entities (based on enum classes) getting translations, we should (naturally) also replicate that logic to the siren class. IMO, it doesn't make sense adding that now, as the ZCL siren completely hardcodes the static values still, select/enum entities do not support translations at all yet, and we don't have nice scripts to auto-generate those parts for translations. It should be easy to implement it here when we do everything else (since it's all enums / state translations).