Skip to content

Add basic siren#665

Merged
TheJulianJES merged 12 commits intozigpy:devfrom
TheJulianJES:tjj/siren_exposed_feature
Mar 25, 2026
Merged

Add basic siren#665
TheJulianJES merged 12 commits intozigpy:devfrom
TheJulianJES:tjj/siren_exposed_feature

Conversation

@TheJulianJES
Copy link
Copy Markdown
Contributor

@TheJulianJES TheJulianJES commented Feb 21, 2026

Proposed change

This adds a "basic siren" to ZHA, which does not expose tone + volume + strobe features. The matching is done via an "exposed feature" (formerly known as quirk_id).

Weirdly enough, all Zigbee sirens I've encountered so far do not expose these features. I'm wondering if it makes sense to flip this around, so have a "basic siren" by default, and "advanced siren" with a quirk. I'll need to look at diagnostics for that.
EDIT: It does look like some sirens we have in our diagnostics are intended to expose the more advanced options (looking at Z2M), so I probably wouldn't risk flipping it around now and feature hinting existing advanced sirens.

Additional information

Implementation

The latest commit 466eb6b (this PR) also introduces a BaseZclSiren (based on BaseSiren).
Both AdvancedSiren and BasicSiren then inherit from the BaseZclSiren. This avoids some code duplication, but I'm not sure if it's fully worth it. If not, I can just revert the commit. Thoughts on this are appreciated.

What it does

So far, this PR should remove these options:
image

Select entities

But not the useless non-ZCL select entities below. This would be easier to do with the exposed feature matching if we assume "basic" is the default, not "advanced". If we don't do that, we'll have to create separate entity classes and override is_supported OR check is_supported in the normal classes. (I think both should work?)

Alternatively, we could also add not_exposed_features to our discovery. It should be a very small change. Would also like opinions on this.

Example of the useless select entities:
image

I think it makes the most sense to have these entities be removed in a follow-up PR, since it's a different platform. EDIT: See:

@puddly
Copy link
Copy Markdown
Contributor

puddly commented Feb 23, 2026

Both AdvancedSiren and BasicSiren then inherit from the BaseZclSiren. This avoids some code duplication, but I'm not sure if it's fully worth it. If not, I can just revert the commit. Thoughts on this are appreciated.

I'm in favor of ZCL base class entities inheriting from the abstract entity. There are few platforms where it actually helps (select is the only other one that comes to mind) but it's the naming convention I want to stick to to differentiate ZHA-discovered "spec" entities versus quirk entities or entirely custom aggregate entities (e.g. Tuya).


But not the useless non-ZCL select entities below. This would be easier to do with the exposed feature matching if we assume "basic" is the default, not "advanced". If we don't do that, we'll have to create separate entity classes and override is_supported OR check is_supported in the normal classes. (I think both should work?)

This is an interesting API gap. We need entities whose discovery depends on the existence of another entity, right? This requires some discovery restructuring, because we right now assume all entities will be discovered in a single pass and thus they are assumed to be order-independent. Right now, we treat the entity itself as the smallest unit for encompassed functionality but I guess we could also think of this as a special case of entity "groups": where groups of entities are created by discovery, not just one? That way, the group of entities has shared discovery logic?


Alternatively, we could also add not_exposed_features to our discovery. It should be a very small change. Would also like opinions on this.

We currently have only a single not_ filter (not_profile_device_types) and it's used only for a single entity: Opening. This is something I'd like to eventually remove. IMO negative filters are not a good pattern.

@TheJulianJES
Copy link
Copy Markdown
Contributor Author

I'm in favor of ZCL base class entities inheriting from the abstract entity.

With the current approach, we have the abstract BaseSiren inheriting from PlatformEntity.
Then, BaseZclSiren (also abstract) inherits from BaseSiren.
Both the final AdvancedSiren and BasicSiren entities inherit from BaseZclSiren, so that would fulfill what you're saying? (Or just revert 466eb6b and we're good? Feel free to also push changes to this PR.)

This is an interesting API gap. We need entities whose discovery depends on the existence of another entity, right? This requires some discovery restructuring [...]

Hmm, I didn't even think about linking the select entities to the presence of the BasicSiren entity, but that would work as well, yeah. It might overcomplicate discovery a bit though? We only really need this for the siren select entities.

IMO negative filters are not a good pattern.

I get that but I also feel like it's essentially a cleaner prevent_default_entity_creation, just as a "feature hint" from the quirk, not as multiple separate calls. I have nothing against preventing entity creation from a quirk via an exposed feature. That's essentially not_exposed_features.

Let's assume we want to do the following:

  • The IasZone binary sensor currently turns on when either bit 0 or 1 is set.
  • We want to add an exposed feature (hint) called ias_zone_split. Adding this one line to quirks allows them to signal that multiple entities should be created for the individual ias_zone Alarm1 and Alarm2 bits.
  • ZHA should not create the combined IasZone entity now, but separate ones. How is the matching of the base one prevented? For example like this with an extra not_exposed_features filter: dev...TheJulianJES:zha:tjj/ias_alarm_bits

@TheJulianJES TheJulianJES force-pushed the tjj/siren_exposed_feature branch from fc5c825 to cbc2230 Compare March 25, 2026 03:05
@TheJulianJES TheJulianJES marked this pull request as ready for review March 25, 2026 03:05
Copilot AI review requested due to automatic review settings March 25, 2026 03:05
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.62%. Comparing base (5a353cf) to head (35b0d6c).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #665   +/-   ##
=======================================
  Coverage   97.62%   97.62%           
=======================================
  Files          62       62           
  Lines       10764    10789   +25     
=======================================
+ Hits        10508    10533   +25     
  Misses        256      256           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a “basic siren” entity variant for IAS WD devices that hides tone/volume/strobe controls, selected via a quirk “exposed feature”, and refactors shared IAS WD ZCL logic into a common base class.

Changes:

  • Introduce BasicSiren (feature-hinted) alongside AdvancedSiren, sharing logic via BaseZclSiren.
  • Add a new PlatformFeatureGroup.SIREN to ensure only the highest-priority siren variant is created per device.
  • Extend siren tests and update device snapshot JSON expectations for the new class names/features.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

File Description
zha/application/platforms/siren.py Adds BaseZclSiren, splits siren into AdvancedSiren vs BasicSiren, and adds feature-group prioritization.
zha/application/platforms/init.py Introduces PlatformFeatureGroup.SIREN for siren entity selection.
tests/test_siren.py Adds coverage for basic siren behavior and discovery via exposed feature.
tests/data/devices/*.json Updates expected entity class names/features in device snapshots.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@TheJulianJES
Copy link
Copy Markdown
Contributor Author

Thought I merged this but GitHub is just flaky since hours...

GitHub screenshot showing the merge was blocked for some reason

@TheJulianJES TheJulianJES merged commit 561f4b8 into zigpy:dev Mar 25, 2026
7 checks passed
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