Skip to content

Handle HomeKit configuration failure more cleanly#14041

Merged
balloob merged 2 commits intohome-assistant:devfrom
mjg59:homekit
Apr 22, 2018
Merged

Handle HomeKit configuration failure more cleanly#14041
balloob merged 2 commits intohome-assistant:devfrom
mjg59:homekit

Conversation

@mjg59
Copy link
Copy Markdown
Contributor

@mjg59 mjg59 commented Apr 22, 2018

Add support for handling cases where HomeKit configuration fails, and give
the user more information about what to do.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

indentation contains mixed spaces and tabs

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

indentation contains tabs
indentation contains mixed spaces and tabs

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

indentation contains mixed spaces and tabs

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

indentation contains tabs
indentation contains mixed spaces and tabs

Add support for handling cases where HomeKit configuration fails, and give
the user more information about what to do.
Copy link
Copy Markdown
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

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

Looking good 👍 Just a small comment that might help with debugging issues in the future - I don't know the homekit pypi module so the comment might very well also not be necessary.

except homekit.exception.UnknownError:
error_msg = "Received an unknown error. Please file a bug."
_configurator = self.hass.data[DOMAIN+self.hkid]
self.configurator.notify_errors(_configurator, error_msg)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's an unknown error, wouldn't it be good to print the stack trace? If I understand correctly, when an UnknownError is thrown the only message that's displayed is "Received an unknown error. [...]" and debugging an issue is going to be difficult with just that error message.

Maybe replace the return on the line below withraise so that a) the configurator still shows an error but b) the complete stack trace is shown?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That seems reasonable. The problem with UnknownError is that the device literally returned what the HomeKit spec defines as an unknown error, so it's unlikely that we're going to get much more from the stack trace, but it's going to be better than nothing.

If we get an UnknownError then we should alert the user but also still
generate the backtrace so there's actually something for them to file in
a bug report.
@balloob balloob added this to the 0.68 milestone Apr 22, 2018
@balloob balloob merged commit e4cb3af into home-assistant:dev Apr 22, 2018
balloob pushed a commit that referenced this pull request Apr 25, 2018
* Handle HomeKit configuration failure more cleanly

Add support for handling cases where HomeKit configuration fails, and give
the user more information about what to do.

* Don't consume the exception for a homekit.UnknownError

If we get an UnknownError then we should alert the user but also still
generate the backtrace so there's actually something for them to file in
a bug report.
@balloob balloob mentioned this pull request Apr 27, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
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.

6 participants