Skip to content

Add Plugwise USB-stick integration#35713

Closed
brefra wants to merge 68 commits intohome-assistant:devfrom
brefra:plugwise_stick
Closed

Add Plugwise USB-stick integration#35713
brefra wants to merge 68 commits intohome-assistant:devfrom
brefra:plugwise_stick

Conversation

@brefra
Copy link
Copy Markdown
Contributor

@brefra brefra commented May 16, 2020

Breaking change

None

Proposed change

This PR adds support for legacy Plugwise Circle+, Circle and Stealth devices. These devices are controlled by using a UBS-stick which communicates directly to the devices. More details is written down in the related documentation PR.

This first PR is based on this custom integration, published in HACS. It contains the switch platform to control the power relays. In future PR's the sensor platform will be added to support multiple sensors (mainly) related to the power consumption.

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

Example entry for configuration.yaml:

No yaml example as it depends on config flow.

# Example configuration.yaml

Additional information

  • This PR fixes or closes issue: n/a
  • This PR is related to issue: n/a
  • Link to documentation pull request: 13466

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
  • 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:

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

  • The manifest file 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:

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

Copy link
Copy Markdown
Member

@CoMPaTech CoMPaTech left a comment

Choose a reason for hiding this comment

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

Nice to see core integration for the older plugs as wel @brefra

Do note that core reviewers may want to start with only one platform per PR so you might have to begin with either sensor or switch but not both. If you indicate clearly that these should only be merged/released together they can add such indications. (We're following that path with plugwise as well now where we will have four platforms).

See this line in config_flow scaffolding.

@brefra
Copy link
Copy Markdown
Contributor Author

brefra commented May 17, 2020

@CoMPaTech thanks for the comment.
As suggested, I just dropped the sensor platform to keep this initial PR as small as small as possible.

@CoMPaTech
Copy link
Copy Markdown
Member

@CoMPaTech thanks for the comment.
As suggested, I just dropped the sensor platform to keep this initial PR as small as small as possible.

On ours one of the core reviewers added a line in the initial comment about not merging on/before date. Just drop a clear comment in that this PR is intended to have a follow-up PR with your sensor component and it should be fine. The put something along like (not sure if it helps if I add it :)) :

Submitter has a followup PR to add sensor that should go out in the same release.

@brefra brefra marked this pull request as ready for review May 18, 2020 12:35
@brefra brefra force-pushed the plugwise_stick branch 3 times, most recently from 1b53a01 to b21016a Compare May 24, 2020 18:00
@brefra brefra force-pushed the plugwise_stick branch 3 times, most recently from 2cb9f44 to 087582c Compare July 8, 2020 20:02
@brefra

