-
-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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 Dyson Pure Cool Link support #7795
Conversation
@CharlesBlonde, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @balloob and @rmkraus to be potential reviewers. |
Just thought about getting one. Is this cloud only? Or does it work in LAN? |
In fact it's a mix between Cloud and LAN: all commands and all sensors values are from the LAN using MQTT. But the MQTT connection used authentication and the only way to get the encrypted credentials is to call Dyson web services (hosted on AWS). Technically, after initial connection, no more remote calls are done from this component. |
Thanks for the info. Great work you did there! |
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.
First round.
Please change your code to use the internal discovery for platforms. Look into andoird_ip_camera
component to see how that should work.
homeassistant/components/dyson.py
Outdated
timeout = config[DOMAIN].get(CONF_TIMEOUT) | ||
retry = config[DOMAIN].get(CONF_RETRY) | ||
|
||
if logged: |
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.
Hold the ident small as possible:
if not logged:
_LOGGER...
return False
homeassistant/components/dyson.py
Outdated
CONF_LANGUAGE = "language" | ||
CONF_TIMEOUT = "timeout" | ||
CONF_RETRY = "retry" | ||
CONF_DEVICES = "devices" |
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 all exists CONF_* from const.py
Following your comments, I updated the components in order to dynamically create fan/sensor components instead of using components configured in |
I updated the component to add auto/night mode as new features and not anymore like a speed. For information:
I updated the frontend component and submit another PR: home-assistant/frontend#300 I also renamed available speeds from string to integer when available (0002 became 2). |
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.
@turbokongen @balloob if the change on init of fan component legtitime or only a platform thing?
from homeassistant.components.dyson import DYSON_DEVICES | ||
|
||
DEPENDENCIES = ['dyson'] | ||
REQUIREMENTS = ['libpurecoollink==0.1.5'] |
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.
Remove this. That will be implicit with DEPENDENCIES
"""Called when new messages received from the fan.""" | ||
_LOGGER.debug("Message received for fan device %s : %s", self.name, | ||
message) | ||
if self.hass and self.entity_id: |
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.
if self.hass
should not needed or you have done a desine misstake. Make sure that your device is receive callback after HOMEASSISTANT_START
or register your device callback inside async_added_to_hass
.
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.
You are right. I guess I add this test before refactoring the constructor.
_LOGGER.info("Creating device %s", device.name) | ||
self.hass = hass | ||
self._device = device | ||
self._device.add_message_listener(self.on_message) |
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.
See commend above. Please move the init as first function inside your object.
I choose to add the new services (set auto/night mode) in the fan parent component in order to have them all in the same place but if you prefer I can move them in the Dyson component. I don't know if other fan devices from other manufacturer support this features. |
You component/platform looks good. It should be ready to merge after your address our comments. If you want this service, you need move it into your platform with namesufix. But I think you need not this service and can use |
Adding auto/night to speed list was what I did before my last commit. But, at least for the night mode, it has to be used in addition to speed level:
But I can remove auto mode and add it to the speed list (it is already in the list in fact). I follow the mobile app who is using AUTO mode like an external option but in fact, it is considered internally like a speed. So, what I have to do (tell me if I'm wrong):
Just one question about the constant SUPPORT_NIGHT_MODE: Should it be in the init.py file or in the fan/dyson.py file ? |
Yes. You can not make custom UI change for platform inside fan card. In this case you need not have a |
I move the I'll do other improvement in the future in other(s) PR. But is there a reason I can not use I agree to add a @property
def support_mode(self):
return [] To the fan parent component but I don't understand the difference between this and the And s***, I broke the Travis build with Python 3.6 ... :'( |
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.
So we go into finishing 👍
_LOGGER.info("Creating device %s", device.name) | ||
self.hass = hass | ||
self._device = device | ||
self._device.add_message_listener(self.on_message) |
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 that into async_added_to_hass
"""Called when new messages received from the fan.""" | ||
_LOGGER.debug("Message received for fan device %s : %s", self.name, | ||
message) | ||
if self.entity_id: |
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.
If you do it right, you need this check not
self.hass = hass | ||
self._device = device | ||
self._name = "{} filter life".format(self._device.name) | ||
self._device.add_message_listener(self.on_message) |
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 that into async_added_to_hass
if self._device.state: | ||
return self._device.state.filter_life | ||
else: | ||
return 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.
return STATE_UNKNOWN
from const.
|
Ok, it make sense, thanks for the quick answer! |
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.
Merge after test are pass
Description:
This PR add Dyson purifier fans support.
This component is still in development and will be improve but I'm using it since months and I have been able to have successful feedback from other users: https://community.home-assistant.io/t/dyson-link-action-and-sensor/10145
So I think it is reliable enough to submit a first release. I have in mind other improvements (other sensors, heating and why not the vacuum 360 robot) so I decided to create a parent Dyson component with Dyson account information and fan/sensors child components.
One word on auto-discovery (mDNS): for an unknown reason, it's not very reliable (sometime it's work, sometime not). So in the documentation I'll add a word on how to setup the component using discovery and how to manually configure it if it's not working.
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2721
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 pass (not tested with Python 3.6)REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass