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

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

Merged
merged 1 commit into from
Dec 3, 2018

Conversation

w1ll1am23
Copy link
Contributor

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.

@ghost ghost assigned w1ll1am23 Dec 2, 2018
@ghost ghost added the in progress label Dec 2, 2018
@w1ll1am23
Copy link
Contributor Author

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
Copy link
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 👍

@w1ll1am23 w1ll1am23 force-pushed the fix_wink_unique_ids branch from aefb465 to d851416 Compare December 2, 2018 16:11
@balloob balloob merged commit 85c0de5 into home-assistant:dev Dec 3, 2018
@ghost ghost removed the in progress label Dec 3, 2018
@balloob balloob mentioned this pull request Dec 3, 2018
@CCOSTAN
Copy link
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 pushed a commit to CCOSTAN/Home-AssistantConfig that referenced this pull request Dec 4, 2018
@w1ll1am23
Copy link
Contributor Author

@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
Copy link
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
Copy link
Contributor Author

@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
Copy link
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

mishuka0222 added a commit to mishuka0222/BearStone-SmartHome that referenced this pull request Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants