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

ZHA component rewrite #20434

Merged
merged 24 commits into from Feb 6, 2019

Conversation

@dmulcahey
Copy link
Contributor

dmulcahey commented Jan 25, 2019

The intent of this PR is to split Zigbee from HA stuff in the ZHA component and to make the zha component easier to test so that we can cover it with a true test suite. Doing this should allow us to have a better internal model in the ZHA component and it should prevent us from running into (read: implementing / causing) things that violate the HA architecture. There is no added functionality in this PR.

Points of interest:

  • core/listeners contains most of the zigbee interaction
  • listeners don't directly touch entities... all data is pushed via dispatchers
  • entities can reach into a listener to retrieve state as they see fit
  • the HA component modules have been streamlined and simplified and only deal with HA stuff
  • the api has been updated to work with the new internal model but it is not in it's final state. It hasn't been cleaned up the way the rest of ZHA has. It will be the next thing tackled if these changes are welcomed
  • there is a clear and consistent life cycle for zigbee device joining/restoring and entity creation. I'll document it and attach it to the PR

EDIT

I know this PR is humongous and I am sorry for that... If the changes are accepted I'll do my best to find a way to find a way to fracture this into smaller PR's. Given how things are fundamentally different I am not sure how easy this will be to do.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 25, 2019

The diff is way too big to keep track of. We can't do this in one PR, especially when there are no tests for zha.

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 25, 2019

@MartinHjelmare did you even read the description? I acknowledged that myself and stated that I’d do my best to fracture this into small parts. I don’t even want to bother trying if everyone thinks this is heading in a bad direction.

I’m looking for feedback and I’ll gladly take any advice you have on how to break up a full rewrite of a component into smaller PR’s. Especially when it changes the entire way the component works. It’s hard to make changes like this in pieces because we’re fundamentally changing the ZHA architecture and starting with pieces can end at dead ends when you tackle later parts. I did this like this to prove out that the solution would work for the entire component. And again, I will attempt to break it up.

I get the tests comment and that’s a stated goal above. I’ll be writing a full test suite after we get a Zigpy release that fixes a circular import error.

Honestly, I’m trying to not get aggravated because you have helped me a ton previously but comments like this aren’t helpful in the slightest and they make me question the amount of time I’m spending trying to add things to ZHA.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 25, 2019

I'm saying I think this is too big to keep track of and do a review. You ask me to review and have an opinion. My answer is that it's too big for me to do that and have that.

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 26, 2019

@MartinHjelmare Sorry if I misinterpreted your initial comments. How is this for a plan of attack:

  1. I'll open a PR that just moves existing files to their final destinations. There won't be any changes aside from fixing imports. The diffs will be large but they will also be irrelevant. #20456
  2. The next PR will introduce the gateway module and some code will move from the init module to that. Still no changes just moving. #20460
  3. The next PR will update the helpers module. #20463
  4. The next PR will introduce the device object. It will just add the module and it will not implement it. So we'll have an extra module in the component that isn't being used yet but it will only be 1 thing to look at. #20469

Here is where I struggle to do anything else in pieces. In order to split zigbee interaction from HA entity classes I need to make sweeping changes to the internals of the gateway module mentioned above, create the new module that handles the zigbee interaction (listeners.py) and then modify the existing entity classes to use them. I'm open to suggestions on how to approach the rest. There is a fundamental issue at hand here IHMO.

Some types of changes just aren't small changes. The goal is to fix the architecture of this component without breaking any of its functionality at any point between here and the end of this series of PR's. What I am concerned with is leaving the component in some caught in between state for a period of time with code that isn't in use / doesn't have a clear purpose making the component hard to understand. I also have to ensure that things aren't left in some in-between state because some PR's got merged and some didn't in the midst of a HA release cycle.

I'm also going to leave this open so that others can potentially comment on it now and so that it can be referenced to understand why some of the intermediate PR's are being opened when it looks like they aren't doing anything useful.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 26, 2019

That sounds good!

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 26, 2019

#20456 PR 1 open

@dmulcahey dmulcahey changed the title WIP / RFC - ZHA component rewrite WIP / RFC - ZHA component rewrite (DO NOT MERGE) Jan 26, 2019

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 26, 2019

#20460 PR 2 opened

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 26, 2019

#20463 opened

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 26, 2019

#20469 opened

@dmulcahey dmulcahey force-pushed the dmulcahey:dm/reorg-zha-submission branch from b78f7a0 to 3991351 Jan 28, 2019

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 28, 2019

@MartinHjelmare I rebased this to reflect the changes that have been merged. Once parts 3 and 4 have been merged I'll update it again. Is this starting to look more manageable? I'ts arguably still huge. I'm not sure how to break up the changes to the listeners and the HA component modules (binary sensor, sensor...). The way they work is completely different. I'm open to suggestions if you have any.

Also, PR's 3 and 4 are independent so 4 is free to be reviewed/merged before 3.

Thanks again for the help and for dealing w/ me!

@dmulcahey dmulcahey force-pushed the dmulcahey:dm/reorg-zha-submission branch 2 times, most recently from 770b1df to c04c6bd Jan 29, 2019

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Jan 31, 2019

@MartinHjelmare rebased again. Any suggestions on how to continue?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 31, 2019

I suggest the next move is to write tests for each platform, also in separate PRs.

If we write them correctly, using the core and depending on the entity state for assertion, they should not be impacted by the refactor. See the recent PRs for homekit_controller for examples, where we could refactor while keeping tests untouched.

When we have tests for all platforms merged, we can be much more confident when looking at the refactor diff.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

zigbee.db was added by mistake I think.

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Feb 5, 2019

shoot... wasn't paying attention

dmulcahey added some commits Feb 5, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 5, 2019

Are we done? 🚀 😄

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Feb 5, 2019

@MartinHjelmare I have to run a round of testing at home... All the changes from today were done remotely. I would like to test removing and joining devices and the config UI to make sure the last set of changes were good. If everything pans out we may be..

dmulcahey added some commits Feb 6, 2019

@dmulcahey dmulcahey changed the title WIP / RFC - ZHA component rewrite (DO NOT MERGE) WIP- ZHA component rewrite Feb 6, 2019

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Feb 6, 2019

@MartinHjelmare I think we're good here. I'll have a quick followup after this is merged for the availability change and another shortly after for the cleanup of the API. Thanks for everything!!!

@dmulcahey dmulcahey changed the title WIP- ZHA component rewrite ZHA component rewrite Feb 6, 2019

@@ -340,15 +232,15 @@ def async_load_api(hass, application_controller, listener):
async def websocket_entities_by_ieee(hass, connection, msg):
"""Return a dict of all zha entities grouped by ieee."""
entities_by_ieee = {}
for ieee, entities in listener.device_registry.items():
for ieee, entities in zha_gateway.device_registry.items():

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 6, 2019

Member

I'd like us to rename entities to eg entity_refs, to avoid thinking that these are actual entities.

This comment has been minimized.

@dmulcahey

dmulcahey Feb 6, 2019

Author Contributor

This PR or in a quick followup?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 6, 2019

Member

Ok, let's do it in the follow up. 👍

DEVICE_INFO: entity.device_info
})
entities_by_ieee[ieee_string].append({
ATTR_ENTITY_ID: entity.reference_id,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 6, 2019

Member

Also here I'd like us to rename entity to eg entity_ref.

This comment has been minimized.

@dmulcahey

dmulcahey Feb 6, 2019

Author Contributor

This PR or in a quick followup?

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Awesome work! 🚀

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 6, 2019

What are the breaking changes? Please highlight those in a paragraph in the PR description for the release notes. Then we can merge.

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Feb 6, 2019

@MartinHjelmare It's the availability stuff... Can I just document that in the next PR. I'll have it opened shortly after this merges. Otherwise, it will get documented one way here now and then a different way in the next PR and that will be confusing...

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 6, 2019

Ok. But then maybe we should remove the breaking change label? If we don't document it here, there's no point in adding the label, I think.

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Feb 6, 2019

That's fine @MartinHjelmare added some notes here as well

@dmulcahey dmulcahey merged commit e6cd04d into home-assistant:dev Feb 6, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 93.352%
Details

@wafflebot wafflebot bot removed the in progress label Feb 6, 2019

@dmulcahey

This comment has been minimized.

Copy link
Contributor Author

dmulcahey commented Feb 6, 2019

BREAKING CHANGE

  • battery now on device entity instead of separate sensor entity
  • event unique_id no longer mirrors entity_id
  • device entity now using HA available property so instead of 'offline' and 'online' states are 'unavailable' and 'online'

bachya added a commit to bachya/home-assistant that referenced this pull request Feb 7, 2019

ZHA component rewrite (home-assistant#20434)
* rebase reorg

* update coveragerc for now

* sensor cleanup

* remove availability tracking for entities

* finish removing changes from tests

* review comments pass 1

* use asyncio.gather - review comments

* review comments

* cleanup - review comments

* review comments

* review comments

* cleanup

* cleanup - review comments

* review comments

* review comments

* use signal for removal

* correct comment

* remove entities from gateway

* remove dead module

* remove accidently committed file

* use named tuple - review comments

* squash bugs

* squash bugs

* add light and sensor back to coveragerc until % is higher

@dmulcahey dmulcahey deleted the dmulcahey:dm/reorg-zha-submission branch Feb 15, 2019

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.