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

Use capability of sensor if present to fix multisensor Wink devices #18907

Merged
merged 1 commit into from Dec 3, 2018

Conversation

@w1ll1am23
Collaborator

w1ll1am23 commented Dec 2, 2018

Description:

Unique ID support was added, but didn't account for Wink devices that are broken out in to several sensors in HA. For example the Wink relay and Wink smoke detectors which show up in Home Assistant as multiple sensors even though they are only one device.

Related issue (if applicable): fixes https://community.home-assistant.io/t/wink-relay-problem-after-upgrade/82269

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Dec 2, 2018

Any user with these devices won't be able to use them. What is the process for getting this fix in the next hotfix?

@balloob balloob added this to the 0.83.3 milestone Dec 2, 2018

@balloob

This comment has been minimized.

Member

balloob commented Dec 2, 2018

To get it into a hotfix, it needs to be tagged with the milestone, which I just did for you 👍

@pvizeli

pvizeli approved these changes Dec 2, 2018

@w1ll1am23 w1ll1am23 force-pushed the w1ll1am23:fix_wink_unique_ids branch from aefb465 to d851416 Dec 2, 2018

@balloob balloob merged commit 85c0de5 into home-assistant:dev Dec 3, 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.009%) to 92.933%
Details

@wafflebot wafflebot bot removed the in progress label Dec 3, 2018

balloob added a commit that referenced this pull request Dec 3, 2018

@balloob balloob referenced this pull request Dec 3, 2018

Merged

0.83.3 #18958

@CCOSTAN

This comment has been minimized.

Contributor

CCOSTAN commented Dec 4, 2018

I think this was a breaking change. My Wink sensors had a new ID of _2.

Easily fixed with a search and replace but all the automations related to the Wink sensors broke.

CCOSTAN added a commit to CCOSTAN/Home-AssistantConfig that referenced this pull request Dec 4, 2018

@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Dec 4, 2018

@CCOSTAN You are correct, it was a breaking change. I wasn't aware of it at the time however due to running a seperate dev instance to implement the fix. Really the first change caused a problem which couldn't be corrected without breaking things. I am not sure how something like this should have been handled. For a user that isn't aware of the .storage directory. Which really no one should be aware of it, they wouldn't know what to do.

@balloob How should something like this be fixed? For the non-technical user (not sure we have too many at this point) this kind of issue would be very frustrating since the core.entity_registry shouldn't be manually modified. It's almost like we need an upgrade processes that can "fix" things if version was X and then Y. However I am sure that would be a nightmare to maintain.

@CCOSTAN

This comment has been minimized.

Contributor

CCOSTAN commented Dec 4, 2018

@w1ll1am23 For my part, the fix was super easy AND super easy to see. When I upgraded, the new sensors floated to the top as badges (I'm not at Lovelace yet). It was apparent that the new IDs had _2. I just did a mass search and replace to correct all automations pointing to the old reference.
I actually took the time to clean up some IDs via the GUI so now that they are in the entity registry, I don't think this will happen again.
I think a simple 'breaking change' tag in the release notes would suffice..

@w1ll1am23

This comment has been minimized.

Collaborator

w1ll1am23 commented Dec 4, 2018

@CCOSTAN so you just updated your automations to use the _2? To fix my prod HA I deleted all of the wink entities from the entity registry so I didn't have to update any automations.

@CCOSTAN

This comment has been minimized.

Contributor

CCOSTAN commented Dec 4, 2018

@w1ll1am23 Originally, that was the plan but I decided to clean up the names a bit (and have them saved in the entity registry). So I basically gave them new names and then updated all automations. You can see the changes here.

CCOSTAN/Home-AssistantConfig@d4138e4

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