Skip to content

Raise exceptions for errors#356

Merged
Danielhiversen merged 5 commits intomjg59:masterfrom
felipediel:exceptions
May 7, 2020
Merged

Raise exceptions for errors#356
Danielhiversen merged 5 commits intomjg59:masterfrom
felipediel:exceptions

Conversation

@felipediel
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

@Danielhiversen @mjg59 What do you think about this?

@felipediel
Copy link
Copy Markdown
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
Copy Markdown
Owner

mjg59 commented May 1, 2020

This looks extremely good to me.

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

Is it still a draft?

@felipediel
Copy link
Copy Markdown
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
Copy Markdown
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