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

Izone component #24550

Merged
merged 22 commits into from Sep 19, 2019

Conversation

@Swamp-Ig
Copy link
Contributor

commented Jun 15, 2019

Description:

New integration for the iZone climate control system.

I have had this as a custom component for some time and have a few existing users. I though it was time to integrate it. It should be pretty solid as it's reasonably well tested.

The translations were basically copied from the daikin component, just changing the names to iZone, so they should be correct.

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9635

Example entry for configuration.yaml (optional, uses config entries):

izone:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.
homeassistant/components/izone/.translations/ca.json Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/discovery.py Outdated Show resolved Hide resolved
homeassistant/components/izone/discovery.py Outdated Show resolved Hide resolved
homeassistant/components/izone/discovery.py Outdated Show resolved Hide resolved
homeassistant/components/izone/discovery.py Outdated Show resolved Hide resolved
@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:izone-component branch from f90dc3e to 97f5568 Jun 16, 2019
@Swamp-Ig Swamp-Ig referenced this pull request Jun 20, 2019
@Nick-Adams-AU

This comment has been minimized.

Copy link

commented Jun 21, 2019

Is this able to be merged now?

@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:izone-component branch from affc4a6 to 81af8f2 Jun 22, 2019
Copy link
Member

left a comment

All PRs that have climate platforms are blocked by climate 1.0 PR unfortunately.

See other comments below.

homeassistant/components/izone/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/izone/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
@home-assistant home-assistant deleted a comment Jun 25, 2019
@Nick-Adams-AU

This comment has been minimized.

Copy link

commented Jul 16, 2019

I have been running this on 0.95 and haven't had any issues. Now that the Climate 1.0 PR has been merged, are we able to merge this component?

@frenck

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

@Nick-Adams-AU The current state will not work on 0.96.

This integration needs to be updated for Climate 1.0 first and some comments are still open that need to be addressed as well.

@MartinHjelmare MartinHjelmare added this to Review in progress in Dev Jul 23, 2019
@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Updated now to Climate 1.0, and addressed all comments. I need to do some tests for the config flow.

@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:izone-component branch from 81af8f2 to c7d075d Jul 31, 2019
@gavc1

This comment has been minimized.

Copy link

commented Aug 5, 2019

Thanks swamp, I have been testing this and noticed the controller status does not match in home assistant.
EG the Aircon is on, but shows off in Home Assistant.
I did notice this in my logs if it helps
Exception in _controller_discovered when dispatching 'izone_controller_discovered': (<pizone.controller.Controller object at 0xb2586990>,)
Traceback (most recent call last):
File "/config/custom_components/izone/discovery.py", line 40, in _controller_discovered
"discovered device that already exists"
AssertionError: discovered device that already exists

@gavc1

This comment has been minimized.

Copy link

commented Aug 5, 2019

Thanks swamp, I have been testing this and noticed the controller status does not match in home assistant.
EG the Aircon is on, but shows off in Home Assistant.
I did notice this in my logs if it helps
Exception in _controller_discovered when dispatching 'izone_controller_discovered': (<pizone.controller.Controller object at 0xb2586990>,)
Traceback (most recent call last):
File "/config/custom_components/izone/discovery.py", line 40, in _controller_discovered
"discovered device that already exists"
AssertionError: discovered device that already exists

Don't worry, I removed the integration and re set it up using the config flow and it all works well now.

homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/climate.py Outdated Show resolved Hide resolved
homeassistant/components/izone/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/izone/discovery.py Outdated Show resolved Hide resolved
homeassistant/components/izone/discovery.py Outdated Show resolved Hide resolved
homeassistant/components/izone/discovery.py Outdated Show resolved Hide resolved
@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

All comments now addressed, and the test case exists.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

There are still comments in the previous review that haven't been addressed.

@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:izone-component branch from ec45076 to 7d495c4 Aug 14, 2019
@Nick-Adams-AU

This comment has been minimized.

Copy link

commented Aug 26, 2019

Is it just the tests that are holding up this merge?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

There are at least four comments left to address.

@Nick-Adams-AU

This comment has been minimized.

Copy link

commented Sep 2, 2019

Hey @Swamp-Ig. Thanks for your work on this so far! Is there a way to pay/sponsor your work to complete the integration of this component?

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

I'm pretty short on spare time at the moment, my one day a week was taken up with other stuff both this week and last. Will see what I can do next Wednesday. Got a lot of demands on my time currently, it's not a financial thing.

Swamp-Ig added 8 commits Aug 7, 2019
@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:izone-component branch from c0c691c to b233e3b Sep 18, 2019
@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Test case is complete and all comments now addressed AFAICS

Swamp-Ig added 3 commits Sep 18, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

There are still four comments left to address:
#24550 (comment)

#24550 (comment)

#24550 (comment)

#24550 (comment)

Make sure to load all comments to be able to see them. Github collapses comments in desktop view when the thread gets too long.

Dev automation moved this from Review in progress to Reviewer approved Sep 18, 2019
Copy link
Member

left a comment

Great!

@MartinHjelmare MartinHjelmare merged commit b68b843 into home-assistant:dev Sep 19, 2019
11 checks passed
11 checks passed
CI Build #20190918.32 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 94.11% of diff hit (target 94.01%)
Details
codecov/project 94.01% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 19, 2019
KJonline added a commit to KJonline/home-assistant that referenced this pull request Sep 20, 2019
* dev: (167 commits)
  Bump aiowwlln to 2.0.2 (home-assistant#26769)
  ZHA siren and warning device support (home-assistant#26046)
  Add transport data from maps.yandex.ru api (home-assistant#26766)
  Type hint additions (home-assistant#26765)
  Bump openwrt-luci-rpc to version 1.1.1 (home-assistant#26759)
  Revert "Add transport data from maps.yandex.ru api (home-assistant#26252)" (home-assistant#26762)
  [ci skip] Translation update
  Updated frontend to 20190919.0
  deCONZ improve gateway tests (home-assistant#26709)
  Add transport data from maps.yandex.ru api (home-assistant#26252)
  Update codeowners (home-assistant#26733)
  Bump influxdb to 5.2.3 (home-assistant#26743)
  Izone component (home-assistant#24550)
  Bump restrictedpython to 5.0 (home-assistant#26741)
  Bumped version to 0.99.1
  Add Plex config flow support (home-assistant#26548)
  Bump pyobihai to fix issue with user account (home-assistant#26736)
  Bump TRADFRI (home-assistant#26731)
  Encode prometheus metric names per the prom spec (home-assistant#26639)
  Bump aiohttp to 3.6.1 (home-assistant#26739)
  ...
KJonline added a commit to KJonline/home-assistant that referenced this pull request Sep 21, 2019
* dev: (92 commits)
  Bump pynws version to 0.8.1 (home-assistant#26770)
  Bump HAP-python to 2.6.0 for homekit (home-assistant#26783)
  [ci skip] Translation update
  Add integration scaffolding script (home-assistant#26777)
  Bump simplisafe-python to 5.0.1 (home-assistant#26775)
  Bump aiowwlln to 2.0.2 (home-assistant#26769)
  ZHA siren and warning device support (home-assistant#26046)
  Add transport data from maps.yandex.ru api (home-assistant#26766)
  Type hint additions (home-assistant#26765)
  Bump openwrt-luci-rpc to version 1.1.1 (home-assistant#26759)
  Revert "Add transport data from maps.yandex.ru api (home-assistant#26252)" (home-assistant#26762)
  [ci skip] Translation update
  Updated frontend to 20190919.0
  deCONZ improve gateway tests (home-assistant#26709)
  Add transport data from maps.yandex.ru api (home-assistant#26252)
  Update codeowners (home-assistant#26733)
  Bump influxdb to 5.2.3 (home-assistant#26743)
  Izone component (home-assistant#24550)
  Bump restrictedpython to 5.0 (home-assistant#26741)
  Bumped version to 0.99.1
  ...
ochlocracy added a commit to ochlocracy/home-assistant that referenced this pull request Sep 30, 2019
* iZone component

* Rename constants to const.

* Changes as per code review.

* Stop listening if discovery times out.

* Unload properly

* Changes as per code review

* Climate 1.0

* Use dispatcher instead of listener

* Free air settings

* Test case for config flow.

* Changes as per code review

* Fix error on shutdown

* Changes as per code review

* Lint fix

* Black formatting

* Black on test

* Fix test

* Lint fix

* Formatting

* Updated requirements

* Remaining patches

* Per code r/v
ochlocracy added a commit to ochlocracy/home-assistant that referenced this pull request Oct 1, 2019
* iZone component

* Rename constants to const.

* Changes as per code review.

* Stop listening if discovery times out.

* Unload properly

* Changes as per code review

* Climate 1.0

* Use dispatcher instead of listener

* Free air settings

* Test case for config flow.

* Changes as per code review

* Fix error on shutdown

* Changes as per code review

* Lint fix

* Black formatting

* Black on test

* Fix test

* Lint fix

* Formatting

* Updated requirements

* Remaining patches

* Per code r/v
@balloob balloob referenced this pull request Oct 9, 2019
@teaching-innovation

This comment has been minimized.

Copy link

commented Oct 10, 2019

Haven’t tested yet but excited to see iZone component released in V 0.100. Great work @swamp-lg for all your hard work.

@teaching-innovation

This comment has been minimized.

Copy link

commented Oct 10, 2019

I have just tested. I moved from V0.93 to V0.100.0b3. It works for mode and fan speed but I am unable to set the "temp_setpoint: 23" from home assistant.

The UI in lovelace also lacks the ability to set the temperature as it is greyed out.
image
image

@frenck

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

@teaching-innovation Please don't use PR's to discuss issues, instead, create a new issue on GitHub. That allows us to track it. Thanks 👍

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