Skip to content

Comments

Add basic siren#665

Draft
TheJulianJES wants to merge 4 commits intozigpy:devfrom
TheJulianJES:tjj/siren_exposed_feature
Draft

Add basic siren#665
TheJulianJES wants to merge 4 commits intozigpy:devfrom
TheJulianJES:tjj/siren_exposed_feature

Conversation

@TheJulianJES
Copy link
Contributor

@TheJulianJES TheJulianJES commented Feb 21, 2026

DRAFT. Steals abstract base class siren changes from #662.
It's easier to look at the diffs from the individual commits.

TODO: Move "siren_basic" string as constant to zha-quirks, so we can later import it.
TODO: Regenerate diagnostics (mostly due to class name change in latest commit).
TODO: Regenerate diagnostics with quirks bump.

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
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
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

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.

2 participants