Add opt-in option to adjust only lit members of a light group - #889
Add opt-in option to adjust only lit members of a light group#889RReverser wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #889 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 57 57
Lines 10560 10565 +5
=======================================
+ Hits 10264 10269 +5
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
LightGroup.async_turn_on(**kwargs) can currently raise a TypeError if only_if_on is ever present in kwargs because it’s forwarded and also passed explicitly to super().
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an opt-in LightGroup behavior to adjust brightness/color only for members that are already on (matching the behavior of group-bound wall dimmers), preventing slider-driven adjustments from turning on the entire group unintentionally.
Changes:
- Add
group_adjust_only_lit_memberstoLightOptions(defaultFalse) and plumb it intoLightGroupbehavior. - Add an internal
only_if_onpath that selects non-“with on/off” ZCL commands and suppressesOnwhen appropriate. - Add test coverage validating command selection and preserved default behavior when the option is disabled.
File summaries
| File | Description |
|---|---|
| zha/application/platforms/light/init.py | Implements the LightGroup “adjust only lit members” behavior via the only_if_on path and command selection changes. |
| zha/application/helpers.py | Adds the new LightOptions.group_adjust_only_lit_members configuration flag. |
| tests/test_light.py | Adds a test validating the opt-in behavior and fallback to default behavior when disabled. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e8059a8 to
502034f
Compare
|
Thanks! As an alternative, we could also expose this as a group-specific config entity once home-assistant/core#162307 lands. This would allow you to configure behavior per-group. |
There was a problem hiding this comment.
🟡 Changes recommended
The new only_if_on path can incorrectly update _state based on level even though move_to_level does not change on/off semantics, which can desync state from actual device behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/test_light.py:2404
- This section validates the command ID but doesn't assert that exactly one Color cluster request was made. Adding a call-count assertion makes the test more robust against accidental extra commands.
assert group_cluster_on_off.request.call_count == 0
assert (
group_cluster_color.request.call_args.args[1]
== group_cluster_color.commands_by_name["move_to_color_temp"].id
)
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
fcba3f7 to
2838e63
Compare
Brightness and colour changes on a `LightGroup` always go through `Move_To_Level_With_On_Off` / turn every member on, even members that are currently off. A group-bound wall dimmer sends the plain `Move_To_Level` instead, so its brightness/colour changes only affect lamps that are already lit and leave the rest off. There is no way to get that behaviour from a `light.turn_on` service call today, which makes any HA slider widget that controls brightness and/or colour (such as mushroom light cards) turn on the entire group on every adjustment. This PR adds a new `group_adjust_only_lit_members` option (default `False`) that makes brightness- or colour-only calls on a group that already has a lit member behave the same way: `Move_To_Level` and `Move_To_Color_Temp`/`Move_To_Color`, no On command. A bare `light.turn_on()`, or one sent while the whole group is off, keeps turning every member on as before, so "turn on at 40%" still works from a fully off group.
2838e63 to
2db9077
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new only_if_on gating currently includes transition-only calls, which expands the behavior beyond “brightness- or colour-only” adjustments and can change expected turn-on semantics for partially-on groups.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
No harm in doing that, but I'm genuinely unsure of any situations where I wouldn't want this behaviour - it feels a lot more natural and in line with how zigbee buttons already work. The only reason I kept this option as off-by-default is backward compatibility for existing setups. I worked around this limitation by creating fake HA entries for every zigbee group in my own setup, but it got cumbersome enough to maintain that I decided upstreaming is worth it. When that other PR lands, we could surely add per-group overrides for both "assume state of group" and "adjust only lights which are already On" but I'd rather not block on it as a dependency. |
There was a problem hiding this comment.
🟡 Changes recommended
only_if_on is currently honored outside LightGroup (risking incorrect state/command behavior if passed) and the new xy_color behavior isn’t covered by tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
zha/application/platforms/light/init.py:580
only_if_onis documented as “LightGroup only”, but_async_turn_on_implwill honor it for any light. If a non-group caller passesonly_if_on=Truewhile the light is off, this can sendmove_to_level(which does not change On/Off state) and then setself._state = True, desyncing internal state from the device. Consider hard-gatingonly_if_ontoLightGroup(or ignoring it entirely for non-groups) before selecting the level command.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Brightness and colour changes on a
LightGroupalways go throughMove_To_Level_With_On_Off/ turn every member on, even members that are currently off. A group-bound wall dimmer sends the plainMove_To_Levelinstead, so its brightness/colour changes only affect lamps that are already lit and leave the rest off. There is no way to get that behaviour from alight.turn_onservice call today, which makes any HA slider widget that controls brightness and/or colour (such as mushroom light cards) turn on the entire group on every adjustment.This PR adds a new
group_adjust_only_lit_membersoption (defaultFalse) that makes brightness- or colour-only calls on a group that already has a lit member behave the same way:Move_To_LevelandMove_To_Color_Temp/Move_To_Color, no On command. A barelight.turn_on(), or one sent while the whole group is off, keeps turning every member on as before, so "turn on at 40%" still works from a fully off group.