Skip to content

Catch exception on ZHA switch setup#16474

Closed
ryanwinter wants to merge 3 commits intohome-assistant:devfrom
ryanwinter:zha_switch
Closed

Catch exception on ZHA switch setup#16474
ryanwinter wants to merge 3 commits intohome-assistant:devfrom
ryanwinter:zha_switch

Conversation

@ryanwinter
Copy link
Copy Markdown

Description:

HA attempts to read the device state on startup, however the devices I have, this throws and exception which means the device add never completes and the device is inoperable.

This change catches the exception so that the device will still be added regardless.

Related issue (if applicable): NA

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io# NA

Example entry for configuration.yaml (if applicable):

NA

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@Adminiuga
Copy link
Copy Markdown
Contributor

Should it bind/configure_reporting only on new_join == True and not every time on Hass restart? Essentially making it to behave like the rest of zha components?

@ryanwinter
Copy link
Copy Markdown
Author

That's a good question. I don't understand the logic behind this function so was looking to just catch the error instead. If these calls should be hated another way I can update the pr.

@fabaff fabaff changed the title ZHA switch: catch exception on device setup Catch exception on ZHA switch setup Sep 7, 2018
@MartinHjelmare
Copy link
Copy Markdown
Member

We need to understand why something errors before we can decide how to make a fix.

@ryanwinter
Copy link
Copy Markdown
Author

That's not true, we can patch the problem to at least get this working again for the bunch of people that are impacted, and then take a look at the exception itself in a second pr.

@Adminiuga
Copy link
Copy Markdown
Contributor

That's a good question. I don't understand the logic behind this function

@ryanwinter This code tells a Zigbee device to push status updates to Coordinator/Hass. For this to work the Zigbee device which pushes the updates needs to know a few things:

  1. Address where to send the update
  2. Which ZCL cluster should report the update.
    the above two are configured through cluster.bind() method. After that the zigbee device needs to be told for what attributes (specific to the above cluster) it should sends the updates, how quickly after the value of the said attribute changes it needs to send the update -- "min_interval", how often to send updates if there are no changes -- "max_interval" and amount of change which triggers a report -- "reportable_change" (as you may not necessarily send an update if temperature changes 0.1C)

Now, this doesn't need to be configured every time, as the above settings are stored in NVRAM on zigbee device, that's why other sensors (binary_sensor/zha.py, sensor/zha.py) do this right away when device joins the network which means the device is available to receive request at least at that point of time. For example sensor/zha.py

    if discovery_info['new_join']:
        cluster = list(in_clusters.values())[0]
        yield from cluster.bind()
        yield from cluster.configure_reporting(
            sensor.value_attribute, 300, 600, sensor.min_reportable_change,
        )

It is expected in Zigbee networks that some requests would fail with "DeliveryError" when devices are unreachable either because they are offline (smart plug unplugged) or weak Zigbee network, interference etc, but it shouldn't cause the application to fail. So yes, catching "DeliveryError" is fine, but at the same time, switch/zha.py should bind()/configure_reporting() every restart as is is not needed and it puts unnecessary traffic on the zigbee network (for every switch you configured) when there's already burst of traffic because of all update_before_add=True in async_add_entitities() for zha devices. The later should be alleviated somewhat with zigpy attribute caching, but still there's a spike of zigbee traffic when ZHA restarts, so no need to put extra unnecessary traffic.

@ryanwinter
Copy link
Copy Markdown
Author

Thanks for that background @alanbowman!

What I'll do is:

  1. Remove the exception catching, and move the two await calls to the await_sensor function gated on the new_join as recommend.
  2. If I still see the exceptions, I will add the try catch just to make HA more resilient to 3rd party library calls.

Linking bellows issue, likely this fix will resolve it:
zigpy/bellows#118

@Adminiuga
Copy link
Copy Markdown
Contributor

@ryanwinter FYI binary_sensor.zha does swallow the exceptions for bind/configure_reporting()
And since most zha devices could benefit from pushing the state updates via attribute reporting, then all of them (with the exception of light and fan component currently) are using the same code to bind/configure_reporting() and that's why I'm proposing #16487 to share that code among different zha components.
If you could give it a test drive and feedback, that would be greatly appreciated. In the next PR which i'm currently testing, I'm planning for each component just to supply a dict of [cluster][attributes] to be configured for reporting, which would simplify component code even more.

@ryanwinter
Copy link
Copy Markdown
Author

@Adminiuga, Looks good to me. Do you want me to push my change into your branch?

@Adminiuga
Copy link
Copy Markdown
Contributor

@ryanwinter yep, if you could test my PR in your environment, then push change into my branch and provide feedback. More feedback is always better.

@ryanwinter
Copy link
Copy Markdown
Author

Submitted the PR, took a look at your PR and it looks good to me.

@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 12, 2018

So we can close this one?

@ryanwinter
Copy link
Copy Markdown
Author

Closing, merged into other pr.

@ryanwinter ryanwinter closed this Sep 12, 2018
@ghost ghost removed the in progress label Sep 12, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed integration: zha small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants