Skip to content

Conversation

@CoMPaTech
Copy link
Member

@CoMPaTech CoMPaTech commented Apr 5, 2020

Submitter has a followup PR to add sensor that should go out in the same release. This PR should be merged on or after May 14th to ensure the both PRs end up in the same release.

Breaking change

To improve user friendly configuration and support Adam and P1 devices in addition to Anna's, starting today Plugwise is configured through Configuration -> Integrations instead of updating the configuration file. Please remove the applicable lines from your configuration file (configuration.yaml) before upgrading. After upgrading add each Smile as an integration as described in the documentation. Note that this update also makes slight changes with regard to entity names to handle more than Anna.

Proposed change

As suggested we (@bouwew and @CoMPaTech) submit this as a draft PR to get our bearings right and are open to feedback on how to improve and get this new and async version of the plugwise component to HA-core. Any recommendations and improvements are welcome! One on our radar is the functionality from #32724.

With our previous PRs it was clear that more components were required to ensure our climate component wasn't full of (other) sensors and data. We decided to do this and make sure there were improvements for users as well. We rewrote the pypi module almost from scratch, async from the start, and added config_flow as suggested on discord#devs to ensure multiple Smiles (i.e. hubs/bridges, Plugwise call them Smiles) can be added for users that have a climate controller and a power meter device (or any combination). As suggested by reviewers from ealier PRs we moved the applicable things to sensor and binary_sensor components.

As this is a draft PR we have not yet modified the documentation.

During testing with some HA-community members we ran into an Anna upgrading to v4, the current plugwise component doesn't support v4. The current plugwise component only handles Anna v3 and v1.

This PR supports both 'Anna' and 'Adam' as climate controllers and 'P1' for power metering (DSMR):

  • Adam v2 and v3 (including plugs (switch) and various climate components (called Tom, Lisa & Floor) (climate/sensor/binary_sensor/switch)
  • Anna v1, v3 and v4 (climate/sensor/binary_sensor)
  • P1 (DSMR) v2 and v3 (sensor)

Thanks in advance for your time, comments and reviews, Bouwe & Tom

Type of change

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

Additional information

We are expecting issues coming in for Anna's with firmware v4 - depending on your thoughts this draft PR we have to check if we can adjust the current component.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
  • 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:

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

If the code communicates with devices, web services, or third-party tools:

  • The [manifest file][manifest-docs] has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

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

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link

Hey there @laetificat, @bouwew, mind taking a look at this pull request as its been labeled with a integration (plugwise) you are listed as a codeowner for? Thanks!

@bouwew
Copy link
Contributor

bouwew commented Apr 5, 2020

OK from my side, as co-creator :)

@CoMPaTech CoMPaTech marked this pull request as ready for review April 6, 2020 18:43
Copy link
Member

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are adding too much with this PR, you can definitely remove the new platforms to separate PRs

@CoMPaTech
Copy link
Member Author

@Kane610 would you suggest to just remove the other 3 components besides climate? Then once this PR is approved we individually PR the additional components I guess - but, would that be split over releases as well? (As that decreases functionality since we moved some measures to sensor and doesn't really work for newer components).

@CoMPaTech
Copy link
Member Author

As for CI: py37 seems to fail with codecov argument issues on most PRs now?

@CoMPaTech
Copy link
Member Author

Just small updates on main PR comment wrt doclink and previous commits referencing water_heater and approach.

Looking forward to help on how we can best move forward and/or on separating PRs.

@codecov

This comment has been minimized.

@Kane610
Copy link
Member

Kane610 commented Apr 8, 2020

@CoMPaTech yes that sounds about right, you have two weeks to get everything in :)

@CoMPaTech
Copy link
Member Author

@Kane610 Done ... climate-only it is (though still a large PR given config_flow and async).

Copy link
Contributor

