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

RSSI_PEER and RSSI_DEVICE are different things (fixes #20900) #20902

Merged
merged 8 commits into from Feb 18, 2019

Conversation

@dagobert
Copy link
Contributor

commented Feb 9, 2019

Description:

Wireless actors are having two RSSI values. The way the component was programmed one of them was overwritten.

Breaking Change:

The rssi attribute of HomeMatic entities has been removed. As a replacement the attributes rssi_device and rssi_peer have been added.

Related issue (if applicable): fixes #20900

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Fix #20900: RSSI_PEER and RSSI_DEVICE are different things
This change is fixing issue #20900.

Wireless actors are having two RSSI values. The way the component was programmed one of them was overwritten.
@homeassistant

This comment has been minimized.

Copy link

commented Feb 9, 2019

Hi @dagobert,

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!

@dagobert

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

If you are worried about breaking existing automations of users by removing the attribute rssi I would suggest to implement:

    'RSSI_PEER': ['rssi_peer', {}],
    'RSSI_DEVICE': ['rssi_device', {}],
    'RSSI_DEVICE': ['rssi', {}],

This way rssi still exists and contains the signal strength measured by the CCU when receiving data from the device.

@andrewsayre

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

If you are worried about breaking existing automations of users by removing the attribute rssi I would suggest to implement...

I think that would be better--then this isn't a breaking-change and instead new-feature. The legacy attribute will continue to function as is. If you eventually want to clean this up, I would suggest adding code to log a warning that the attribute has been deprecated and will be removed in 0.90 or something (which is at least 2 versions ahead).

Do the docs need to be updated to describe these new attribs?

@andrewsayre andrewsayre self-assigned this Feb 10, 2019

@fabaff fabaff changed the title Fix #20900: RSSI_PEER and RSSI_DEVICE are different things RSSI_PEER and RSSI_DEVICE are different things (fixes #20900) Feb 10, 2019

@dagobert

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

I think that would be better--then this isn't a breaking-change and instead new-feature. The legacy attribute will continue to function as is. If you eventually want to clean this up, I would suggest adding code to log a warning that the attribute has been deprecated and will be removed in 0.90 or something (which is at least 2 versions ahead).

I agree. I have to admit that I have no deep knowledge about how to implement this warning.

Do the docs need to be updated to describe these new attribs?

In the documentation of the component is nothing written about specific attributes of devices. There do not need to be any changes.

@andrewsayre

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

I agree. I have to admit that I have no deep knowledge about how to implement this warning.

Don't worry about it for now -- just make the change and add a comment above the entry indicating it's preserved for backwards compatibility.

dagobert and others added some commits Feb 12, 2019

danielperna84 added some commits Feb 12, 2019

@danielperna84

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I had another look at the code. The solution with the duplicate key won't work. This will still be a breaking change. The relevant code:

        attr = {}

        # Generate a dictionary with attributes
        for node, data in HM_ATTRIBUTE_SUPPORT.items():
            # Is an attribute and exists for this object
            if node in self._data:
                value = data[1].get(self._data[node], self._data[node])
                attr[data[0]] = value

This will never update rssi because only one key of the dictionary can be used.

In my opinion the key with just rssi should be removed (like originally intended) and it should be mentioned as a breaking change in the release. Monitoring the RSSI isn't the most popular thing to do, so I think this is ok to do.

@andrewsayre andrewsayre merged commit ce7f678 into home-assistant:dev Feb 18, 2019

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.6%) to 92.796%
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 Feb 18, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

Please highlight the breaking change in a paragraph in the PR description for the release notes.

@balloob balloob referenced this pull request Mar 6, 2019

Merged

0.89.0 #21712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.