Add quirks v2 siren entity based on SwitchMetadata#667
Conversation
Co-authored-by: puddly <32534428+puddly@users.noreply.github.com>
|
Maybe we add another method to the builder to make it clearer we are making a siren ex: QuirkBuilder(zigpy_dev.manufacturer, zigpy_dev.model)
.switch(
general.Basic.AttributeDefs.power_source.name,
general.Basic.cluster_id,
on_value=1,
off_value=0,
entity_platform=EntityPlatform.SIREN,
translation_key="siren",
fallback_name="Siren",
)
.add_to_registry()
)makes sense to us but maybe will confuse potential contributors. |
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Reviewing this together with the sibling #668. tl;dr: I'd recommend closing this in favor of #668, which lands the same ConfigurableAttributeSiren and BaseSiren/BaseZclSiren/AdvancedSiren/BasicSiren refactor plus an EnumSiren based on ZCLEnumMetadata. Comments below are framed around that.
Design vs #668
The direct motivator on the quirks side is zigpy/zha-device-handlers#4637 (smoke sensor with multi-tone siren). The SwitchMetadata-only shape in this PR can't express that:
- No tone selection. A
SwitchMetadataboolean-mapped to two attribute values can only flip the alarm on/off; it can't surface the enum of available tones to HA. #668'sEnumSirenreads each enum member as an entry inavailable_tones, which is what the smoke sensor needs to exposefire/burglar/emergency/etc. as selectable tones. - No
DURATIONfeature.ConfigurableAttributeSirenwrites a single attribute and reportsTURN_ON | TURN_OFFonly.BasicSirenin both PRs addsDURATIONbecause it can passwarning_durationthroughissue_start_warning; an attribute-write siren has no equivalent. That's fine as a documented limitation for write-only-attribute devices, but it does mean any quirk authored against this PR loses the duration knob. - Inverter semantics that don't really apply. The PR body already flags this: copying the
force_inverted/invert_attribute_nameplumbing fromConfigurableAttributeSwitchcarries baggage that's odd on a siren. #668 inherits the sameSwitchMetadata-based class, so it doesn't fix this either, but it's worth keeping in mind ifConfigurableAttributeSirenstays at all — theEnumSirenpath sidesteps it.
API shape (dmulcahey's comment)
dmulcahey's question about .switch(..., entity_platform=EntityPlatform.SIREN) reading awkwardly to quirks authors lands harder here than on #668. With only the switch-based path, every siren quirk has to write .switch(...)-to-make-a-siren, which is exactly the kind of thing that confuses contributors browsing the registry. #668 still has the same call shape for the SwitchMetadata-backed variant, but most siren quirks for tone-capable devices will naturally use .enum(..., entity_platform=EntityPlatform.SIREN) instead, which is at least closer to what the underlying data model actually is (a set of named tones). If the .switch_from_… / siren_from_switch / entity(entity_cls, …) builder-method idea from that thread is worth pursuing, it's better designed once over #668's wider entity surface.
Testing note
As the PR body says, this still needs the EntityPlatform.SIREN = "siren" addition in zigpy. With the currently-released zigpy, the three test_siren_configurable_attribute* tests fail at collection with AttributeError: type object 'EntityPlatform' has no attribute 'SIREN'. Not blocking for a draft — just noting it's load-bearing on the unmerged zigpy side.
No inline comments since this codepath is being re-reviewed in #668. Happy to close this if you'd like to consolidate effort on #668.
DRAFT. TODO: Requires zigpy change adding
SIREN = "siren"toEntityPlatform.Based on:
Proposed change
This allows quirks v2 entities to provide
entity_platform=EntityPlatform.SIRENfor.switch()entities to create a siren entity on the HA side, instead of a switch entity.Additional information
A lot of the logic and tests are copied from the switch entity, but there's no nice way around that. A downside of using
SwitchMetadatais that we also support the "inverter attribute" logic which is not really necessary in all cases.. I thought of just ignoring that for the siren platform, but it's kept in here for now.The logic is more similar to
ConfigurableAttributeSwitchand its tests then.It's a bit weird doing
.switchto create a siren entity, but it's very similar to what we do with.enumto crate an enum sensor entity when providing a differententity_platform(andentity_typedue to HA restrictions).