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

Rewrite of Toon component #21186

Merged
merged 27 commits into from Feb 26, 2019

Conversation

@frenck
Copy link
Member

frenck commented Feb 18, 2019

Description:

This is a rewrite of the Toon component for Home Assistant.

The current version uses an HTTP endpoint, originally created by the vendor for their web app. They discontinued this application quite some time ago, and now the service was taken offline as well; hence the component is now broken (see #21010 and the portal that was taken down).

Luckily, Toon has an API available, which this PR implements by using the toonapilib package created by @costastf. This package has been used in the custom Toon component by @krocat. The toonapilib therefore has been matured for quite some time already.

This component rewrite is not a copy, nor does it use the custom component listed above. This rewrite is focussed to integrate within Home Assistant using a configuration flow, to be async and uses the device & entity registry.

This component will, just like the current version, still have the issue in setting a temperature via Google Assistant. In order to fix this, without doing ugly things, requires home-assistant/architecture#22 to be resolved.

Support for smoke detectors and switches (wall-plugs) has been removed. Support for lights is not implemented/added. The devices supported by Toon are also supported by HA natively (Hue/Fibaro).

⚠️ Please note, this is my first serious contribution to the Home Assistant code base and I don't use Python on a daily basis. A little more extensive answer on possible issues and code review comments would, therefore, be appreciated.

Personal reference: frenck-2019-0026

Breaking Change:
Rewrite of the Toon component: now requires a Toon API developer account and can be configured using via the Home Assistant integrations. Support for Switches & Smoke detectors has been removed.

Related issue (if applicable): fixes #21010

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

Example entry for configuration.yaml (if applicable):

toon:
  client_id: fxiyXLtFfhimADJlmFjTjXdHoxZ8AFg7
  client_secret: kPUCx88CTCSMAaBA

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.

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.

@frenck frenck requested a review from home-assistant/core as a code owner Feb 18, 2019

@wafflebot wafflebot bot added the in progress label Feb 18, 2019

@frenck frenck referenced this pull request Feb 18, 2019

Merged

Rewrite of Toon component #8627

2 of 2 tasks complete

@frenck frenck removed the docs-missing label Feb 18, 2019

@frenck frenck changed the title Rewrite of Toon component WIP: Rewrite of Toon component Feb 19, 2019

@scheric

This comment has been minimized.

Copy link

scheric commented Feb 21, 2019

Data i get from the toonapi

power and gas cost

As disused in the documented issue the API does not send data like cost for power and gas. I retrieved this data.: the power and gas doe have daily cost. could this just be the toonapilib.
1

@frenck frenck force-pushed the frenck-2019-0026 branch from 9c818dd to 3175728 Feb 23, 2019

@frenck frenck changed the title WIP: Rewrite of Toon component Rewrite of Toon component Feb 23, 2019

@frenck

This comment has been minimized.

Copy link
Member Author

frenck commented Feb 23, 2019

Addressed al the comments and added some nice features in the process.
Finally rebased it as well.

From my end, I think I'm done now.

Final screenshot:
image

Show resolved Hide resolved requirements_all.txt Outdated
Show resolved Hide resolved homeassistant/components/toon/__init__.py Outdated
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 25, 2019

Looking all good ! 🎉

Final step is to add a test for the config flow.

frenck added some commits Feb 23, 2019

@frenck frenck force-pushed the frenck-2019-0026 branch from 46622bd to 192335e Feb 26, 2019

@frenck

This comment has been minimized.

Copy link
Member Author

frenck commented Feb 26, 2019

Rebased in order to pick up the increased Travis timeout from #21447.

@balloob balloob merged commit f3c9327 into dev Feb 26, 2019

3 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
coverage/coveralls Coverage decreased (-0.002%) to 92.739%
Details

@delete-merged-branch delete-merged-branch bot deleted the frenck-2019-0026 branch Feb 26, 2019

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


conf = hass.data.get(DATA_TOON_CONFIG)

toon = Toon(entry.data[CONF_USERNAME], entry.data[CONF_PASSWORD],

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 26, 2019

Member

This seems to make requests upon instantiation, ie I/O. We're not allowed to do I/O in coroutines, as they are executed in the event loop.


app = self.hass.data.get(DATA_TOON_CONFIG, {})
try:
toon = Toon(user_input[CONF_USERNAME],

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 26, 2019

Member

See above.


app = self.hass.data.get(DATA_TOON_CONFIG, {})
try:
Toon(self.username,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 26, 2019

Member

See above.

"""Get the latest data from the sensor."""
self.thermos.update()
section = getattr(self.toon, self.section)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 26, 2019

Member

This also seems to do requests if the cache is old.


async def async_update(self) -> None:
"""Get the latest data from the binary sensor."""
section = getattr(self.toon, self.section)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 26, 2019

Member

This also seems to do requests if the cache is old.

@frenck frenck referenced this pull request Mar 4, 2019

Merged

Fix Toon doing I/O in coroutines #21657

4 of 9 tasks complete
@frenck

This comment has been minimized.

Copy link
Member Author

frenck commented Mar 4, 2019

Above comments by @MartinHjelmare addressed in #21657

@scheric

This comment has been minimized.

Copy link

scheric commented Mar 9, 2019

A question to everyone who have boiler modulation

When I look at the boiler modulation level I came to the conclusion that the status is not right.
I have a consistent delta of 3 %

HA gives API gives Boiler gives
89 89 92
78 78 80

Who has a difference in modulation level?
Is the difference 3% too?


note: I have a nefit Trendline.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 9, 2019

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Mar 9, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.