-
-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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
Add initial support fo HomematicIP components #12761
Conversation
Hello @mxworm, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
cc @danielperna84, maybe @balloob I think the name suggestion is wrong. Maybe My personal: this interface is not public from EQ3. Buy a raspberrypi3 and HomeMaticIP USB stick from ELV and use the hass.io add-on or other implementation for this and you need never care about any firmware updates they change things on interface. CCU2 from EQ3 work also. |
Hi, I have added my email address to GitHub. Ciao Mattias |
I would vote against that. Also I do see a value in this component. People who once started out with the IP access point and now want to have a look how it works with HA should be able to do that. A reverse engineered approach though may break as soon as eq3 breaks it with some firmware update. |
last = self._device.lastStatusUpdate | ||
if last is not None: | ||
attr.update({ATTR_LASTSTATUS_UPDATE: | ||
last.strftime('%d-%m-%Y %H:%M:%S')}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last.isoformat()
Shouldn't this be integrated into |
No, this targets a different hub-device that's not accessable via the same API. pyhomematic (for usage with the classic Homematic hub) uses XML-RPC to set states and has a XML-RPC server thread to receive the state changes (local push in HA-terms). |
Hi, yes I agree... all data is going to the HMIP cloud servers and then back to the access point. But a least there is a bit of direct communication between the different devices. By the way: how do I have to import the homematicip module?
Thanks Mattias |
@danielperna84 thanks for the explanation. 2 is the correct one. You just didn't run I don't think that we should name the component after the interface that is being used to communicate. It should be named after the API name. One should be able to speak this API over the cloud or locally, it shouldn't matter what the name is. Even REST is an implementation detail (and a bad name as it uses websockets too?) Do the new generation of homematic hubs have a name? or maybe the API? |
Hi, Can someone help me to understand what went wrong in the travis build? Thanks Mattias |
@mxworm just a flaky test, I restarted your build. |
What is with |
@pvizeli I would like to keep complexity low - there are already a lot of HMIP components :-) |
Oh, heh. My bad, I didn't realize that Homematic IP is the product name. I'm fine with naming it just |
@balloob I would keep the naming for the moment, whats next? |
async_dispatcher_connect( | ||
self.hass, EVENT_DEVICE_CHANGED, self._device_changed) | ||
|
||
@callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this one is correctly async. The dispatcher will execute this method in an async context because of this decorator.
|
||
def setup_platform(hass, config, add_devices, discovery_info=None): | ||
"""Set up the HomematicIP sensors devices.""" | ||
# pylint: disable=import-error, no-name-in-module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These shouldn't have to be disabled since you updated requirements_all.txt
"""Initialize the access point sensor.""" | ||
self.hass = hass | ||
self._home = home | ||
async_dispatcher_connect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use sync version
I like |
Hi folks, I would love to add some more components e.g binary_switches, climate and security for HomematicIP. But first I would like to clarify the name of the platform. homematicip_cloud is fine for me, if everyone involved agrees I will change the name. |
Please do not add more types in this PR. Let's get this PR merged and then add more types in a follow up PR. I am fine with |
Yes, I agree this PR is already a bit of a mess... sorry for that. Its my first PR on such a big project and my python is quite limited. Thanks for the support. I will rename as a last thing in this PR, next devices will be provided as separate PRs. |
Awesome work! 🎉 Good to go! When adding platforms to this component, please stick to 1 platform / PR. That will make reviewing and merging a lot faster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comments for a future PR.
Support for HomematicIP components. | ||
|
||
For more details about this component, please refer to the documentation at | ||
https://home-assistant.io/components/homematicip/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the url to point at the new component name.
from socket import timeout | ||
|
||
import voluptuous as vol | ||
from homeassistant.core import callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a blank line between 3rd party and homeassistant imports.
|
||
DOMAIN = 'homematicip_cloud' | ||
|
||
CONF_NAME = 'name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import this from const.py.
authtoken = device.get(CONF_AUTHTOKEN) | ||
|
||
home = Home() | ||
if name.lower() == 'none': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will this happen? Default name is an empty string in the config schema.
Support for HomematicIP sensors. | ||
|
||
For more details about this component, please refer to the documentation at | ||
https://home-assistant.io/components/homematicip/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at other platforms for the format of the url.
"""Initialize the access point sensor.""" | ||
self.hass = hass | ||
self._home = home | ||
dispatcher_connect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to the coroutine method async_added_to_hass
, to avoid a race where the callback is called before the entity has been added to home assistant.
class HomematicipAccesspoint(Entity): | ||
"""Representation of an HomeMaticIP access point.""" | ||
|
||
def __init__(self, hass, home): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't pass in hass
. It will be set on the entity when it has been added to home assistant.
class HomematicipGenericDevice(Entity): | ||
"""Representation of an HomematicIP generic device.""" | ||
|
||
def __init__(self, hass, home, device, signal=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't pass in hass
. It will be set on the entity when it has been added to home assistant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
signal
parameter doesn't seem to be used.
self.hass = hass | ||
self._home = home | ||
self._device = device | ||
async_dispatcher_connect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to the coroutine method async_added_to_hass
.
@mxworm @balloob @MartinHjelmare The homematic-rest-api also has a asyncio component. I created a custom hmip component using the asyncio capabilities with the intention of later creating a component out of it. I paid a lot of attention making the websocket implementation robust as I experienced frequent connection drops making hass losing track of state changes rendering the component useless. This involved changing the websocket code on the library as well as on my custom component side. This has all been done on the asyncio part of the homematic-rest-api library. Do you have any experience with the code which is just merged and the robustness of the websocket connection ? I really think this needs some more attention... For reference: Here is my custom component: |
FYI if you want a (in my opinion) good example of an asyncio aiohttp websocket connection with retry logic, check the Home Assistant Cloud code: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/cloud/iot.py |
@ballob Thanks. I faced issues with the heartbeat parameter in aiohttp. Hopefully with the new release that has changed. My custom component is now running without any issues for 2 months now. It took a while to get there as the homematic webserver is/used to be notoriously unstable. (They are making great progress and as a whole the platform absolutely rocks). Question remains what to do with this pull request? Obviously I highly appreciate the effort made by @mxworm, but I am also sure this component will stop working after a while. That is bad for hass. I am also reluctant to improve on this PR as I have my own implementation. Which is really different from this one. |
@sander76 Would this be way to go for? @balloob Ciao |
@mxworm |
@mxworm Please let me know which repo you'll be using for making the changes. I will try to help where possible. Looking at the code from balloob my code can use a bit of improving too... |
@sander76 OK, then I will use for the moment your last code from https://github.com/sander76/custom_components I would like to implement climatic devices and groups as well. But first I would like to have a solid base for further development. @balloob Would this be OK for you as well? I know its annoying to have a complete rewrite of the code after the first PR. |
@mxworm @balloob Would it be possible to reject this merge and start over ? |
@mxworm First shot at balloobs suggestion. Much cleaner. |
Let's not pull out this PR. It's fine for the PR to be released and the guts changed out in the next PR. Would be a non breaking change for users since the config options should not change. |
@mxworm Did you already make a start with moving to async ? Please let me know, I can help. If not, is it ok if I give it a go ? |
@sander76 So far there is a PR pending with a async version, but just with sensors. All components I have implemented you can find under: |
Description:
Initial support fo HomematicIP components
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.