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

Implement config flow in the Broadlink integration #36914

Merged
merged 43 commits into from
Aug 20, 2020

Conversation

felipediel
Copy link
Contributor

@felipediel felipediel commented Jun 18, 2020

Breaking change

1. Devices are now configured via configuration flow

To set up a Broadlink device, click Configuration in the sidebar and click Integrations.

The devices will be imported from your configuration files to that page. If you see your device there, click Configure. If not, click the + icon in the lower right, click Broadlink, enter the host and follow the instructions to complete the setup.

The name you choose will serve as a template for the entities. You can change the entity name and id in the entity settings on the frontend. You may need to change some names or ids to make everything look the same as it was before this update.

2. Discontinue broadlink.learn and broadlink.send services

remote.learn_command and remote.send_command are now registered automatically. Now you can use remote.send_command to send base64 codes.

instead of broadlink.learn

# Example old configuration.yaml
script:
  learn_tv_power:
    sequence:
      - service: broadlink.learn
        data:
          host: 192.168.0.107

use remote.learn_command

# Example new configuration.yaml
script:
  learn_tv_power:
    sequence:
      - service: remote.learn_command
        data:
          entity_id: remote.bedroom
          device: tv
          command: power

instead of broadlink.send

# Example old configuration.yaml
script:
  send_tv_power:
    sequence:
      - service: broadlink.send
        data:
          host: 192.168.0.107
          packet: JgAcAB0dHB44HhweGx4cHR06HB0cHhwdHB8bHhwADQUAAAAAAAAAAAAAAAA=

use remote.send_command

# Example new configuration.yaml
script:
  send_tv_power:
    sequence:
      - service: remote.send_command
        data:
          entity_id: remote.bedroom
          command: b64:JgAcAB0dHB44HhweGx4cHR06HB0cHhwdHB8bHhwADQUAAAAAAAAAAAAAAAA=

keep it clean

# Example new configuration.yaml
script:
  send_tv_power:
    sequence:
      - service: remote.send_command
        data:
          entity_id: remote.bedroom
          device: tv
          command: power

3. Discontinue all platforms, except switch

Entities are now registered automatically. The only exception is the switch platform, which continues to exist for RM switches. The config schema has changed. The host and type are no longer required and the name serves as a template for the entity id.

instead of:

# Example old configuration.yaml
switch:
  - platform: broadlink
    host: 192.168.0.107
    mac: 34:ea:34:b4:5d:2c
    type: rm_mini3_redbean
    switches:
      sony_tv:
        friendly_name: Sony TV
        command_on: JgAcAB0dHB44HhweGx4cHR06HB0cHhwdHB8bHhwADQUAAAAAAAAAAAAAAAA=
        command_off: JgAaABweOR4bHhwdHB4dHRw6HhsdHR0dOTocAA0FAAAAAAAAAAAAAAAAAAA=
      lg_tv:
        friendly_name: LG TV
        command_on: JgAcAB0dHB44HhweGx4cHR06HB0cHhwdHB8bHhwADQUAAAAAAAAAAAAAAAA=
        command_off: JgAaABweOR4bHhwdHB4dHRw6HhsdHR0dOTocAA0FAAAAAAAAAAAAAAAAAAA=

use this:

# Example new configuration.yaml
switch:
  - platform: broadlink
    mac: 34:ea:34:b4:5d:2c
    switches:
      - name: Sony TV
        command_on: JgAcAB0dHB44HhweGx4cHR06HB0cHhwdHB8bHhwADQUAAAAAAAAAAAAAAAA=
        command_off: JgAaABweOR4bHhwdHB4dHRw6HhsdHR0dOTocAA0FAAAAAAAAAAAAAAAAAAA=
      - name: LG TV
        command_on: JgAcAB0dHB44HhweGx4cHR06HB0cHhwdHB8bHhwADQUAAAAAAAAAAAAAAAA=
        command_off: JgAaABweOR4bHhwdHB4dHRw6HhsdHR0dOTocAA0FAAAAAAAAAAAAAAAAAAA=

The above example creates switch.sony_tv and switch.lg_tv to be controlled using the device with the MAC address 34:ea:34:b4:5d:2c. This device needs to be configured first via configuration flow.

When you finish configuring the devices you can delete all your Broadlink configuration files except the RM switches. These switches are the only platform that still exists in yaml. They won't be imported. If you delete the file, they are gone.

The problem

Many users are having trouble configuring Broadlink devices. Broadlink recently launched an RM mini 3 - the infamous 0x5f36 - that is identifical to the others, but communicates differently, and the worse: it has a lock. I recently added support for these devices, but since we don't have device discovery, the user needs to specify the device type, so that the library can control it correctly. And if the device is locked, he needs to factory reset the device to unlock it.