This comment has been minimized.

)
errors = await validate_connection(self.hass, device_path)
if not errors:
return self.async_create_entry(
Copy link
Copy Markdown
Contributor

@bouwew bouwew Sep 13, 2020

Choose a reason for hiding this comment

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

You should add add this before return self.async_create_entry():

                await self.async_set_unique_id("unique_id", raise_on_progress=False)
                self._abort_if_unique_id_configured()

The config_flow also needs a unique_id, otherwise you will be able to add the integration more than once. Also, there will be issues when adding a 2nd Stick. I know, unlikely, but there's always someone that will do this :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll add an unique_id to the the config entry.

else:
device_path = await self.hass.async_add_executor_job(
get_serial_by_id, user_selection
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: here you could add something like:

            for entry in self._async_current_entries():
                if entry.data.get("parameter") == user_input["parameter"]:
                    return self.async_abort(reason="already_configured")

This, together with the unique_id, see below, stops the possibility of adding a configuration 2 times.
This requires also a few addition lines in string.json.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do prevent adding the same USB-stick in a manually started second config flow at L103-L105 together with L18-L24 which gives a nice error message:
image

I'm not sure it is user friendly to abort the config flow instead. To me aborting the configflow seems to be only usefull when automatic discovery triggers the config flow, which is not the case for serial devices.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very good! Then please ignore my comment.

@MartinHjelmare
Copy link
Copy Markdown
Member

We normally integrate all devices from the same brand within the same integration. If there are different protocols in use we use different logic paths for the different protocols and let the user pick the device category to integrate if needed.

@brefra
Copy link
Copy Markdown
Contributor Author

brefra commented Sep 20, 2020

@CoMPaTech & @bouwew do you think it feasible to integrate the support for the legacy USB-stick into the current Plugwise integration as Martin suggests?

@CoMPaTech
Copy link
Copy Markdown
Member

Prologue: Bouwe and me sort of discussed this already seeing the comment made, but didn't conclude anything yet. Enjoying 0.115 and some food/drinks with family while enjoying the sun was a higher priority most of this weekend :) So below my take on it. In summary/tldr: combining forces (for us and HA) and a single yet clear-to-configure component (for users) seems like a good idea and community spirit.

I guess we should (@MartinHjelmare please confirm) mostly create 'network' vs 'usb' versions of both the current __init__.py and the one you created and device a new one with central logic to disperse between zeroconf-discovery/usb-discovery and manual setup. For manual we should probably look at the 'dropdown' and/or some clickable item through config_flow to have the user select which Plugwise product they'd like to choose (we looked at the dropdown config_flow thing back in the days (iirc for scan_interval)).
As we have network discovery and you have USB-detection/discovery (right?) that makes the prefill/-select of config_flowworkable, we should just have a way for manual configuration. @MartinHjelmare is that what you meant with let the user pick the device category to integrate and/or do we have an example integration to look at/pick from?

Aside from that, we should remember that you toned down your PR to just switch as per requirements for new integrations so sensor and binary_sensor will be getting back (right?). Integrating the various platforms should be do-able but it might mean either {if usb: -> something_usb -> return} something_network -> return or vice versa. Choosing the right guarding (compacting and enhancing readability) vs having lots of potential duplicates is something we all have to work at I guess. But some things might be easier in other ways.

Just quickly again sifting through the PR

  • today_energy_kwh and current_power_w should be integrated (later?) to sensors
  • entity_registry_enabled_default might be something usefull for both (or all three depending on how you count) types (spotted by Bouwew)
  • unique_id isn't a big, just needs to be addressed (mac@usb vs id@network) right in the code
  • the async_setup_entry has to be properly 'if'ed/guarded as that is different, I reckon that is where the most work will be, again if we can largely 'def {usb|hub}our current __init__ parts that might be easy, but I'd like to be sure before it doesn't pass review. Again, having an example integration might be helpful (and sorry didn't take time togrep` it out of the current tree yet).

Side question - do we keep both custom_components separate, also joined (easier testing with friendly users and getting properly pre-field tested PRs to Core) or just the Core component. As the testers are very helpfull, but not all of them have the ability to run branch-code (custom_components is no biggy for them on our side) I'd opt for joined (both in custom_component and HA-core) assuming your test-users are the same or your personal setup is enough to test all. (Background: We have a lot of firmware variations, people with/-out boilers, people with plugs or circles/stealths-through-stretches, etc.)

@MartinHjelmare
Copy link
Copy Markdown
Member

Yes, if we can't detect automatically somehow what device has been connected, we can let the user pick the correct integration method, usb or network etc, in the config flow.

@CoMPaTech
Copy link
Copy Markdown
Member

Yes, if we can't detect automatically somehow what device has been connected, we can let the user pick the correct integration method, usb or network etc, in the config flow.

Yup, but what if it is configured manually? Just create two seperate forms in config_flow and hook them like homeassistant/components/hue/config_flow.py does with showing multiple bridges or manual (only in our case present an option for usb or networked device)? Talking L102/L104 setup style of that example?

@MartinHjelmare
Copy link
Copy Markdown
Member

Yes, that sounds good.

@bouwew
Copy link
Copy Markdown
Contributor

bouwew commented Sep 21, 2020

@brefra, as @CoMPaTech has written in many words ;) yes we think it's feasible.
I would suggest we implement this first in a custom_component, have it properly tested by ourselves and a group of friendly users and then move the changes into the HA Core Plugwise component.

@brefra are you on Discord?

@brefra
Copy link
Copy Markdown
Contributor Author

brefra commented Sep 21, 2020

Great, I'll accept the challenge to "merge" this PR into the current Plugwise integration, and agree to start with a merged custom_component. I do have discord account (brefra), but not really an active user to be honest.

@CoMPaTech
Copy link
Copy Markdown
Member

We have made some initial progress in our beta-environment, from that: we created a preparation PR (#41201) to allow for integration of USB. Further PRs will be made to have this PR (and the original wider platform support) included, currently the rough outline of this is in https://github.com/plugwise/plugwise-beta/pull/117/files where we welcomed @brefra as a member too to streamline our combined efforts.

@MartinHjelmare if your time allows #41201 provides scaffolding for the user step to become a selector (schema-basedvol.In) as determining the logic to disperse between USB and network. If that is acceptable we will build-up from that (scaffold/prep) PR towards switch (as intended by this PR) and sensor platforms for the full USB-support that @brefra is looking to integrate.

@brefra brefra marked this pull request as draft October 17, 2020 19:17
@brefra
Copy link
Copy Markdown
Contributor Author

brefra commented Oct 17, 2020

Marked as draft until alternative PR for the existing plugwise integration is submitted to add the USB-Stick functionality.
Some beta testing is currently happening at plugwise-beta custom integration.

@bouwew
Copy link
Copy Markdown
Contributor

bouwew commented Jan 24, 2021

@brefra Let's close this PR now that the replacement-PR is up.

@brefra brefra closed this Jan 26, 2021
@brefra brefra deleted the plugwise_stick branch December 31, 2021 14:53
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.

7 participants