Skip to content

Clean up custom polling in ZHA device and light#32653

Merged
dmulcahey merged 19 commits intohome-assistant:devfrom
dmulcahey:dm/zha-cleanup
Mar 11, 2020
Merged

Clean up custom polling in ZHA device and light#32653
dmulcahey merged 19 commits intohome-assistant:devfrom
dmulcahey:dm/zha-cleanup

Conversation

@dmulcahey
Copy link
Copy Markdown
Contributor

@dmulcahey dmulcahey commented Mar 10, 2020

Proposed change

This PR cleans up custom polling usage in the ZHA device and in the ZHA light entity class. In device we weren't cleaning up the timer when the device was removed from HA.

In the light entity we had a few issues:

  • We had custom polling configured with a static time which would cause all lights to refresh around the same time. On networks with lots of bulbs this can be bad especially if there are any forms of interference that cause poor network performance in general
  • We were defining SCAN_INTERVAL and PARALLEL_UPDATES even though should_poll is false. I left PARALLEL_UPDATES in light.py at this point but I think it should be removed or cranked way up. I think this only applies to async_update calls from polling which is disabled and to update_before_add=True calls when adding entities. Considering all this does is a cache read in memory maybe we should crank the # up in another PR? @balloob @MartinHjelmare any guidance you can offer on this would be appreciated.
  • We were using HA's SCAN_INTERVAL for our custom polling interval - this could be misleading to someone looking at the integration
  • We weren't cleaning up the timer when the entities were removed from HA

This PR corrects the above issues.

This PR also fixes writing state to the HA state machine based on this comment: #32291 (comment) This should remove a bunch of unnecessary awaits from ZHA.

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

Example entry for configuration.yaml:

# Example configuration.yaml

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 @Adminiuga, mind taking a look at this pull request as its been labeled with a integration (zha) you are listed as a codeowner for? Thanks!

@dmulcahey dmulcahey changed the title Cleanup custom polling in ZHA device and light WIP - Cleanup custom polling in ZHA device and light Mar 10, 2020
@dmulcahey dmulcahey changed the title WIP - Cleanup custom polling in ZHA device and light Cleanup custom polling in ZHA device and light Mar 10, 2020
Copy link
Copy Markdown
Contributor

@Adminiuga Adminiuga left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread homeassistant/components/zha/core/device.py
@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2020

Set PARALLEL_UPDATES to 0 to disable it. It is used to throttle the number of parallel calls we make into your lib via either update or turn on/off or other service calls.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2020

btw the default for async integrations is to set parallel updates to 0, assuming you have your own locking mechanism inside the lib.

Comment thread homeassistant/components/zha/core/device.py Outdated
}


@asynctest.mock.patch(
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.

The diff will be smaller in the future, when we reach Python 3.8 as minimum version and can replace asynctest with std lib again, if we import patch from asynctest.

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.

Will we address this later in a future PR?

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.

Yes. I’d like to get this PR into the beta release

Comment thread tests/components/zha/test_light.py Outdated
Comment thread tests/components/zha/test_light.py Outdated
Comment thread homeassistant/components/zha/light.py
Comment thread tests/components/zha/test_light.py
Comment thread tests/components/zha/test_light.py Outdated
@springstan springstan changed the title Cleanup custom polling in ZHA device and light Clean up custom polling in ZHA device and light Mar 11, 2020
@dmulcahey
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare I am merging this PR now and i'll immediately follow up with another to change that import and cleanup something else you caught after an earlier PR. I want to ensure this PR hits the beta. Thanks for all that you do and for engaging in good discussion!

@dmulcahey dmulcahey merged commit 4248893 into home-assistant:dev Mar 11, 2020
@dmulcahey dmulcahey mentioned this pull request Mar 11, 2020
20 tasks
@lock lock Bot locked and limited conversation to collaborators Mar 12, 2020
@dmulcahey dmulcahey deleted the dm/zha-cleanup branch March 26, 2020 11:17
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.

5 participants