Skip to content

Add support for topdown shades to hunterdouglas_powerview#62788

Merged
bdraco merged 63 commits intohome-assistant:devfrom
kingy444:add-powerview-topdown
May 30, 2022
Merged

Add support for topdown shades to hunterdouglas_powerview#62788
bdraco merged 63 commits intohome-assistant:devfrom
kingy444:add-powerview-topdown

Conversation

@kingy444
Copy link
Copy Markdown
Contributor

@kingy444 kingy444 commented Dec 26, 2021

Breaking change

Top Down/Bottom Up shades will now have an entity to control each motor. Users will need to manually remove any old entities by selecting the unavailable entities from the Home Assistant Interface, selecting REMOVE ENTITY and then confirming the removal by clicking REMOVE

  • Users who have automations to set shade position based on entity id will need to reconfigure these as the new entities will be named differently.
  • Users who only have automations set to trigger scenes do not need to reconfigure automations.

Proposed change

Add Top/Down Bottom/Up support to powerview
Create separate entities for top/down bottom/up blinds so both motors can be controlled
Fix shadetype being displayed as number and not string

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

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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

To help with the load of incoming pull requests:

@kingy444 kingy444 requested a review from bdraco as a code owner December 26, 2021 05:22
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @kingy444,

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!

@probot-home-assistant
Copy link
Copy Markdown

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

@kingy444
Copy link
Copy Markdown
Contributor Author

kingy444 commented Jan 7, 2022

Just wanted to update because I didn’t want to seem rude and appreciate your quick review to start with.

I will make the required changes and update - just a little delay from my side with a fried motherboard - will have to set my lab back up etc but hopefully get back to you soon

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 7, 2022

No rush. I'm pretty swamped right now so it will take a while to doing another review pass on another turn. Next week is better if there is any follow up needed

@kingy444
Copy link
Copy Markdown
Contributor Author

I have made the changes suggested, removing options flow and setting disabled by default.

This did introduce a breaking change i was trying to avoid - but one i thing should really be there anyway.

Essentially users that have a top down/bottom up motor will need to reconfigure any automations etc they already had setup as the new entity wont work with their old config.

@kingy444 kingy444 requested a review from bdraco May 28, 2022 14:28
@kingy444
Copy link
Copy Markdown
Contributor Author

🤞 hopefully this has it across the line
Thanks for your input with the data coordinator

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 28, 2022

I found a few more issues in testing:

  • We update the state before the move call is successful
  • supported features is wrong for v1 hub (no stop support)
  • when you call open/close opening/closing isn't updated
  • we already get the data back from the move response so we end up processing it twice

I've fixed the issues I found in bdraco@22ca42e

I was unsure of one part here bdraco@22ca42e#r74809571

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 28, 2022

@kingy444

🤞that last pass doesn't break anything since I don't have the tdbu or tilts to test with and I'd really like to get this merged

@kingy444
Copy link
Copy Markdown
Contributor Author

kingy444 commented May 30, 2022

🤞that last pass doesn't break anything since I don't have the tdbu or tilts to test with and I'd really like to get this merged

@bdraco worked really well thanks - only a few small things i needed to add to support proper open/close positioning.
Mainly needing to send some extra positioning at the moment but I have also done a lot of work with people on the forums to get proper support for all shade types and many of these need uniquie open/close positions to be sent and not just 0 / 100 positioning.

Hopefully this small change works for you - tried to keep it as simple as i could

also just took the opportunity to rename get_transition_steps to transition_steps based on your previous feedback that as a property 'get' is the only thing they do anyway

edit: overall i tried to think how i need to implement the other shade models too - and only made changes i see required for the support of all shades

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 30, 2022

Your changes look fine. It meets the goal of avoiding a lot of boiler plate when new models are added so it works for me.

I'll do some testing in a bit.

Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Testing looks good with my devices.

Thanks @kingy444

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 30, 2022

I checked the docs are we don't mention which device types are supported. It would be good add that at some point so we don't get lots of feature requests for everyone's models they assume are supported

https://github.com/home-assistant/home-assistant.io/blob/next/source/_integrations/hunterdouglas_powerview.markdown

@bdraco bdraco changed the title Add powerview topdown Add support for topdown shades to hunterdouglas_powerview May 30, 2022
@bdraco bdraco merged commit 45e4dd3 into home-assistant:dev May 30, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2022
@kingy444 kingy444 deleted the add-powerview-topdown branch June 1, 2022 15:30
@frenck
Copy link
Copy Markdown
Member

frenck commented Jun 2, 2022

@kingy444 @bdraco This PR is marked as a breaking change, but the breaking change description is missing in the PR description.

Could you provide a breaking change description that we can use for the release notes? Aimed towards the end-user?

Thanks 👍

../Frenck

@home-assistant home-assistant unlocked this conversation Jun 2, 2022
@kingy444
Copy link
Copy Markdown
Contributor Author

kingy444 commented Jun 2, 2022

Sorry about that - this PR changed a lot over the last 6 months so glad to get it there finally. Can provide more information if needed but figured this one is pretty straightforward so keep it simple.

The new support for Top Down Bottom Up Shades will provision 2 new entities, one for each motor. Stale entities from previous releases can either be removed manually one by one, or users can remove the entire Hunter Douglas Powerview Integration and reconfigure as a new hub.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jun 2, 2022

@frenck Thanks for catching that

@kingy444 I edited the PR description a bit. Can you clean it up a bit more to give better instructions on what they need to do. I'm not sure anyone will have to do much as these didn't really work well before.

@kingy444
Copy link
Copy Markdown
Contributor Author

kingy444 commented Jun 2, 2022

@frenck Thanks for catching that

@kingy444 I edited the PR description a bit. Can you clean it up a bit more to give better instructions on what they need to do. I'm not sure anyone will have to do much as these didn't really work well before.

Done

Also PR home-assistant/home-assistant.io#22944 in for updating the doco on supported shades.

We probably want to get it merged into current though as its current content relates to the Silhoutte shades going live in 2022.6

I wouldnt be surprised if these cause people to request help as tilting doesnt work as expected for them

The pull currently has tdbu listed as unsupported - which we would change when this PR is live.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 3, 2022
@bdraco bdraco added the noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Jun 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking-change cla-signed integration: hunterdouglas_powerview new-feature noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants