Skip to content

Fix on/off-only lights reverting to off after rapid turn_off/turn_on#767

Open
TheJulianJES wants to merge 1 commit into
zigpy:devfrom
TheJulianJES:tjj/fix-light-transitioning-flag-on-off-only
Open

Fix on/off-only lights reverting to off after rapid turn_off/turn_on#767
TheJulianJES wants to merge 1 commit into
zigpy:devfrom
TheJulianJES:tjj/fix-light-transitioning-flag-on-off-only

Conversation

@TheJulianJES
Copy link
Copy Markdown
Contributor

@TheJulianJES TheJulianJES commented May 19, 2026

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_on for lights supporting brightness, but always in turn_off, which should not have been the case. Thus, the turn_on call didn't cancel/restart the turn_off timer.

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

As 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_off unconditionally armed the transitioning flag and a ~1.5s timer whenever _zha_config_enable_light_transitioning_flag was set. For on/off-only lights this causes a rapid turn_off → turn_on sequence 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_buffer and cancels the pending timer. async_turn_on calls it only when its set_transition_flag predicate is true:

set_transition_flag = (
    brightness_supported or color_temp is not None or xy_color is not None
) and self._zha_config_enable_light_transitioning_flag

For an on/off-only light, brightness_supported is False and there are no color_temp / xy_color runtime params, so async_turn_on never resets the buffer or cancels the timer left over from async_turn_off. The sequence:

  1. turn_off: flag armed, 1.5s timer scheduled, optimistic _state = False
  2. Device's on_off=False report arrives → _transition_brightness_buffer = 0 (because is_transitioning is True)
  3. turn_on: set_transition_flag is False → async_transition_set_flag is not called → buffer and timer survive; the only state change is the optimistic _state = True
  4. Device's on_off=True report arrives → ignored: the buffered-attribute handler only resets the buffer when the value is falsy
  5. Original timer fires → async_transition_complete reads buffer == 0 and overrides _state = False ⇒ revert to off

Fix

Mirror async_turn_on's predicate in async_turn_off: gate the flag set, timer start, and stuck-cleanup on brightness_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_on already arms the flag (since brightness_supported is 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_on is called with no color_temp / xy_color — is also covered by gating on brightness_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, calls turn_off then turn_on, replays both attribute reports, sleeps past the old 1.5s window, asserts state stays on. Verified the test fails on dev HEAD without the fix and passes with it.

…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.
Copilot AI review requested due to automatic review settings May 19, 2026 11:11
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.64%. Comparing base (7ade89e) to head (c328f16).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_off behind brightness_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.

Comment thread tests/test_light.py
Comment on lines +2121 to +2123
# Wait past the old 1.5s timer window. State must remain on.
await asyncio.sleep(2)
await zha_gateway.async_block_till_done()
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have this elsewhere and this isn't a real 2 second wait (due to pytest-looptime). I think this is fine.

Comment thread tests/test_light.py
# 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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is bool(...) needed? The data type of on should be bool.

Comment thread tests/test_light.py
assert not entity.is_transitioning

# The device's on_off=False report from the off command arrives.
await send_attributes_report(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Collaborator

@zigpy-review-bot zigpy-review-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tight, surgical bugfix with a load-bearing regression test.

Why this is correct

The diagnosis matches the code exactly:

  • async_turn_on's set_transition_flag predicate (line 348) requires brightness_supported or color_temp is not None or xy_color is not None, so on an on/off-only light a follow-up turn_on never calls async_transition_set_flag(), which is the only path that nulls _transition_brightness_buffer and cancels the pending _transition_listener.
  • handle_cluster_handler_attribute_updated (line 1018–1024) buffers on_off=False to _transition_brightness_buffer = 0 while transitioning, and ignores on_off=True reports entirely.
  • async_transition_complete (line 794) then overrides _state = False when 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 (looptime correctly virtualizes the asyncio.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__.py against the venv: 1 pre-existing error on line 438, same as dev — no regression.
  • All CI green; title and label match recent merged PRs.

Nice fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This PR fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

light_transitioning_flag causes spurious state-revert on rapid turn_off→turn_on for some on/off-only lights

4 participants