But how can the user find out the device type? And how can he find out that the device is locked for authentication and needs a factory reset? The short answer is "read the docs", but the right answer is "he shouldn't have to do that".

We shouldn't be getting the device type from the user. The device knows its type and sends it in response when we say "hello". But we are skipping this step and going straight to authentication.

If we said hello, we could obtain valuable information in response, such as name, type, class, MAC address, model, manufacturer and lock. We are demanding a lot of these information from the user. He should not have to worry about these technical details.

Things could be simpler. We are breaking etiquette and making they pay for it. This PR comes to fix this.

Proposed change

This PR implements a config flow in the Broadlink integration.

  • In this flow, we say "hello" to the device and we create a config entry with the information we extract from the response.

  • The user no longer needs to provide the device type and MAC address.

  • The user no longer needs to set up platforms. We forward entry setup to related domains according to the device type.

  • The user gets notified when the device is locked, and a config flow is created to assist with unlocking.

  • Information such as name, model, manufacturer and firmware version are now available on the frontend.

  • Now the device is a singleton shared by the entities. This will improve communication as we will only have one connection per device.

  • Devices and entities are registered and associated. The entities are renamed when the device is renamed, updated when the device is updated, and removed when the device is removed. Everything happens in a coordinated and harmonious way.

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

@probot-home-assistant
Copy link

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

@felipediel
Copy link
Contributor Author

This PR is a great improvement. I had to work on many things which could not be done separately without causing pain to the users. So the PR ended up getting really big. But don't panic: many of these lines are just messages for the config flow 😄

I performed local tests and everything is working 🚀

@balloob @MartinHjelmare @Danielhiversen Do you have time to review?

@frenck
Copy link
Member

frenck commented Jun 22, 2020

@felipediel Please, don't ping/mention for reviews to random people. That is not appreciated, instead have some patience while we working on the currently 400 open PRs in the organization.

.coveragerc Outdated Show resolved Hide resolved
homeassistant/components/broadlink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/device.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/strings.json Outdated Show resolved Hide resolved
homeassistant/components/broadlink/switch.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/translations/pt-BR.json Outdated Show resolved Hide resolved
homeassistant/components/broadlink/updater.py Outdated Show resolved Hide resolved
Dev automation moved this from By Code Owner to Review in progress Jun 22, 2020
@felipediel
Copy link
Contributor Author

felipediel commented Jun 22, 2020

@frenck Thanks for the advice. I understand this and I am not pushing.

Those guys are not random. I chose to torture them because they are familiar with this integration and I appreciate their reviews. If I were you, I wouldn't stare at this code for long. You can be the next 😄

@MartinHjelmare Thanks for helping me with this dinossaur. Tonight I will start working on the updates.

@felipediel felipediel marked this pull request as draft June 24, 2020 00:21
@embak embak mentioned this pull request Jun 24, 2020
8 tasks
@felipediel felipediel marked this pull request as ready for review June 30, 2020 03:31
@felipediel
Copy link
Contributor Author

@MartinHjelmare Do you think this is a good idea?

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.

Are we ready?

@felipediel
Copy link
Contributor Author

Yes.

@MartinHjelmare MartinHjelmare merged commit a2c1f08 into home-assistant:dev Aug 20, 2020
Dev automation moved this from Reviewer approved to Done Aug 20, 2020
weissm pushed a commit to weissm/core that referenced this pull request Aug 28, 2020
)

* Implement config flow in the Broadlink integration

* General improvements to the Broadlink config flow

* Remove unnecessary else after return

* Fix translations

* Rename device to device_entry

* Add tests for the config flow

* Improve docstrings

* Test we do not accept more than one config entry per device

* Improve helpers

* Allow empty packets

* Allow multiple config files for switches related to the same device

* Rename mock_device to mock_api

* General improvements

* Make new attempts before marking the device as unavailable

* Let the name be the template for the entity_id

* Handle OSError

* Test network unavailable in the configuration flow

* Rename lock attribute

* Update manifest.json

* Import devices from platforms

* Test import flow

* Add deprecation warnings

* General improvements

* Rename deprecate to discontinue

* Test device setup

* Add type attribute to mock api

* Test we handle an update failure at startup

* Remove BroadlinkDevice from tests

* Remove device.py from .coveragerc

* Add tests for the config flow

* Add tests for the device

* Test device registry and update listener

* Test MAC address validation

* Add tests for the device

* Extract domains and types to a helper function

* Do not patch integration details

* Add tests for the device

* Set device classes where appropriate

* Set an appropriate connection class

* Do not set device class for custom switches

* Fix tests and improve code readability

* Use RM4 to test authentication errors

