Turn off not cancellable scripts automatically HomeKit

[quthla]

Description:

When there's no delay in a script, thus the script is not cancellable, the entity in HomeKit will just stay on unless turned off manually. This PR will set HomeKit state to off after 1 second if the script is not cancellable.

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• Tests have been added to verify that the new code works.

[cdce8p]

I see the issue your describing. However I would prefer to implement it differently.

set_state should only be the callback method to do the initial stuff. We do all the reactions inside update_state. In fact, if we were to eliminate the flag and call self.char_on.set_value(current_state) all the time, it should do the trick. That however might not be the best solution either, since originally the flag surfs the purpose to reduce unnecessary calls. Maybe only exclude the script domain?

if not self._flag_state or self._domain == 'script':
    self.char_on.set_value(current_state)

[cdce8p]

See comment

[quthla]

Yeah that's better. Updated it.

I guess the flag is then just so no updates are sent for which HomeKit state should already be new_state?

[cdce8p]

Sorry for the long delay. I ran some tests to see how it works, but now I'm not sure we should really add this feature to the HomeKit component. IMO the added benefit of a switch turning off automatically isn't worth the additional headache this would cause.

If we turn it off directly, like it's now, people might not know if the script has been called or not. To solve this we would need to add a delay which will definitely cause many other issues (two calls within a short period, users might think it's cancel_able although it isn't).

As I see it, the support for scripts is more of a hack than anything else, especially since HomeKit doesn't have any related accessory type for it and thus I would prefer it to stay as it is.

That doesn't mean however that we shouldn't support scene with the same hack we're already using. I'll take a look at that next.

[quthla]

That particular hack doesn't work with scenes as scenes are always scening, never changing state. You can try yourself but for me it wouldn't work.

And is it the right solution to leave scripts just broken? I think it's not less confusing if the script just stays on forever (and can not be ran again unless manually turned off) than having the script turn off after it ran and thus having consistent state in HA and HK.

[cdce8p]

IMO not ideal behavior like it's at the moment is way better then unpredictable one that might happen if HomeKit doesn't register a change correctly.

As for scenes: I don't use them personally, but I used a simple one for testing an it worked without issues. Can you share yours so I can reproduce the issue?

[quthla]

Is the scene turning off automatically for you or will it just stay on like scripts?

[cdce8p]

It behaves just like non cancel-able scripts.

[quthla]

Meaning the scene will just stay on in HK unless manually turned off?

[cdce8p]

Yes

[quthla]

But that's definitely a problem. Say you have a good morning scene in HK which toggles the HA scene to on, it will only work once because the scene will be on permanently

[cdce8p]

The HomeKit integration is not really designed to do this, but a solution that is already possible would be:

• Create an automation that is triggered once the switch is activated
• The automation should activate a (HomeKit) scene
• Which in turn deactivates the switch

[cdce8p]

I had some time to think about this PR again. While I stand to everything I've said before, you do have a point. I just wasn't sure if it's viable. Turns out, I should have listen to you sooner, it's not as difficult as I though 😅

Would you mind taking a look and testing it? I've push the changes to your branch.
Adding the scene component later should be pretty easy as well.

[quthla]

That's basically what I had apart from ignoring turn off sent from HK so I suppose it should work. But I wonder if this would survive a script.reload service call in case you have a script with delay before and remove the delay for instance.

[quthla]

Tested and confirmed working though as expected it will not survive script.reload

That can be fixed by moving the self.activate_only set to set_state and altering it as follows self.activate_only = self._domain == 'scene' or self._domain == 'script' and not can_cancel

[cdce8p]

Didn't though about this. It might be even better to move the check to update_state instead, since this should be triggered by script.reload if the script changed. Will post an update shortly.

[cdce8p]

scene support could be added here

[quthla]

As you can see in my comment above, I already added it. Works fine too

[cdce8p]

I'll merge this PR then, once the tests pass. Sorry for the messy review, I should have handled this better.

[quthla]

We came to a common conclusion and that's what matters 😊