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

Ignore duplicate state changes GarageDoor HomeKit #18149

Merged
merged 2 commits into from Nov 5, 2018

Conversation

Projects
None yet
5 participants
@cdce8p
Member

cdce8p commented Nov 3, 2018

Description:

Ignore duplicate state chage calls from HomeKit for GarageDoor accessories. Those could happen with the use of HomeKit automations. This PR is necessary after I had to revert the HAP-python update #18069. It's also largely based on the work @adrum did with #17453. Just added the tests for it.

Related issue (if applicable): fixes #17557

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.

CC: @wasp100

@wasp100

This comment has been minimized.

wasp100 commented Nov 3, 2018

Thanks for addressing this bug. Can you please also implement for locks?

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

@wasp100 I would, but why do you think this is necessary? For garage doors I could reproduce your issue, but that doesn't exist for locks, or am I missing something?

@wasp100

This comment has been minimized.

wasp100 commented Nov 3, 2018

In my previous testing, when using the homekit lock function, when running a scene to lock all doors at night, it would trigger the relay even though the door was locked. Essentially the same issue that exists with the garage door.

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

Yeah, I does. But that happens with every accessory and I don't really see the negative consequences that exists for the garage door representation.

@wasp100

This comment has been minimized.

wasp100 commented Nov 3, 2018

Correct, it has no negative effect on the implementation of the garage door opener. The lock function should also be addressed because it poses a security risk, ie, it unlocks an already locked door when running an automation to ensure the doors are locked.

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

I don't see that. Since the lock implementation uses no transitioning states in contrast to the garage door, the only thing that happens is an additional call to lock an already locked door.

@wasp100

This comment has been minimized.

wasp100 commented Nov 3, 2018

I’ll test again and revert back to you.

@adrum

This comment has been minimized.

adrum commented Nov 3, 2018

@cdce8p HomeKit supports transitioning states for locks. I never got around to experiencing/fixing this scenario because I'm still waiting on a fix for my ZWave locks to properly function.

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

@adrum Technical correct, however we don't set them like we do with the garage door here: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/homekit/type_covers.py#L48-L53
That's why for locks it's just setting the same state again, whereas for the garage door we would go to opening or closing first.

@adrum

This comment has been minimized.

adrum commented Nov 4, 2018

Yes I assumed this might be the case without digging into it. Should we add support for this temporary state scenario around here: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/homekit/type_locks.py#L52

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

There is no need to add anything for locks. As I've said, I would just call lock again without any consequences. Thus there is no need to ignore the call. Especially considering that we don't know if the internal state is correct: #17453 (comment)

@adrum

This comment has been minimized.

adrum commented Nov 4, 2018

I now have 2 questions:

  1. Since we don't know if the internal state is correct, should we change the code in this PR to only ignore setting the state to opening/closing and still allow the service call to go through? That way, it would continue to function the same as locks do, without getting into the weird state situation.

  2. I have absolutely no knowledge on this subject (beyond the scope of this issue as well), but is there anything we can do to improve the reliability of the internal state? Are there any issues/PRs I could read through to get up to speed?

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

To be honest, in most situations the internal state should be correct. At least I haven't seen anything else, since I started using HA.

IMO we should be fine with the way it's now, but if you like to, I can allow the service_call.

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

@adrum I've update the PR to account for 1)

@balloob

balloob approved these changes Nov 5, 2018

@balloob balloob merged commit 26ba4a5 into home-assistant:dev Nov 5, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0002%) to 93.065%
Details

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

@cdce8p cdce8p deleted the cdce8p:homekit-fix-garage-door branch Nov 5, 2018

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

@wasp100

This comment has been minimized.

wasp100 commented Nov 29, 2018

Hi @cdce8p,
Just updated to 0.83 to test the change for the garage door opener.
This is still not working correctly for me. If the door is closed and I run a scene like Good Night (which closes the open garage, switches off lights etc), the relay is still being triggered, which results in the garage door opening.
The same happens with the lock function. When running the scene, the relay triggers which results in the door unlocking.
Is there any way to not send any command to the device if the last state was closed/ locked, etc??
Thanks for your help thus far.
It worked perfectly when the initial changes were made, but I understand that it was causing problems with lights.

@wasp100

This comment has been minimized.

wasp100 commented Nov 29, 2018

Would ignoring the service call help resolve this as I’ve never found the internal state to be incorrect.
Could you apply this (ignore service call) to locks as well please?

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 29, 2018

@wasp100 We shouldn't discuss this here, since the PR is already closed. Please open a new issue for this and link me.

However I don't want to give you false hope, I tested it again a moment ago an everything works as expected. I believe the issue is with your setup (more specific the relays). Please post the configs for those too if you open an issue.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Nov 29, 2018

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