Add Philips Hue contact sensor quirk

[mguaylam]

Proposed change

This adds the custom Philips Hue contact cluster with all its custom attributes.
A v2 quirk entity for the binary temper sensor is also added.

Additional information

Needed by this PR: zigpy/zha#238
Fixes: #3314

Checklist

• The changes are tested and work correctly
• pre-commit checks pass / the code has been formatted using Black
• Tests have been added to verify that the new code works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.39%. Comparing base (9366c42) to head (e1e01d3).
Report is 1 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3432      +/-   ##
==========================================
+ Coverage   89.37%   89.39%   +0.01%     
==========================================
  Files         308      309       +1     
  Lines        9990    10004      +14     
==========================================
+ Hits         8929     8943      +14     
  Misses       1061     1061              

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

[mguaylam]

As discussed here and on the HA Discord, I am advocating in handling the functions of the sensor entirely based on the stateful manufacturer specific cluster as it was intended by Signify.

It is a breaking change.

What the device-handler does

• Implement the custom cluster with the contact and tamper attributes. I did not implement their timers as it has no real world usage to HA. Could be considered in the future.
• Implement the On/Off cluster in a custom cluster to make it still bindable.

Reasons supporting this

• It's what the manufacturer intended. The On/Off cluster even if usable, is only meant for binding.
• Any state change during a network shutdown or failure will fail to be reported. Being a security device this feature is important. I have lived and still live with not knowing if my doors are closed or opened after a power outage and often then not, forced to cycle the state of the sensor to get it back to a known state (Xiaomi example here).
• Failure of the network to deliver the message may cause an out of sync state of the attribute (I believe, if it is not corrected by the maximum report period).

Other reasons supporting my decision

• I tried several contact sensor (IKEA, Xiaomi, Konke and Sonoff) and got tired of their poor reliability : broken reporting, fast battery drain, out of sync attribute, dropping off the network.. My most reliable devices are Philips Hue's so I decided to spend the money to purchase this device and until now, it does deliver on the promise.
• dresden-elektronik did the integration similar as me and came to a similar conclusion : Hue Secure Contact Sensor dresden-elektronik/deconz-rest-plugin#7226. I know ZigBee2MQTT did not implement this cluster at all [New device support]: Philips Hue Secure contact sensor (SOC001) Koenkk/zigbee2mqtt#19054 but I think it was due to just not knowing what it incorporate as the device does not reply with a list of attributes.
• I still think it's the right time to implement this breaking change as the device is brand new on the market. Only purchasable in Canada for 2 months now. Combined to the very high price, I don't think many people have it in their hands and I'm the first one to ask and propose a device handler to enable the tamper feature demonstrating the still low usage of the device.

I hope this proposal will convince to consider this pull request.

Other notes

• This device-handler v2 depends on Add entity_type to quirks v2 binary sensor and number zigpy#1487
• Bind and set up reporting for Hue contact sensor zha#238 depends on this pull request.

[TheJulianJES]

I don't think we should/can do this. We're replacing a ZCL cluster and are also changing the ep_attribute here.
IIRC there are places where ZHA expects the ep_attribute to be unchanged for a standard ZCL cluster.
Not subclassing OnOff (and its AttributeDefs) is also not good.

[TheJulianJES]

Like I mentioned on Discord, I'd remove the custom OnOff cluster and custom "contact" binary sensor for now. The attribute for it can stay.
Then, we could merge this PR already and also the ZHA PR afterwards that binds the custom Philips contact cluster.

We can still consider on how we want to deal with the "contact" binary sensor then. It's not trivial, as breaking changes are always bad (especially for a security device where the behavior might go unnoticed in automations until it's too late).
We need to find a good solution here, because we could theoretically avoid a breaking change. It's not easy though.

The parts where the change would be breaking:

• replacing the existing contact binary sensor
• re-pairing or reconfiguration of the sensor is required to bind the new Philips contact cluster to the coordinator

[TheJulianJES]

Also, to re-iterate some of the points I mentioned on Discord:

It's what the manufacturer intended.

Tuya intends that we shouldn't work with its devices all.. 😄

The On/Off cluster even if usable, is only meant for binding.

Well, we're doing exactly that. ZHA requests the OnOff cluster to be bound to the coordinator. We're receiving the "on"/"off" commands then and set the entity state in HA based on that.

Any state change during a network shutdown or failure will fail to be reported.

Even for a binary sensor based on the custom attribute, it wouldn't fix this. If ZHA is restarted after a power outage, we only poll some attributes of mains-powered devices (based on what's defined in the ZHA cluster handler).
You'd need to run homeassistant.update_entity for the new binary sensor entity to (hopefully) poll new state once (after ZHA has started and the network has settled down).

Failure of the network to deliver the message may cause an out of sync state of the attribute

But the same behavior would happen for the binary sensor based on the custom attribute. ZHA doesn't poll entities by default, especially not ones on battery-powered devices.
You'd need to regularly run homeassistant.update_entity to poll the device then.

---

The only benefit that I'm seeing for such a breaking charge (or large change if we make it not breaking) is that you can poll the entity state (assuming the device is always somewhat awake, but that does seem to be the case according to what you mentioned).
The majority of people isn't going to use homeassistant.update_entity to poll the entity state regularly.

So just to repeat myself, I'd change the scope of this PR to only include the tamper entity for now (and the custom contact cluster with both custom attributes defined). We can also merge the ZHA PR then. Everyone who installs/pairs a Hue sensor after a release with those changes will at least have the custom Hue cluster bound (and a tamper entity), so if we make the binary sensor switch in the future (whatever that may look like), they won't have to reconfigure or re-pair the device at least.

[TheJulianJES]

Another thought I'm not sure works (ideally for a future PR):
We might be able to just catch attributes reads on the output OnOff cluster, redirect them to custom contact cluster, then redirect the result to the on_off attribute.
It's not really a clean way, but it would keep using the ZHA entity for the OnOff cluster, whilst allowing to poll the sensor state.

This won't solve the issue of needing to re-pair the sensor though, unless we still listen to the actual OnOff on/off commands and only use the custom contact cluster when the OnOff on_off attribute is trying to be read/polled.

[mguaylam]

I just want to say first, thank you so much for following up. I recognise the quality of your free time spent on this project and the effort you took to assist me.

For this reason I was about to submit as you proposed because I don't want to drag you forever for something with mild consequences.

But I wanted to make sure the consequences we're as you described so it can be at worst, left like this.

I did 2 tests :

1. No quirk, left as is using the on_off attribute : I produce a network failure and change the contact state while the network is offline.
   Result : After an hour, there never was a report back of the open state, for Home Assistant it would remain closed based on the cached attribute state forever until the sensor is interacted with.

2. With the quirk, using the custom cluster and attribute reporting configuration: I produce a network failure and change the contact state while the network is offline.
   Result : After 15 minutes since the last tried report, it produce a new one updating the attribute and now display the correct state by itself in Home Assistant.

This also apply to a transmission failure, I've seen the sensor fail 1 or 2 times to transmit on the network but tried again right after.

And the reason is simple enough :
There is no reporting on the on_off attribute, only a binding on the cluster :
(I did the report without the quirk but forgot to write it down, the result was the same)

result_conf:
  - cluster: On/Off
    cluster_id: "0x0006"
    ep: 2
    attr_id: "0x0000"
    direction: 0
    status: 134 #(not supported)
    attr: on_off

Where on the custom cluster :

result_conf:
  - cluster: Philips contact cluster
    cluster_id: "0xFC06"
    ep: 2
    attr_id: "0x0100"
    direction: 0
    status: 0
    type: "0x30"
    min_interval:
      - 0
    max_interval:
      - 900
    reportable_change:
      - null
    attr: contact

The on_off cluster in this case is a client type and does not seem to support reporting configuration but will report a change on the bind device. I don't know if it's per specification but seems logical to not report a state multiple times if it's to command a light as you might have turned off the light with a remote meanwhile.

Also it's a light sleeper, it does respond to commands within the timeout of the network.

You're the maintainer and I want to respect that. If the breaking change is unacceptable I'll submit without it.
But I want to convince you one last time as I really believe having a device with proper reporting is more important and would make the device more reliable. A change of state during ZHA being offline in the current version will never be known to ZHA.

[TheJulianJES]

With the quirk, using the custom cluster and attribute reporting configuration: I produce a network failure and change the contact state while the network is offline.
Result : After 15 minutes since the last tried report, it produce a new one updating the attribute and now display the correct state by itself in Home Assistant.

Ok, this is good to know.

But I want to convince you one last time as I really believe having a device with proper reporting is more important and would make the device more reliable.

Yeah. Using the OnOff cluster like done in this PR to block the entity from generating isn't ideal. I have an idea on how to make this potentially non-breaking, but if it needs to be a breaking change, we should consider how we want to block the OnOff entity from being created: directly in ZHA? allow quirks to properly disable individual entity generation? just use a LocalDataCluster and make the on_off attribute unsupported, so ZHA doesn't create? I believe that would make binds non-functional though.. But marking just the on_off attribute as unsupported should be fine.
... or have both entities (temporarily)?

Just redirecting attribute reports from the custom Hue cluster to the OnOff cluster would get those 15 minute interval reports to come through. Depending on the implementation, it may also allow for manual polling.
This is something I want to explore soon-ish.

Anyways, like mentioned before: We can basically instantly merge this PR for the tamper entity if the contact entity and custom OnOff cluster is taken out for now. At least that should land in the next major HA release and so people will already have their Hue contact cluster bound when pairing such a device (since we can also merge the ZHA PR then).
If you want, you can then open another PR after this one is merged that adds the custom contact sensor entity back.
There will be some further discussion and we can see how to best make that change. That very likely won't be in the next major HA release though.

[mguaylam]

Using the OnOff cluster like done in this PR to block the entity from generating isn't ideal.

Absolutely. It was a crude way to get to the result. There's surely a proper way to do it.

I have an idea on how to make this potentially non-breaking, but if it needs to be a breaking change, we should consider how we want to block the OnOff entity from being created: directly in ZHA? allow quirks to properly disable individual entity generation? just use a LocalDataCluster and make the on_off attribute unsupported, so ZHA doesn't create? I believe that would make binds non-functional though.. But marking just the on_off attribute as unsupported should be fine.
... or have both entities (temporarily)?

I agree to look at that in another PR so we at least start to bind and report as soon as possible.

That very likely won't be in the next major HA release though.

I'm ok with that. As long as we have a reliable sensor at the end.

Let's get that merge and I could learn how to do tests at some point.

[TheJulianJES]

This should be good. Thanks!

[TheJulianJES]

Let's get that merge and I could learn how to do tests at some point.

We can't really do a lot in terms of unit tests for this quirk. Entity creation only happens in ZHA/HA, so there's no real code to test here.