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?

[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).

[burmistrzak]

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

That's actually quite interesting because it seems to confirm behavior that I've seen before elsewhere. 👌

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

Only manufacturerIDs and the read targets are/can be device-specific. The trigger commands that Z2M is listening for are the same for all enabled lights.

[Koenkk]

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

This looks like a nice solution and allows for a lot of flexibility.

[burmistrzak]

I just realized that there's no retry mechanism for failed polls. 😳

Just had one older Atmel-based Hue fixtures' OnOff state in HA get stuck because the MCU was seemingly too busy executing a scene... LMAO. No wonder Hue switched to Silabs.

@andrei-lazarov Do you still have Atmel-based Hue lights?

Another cool feature would be to trigger/publish a scene's state when we notice a scene was recalled via direct bindings. This would reduce polling associated with scenes to zero.
Thoughts?

[andrei-lazarov]

I have only one Atmel bulb

[burmistrzak]

I have only one Atmel bulb

@andrei-lazarov Lucky you! 😜 Have you ever had it act up or be unreliable?

[andrei-lazarov]

Yes! It took the whole network down with its typical address 0 conflict. So I put it back in the box 🙂

[burmistrzak]

Yes! It took the whole network down with its typical address 0 conflict. So I put it back in the box 🙂

@andrei-lazarov Oh no... If I'm correct and the EMI shield of the Hue Engine is indeed hiding an Atmel chip...
We're kinda cooked over here. About ten Hue fixtures with that board inside... 😳

Unfortunately, the images from the FCC filings are low resolution and no chip markings are visible.

[burmistrzak]

Folks, after some minimal invasive surgery, I was able to confirm that the Hue 100B-010E platform is indeed based on the Atmel ATSAMR21E family!

This means that all Hue fixtures that contain a Hue Engine v1 are quite underpowered, even when compared to the first generation of EFR32-based Hue Connect modules.
So it makes total sense that they're only ZLL certified. These modules can barely handle more than one transaction at a time, causing spurious timeouts here and there.

In an ideal world, Signify would offer replacement parts so that fixtures could be easily repaired and upgraded. The difference between a ZLL and Z3 fixture is literally just the Hue Connect module. Oh well...

@andrei-lazarov And yes, after a thorough clean and repair job, the fixture incl. module is working just fine. ☺️

[github-actions]

This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days