Buffer brightness attribute reports during light transitions#671
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #671 +/- ##
==========================================
+ Coverage 97.44% 97.48% +0.03%
==========================================
Files 62 62
Lines 10731 10742 +11
==========================================
+ Hits 10457 10472 +15
+ Misses 274 270 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I think this is also fine to merge. Depending on #672, it or this may need to rebased after merging. (The light class could also be cleaned up in general. If we could just get rid of the non-success status guards mentioned in the other PR (that we never hit?), that would already be good. Test coverage could also be improved somewhat.) |
puddly
left a comment
There was a problem hiding this comment.
This looks good to me. Just for clarity, the increase of the transition timer delay by 250ms won't result in the entity state change emitting "slower", right?
|
Yes. There's no extra delay to state at all. It will still be updated directly after an on/off command is sent and received by the light. No change there. It just lengthens the timer a bit until "unexpected" attribute reports immediately impact state again. (This is because we sometimes had the "final attribute report" after a transition be received outside of the timer range. If that were to happen now, we'd restore the previous one, where the brightness doesn't yet match the target brightness (we already set state to for the successful command), so we would run the risk of briefly bouncing the light to (e.g.) 90% at the end of the transition, before the final report arrives and bumps it back to 100%.) |
Proposed change
Instead of ignoring brightness attribute reports during a light transition, we buffer them and apply the last received value when the transition completes.
Additional information
Currently, we ignore all brightness attribute reports during a light transition. If a user were to use a remote directly bound to the light during a transition, we'd currently have an incorrect state at the end.
Also, there are some lights (or dimmers to be exact) which acknowledge an
oncommand during an already ongoing transition to off-state, but then immediately send an attribute report from theOnOffcluster that they're off. We expect them to be on, as that's what the user last sent.This seems to be some kind of firmware bug unique to these dimmers, but we should still handle this better in ZHA.
But it seems like the device does support
move_to_level_with_on_offcommands during an off-transition to turn back on, just not pureoncommands.....Side-note/future issue
There's likely also an issue where a HA
restartmode automation cancels the ongoingturn_ontask during the Zigbeeoncall, so we end up with a flag that is never reset, as the timer has not started.Maybe we can just catch
CancelledErrorand reset the flag (+ initiate a poll and/or emit updated state, as the light can be in some kind of in-between state)? Or actually, we can just usefinally:to complete the timer?AI summary
Problem
is_transitioningwasTrue, and we showed the optimistically-set target brightness, until another attribute report came in AFTER the transition.Status.SUCCESSfor the turn-on command, we'd always show the (never-reached) target brightness, not updating it to the actual brightness afterwards, even if it sent a "correct"on_off=falseattribute report during the supposed transition.on_off=falseto correctly reflect that it is still off. Previously this report was silently dropped, causing HA to show the light as on indefinitely.DEFAULT_EXTRA_TRANSITION_DELAY_SHORT(previously 0.25 s) was short enough that a late-arriving attribute report could slip through after the flag cleared and be applied directly to state, briefly showing a stale intermediate brightness.Solution
Instead of discarding attribute reports during a transition, the last received value is now buffered in
_transition_brightness_buffer. Whenasync_transition_completefires at the end of the transition window, the buffer is applied before emitting state:_brightness(reflecting what the device actually reached).0— written when anon_off=falsereport arrives — overrides_statetoFalse, correctly reflecting a device that refused to turn on.on_off=truereports during a transition are still skipped (the optimistic state is already correct).Reports during transition are still not shown in Home Assistant (no intermediate state flicker). The buffer is cleared when a new transition starts so stale values can never bleed through.
DEFAULT_EXTRA_TRANSITION_DELAY_SHORTis increased from 0.25 s to 0.5 s to ensure any report that arrives slightly after the physical transition ends is still captured in the buffer rather than applied to state directly.Changes
zha/application/platforms/light/const.py—DEFAULT_EXTRA_TRANSITION_DELAY_SHORT: 0.25 → 0.5zha/application/platforms/light/__init__.py_transition_brightness_bufferfield toBaseClusterHandlerLightasync_transition_set_flag)handle_cluster_handler_set_levelinstead of silently ignoring iton_off=falseas brightness0inhandle_cluster_handler_attribute_updatedasync_transition_complete: non-zero → update_brightness; zero → set_state = Falsetests/test_light.pytest_transition_brightness_buffering: verifies intermediate reports are buffered and the last value is applied on completion; also verifies that when no reports arrive the optimistic target is preservedtest_turn_on_during_off_transition: verifies that a device which refuses to turn on during an off-transition correctly ends up shown as off in HA once the transition window closes