New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn off not cancellable scripts automatically HomeKit #17793

Merged
merged 3 commits into from Nov 5, 2018

Conversation

Projects
None yet
4 participants
@quthla
Contributor

quthla commented Oct 25, 2018

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

This comment has been minimized.

Member

cdce8p commented Oct 25, 2018

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 quthla force-pushed the quthla:homekit_script branch 2 times, most recently from a043e94 to 4505ac6 Oct 25, 2018

@quthla

This comment has been minimized.

Contributor

quthla commented Oct 25, 2018

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 cdce8p referenced this pull request Oct 25, 2018

Merged

Add scenes as switches HomeKit #17799

4 of 4 tasks complete
@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

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

This comment has been minimized.

Contributor

quthla commented Nov 3, 2018

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

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

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

This comment has been minimized.

Contributor

quthla commented Nov 3, 2018

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

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

It behaves just like non cancel-able scripts.

@quthla

This comment has been minimized.

Contributor

quthla commented Nov 3, 2018

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

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

Yes

@quthla

This comment has been minimized.

Contributor

quthla commented Nov 3, 2018

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

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

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

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

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

This comment has been minimized.

Contributor

quthla commented Nov 4, 2018

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

This comment has been minimized.

Contributor

quthla commented Nov 4, 2018

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

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

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.

can_cancel = state.attributes.get(ATTR_CAN_CANCEL)
if self._domain == 'script' and not can_cancel:
return True
return False

This comment has been minimized.

@cdce8p

cdce8p Nov 4, 2018

Member

scene support could be added here

This comment has been minimized.

@quthla

quthla Nov 4, 2018

Contributor

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

@cdce8p cdce8p added the new-feature label Nov 4, 2018

@cdce8p

cdce8p approved these changes Nov 4, 2018

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

@cdce8p cdce8p changed the title from Turn off not cancellable scripts automatically to Turn off not cancellable scripts automatically HomeKit Nov 4, 2018

@quthla

This comment has been minimized.

Contributor

quthla commented Nov 4, 2018

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

@cdce8p cdce8p merged commit 8ee0e0c into home-assistant:dev Nov 5, 2018

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 93.068%
Details
Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Nov 5, 2018

@cdce8p cdce8p removed the new-feature label Nov 5, 2018

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

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