Skip to content
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

HomeKit: Ignore Garage state command if target state is current state #17453

Closed
wants to merge 2 commits into from

Conversation

adrum
Copy link
Contributor

@adrum adrum commented Oct 14, 2018

Description:

I have a HomeKit automation that makes sure my garage is closed at a certain time. If the garage was already closed, it would improperly call the service then set the state as closing. Home Assistant would safely ignore the service call, but HomeKit would show the wrong state (closing). This code adds a check to ensure the the target state is different than the current state before issuing a service call and changing the state.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

I have a HomeKit automation that makes sure my garage is closed at a certain time. If the garage was already closed, it would improperly call the service then set the state as closing. Home Assistant would safely ignore the service call, but HomeKit would show the wrong state (closing). This code adds a check to ensure the the target state is different than the current state before issuing a service call.
@adrum adrum requested a review from cdce8p as a code owner October 14, 2018 18:00
@homeassistant
Copy link
Contributor

Hi @adrum,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@ghost ghost added the in progress label Oct 14, 2018
@cdce8p cdce8p self-assigned this Oct 14, 2018
@balloob
Copy link
Member

balloob commented Oct 16, 2018

We shouldn't do this as we don't know if the state that we have internally is correct, and so might ignore a perfectly fine call.

@cdce8p
Copy link
Member

cdce8p commented Oct 16, 2018

I could reproduce the error you've been describing @adrum.
I do think this should be implemented, but I would prefer it to be in HAP-python.

A bit of background (also for @balloob) why I think so

HomeKit is designed to only publish state changes that result in a new state. E.g. it doesn't call open on an open cover/garaged door. That's true for HomeKit scenes as well, but I wasn't aware that automations don't seem to respect that. I'm not sure it's that way by design (maybe a bug?).

Why do I think this should be in HAP-python?

If that's a problem that exists with automations, it's not just limited to garage doors. Additionally implementing it upstream will reduce complexity, since we don't have to add checks to every setter.

@cdce8p cdce8p closed this Oct 16, 2018
@ghost ghost removed the in progress label Oct 16, 2018
@adrum adrum deleted the patch-1 branch October 16, 2018 14:12
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants