core icon indicating copy to clipboard operation
core copied to clipboard

Update daikin sensors

Open mlemainque opened this issue 2 years ago • 3 comments

Proposed change

Following discussions on #72545:

  • Create a per-device "total increasing" energy consumption sensor to have a better integration in HA energy dashboard.
  • Ensure cross-device sensors are created only once as it was confusing. E.g: power consumption and outside temperature which will report the same values on all the devices.
  • Does not enable all sensors by default as there are more and more.

Screenshots

Default enabled sensors

image

HA Energy Dashboard

image

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [X] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #72545
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/24093

Checklist

  • [X] The code change is tested and works locally.
  • [X] Local tests pass. Your PR cannot be merged unless tests pass
  • [X] There is no commented out code in this PR.
  • [X] I have followed the [development checklist][dev-checklist]
  • [X] The code has been formatted using Black (black --fast homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

  • [X] Documentation added/updated for [www.home-assistant.io][docs-repository]

The integration reached or maintains the following [Integration Quality Scale][quality-scale]:

  • [X] 🏆 Platinum

mlemainque avatar Sep 11 '22 16:09 mlemainque

Hey there @fredrike, mind taking a look at this pull request as it has been labeled with an integration (daikin) you are listed as a code owner for? Thanks! (message by CodeOwnersMention)

Hello @emufan, Does it look good to you?

mlemainque avatar Sep 13 '22 10:09 mlemainque

Here is the change log from v2.6.0 -> v2.7.2 https://bitbucket.org/mustang51/pydaikin/branches/compare/v2.7.2%0Dv2.6.0

fredrike avatar Sep 21 '22 12:09 fredrike

This PR seems to be stuck around the outdoor unit changes and while I agree it could be useful to split it into it's own device/entities it would be nice to get the other changes merged right away.

So if possible I propose splitting up this PR into two: PR 1 would focus on the combined total-increasing energy sensor, default sensor state (enabled/disabled) and rename things to follow the entity naming guidelines.

PR 2 would then focus on a solution for the outdoor unit. This way we can get the small changes that don't affect "design" or "architecture" of the integration merged and get the benefits from those while the outdoor unit parts are being worked out.

VIKTORVAV99 avatar Nov 15 '22 09:11 VIKTORVAV99

I messed up the rebase, sorry... I've created a new clean PR here: https://github.com/home-assistant/core/pull/82441

mlemainque avatar Nov 20 '22 22:11 mlemainque