feat: Poll manuSpecificPhilips2 for Hue lights

[burmistrzak]

As mentioned in Koenkk/zigbee-herdsman-converters#12256 and #31960, Z2M automatically reads (i.e. polls) specific attributes (brightness, state, etc.) when a device is being controlled via Zigbee Bindings.

Instead of reading only a limited amount of standard attributes, the Philips Hue-exclusive manuSpecificPhilips2 cluster can be used to reduce airtime and improve responsiveness by fetching the complete device state with a single command.

To my knowledge, the only Hue device that doesn't feature this cluster is the Hue Smart Plug, but it is capable of reporting anyways.

cc. @andrei-lazarov

[andrei-lazarov]

Cool! Can you explain the logic behind polling a little bit, to make sure I understand it?

Is it like this?
When Z2M sees "turn on group x" message from a remote, it reads the onOff state of each group member, just in case they don't support reporting. And we make the distinction by manufacturer?

[burmistrzak]

@andrei-lazarov Correct. Same thing for device-to-device bindings. 😊

[Koenkk]

LGTM! Could you fix the CI?

[burmistrzak]

LGTM! Could you fix the CI?

@Koenkk I was about to do that, but wouldn't it be better to confirm before polling that the manuSpecificPhilips is actually supported by the device?

[burmistrzak]

I've been thinking: Why not use the state of the Hue Native Control device option to dynamically override the poll target?
Because as it stands, the override would automatically take effect for every supported Hue light, regardless of user preferences.

@Nerivec Would that be doable with existing APIs and also actually be testable?

[Nerivec]

Not sure how this was implemented. I assume there's a meta value or something in the device's ZH layer that's being stored by the converter?
You should be able to access anything in ZH that ZHC stored using device.zh.xyz.
e.g.:

zigbee2mqtt/lib/extension/configure.ts

Line 70 in cfc7ab8

if (data.device.zh.meta.configured !== undefined) {

[burmistrzak]

@Nerivec Would you mind sanity checking my test harness? 😊

I think the issue was that hue_native_control is an endpoint-level meta option... Duh!

[burmistrzak]

@andrei-lazarov Would you be able to test this PR on your end?

[Nerivec]

Actually, looking at the philips branch in the context of the whole poll fn, is this the right place to do this?
It would end up triggering for every polls, seems weird?
I'm not familiar with any of the hue stuff, so, could be wrong, but my thinking was, it should be done as an if/else directly around for (const poll of polls) { (i.e. if philips, send to custom cluster, else, enter for polls loop).
Also doesn't fit very well with the POLL_ON_MESSAGE.indexOf(poll) logic bit.
Seems technically you even could go higher, since the only requirement is the toPoll logic, rest is not used in the philips branch. And it might actually end up being more of a problem than anything (checks like !endpoint.supportsInputCluster(poll.read.cluster) don't make sense if sending to custom cluster).

[burmistrzak]

@Nerivec Ok, how about a separate branch incl. debounce for manuSpecificPhilips2 right after the (const poll of polls) { block?

Edit: Actually, triggering the Hue branch for every poll is correct. We're simply swapping out the cluster/attribute used during polling to provide more accurate/efficient state updates for supported Hue lights with hue_native_control enabled.

[burmistrzak]

I think we're switching to the Hue-specific cluster as soon as possible without having to refactor the entire poll method.

Also, lots of typos in my commit messages today, huh? 😅

[Nerivec]

I thought the intent was to make one request instead of many? This ends up same as before, just a different cluster used, doesn't it? Technically less attributes (bit smaller payload), but same number of requests.

Why revert the block change?

@Koenkk isn't there a way to keep this custom stuff in ZHC? Poll logic override like custom time read or something?
As soon as we start introducing ZHC stuff in ZH/Z2M we end up having to type-assert, workaround & whatnot 🥵

[burmistrzak]

I thought the intent was to make one request instead of many? This ends up same as before, just a different cluster used, doesn't it? Technically less attributes (bit smaller payload), but same number of requests.

Yes, just a different cluster (for now).
However, I'm currently cooking up a patch that produces only one manuSpecificPhilips2.state request per endpoint.

Why revert the block change?

See above. 😅

@Koenkk isn't there a way to keep this custom stuff in ZHC? Poll logic override like custom time read or something? As soon as we start introducing ZHC stuff in ZH/Z2M we end up having to type-assert, workaround & whatnot 🥵

AFAIK no... But would be a much clearer approach tho.

[burmistrzak]

@Nerivec Let's put this PR on hold until we have a more maintainable way of implementing such overrides from ZHC. Cramming manufacturer-specific code into ZH/Z2M isn't the way to go. 😊

[Nerivec]

About the revert, that wasn't strictly related to this PR, just a way to avoid executing the logic if we're not going to enter the if anyway (not needed otherwise).

---

About moving this to ZHC, I think we can just introduce something on the definition, since this used by Z2M, not ZH, we don't have to introduce anything at ZH level.
Something like onEvent.

zigbee2mqtt/lib/extension/onEvent.ts

Line 85 in cfc7ab8

await device.definition?.onEvent?.(event);

onBindPoll?: (device: Zh.Device, endpoint: Zh.Endpoint) => Promise<void>

Note: just a quick overview, need to dig into the requirements from Z2M, and the possible needs from ZHC.

[burmistrzak]

About the revert, that wasn't strictly related to this PR, just a way to avoid executing the logic if we're not going to enter the if anyway (not needed otherwise).

Thanks for clarifying!

About moving this to ZHC, I think we can just introduce something on the definition, since this used by Z2M, not ZH, we don't have to introduce anything at ZH level. Something like onEvent.

Sounds like an elegant solution to me! 💅

Note: just a quick overview, need to dig into the requirements from Z2M, and the possible needs from ZHC.

I assume the proposed onBindPoll would only be called once for each POLL_ON_MESSAGE command received?

[Koenkk]

@Koenkk isn't there a way to keep this custom stuff in ZHC? Poll logic override like custom time read or something?
As soon as we start introducing ZHC stuff in ZH/Z2M we end up having to type-assert, workaround & whatnot 🥵

Then I would propose to add a "whatToPoll" function in index.ts which returns the attributes to read for a given command.

[burmistrzak]

@Nerivec Ok, so we now got a rough idea of how to implement this. ☺️

• New onBindPoll function in ZHC index.ts similar to onEvent.
• if (endpoint.meta?.options?.hue_native_control === true) in onBindPoll enables hue_native_control override.
• No onBindPoll in device definitions, only centralized in ZHC index.ts.

But how do we go from here?
Where would we call onBindPoll in Z2M bind.ts? And what would we override exactly? Do we replace POLL_ON_MESSAGE? 🤔

[Nerivec]

Then I would propose to add a "whatToPoll" function in index.ts which returns the attributes to read for a given command.

You mean moving the whole POLL_ON_MESSAGE?

I was thinking having it as a possible override mechanism that definitions can declare. We need a way to declare, "this is the default" (current), and "this is what this device should do" (intent of this PR).

[burmistrzak]

I was thinking having it as a possible override mechanism that definitions can declare. We need a way to declare, "this is the default" (current), and "this is what this device should do" (intent of this PR).

Leaving it up to individual definitions doesn't make much sense, IMHO, because that hue_native_control override affects (when enabled) an entire group of devices the same way.

[Nerivec]

Just would need to add the entry in the same modern extend as the hue_native_control.
Makes sense to avoid this logic entirely for unrelated devices (it's not expensive per-say, but it's pointless for any other brand/model).

[burmistrzak]

Just would need to add the entry in the same modern extend as the hue_native_control. Makes sense to avoid this logic entirely for unrelated devices (it's not expensive per-say, but it's pointless for any other brand/model).

Good point.
I think we can simply override POLL_ON_MESSAGE with the object provided by onBindPoll and fallback to the default when nothing is returned?

[Nerivec]

Depends if we want something a bit more flexible (possible future ZHC uses). A function that takes device+endpoint and does what it needs to do instead of the default in Z2M is more future-proof (avoids refactoring 😁).

[burmistrzak]

Depends if we want something a bit more flexible (possible future ZHC uses). A function that takes device+endpoint and does what it needs to do instead of the default in Z2M is more future-proof (avoids refactoring 😁).

A more general purpose override function sounds quite useful, but implementation will be a bit more tricky. 😅

[andrei-lazarov]

I had an idea that I want to share, a bit off-topic from the Hue cluster.
Maybe we can avoid keeping a list of non-reporting models/brands:

if (user enables binding):
  try: enable reporting(attr)
  catch (unreportable_attribute or table_full): enable polling(attr)

Would you be able to test this PR on your end

Sorry, will do!

[burmistrzak]

I had an idea that I want to share, a bit off-topic from the Hue cluster. Maybe we can avoid keeping a list of non-reporting models/brands:

if (user enables binding):
  try: enable reporting(attr)
  catch (unreportable_attribute or table_full): enable polling(attr)

@andrei-lazarov Oh, that's an interesting idea! Having less hardcoded stuff around is always good.
Are we sure that every device responds with an appropriate error?

Would you be able to test this PR on your end

Sorry, will do!

No need to rush. 😊
The PR works, but we're not quite satisfied with the overall implementation.

A general purpose override function that allows ZHC to alter device-specific behavior in Z2M would be the most flexible solution. However, it's not yet clear how exactly such overrides would be called from Z2M extensions.

I'm in favor of a compromise that moves the override into ZHC but is limited in scope, i.e. not general purpose.

[Nerivec]

However, it's not yet clear how exactly such overrides would be called from Z2M extensions.

The same as onEvent, like I mentioned above. Can just call anything from the definition, with whatever args are needed.
Could look like this (greatly simplified):

// in ZHC definition type
onBindPoll?: (device: Zh.Device, toPoll: Set<Zh.Endpoint>: existingPollers: Map<string, () => void>) => Promise<void>

// example use in ZHC definition
...
onBindPoll: (device, toPoll, existingPollers) => {
    for (const endpoint of toPoll) {
        const key = ...;
        let poller = existingPollers.get(key);

        if (!poller) {
            poller = debounce(async () => {
                try {
                    await endpoint.read<"manuSpecificPhilips2", ManuSpecificPhilips2>("manuSpecificPhilips2", ["state"]);
                } catch (error) {
                    logger.error(`Failed to poll ${readAttrs} from ${device.ieeeAddr} (${(error as Error).message})`);
                }
            }, 1000);

            existingPollers.set(key, poller);
        }

        poller();
    }
}
...

// in Z2M
#bindPollers = new Map<string, () => void>(); // replaces `bindDebouncers`

if (data.device.definition?.onBindPoll) {
    const toPoll = this.#getEndpointsToPoll(data.device.zh.endpoints, data.groupID);

    await data.device.definition.onBindPoll(device, toPoll, this.#bindPollers);
} else {
    // default Z2M logic
}

For the real thing, need to pass some of eventdata.DeviceMessage to ZHC to be able to do all the logic needed.
Should also make a function out of the gathering of endpoints to poll (Set<zh.Endpoint>), so it can be easily called in the two branches.
Should also make debounce an explicit dependency of ZHC (it's already there, but not explicit).

[burmistrzak]

Quite involved for such a "simple" enhancement. 😅

I'm a little bit concerned about the potential overhead caused by the back and forth (ZHC/Z2M) when compared to a static POLL_ON_MESSAGE, tbh.
Last thing I want is to introduce additional latency.

But if everything checks out on that end, I can certainly give it a shot.

[Nerivec]

It's only a function call on the definition, if defined, barely any impact at all.
Actually, it's likely that function will have a static logic (custom approach) in a lot of cases instead of the auto-parsing from POLL_ON_MESSAGE, so, could end up faster than the plain default logic branch. 😅
Auto-parsing from objects takes less code, but is never as good as a plain function call (e.g.)

@Koenkk thoughts on #32078 (comment) ?

Seems a lot of stuff in POLL_ON_MESSAGE is manuf-spe/custom clusters, might be worth moving some of that to ZHC as well, better typing & co (can do it over time, like we did custom clusters).