Fix on/off-only lights reverting to off after rapid turn_off/turn_on#767
Fix on/off-only lights reverting to off after rapid turn_off/turn_on#767TheJulianJES wants to merge 1 commit into
Conversation
…lights `async_turn_off` unconditionally set the transitioning flag and started a ~1.5s timer. For on/off-only lights, a follow-up `async_turn_on` does not call `async_transition_set_flag`, so neither the stale `_transition_brightness_buffer = 0` (from the device's on_off=False report after the off command) nor the pending timer get cleared. When the timer fires it overrides the optimistic on-state back to off. Mirror the predicate from `async_turn_on`: only set the flag and start the timer when brightness is actually supported. On/off-only lights have no intermediate state to buffer, so the flag has no useful purpose for them. Fixes zigpy#766.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #767 +/- ##
=======================================
Coverage 97.64% 97.64%
=======================================
Files 62 62
Lines 10872 10873 +1
=======================================
+ Hits 10616 10617 +1
Misses 256 256 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a ZHA light state regression where on/off-only lights could revert back to off after a rapid turn_off → turn_on sequence when enable_light_transitioning_flag is enabled, by avoiding arming the transition buffering/timer for lights that don’t support brightness transitions.
Changes:
- Gate transition-flag arming/timer scheduling in
async_turn_offbehindbrightness_supported(in addition to the config flag). - Ensure cancellation cleanup uses the same gated predicate to avoid “stuck transitioning” only when relevant.
- Add a regression test covering rapid off→on toggling for an on/off-only light.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
zha/application/platforms/light/__init__.py |
Prevents transition buffering/timer behavior from being applied to on/off-only lights, eliminating the delayed revert-to-off. |
tests/test_light.py |
Adds a regression test validating that rapid off→on toggling on on/off-only lights does not revert after the prior ~1.5s window. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Wait past the old 1.5s timer window. State must remain on. | ||
| await asyncio.sleep(2) | ||
| await zha_gateway.async_block_till_done() |
There was a problem hiding this comment.
We already have this elsewhere and this isn't a real 2 second wait (due to pytest-looptime). I think this is fine.
| # Turn off, then immediately turn back on (within the default ~1.5s window). | ||
| await entity.async_turn_off() | ||
| await zha_gateway.async_block_till_done() | ||
| assert bool(entity.state["on"]) is False |
There was a problem hiding this comment.
nit: is bool(...) needed? The data type of on should be bool.
| assert not entity.is_transitioning | ||
|
|
||
| # The device's on_off=False report from the off command arrives. | ||
| await send_attributes_report( |
There was a problem hiding this comment.
Not sure if it's useful to have as an explicit test but this also passes (as expected) when the reports both come in after the commands succeed (i.e. if you manage to spam the on/off toggle faster than the light reports).
zigpy-review-bot
left a comment
There was a problem hiding this comment.
LGTM. Tight, surgical bugfix with a load-bearing regression test.
Why this is correct
The diagnosis matches the code exactly:
async_turn_on'sset_transition_flagpredicate (line 348) requiresbrightness_supported or color_temp is not None or xy_color is not None, so on an on/off-only light a follow-upturn_onnever callsasync_transition_set_flag(), which is the only path that nulls_transition_brightness_bufferand cancels the pending_transition_listener.handle_cluster_handler_attribute_updated(line 1018–1024) bufferson_off=Falseto_transition_brightness_buffer = 0while transitioning, and ignoreson_off=Truereports entirely.async_transition_complete(line 794) then overrides_state = Falsewhen the buffer is 0.
Mirroring async_turn_on's predicate (sans the color params, which async_turn_off doesn't take) closes the asymmetry. The _async_cleanup_transition_if_stuck(set_transition_flag) change in the finally block is also correct — passing False for on/off-only lights means the stuck-cleanup is a no-op there, which is what we want since we never armed the flag.
Brightness-supporting lights are unaffected because their turn_on arms the flag and resets buffer + timer cleanly via async_transition_set_flag(). Color-only-with-no-Level hardware (where xy_color/color_temp is set on turn_on but brightness_supported is False on turn_off) is essentially nonexistent in practice and would still recover the next time the flag is armed.
Verification
- Ran the new test on PR HEAD: passes in 1.6s (
looptimecorrectly virtualizes theasyncio.sleep(2)). - Reverted the production change locally with the test kept — test fails on the timer-fires assertion, confirming it actually exercises the bug.
- Full
tests/test_light.py(21 tests) passes on PR HEAD. mypy zha/application/platforms/light/__init__.pyagainst the venv: 1 pre-existing error on line 438, same asdev— no regression.- All CI green; title and label match recent merged PRs.
Nice fix.
Proposed change
This fixes an issue where on/off-only lights (so ones that do not support brightness) incorrectly revert their state to "off" when quickly turning the light off, and then back on again.
The underlying issue was that we only set the transitioning flag in
turn_onfor lights supporting brightness, but always inturn_off, which should not have been the case. Thus, theturn_oncall didn't cancel/restart theturn_offtimer.To fix this, we just do not set the flag for on/off-only lights. They do not need at all, since they have no brightness/transitions, so we can always parse attribute reports.
Additional information
light_transitioning_flagcauses spurious state-revert on rapid turn_off→turn_on for some on/off-only lights #766As a little background: The transitioning flag and timer is needed because Zigbee lights always send their current brightness, even during ongoing transitions.
However, in Home Assistant, we need to show the target brightness instead. Otherwise, you see the brightness slider going back down instantly after moving it up, then slowly moving back up (but only for lights that support attribute reporting). There's also some other issues this fixes (including for group lights that would have very jumpy/weird state otherwise, especially if some lights do not support attribute reporting).
In general, the light class needs to cover a lot(!) of edge-cases, and generally does so successfully. But there are plans to clean up the entire class a bit, e.g. with #734.
AI summary
Useful-ish AI summary (click to expand)
Fixes #766.
async_turn_offunconditionally armed the transitioning flag and a ~1.5s timer whenever_zha_config_enable_light_transitioning_flagwas set. For on/off-only lights this causes a rapidturn_off → turn_onsequence to revert to off after the timer fires, even though the device is on and never received another command.Root cause
async_transition_set_flag(light/__init__.py:734) is the only path that resets_transition_brightness_bufferand cancels the pending timer.async_turn_oncalls it only when itsset_transition_flagpredicate is true:For an on/off-only light,
brightness_supportedis False and there are nocolor_temp/xy_colorruntime params, soasync_turn_onnever resets the buffer or cancels the timer left over fromasync_turn_off. The sequence:turn_off: flag armed, 1.5s timer scheduled, optimistic_state = Falseon_off=Falsereport arrives →_transition_brightness_buffer = 0(becauseis_transitioningis True)turn_on:set_transition_flagis False →async_transition_set_flagis not called → buffer and timer survive; the only state change is the optimistic_state = Trueon_off=Truereport arrives → ignored: the buffered-attribute handler only resets the buffer when the value is falsyasync_transition_completereadsbuffer == 0and overrides_state = False⇒ revert to offFix
Mirror
async_turn_on's predicate inasync_turn_off: gate the flag set, timer start, and stuck-cleanup onbrightness_supported. On/off-only lights have no transition to wait for, so the flag had no useful purpose for them.Brightness-supporting lights are unaffected: any follow-up
turn_onalready arms the flag (sincebrightness_supportedis true), which resets the buffer and cancels the previous timer cleanly. That existing path is unchanged.A narrow theoretical edge case — a color-only light with no Level cluster, where
turn_onis called with nocolor_temp/xy_color— is also covered by gating onbrightness_supported. Such hardware is rare in practice.Regression test
Added
test_on_off_only_rapid_toggle_does_not_revert: on an on/off-only light, callsturn_offthenturn_on, replays both attribute reports, sleeps past the old 1.5s window, asserts state stays on. Verified the test fails ondevHEAD without the fix and passes with it.