@bouwew bouwew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For hvac_action, if only climate.py is part of the PR, return to the old code:

        if self._heating_state or self._boiler_state or self._dhw_state:
            return CURRENT_HVAC_HEAT
        if self._cooling_state:
            return CURRENT_HVAC_COOL
        return CURRENT_HVAC_IDLE

@CoMPaTech
Copy link
Member Author

@Kane610 can you assign both @bouwew and myself as assignees and should we include other reviewers?

@Kane610
Copy link
Member

Kane610 commented Apr 14, 2020

Reviewers will come automatically when they have time

@CoMPaTech
Copy link
Member Author

@CoMPaTech yes that sounds about right, you have two weeks to get everything in :)

Dear reviewer(s): Please note that we still would prefer to have both climate and sensor (at minimum) on release to ensure existing functionality for 'Anna' (thermostat-only) users - if feasible all platforms should be included on (breaking) release.

As requested the PR currently only holds climate and we would need additional PRs on binary_sensor, sensor, switch.

@bouwew bouwew removed their assignment Apr 22, 2020
@CoMPaTech CoMPaTech force-pushed the plugwise-async-platform branch from 95cfeb3 to 2ee2476 Compare April 25, 2020 18:55
@CoMPaTech
Copy link
Member Author

Catching up with #34591

@CoMPaTech CoMPaTech removed their assignment Apr 27, 2020
@CoMPaTech
Copy link
Member Author

Dear reviewer(s): Please note that we still would prefer to have both climate and sensor (at minimum) on release to ensure existing functionality for 'Anna' (thermostat-only) users - if feasible all platforms should be included on (breaking) release.

As requested the PR currently only holds climate and we would need additional PRs on binary_sensor, sensor, switch.

@bdraco
Copy link
Member

bdraco commented May 26, 2020

Everything still working as expected?

@CoMPaTech
Copy link
Member Author

Everything still working as expected?

I still have to try my dev-HA tonight at home near the bridge, but changing the custom_component code in my hassos deployment that is running at home seems fine.

But I'd still like to try it on the actual devcontainer on 0.111

@CoMPaTech
Copy link
Member Author

Everything still working as expected?
But I'd still like to try it on the actual devcontainer on 0.111
Really wanted to see it 'live', but config_flow works fine on devcontainer.

CoMPaTech pushed a commit to plugwise/home-assistant.core that referenced this pull request May 28, 2020
@CoMPaTech
Copy link
Member Author

Created #36219 as the indicated sensor-part for this 'parent'-PR.

@bdraco
Copy link
Member

bdraco commented May 28, 2020

@CoMPaTech. Would you prefer this gets merged now or wait until the start of 0.112 dev?

@CoMPaTech
Copy link
Member Author

@CoMPaTech. Would you prefer this gets merged now or wait until the start of 0.112 dev?

@bdraco we’ll have to improve the base class slightly as per your review of the child PR. I think we can iron things out before beta Wednesday - but depends on reviewer and merger availability too. Should we iterate this PR once more for base class improvement first, I can work on that later tonight?
Or merge this and fix base class and the other comments through sensor PR.

@bdraco
Copy link
Member

bdraco commented May 28, 2020

This one is already large enough so lets not add anything else.

It should like you have the bandwidth to get things ready by Wed. I should be able to find the time to do the reviews. I'll merge this now.

@bdraco bdraco merged commit 7e693af into home-assistant:dev May 28, 2020
@CoMPaTech
Copy link
Member Author

Tnx, we'll start working on the other one (we have to now anyway :))

@CoMPaTech CoMPaTech deleted the plugwise-async-platform branch May 28, 2020 20:24
CoMPaTech pushed a commit to plugwise/home-assistant.core that referenced this pull request May 28, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Some comments to address in a future PR.

@@ -0,0 +1,22 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other translations than the default autogenerated English are pointless to add. This is handled by Lokalise.

@CoMPaTech
Copy link
Member Author

@MartinHjelmare pushed in a7c0748

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.

Plugwise Anna broken after upgrade; KeyError: 'setpoint'

6 participants