Fix stuck light transitioning flag after task cancellation#672
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #672 +/- ##
==========================================
+ Coverage 97.44% 97.46% +0.01%
==========================================
Files 62 62
Lines 10731 10887 +156
==========================================
+ Hits 10457 10611 +154
- Misses 274 276 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if result[1] is not Status.SUCCESS: | ||
| # 'On' call failed, but as brightness may still transition | ||
| # (for FORCE_ON lights), we start the timer to unset the flag after | ||
| # the transition_time if necessary. | ||
| self.async_transition_start_timer(transition_time) | ||
| self.debug("turned on: %s", t_log) | ||
| return |
There was a problem hiding this comment.
The missing coverage is because all of these lines were indented. They don't have any coverage.
I'm also not sure if they're really needed, since we should just raise anyway if the device doesn't respond, right? I forgot about what the zigpy request can return though. I think we also raise for unsupported commands..? If so, we should be able to remove all of these in a future PR.
|
Diff for For This is somewhat of a crucial fix, so it might be nice to get this in. |
| @@ -527,6 +563,11 @@ async def async_turn_on( | |||
| ) | |||
| t_log["move_to_level_if_color"] = result | |||
| if result[1] is not Status.SUCCESS: | |||
There was a problem hiding this comment.
I'm thinking of having zigpy soon handle all ZCL default responses directly, throwing an error for any non-SUCCESS codes. Is it possible for a light to return a non-successful result?
There was a problem hiding this comment.
Yeah, that's the same question I asked in the comment above. I hoped you would have an answer 😄
I don't think we ever hit these paths..? Before I first touched the light class like 6(?) years ago, this code already existed. Maybe we can just remove soon?
There was a problem hiding this comment.
Ah, the existing comments were collapsed in the /changes view...
Yeah, let's remove them and see what breaks.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, the existing comments were collapsed in the
/changesview...
I can't tell you how much I hate that new feature 😅
I'll put up another PR to remove them. We don't have to merge that for this release yet.
There was a problem hiding this comment.
There are quite a few of these
Oh, yeah... The isinstance(result, Exception) check can most definitely go as well. Returning an exception instead of raising seems interesting 😄
There was a problem hiding this comment.
Definitely. We got rid of most of them but the root cause seems to be asyncio.gather(..., return_exceptions=True): https://github.com/search?q=repo%3Azigpy%2Fzha%20return_exceptions%3DTrue&type=code
There was a problem hiding this comment.
Ah, ok. That's at least always in places where it kind of makes sense.
puddly
left a comment
puddly
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks!
2 participants
The Git diff is horrible to look at, but it's just
try/finallybeing added in those parts and one small guard (59611bf).EDIT: Git diff is slightly improved by introducing another method where the actual implementation is moved to, with
async_turn_onnow calling that with but wrapped withtry / raiseto release the flag at the end.AI summary
When a
mode: restartautomation fires while a light turn-on or turn-off is in progress, Home Assistant cancels the running asyncio task. If that cancellation lands betweenasync_transition_set_flag()andasync_transition_start_timer(), the_transitioning_individualflag is leftTruewith no timer to clear it. The light then ignores all incoming attribute reports indefinitely.async_turn_onandasync_turn_offintry/finallyblocks. Thefinallyclause callsasync_transition_complete()when the flag is stuck (set but no timer running), covering task cancellation and unexpected exceptions.async_turn_onwas essentially renamed and the newasync_turn_onnow just calls that method withtry / raise, so we don't have to indent the entire turn on method. This reduces diff a bit and might make the intention more clear of what we're doing. I haven't done that forasync_turn_offbecause it's a much shorter method. I can obviously revert this to just indent everything, also do it for turn off, or just leave as is.move_to_levelcall in thenew_color_provided_while_offpath failed, the code returned without callingasync_transition_complete()or starting the transition timer — leaving the flag stuck.