[Bug] Turned off lights don't retain brightness on Homebridge restart

burmistrzak commented 5 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the bug

When restarting Homebridge, lights that are turned off don't retain their brightness level.

This seems to be cause by the plugin getting the all keys (including brightness) from Z2M on startup. https://github.com/itavero/homebridge-z2m/blob/31b0fd9c4624bfd84e3e6e5bc62f6ee2f27baf3e/src/platformAccessory.ts#L100-L110 Because the light is already off, Z2M naturally reports 1 as current brightness.

A possible solution could be to simply not /get the current brightness when we know a light is turned off?

Related devices

No response

Related Devices

No response

Steps To Reproduce

Turn on a light and set its brightness to e.g. 50% (ensure retain in Z2M is enabled)
Turn off the light (doesn't matter how)
Restart Homebridge (ensure Mosquitto & Z2M are still running)
Turn on the light (via Home.app or a Zigbee remote)

Expected behavior

Light turns on at 50% brightness

Device entry

No response

Status update

No response

Messages from this plugin

No response

This plugin

1.11.0-beta.5

Homebridge

1.7.0

Zigbee2MQTT

1.38.0-dev

Homebridge Config UI X (if applicable)

No response

burmistrzak commented 5 months ago

I'll have to test if the scheduled updates are also affected. We'll see... 👀

itavero commented 5 months ago

Never noticed this myself, so I'm not sure if every light behaves the same in this situation. I'm actually quite sure I've seen lights report their old brightness value with state "off".

Anyway, instead of not requesting it, it might be better to ignore the brightness if the state is off (and perhaps if the brightness is also below a certain level).

After all, Homebridge is not the only source that can change the brightness or state.

burmistrzak commented 5 months ago

Never noticed this myself, so I'm not sure if every light behaves the same in this situation. I'm actually quite sure I've seen lights report their old brightness value with state "off".

At least for Philips Hue, when you /get the brightness level and they're already off, they'll just return 1. I can reliably reproduce this behavior with the little blue refresh button in Z2M.

Anyway, instead of not requesting it, it might be better to ignore the brightness if the state is off (and perhaps if the brightness is also below a certain level).

Well, it's the act of requesting the brightness level itself, that's changing the cached state in Z2M. 😅 Can't we just use the retained state from MQTT?

@itavero Also, doesn't requesting the state of every single device, generate a lot of unnecessary Zigbee traffic, especially in larger setups? Shouldn't we be a bit more efficient here?

itavero commented 5 months ago

I don't have any retained state on my setup as far as I'm aware.

The traffic is only once on startup, so I don't see an issue with that.

We could probably add a configuration to disable getting the state on startup, or look at the configuration of Zigbee2MQTT to see if it has a retained state (I reckon the information would be available there?).

burmistrzak commented 5 months ago

I don't have any retained state on my setup as far as I'm aware.

Z2M essentially has two types of cache. One internal which is enabled by default, the other at the MQTT broker, but only if retain: true is set for a given device. I use retain for basically all devices, except for remotes, so MQTT-connected services like Homebridge or Node-RED immediately receive device states.

The traffic is only once on startup, so I don't see an issue with that.

I guess it depends on your network's size. Ours is on the bigger side, with 121 Zigbee devices in total, and 95 of them exposed to HomeKit.

Also, aren't we getting all keys regularly, like every four hours? https://github.com/itavero/homebridge-z2m/blob/31b0fd9c4624bfd84e3e6e5bc62f6ee2f27baf3e/src/platformAccessory.ts#L100-L106

We could probably add a configuration to disable getting the state on startup,

Yes, but we probably should add two options: The first to disable getting all keys on startup, the second to turn off the scheduled updates (~ every 4h) mentioned above.

or look at the configuration of Zigbee2MQTT to see if it has a retained state (I reckon the information would be available there?).

It's possible to see if a device has retain enabled, yes. This can be found for each device here: zigbee2mqtt/bridge/info with config.devices.<IEEE Addr>.retain.

itavero commented 5 months ago

Z2M essentially has two types of cache.

One internal which is enabled by default, the other at the MQTT broker, but only if retain: true is set for a given device.

I use retain for basically all devices, except for remotes, so MQTT-connected services like Homebridge or Node-RED immediately receive device states.

I don't think I have either of them enabled, which is probably why I ended up with this implementation. In the past I had the internal cache on, but this just caused old properties to remain forever. Not a big deal now that we have the exposes information, but still quite annoying when debugging.

I guess it depends on your network's size. Ours is on the bigger side, with 121 Zigbee devices in total, and 95 of them exposed to HomeKit.

My "production" environment is only about 80 devices.

Also, aren't we getting all keys regularly, like every four hours?

https://github.com/itavero/homebridge-z2m/blob/31b0fd9c4624bfd84e3e6e5bc62f6ee2f27baf3e/src/platformAccessory.ts#L100-L106

This timer only elapses if there wasn't any state update for 4 hours (timer gets reset on every state update).

Yes, but we probably should add two options: The first to disable getting all keys on startup, the second to turn off the scheduled updates (~ every 4h) mentioned above.

Just to be clear, it doesn't get all keys. Only the ones marked as "gettable". But we could make these two settings.

Still the default for the startup should be based off of the Zigbee2MQTT device configuration, I guess. If retain is on, then it should be disabled as default.

For the other one, it's difficult. If reporting is enabled (can probably also be found in the config) and working properly, the updates should not be needed.

Once I have some time I can look into implementing something like that, although I'm not planning to test this on my own environment.

burmistrzak commented 5 months ago

I don't think I have either of them enabled, which is probably why I ended up with this implementation. In the past I had the internal cache on, but this just caused old properties to remain forever. Not a big deal now that we have the exposes information, but still quite annoying when debugging.

Yea, the internal Z2M cache is enabled by default, so we should probably take this into account going forward.

My "production" environment is only about 80 devices.

You'll get there. 😉

This timer only elapses if there wasn't any state update for 4 hours (timer gets reset on every state update).

Aha, hoped it would be implemented this way. Does any state update (incl. set) reset the timer?

Just to be clear, it doesn't get all keys. Only the ones marked as "gettable". But we could make these two settings.

Sure, but that's potentially still a lot of traffic. A single Zigbee light has about five gettable keys on average. Just something we'll have to keep in mind. ✌️

Still the default for the startup should be based off of the Zigbee2MQTT device configuration, I guess. If retain is on, then it should be disabled as default.

Yes, retain is set per device or as a default for all devices in Z2M. However, it's unfortunately (at least currently) not exposed alongside other device properties, making the whole thing a bit more difficult. My suggestion:

Add a config option to disable "get all keys on startup" per-device with a global default.

Fetching the retain value for each device feels unnecessarily complicated (at least at this point). Let's just keep the current behavior as the default, but make it configurable.

For the other one, it's difficult. If reporting is enabled (can probably also be found in the config) and working properly, the updates should not be needed.

You mean the scheduled state updates (~ 4h)? Reporting isn't strictly necessary, unless the coordinator isn't involved, e.g. direct bindings or on-device inputs. However, well maintained converters are already taking these things into account and configure reporting accordingly. So I see (and please correct me) no real reason to poll for updates.

Once I have some time I can look into implementing something like that, although I'm not planning to test this on my own environment.

Happy to do the extensive testing for you. 😊 While I'm not yet super familiar with this particular part of the plugin, I might be able to come up with a PR. Any hints regarding implementation are certainly welcome.

itavero commented 4 months ago

Note to self: Zigbee2MQTT configuration is already being parsed in platform.ts for the availability feature. Should probably clean up that code and add a model there.

The check should probably be that both retain is on in the device config (or default config) and cache_state is enabled in the "advanced" settings of Zigbee2MQTT.

burmistrzak commented 4 months ago

The check should probably be that both retain is on in the device config (or default config) and cache_state is enabled in the "advanced" settings of Zigbee2MQTT.

@itavero Ok, so we would add an option (per device and default) to indicate whether retain is enabled for a given device. If that option is set, and we've confirmed that cache_state is also enabled in Zigbee2MQTT, we simply skip the entire this.queueAllKeysForGet() thing, correct?

burmistrzak commented 4 months ago

@itavero I have a rough draft ready, but I'm currently not checking if cache_state is enabled. Do you first want to make the availability code more generic so I can reuse it, or should we add that particular check later on? cache_state is enabled by default anyway, so... 😅

burmistrzak commented 2 months ago

@itavero I guess a global Regularly poll for updates option would be sufficient after all. A per-device option can still be added later on.

This new option would bypass the above mentioned code block completely, and thus use retained/incoming MQTT states instead. On boot, when likely no retained MQTT messages are available, Homebridge will, AFAIK, use its internal accessory state cache.

burmistrzak commented 2 months ago

@itavero Hmm, my initial idea of simply skipping the ExtendedTimer in the constructor isn't quite working as intended...

What about not requesting brightness at all? It should be possible to filter that out here, before sending the queue, right? https://github.com/itavero/homebridge-z2m/blob/4f405a18f2229a6bff4ef5a552239fec8a28412e/src/platformAccessory.ts#L296-L305

burmistrzak commented 1 month ago

@itavero I've implemented an experimental option in https://github.com/itavero/homebridge-z2m/pull/931 based on my previous https://github.com/itavero/homebridge-z2m/issues/882#issuecomment-2332818834. Any thoughts? 😊

itavero / homebridge-z2m