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

Add Broadlink cover platform #30156

Closed
wants to merge 8 commits into from
Closed

Conversation

embak
Copy link

@embak embak commented Dec 22, 2019

Description:

This PR introduces a new platform for controlling IR/RF covers using 'Broadlink' implementation for RM devices. I know, we already have the 'Broadlink' switch platform and 'Template Cover' that can be combined to do pretty much the same. I consider this an enhancement to the excellent work done by mjg59 and Danielhiversen.

This platform introduces the following improvements:

  1. Instead of being hidden behind switches, the Broadlink cover is a fully integrated HA cover. As a result, only a single component is needed instead of 3 ( 2 switches+1 cover template) for a cover with up / down / stop control.
  2. When provided with the opening and closing time configuration parameters, this platform handle position control. This is an 'optimistic mode' implementation, as the cover position is maintained internally. This time based control is fully integrated with the UI and more responsive.
  3. Previously custom automations where required to achieve timer based position control. Resulting position precision is within 0,5 sec. The estimated cover position is adjusted accordingly.
  4. Travelling can be stopped at any time and the resulting position is also returned.
  5. As such it integrate seamlessly like the other regular cover in scenes or automations.
  6. Support for main cover and tilt.

I successfully used this cover platform, as a custom component, trough UI, scenes, automations and voice control for more than 9 months with 4 RF covers.

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#11517

Example entry for configuration.yaml :

cover:
  - platform: broadlink
    host: IP_ADDRESS
    mac: MAC_ADDRESS
    timeout: 10
    covers:
      # Cover without position control.
      dining_room_window:
        friendly_name: "Dining room window"
        command_open:  'sgOEAwwYDBgMGwYDBgLGRcNCgMGQsYGAAF3AAAAAA='
        command_close: 'sgOEAwsZCxDAwYDBgYDBcNDBcMGBgMGAAF3AAAAAA='
        command_stop:  'sgSEYFw0MFxgNCxkerghuhttgXDAwYGAAF3AAAAAA='
      # Cover with position control. 
      kitchen_window:
        friendly_name: "Kitchen Window"
        command_open:  'sgBuAQwYDBgMGBgMDBgMFMGAwYDBgMDAAF3AAAAAAAAAAAAAA='
        command_close: 'sgAeAQ0XDRcNFw0WDRcZCNFCxkLGQsNFw0ABdwAAAAAAAAAA'
        command_stop:  'sgDgAAwYFw0XDQsGA0LGYDAwMFxgNCxgYAAXcAAAAAAAAAAA='
        opening_time: 9.5
        closing_time: 9
      # Cover with position and tilt open / close control
      living_room_window:
        friendly_name: "Living Room Window"
        command_open:  'sgCwAaBRNQsVCwsWChYKFgsVCwwUFgsF3AAAAAAAAAAA'
        command_close: 'sgByAgsVCxUWCwsVDxYLFRULFcgyhCxUWAAAAAAAAAAA'
        command_stop:  'sgDsABULFgoWCgwVFRYULFQdVFgoMFBYAAAAAAAAAAAA'
        opening_time: 18
        closing_time: 12
        tilt_command_open: 'sgOEAwwYDBgMGBwLGQsYGAwMGAwYDBgMGBcNAAAA'
        tilt_command_close: 'sgDsABULFgoWCgoLFRQwdrVFgoMFBYAAAAAAAAAA'```

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.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

@homeassistant
Copy link
Contributor

Hi @embak,

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

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

@frenck
Copy link
Member

frenck commented Dec 23, 2019

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

@embak
Copy link
Author

embak commented Dec 23, 2019

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

Done. Thanks.

@springstan springstan changed the title Broadlink cover platform Add Broadlink cover platform Jan 2, 2020
@stale
Copy link

stale bot commented Mar 21, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Mar 21, 2020
@frenck
Copy link
Member

frenck commented Mar 21, 2020

Thanks, stalebot, but this PR awaits an initial review from us.
Please keep open for now.

@stale stale bot removed the stale label Mar 21, 2020
}
)

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(
Copy link
Member

Choose a reason for hiding this comment

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

We no longer accept additions or changes to the yaml config of integrations. See: https://github.com/home-assistant/architecture/blob/master/adr/0010-integration-configuration.md

This integration needs to be refactored to use a config flow and config entries.

https://developers.home-assistant.io/docs/config_entries_config_flow_handler

Copy link
Contributor

Choose a reason for hiding this comment

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

I am working on the refactor. Please hold this PR a little longer so we can integrate platforms.

Copy link
Author

Choose a reason for hiding this comment

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

When submitted last December this PR purpose was for a platform addition to the Broadlink integration. With this April 14 ADR10, my understanding is that the scope of this PR needs to be extended to a complete refactoring of the entire Broadlink integration to make it an UI Integration.
@felipediel is it what you meant by refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

@embak Home Assistant has changed and we need to adapt the Broadlink integration to be configured via config flow. I'm working on the update. When I'm done I'll be back here to help you adapt the code.

Copy link
Author

Choose a reason for hiding this comment

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

This PR will be refactored pending config flow implementation in the Broadlink integration. ( #36914 )

Copy link
Member

Choose a reason for hiding this comment

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

Can we in meantime close this PR and re-open when ready?

As we keep looking at things, that cannot move forward at this point, which is highly inefficient for us reviewers.

Thanks for considering 👍

Dev automation moved this from Needs review to Review in progress Apr 15, 2020
@MartinHjelmare MartinHjelmare self-assigned this Apr 19, 2020
@stale
Copy link

stale bot commented May 19, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added stale and removed stale labels May 19, 2020
@stale
Copy link

stale bot commented Jun 23, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added stale and removed stale labels Jun 23, 2020
@stale
Copy link

stale bot commented Jul 25, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Jul 25, 2020
@MartinHjelmare
Copy link
Member

Closing here for now. Ping me when we can move forward.

Dev automation moved this from Review in progress to Cancelled Jul 25, 2020
@embak embak deleted the broadlink-cover branch August 9, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

6 participants