Skip to content

Add quirks v2 siren entity based on SwitchMetadata#667

Draft
TheJulianJES wants to merge 7 commits into
zigpy:devfrom
TheJulianJES:tjj/siren_v2_quirk
Draft

Add quirks v2 siren entity based on SwitchMetadata#667
TheJulianJES wants to merge 7 commits into
zigpy:devfrom
TheJulianJES:tjj/siren_v2_quirk

Conversation

@TheJulianJES
Copy link
Copy Markdown
Contributor

@TheJulianJES TheJulianJES commented Feb 21, 2026

DRAFT. TODO: Requires zigpy change adding SIREN = "siren" to EntityPlatform.

Based on:

Proposed change

This allows quirks v2 entities to provide entity_platform=EntityPlatform.SIREN for .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 SwitchMetadata is 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 ConfigurableAttributeSwitch and its tests then.

It's a bit weird doing .switch to create a siren entity, but it's very similar to what we do with .enum to crate an enum sensor entity when providing a different entity_platform (and entity_type due to HA restrictions).

@dmulcahey
Copy link
Copy Markdown
Contributor

Maybe we add another method to the builder to make it clearer we are making a siren ex: siren_from_switch? or if this doesn't generalize well maybe something more generic like entity(entity_cls, ...) and we specify the actual class being created and what is needed? I feel like what is in the test for the builder:

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.

Copy link
Copy Markdown
Collaborator

@zigpy-review-bot zigpy-review-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SwitchMetadata boolean-mapped to two attribute values can only flip the alarm on/off; it can't surface the enum of available tones to HA. #668's EnumSiren reads each enum member as an entry in available_tones, which is what the smoke sensor needs to expose fire/burglar/emergency/etc. as selectable tones.
  • No DURATION feature. ConfigurableAttributeSiren writes a single attribute and reports TURN_ON | TURN_OFF only. BasicSiren in both PRs adds DURATION because it can pass warning_duration through issue_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_name plumbing from ConfigurableAttributeSwitch carries baggage that's odd on a siren. #668 inherits the same SwitchMetadata-based class, so it doesn't fix this either, but it's worth keeping in mind if ConfigurableAttributeSiren stays at all — the EnumSiren path 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants