feat: Bind: Retry poll on device timeout

[burmistrzak]

As mentioned in #32078 (comment), Hue fixtures with Atmel-based PCBs tend to get overwhelmed quite quickly when controlled via bindings (for some reason).
Because these fixtures can't report their state at all, we have to poll them for updates.

So when a fixture doesn't respond in time, Z2M is left with a stale state in cache. No good. 😳
This PR adds a retry mechanism (with jitter) to work around this specific issue, giving fixtures up to 1500 ms of rest before a second attempt is made.

The number of total attempts per attribute is currently fixed at 2, but could be made configurable in the future.

[Nerivec]

I think it's worth mentioning that these old Hue Atmel devices have been hit by terrible security flaws. While known ones were presumably patched (assuming users updated firmware), between that and the fact they are old, I don't think these should be "promoted" or that we should spend much time on fixing or working around their other flaws. They are clearly no good on multiple fronts, and cause for great concerns... 😰

About the PR itself, need to dig deeper in how that works out in practice with the potential for multi-timeouts, the delays and the debouncing. It mixes several timer-based aspects. At first glance, I'd say that at least introduces a bigger potential for concurrency & race issues (was present before but less likely with single-await).

[burmistrzak]

I think it's worth mentioning that these old Hue Atmel devices have been hit by terrible security flaws. While known ones were presumably patched (assuming users updated firmware), between that and the fact they are old, I don't think these should be "promoted" or that we should spend much time on fixing or working around their other flaws. They are clearly no good on multiple fronts, and cause for great concerns... 😰

@Nerivec I see where you coming from and share your concerns. These "older" Hue fixtures also do not feature install codes, making them overall less secure.
Unfortunately, integrated Hue fixtures (while pretty) are quite expensive and cannot (officially) be retrofitted. So folks are more or less stuck with their ZLL models, unable to obtain replacement PCBs that are Z3-certified. 🤬

About the PR itself, need to dig deeper in how that works out in practice with the potential for multi-timeouts, the delays and the debouncing. It mixes several timer-based aspects. At first glance, I'd say that at least introduces a bigger potential for concurrency & race issues (was present before but less likely with single-await).

I tried keeping concurrency issues in mind and took inspiration from existing retry mechanisms in Z2M.
Pivoting to a single retry should reduce the risk of unintended side effects. 🤞

Edit: IMHO, devices powered by anything below a EFR32MG21 can't be recommended for serious deployments. Sadly, getting such details before purchasing isn't trivial...

[Koenkk]

Since this is very specific for a certain device (Hue Atmel), I think it's better to handle this in the device definition onEvent?

[burmistrzak]

Since this is very specific for a certain device (Hue Atmel), I think it's better to handle this in the device definition onEvent?

@Koenkk Well, it's a generic retry mechanism that should work with every light that's too slow or misbehaving. 😊
So we only make a second attempt if the attribute poll times out.

[Nerivec]

It doesn't solve what I mentioned before though. We're going to see concurrency & race condition problems with this. Probably already have a few (debounce vs potential timeout), but limited due to letting the poll timings basically do the "retry" (eventually).
I don't think it's worth increasing these risks for all, to "solve" a short period of staleness for a few devices that generally misbehave.

[burmistrzak]

It doesn't solve what I mentioned before though. We're going to see concurrency & race condition problems with this.

@Nerivec Guess I'll have to fixed that then. 😅

Probably already have a few (debounce vs potential timeout), but limited due to letting the poll timings basically do the "retry" (eventually).

But there's no "retry" for single command (OnOff, Scene, etc.) actions...
Example:

1. Group of lights gets turned off via bindings (e.g. remote) and/or a scene. All lights turn off.
2. Z2M polls the group members for their current state.
3. One light doesn't respond in time (for whatever reason).
4. Result: Z2M (and connected systems) assume that one light is still turned on (it's not).
5. Result: Automations that expect a specific light state (e.g. off) are blocked.

I don't think it's worth increasing these risks for all, to "solve" a short period of staleness for a few devices that generally misbehave.

See above.

[Nerivec]

But there's no "retry" for single command (OnOff, Scene, etc.) actions...

I meant the logic re-runs every time we see a message from the device, which should clear the stale state automatically (eventually).
Plus there are retries with jitter in lower level sendZclFrameToEndpoint (at least for zstack/ember).

I don't think a retry is doable in current logic, at least not internally to the debounce; any increase in potential time-spent increases the risks mentioned.
Realistically, we can't introduce something that could potentially affect everyone negatively, if it might fix a couple devices.

[burmistrzak]

I meant the logic re-runs every time we see a message from the device, which should clear the stale state automatically (eventually).

@Nerivec Sorry, but I'm not following... 😳
How would that work exactly? AFAICT, we're only polling when we see e.g. dimmer commands, no?

zigbee2mqtt/lib/extension/bind.ts

Lines 570 to 577 in 756a824

	/**
	* This method poll bound endpoints and group members for state changes.
	*
	* A use case is e.g. a Hue Dimmer switch bound to a Hue bulb.
	* Hue bulbs only report their on/off state.
	* When dimming the bulb via the dimmer switch the state is therefore not reported.
	* When we receive a message from a Hue dimmer we read the brightness from the bulb (if bound).
	*/

Plus there are retries with jitter in lower level sendZclFrameToEndpoint (at least for zstack/ember).

That's good to know.

I don't think a retry is doable in current logic, at least not internally to the debounce; any increase in potential time-spent increases the risks mentioned. Realistically, we can't introduce something that could potentially affect everyone negatively, if it might fix a couple devices.

Valid point, certainly. Once the debounce fires and the first poll is issued, it's over anyways.

What about introducing a sort of backlog outside the poll method that would collect a certain number of failed read commands to retry later?
Any other ideas (besides replacing 10+ Hue fixtures)? 😅

[burmistrzak]

Ok, so I was able to capture what a missing poll response looks like in Wireshark:

As you can see, the yellow read command for current level gets an APS ACK, but no actual response.
The other two read commands get their respective responses just fine.

Here's my theory: Maybe we're requesting the brightness level too soon, while the fade off transition is still in progress..?
@Nerivec Thoughts? 🤔