* Handle BroadlinkException in the authentication
leikoilja pushed a commit to leikoilja/core that referenced this pull request Sep 6, 2020
)

* Implement config flow in the Broadlink integration

* General improvements to the Broadlink config flow

* Remove unnecessary else after return

* Fix translations

* Rename device to device_entry

* Add tests for the config flow

* Improve docstrings

* Test we do not accept more than one config entry per device

* Improve helpers

* Allow empty packets

* Allow multiple config files for switches related to the same device

* Rename mock_device to mock_api

* General improvements

* Make new attempts before marking the device as unavailable

* Let the name be the template for the entity_id

* Handle OSError

* Test network unavailable in the configuration flow

* Rename lock attribute

* Update manifest.json

* Import devices from platforms

* Test import flow

* Add deprecation warnings

* General improvements

* Rename deprecate to discontinue

* Test device setup

* Add type attribute to mock api

* Test we handle an update failure at startup

* Remove BroadlinkDevice from tests

* Remove device.py from .coveragerc

* Add tests for the config flow

* Add tests for the device

* Test device registry and update listener

* Test MAC address validation

* Add tests for the device

* Extract domains and types to a helper function

* Do not patch integration details

* Add tests for the device

* Set device classes where appropriate

* Set an appropriate connection class

* Do not set device class for custom switches

* Fix tests and improve code readability

* Use RM4 to test authentication errors

* Handle BroadlinkException in the authentication
@labachev
Copy link

labachev commented Sep 17, 2020

Hello there,

Power Strip MP2 was supported before 0.115 by defining it as a type “sp2”.
Now type cannot be defined anymore and the device cannot be used.
It is recognized as some device but it does not add any devices or entities.
The only possible option is to rename it.
IMG_0260

Is there any workaround for this issue?

Thanks

@felipediel
Copy link
Contributor Author

felipediel commented Sep 17, 2020

Please open a terminal on your computer, install the python-broadlink library (pip install broadlink), open Python 3 and type:

import broadlink as blk

devs = blk.discover(timeout=5)
print([(d.host[0], hex(d.devtype)) for d in devs])

devs = blk.discover(discover_ip_address="192.168.0.17", timeout=5)  # Your device IP address
print([(d.host[0], hex(d.devtype)) for d in devs])

What is the output?

@felipediel
Copy link
Contributor Author

Use your computer. I suggest you to create a venv.

python3 -m venv venv
source venv/bin/activate
pip3 install broadlink

Now you can type python3 and use the terminal. If you want to make a script, you should consider adding #!/usr/bin/python3 to the top. If it does not work you should use which python3 to find the location.

@labachev
Copy link

Returns empty brackets "[]"

@felipediel
Copy link
Contributor Author

Even if you try the second option? Are you using a VPN?

@labachev
Copy link

labachev commented Sep 17, 2020

[('192.168.1.48', '0x7540')]
Does this help?

@felipediel
Copy link
Contributor Author

@labachev Thank you! Now we discover the devices to get the information we need.

I added your device to the library. It will be supported soon. You can follow up here.

@felipediel
Copy link
Contributor Author

felipediel commented Sep 17, 2020

Also after the changes in the configuration my RM3 pro does not work. It does not send anything IR nor RF

Oh gosh, are you sure? What is your device type? Please check if the entity_id is correct first.

@labachev
Copy link

Actually it does not find it at all. The device is RM PRO. The script sees it anyway. device type '0x279d'.

the config is under switch domain:

  • platform: broadlink
    mac: !secret mac_rm_pro
    switches:
    • name: TV Sam RM
      command_on: my_command
      command_off: my_command

@labachev
Copy link

Should I manually add the existing device and re-learn all of my commands(IR/RF)?

@felipediel
Copy link
Contributor Author

felipediel commented Sep 17, 2020

You don't need to relearn the commands. Did you add the device via config flow? Did you check the MAC address?

@labachev
Copy link

After re-configure via Integrations, it is working. Sorry for bothering you. Do you know when the MP2 change will be released? in next patch or later? I am asking because it is connected to a lot of automations I have. :(

@felipediel
Copy link
Contributor Author

I have no control over releases. I can help you to workaround:

  1. Open a terminal and connect to your device via SSH.
ssh USERNAME@HOST
  1. Access the python-broadlink library:
docker exec -it homeassistant /bin/bash
cd /usr/local/lib/python3.8/site-packages/broadlink

Now you are in the python-broadlink library folder. You can manually add your type to the file __init__.py. Just make sure to backup your config first.

@labachev
Copy link

I am using HASSIO but not docker installation.
Can I try to use it as custom_component?

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Sep 17, 2020
@MartinHjelmare
Copy link
Member

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

9 participants