Koenkk/zigbee-herdsman-converters

Lower bound of "turnsOffAtBrightness1" ignored by "brightness_move" & "brightness_step" commands

burmistrzak opened this issue ยท 14 comments

Hi Koen ๐Ÿ‘‹

just stumbled upon an interesting issue, apparently first reported here, that sent me down a deep rabbit hole to find the root cause. ๐Ÿ•ณ๏ธ๐Ÿ‡

My findings so far:
When a light, that has the turnsOffAtBrightness1 option set (e.g. Philips Hue), and receives either a brightness_move or brightness_step (e.g. -85) command, it will almost always be dimmed down to a brightness level of 1, causing all sorts of weird issues downstream.
One does only encounter this bug when controlling lights with automations (Node-RED, etc.) which utilize the aforementioned commands.

Note: This behavior is reproducible, but sometimes the bightness_move will just stop at 5 instead of going all the way down to 1... Why that's happening, I'm not quite sure yet. ๐Ÿคท

Expected behavior

Dimming stops at a brightness level of 2, when a given bulb has turnsOffAtBrightness1 set.

Actual behavior

Lights always get dimmed down to 1, ignoring any lower bound set by the turnsOffAtBrightness1 option.

System details

Zigbee2MQTT version: 1.35.1@9eaaa0f
Coordinator type: zStack3x0
Coordinator revision: 20230507
Frontend version: 0.6.151
Zigbee-herdsman-converters version: 18.9.0
Zigbee-herdsman version: 0.30.0

Oh no! The metaphorical hole is apparently even deeper than I thought! ๐Ÿ˜ฎ

Because brightness_move uses the native Zigbee cluster genLevelCtrl under the hood, there aren't many tunable options available.

await entity.command('genLevelCtrl', command, payload, utils.getOptions(meta.mapped, entity));

The specific cluster command utilized is move, which only supports two parameters: movemode and rate, none of which are particularly useful in this case.

As far as I know, and please someone correct me here, the actual brightness range is controlled by a given bulb's firmware... ๐Ÿค”๐Ÿค”๐Ÿค”

As far as I know, and please someone correct me here, the actual brightness range is controlled by a given bulb's firmware...

That's true, I don't this is fixable for z2m for these native commands.

@Koenkk Yeah, that's what I feared... ๐Ÿ˜จ

One possible workaround could be to send {"brightness":2,"transition":3} to dim down lights, but that isn't really elegant because the new brightness level would immediately be passed down to HomeKit & Co., even before a stop command is issued.

While brightness_move most likely can't be fixed at our end, brightness_step could, at least in theory, take turnsOffAtBrightness1 into account and stop at 2, right?

const payload = {stepmode: mode, stepsize: Math.abs(value), transtime: transition};

A more comprehensive solution would probably also include:

  1. Open a ticket with the manufacturer (i.e. Signify) and politely ask them to fix their implementation.
  2. Improve the handling of extra low brightness values and/or range mappings in downstream projects, e.g. itavero/homebridge-z2m#674

While brightness_move most likely can't be fixed at our end, brightness_step could, at least in theory, take turnsOffAtBrightness1 into account and stop at 2, right?

True, would you mind submitting a PR for that?

True, would you mind submitting a PR for that?

I certainly wouldn't mind, it's the least I can do. However, I'm not yet familiar enough with the codebase to make a quality contribution.
Nevertheless, I'll read myself into it and give it a shot. No promises though. ๐Ÿ˜…

I assume meta.state.brightness is the current brightness level and value the step size?
If so, checking whether turnsOffAtBrightness1 is set, meta.state.brightness isn't already at 2, and meta.state.brightness + value doesn't go below 2, for a normal brightness_step operation, should be sufficient?

There's also the slight possibility of a hit in performance, because we would have to run that code before every single dimming step... ๐Ÿค”

That's indeed the solution I expect!

There's also the slight possibility of a hit in performance, because we would have to run that code before every single dimming step...

A lot of code runs before this command, I have no worries about a performance hit ๐Ÿ˜„

That's indeed the solution I expect!

Great to hear! When I have some time, I'll get a z2m dev setup ready. ๐Ÿ˜Š

However, it still bugs me to no end that brightness_move can't be fixed...
Does turnsOffAtBrightness1 even apply to modern Hue bulbs? Maybe Signify has changed this behavior with recent updates, why else would my Hue lights dim below 2? ๐Ÿ‘€

I don't have any modern Hue lights to test with, but you can try and see if it works without.

I don't have any modern Hue lights to test with, but you can try and see if it works without.

So I can already confirm that my modern Hue bulbs (i.e. with Bluetooth) allow dimming to 1 with brightness_move and brightness_step via MQTT. However, setting a value < 2 directly doesn't work, obviously.
Is there a quick way to override the turnsOffAtBrightness1 argument, without creating a custom build?

Also, what exactly should I see?
I assume {"brightness":1} would automatically turn off an affected bulb?

Edit: I might be totally hallucinating, but I feel like my Hue lights at a Zigbee brightness of 1 are actually flickering!? ๐Ÿ˜ฌ Have to investigate this further...

Edit 2: Seems like my eyes were deceiving me. Probably I'm just super sensitive to PWM... ๐Ÿ˜…

Also, what exactly should I see? I assume {"brightness":1} would automatically turn off an affected bulb?

Used the z2m dev console to set my Hue bulb (LTA001, "Hue white ambiance E27 with Bluetooth") directly to a brightness level of 1, by issuing command 0 with payload {"level":1, "transtime":4} to cluster 8 at endpoint 11.

No surprises here! As with brightness_step, even at 1/254, the bulb stays on! ๐Ÿ’ก

However, as soon as I use a *OnOff (e.g. moveToLevelWithOnOff) command, the story is different! So executing command 4 with payload {"level":1, "transtime":4} on cluster 8 at endpoint 11, the bulb will indeed turn off! ๐Ÿฅธ
Consequently, the lowest brightness supported by *OnOff commands is 2/254.

Why it's implemented this way, I'm not sure. But it also seems to cause issues when a toggle command is used right after a light has been turned off that way...

@Koenkk Oh, that's a handy feature! Already wondered what that button was about. ๐Ÿ‘€

My preliminary results indicate that Hue bulbs still need turnsOffAtBrightness1, at least for the various *OnOff commands.

So the proposed workaround would more or less extend this weird limitation to non-OnOff commands, which doesn't make much sense, IMHO. Why should we artificially limit brightness when it's a native function of these lights?

@burmistrzak you mean fixing it for brightness_step?

@burmistrzak you mean fixing it for brightness_step?

Yeah, because we wouldn't be fixing anything, only adding more restrictions.
It's unfortunate that *OnOff commands are implemented this way, but unless manufacturers change their firmware, we shouldn't change how z2m handles brightness.

Conclusion

Everything at z2m's end is working as good as it can, but downstream projects like homebridge-z2m aren't handling really low brightness values that well.
Very low brightness values from Zigbee (e.g. 2/254) get translated to <0.5%, meaning HomeKit for example will round them down to 0%, a technically invalid state when the light is still on!
This in turn is actually causing these weird UI/UX issues.

Mystery solved, I guess? ๐Ÿ˜‡