Skip to content

Fix connection problems in the Broadlink integration#34670

Merged
MartinHjelmare merged 13 commits intohome-assistant:devfrom
felipediel:fix_connection
May 13, 2020
Merged

Fix connection problems in the Broadlink integration#34670
MartinHjelmare merged 13 commits intohome-assistant:devfrom
felipediel:fix_connection

Conversation

@felipediel
Copy link
Copy Markdown
Contributor

@felipediel felipediel commented Apr 25, 2020

The problem

Some users are experiencing connection problems with Broadlink devices. The root of these problems is that we are ignoring the error codes that are being sent by the device in its responses.

Let's take a practical example. We need authorization to send a command. If we send a command without being authorized we will receive an authorization error. It's simple to deal with it, we just need to re-authenticate and send the command again. But currently we are not handling this exception. So once the connection is closed for some reason, the user needs to restart Home Assistant to make the device work again.

Here is a real case. Once @AndroVet's device closes the connection, the device object breaks silently because it doesn't know it has been logged out.

Proposed change

I updated the library to propagate errors in the response as exceptions. Now we just need to deal with these exceptions in Home Assistant. These are the changes I propose:

  • Create a separate class to handle authentication, availability and requests to the device.
  • This class will handle any communication errors that may occur and keep availability up to date.
  • Import this class in the platforms and use it as a bridge to send requests to the device.

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
Copy Markdown

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!

Copy link
Copy Markdown
Contributor Author

@felipediel felipediel left a comment

Choose a reason for hiding this comment

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

@Danielhiversen How about we clean the airstrip for the next upgrade?

Comment thread homeassistant/components/broadlink/__init__.py Outdated
@felipediel felipediel changed the title Fix Broadlink switches Fix connection problems in the Broadlink integration May 7, 2020
@Danielhiversen
Copy link
Copy Markdown
Member

Broadlink library 0.14.0 is now released

Comment thread homeassistant/components/broadlink/device.py
@felipediel felipediel marked this pull request as ready for review May 11, 2020 00:02
Comment thread homeassistant/components/broadlink/device.py
@felipediel
Copy link
Copy Markdown
Contributor Author

@Danielhiversen All ready. Is there any chance of merging this update this week so I can proceed with the config entries?

@MartinHjelmare @springstan Your reviews would be appreciated.

Comment thread homeassistant/components/broadlink/remote.py
Comment thread homeassistant/components/broadlink/sensor.py Outdated
Comment thread homeassistant/components/broadlink/sensor.py Outdated
Comment thread homeassistant/components/broadlink/switch.py Outdated
Comment thread homeassistant/components/broadlink/switch.py Outdated
Comment thread homeassistant/components/broadlink/switch.py Outdated
Comment thread homeassistant/components/broadlink/switch.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

The changes look very good, in general!

@MartinHjelmare MartinHjelmare merged commit 6464c94 into home-assistant:dev May 13, 2020
@lock lock Bot locked and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broadlink switches not working after upgrading to 0.106.0

5 participants