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

Raise exceptions for errors #356

Merged
merged 5 commits into from
May 7, 2020
Merged

Conversation

felipediel
Copy link
Collaborator

@felipediel felipediel commented May 1, 2020

The problem

Things are failing silently. When we send a request and an error occurs, the device sends an error code in the response. This is valuable information for dealing with the problem. But this is being ignored in the current implementation. Errors are passing by without leaving clues.

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 (0xfff9) in the response. It's simple to deal with this error, we just need to re-authenticate and send the command again. But it is currently not possible to catch this error, so we cannot deal with it.

Here is a real case. Once @AndroVet's device closes the connection for some reason, the device object breaks silently (because it doesn't know it has been logged out). @AndroVet is being logged out due to the large number of connections, probably. We are not detecting this error and his switches are failing silently. We should inform the integrations when an error occurs so they can deal with it.

Proposed changes

My suggestion is to raise an exception whenever the device informs an error. In this way, integrations will have material to decide how to respond. I found the error codes in the official documentation. I compared some with error codes that I captured in a controlled environment. They match.

These are the changes I propose:

  • Create a separate file called exceptions.py to keep thing organized.
  • Create a common base class for all Broadlink exceptions.
  • Create a Broadlink exception for each type of error.
  • Create a dictionary that maps error codes to exceptions.
  • Create a helper function to serve as an interface for this dictionary.
  • Create a function that checks for errors and raises the Broadlink exception corresponding to the error found.
  • Use this function to check all responses received from the device.

Breaking changes

These are breaking changes because we are gonna start raising exceptions, we will no longer fail silently as before. Some minor changes will be needed in the integrations. I commit to bringing these changes to Home Assistant.

@felipediel felipediel changed the title Exceptions Raise exceptions for errors May 1, 2020
@felipediel
Copy link
Collaborator Author

@Danielhiversen @mjg59 What do you think about this?

@felipediel
Copy link
Collaborator Author

I will move to draft until I make fine adjustments in Home Assistant. But you can review the code anyway, since I don't intend to change it anymore.

@felipediel felipediel marked this pull request as draft May 1, 2020 07:21
@mjg59
Copy link
Owner

mjg59 commented May 1, 2020

This looks extremely good to me.

Copy link
Collaborator

@Danielhiversen Danielhiversen left a comment

Choose a reason for hiding this comment

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

@felipediel
You are doing a great job improving this library.
Thanks

@Danielhiversen
Copy link
Collaborator

Is it still a draft?

@felipediel
Copy link
Collaborator Author

@Danielhiversen This code is ready, but we still have to update Home Assistant to handle these exceptions. I'm gonna mark it as ready and work on the updates tonight.

@felipediel felipediel marked this pull request as ready for review May 4, 2020 18:40
@felipediel felipediel marked this pull request as draft May 5, 2020 23:40
@felipediel felipediel marked this pull request as ready for review May 6, 2020 22:55
@felipediel
Copy link
Collaborator Author

Here we go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants