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

Add sensor support for Dyson Purecool 2018 Air Purifiers models TP04 and DP04 #22578

Merged
merged 1 commit into from Apr 30, 2019

Conversation

@etheralm
Copy link
Contributor

commented Mar 31, 2019

Description:

This PR adds sensor and air quality support for the dyson purecool 2018 air purifier models (DP04 and TP04).

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

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

@MartinHjelmare as per your request, I've moved the sensor and air quality changes for the new dyson fans to this pr.

@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

I still need to update the documentation for this one, I'll try to do so later today.

@frenck

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Seems like a dupe of #22215 (merged).

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

We need to rebase here to undupe it.

@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

I had to replicate the library changes from the other pr here to get this one to build. I'll rebase it when I get to a PC.

@etheralm etheralm force-pushed the etheralm:dyson_sensors branch from f5aea32 to 3b5efbb Apr 2, 2019

@etheralm etheralm referenced this pull request Apr 2, 2019
2 of 2 tasks complete
homeassistant/components/dyson/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/sensor.py Outdated Show resolved Hide resolved
tests/components/dyson/test_air_quality.py Outdated Show resolved Hide resolved
tests/components/dyson/test_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved

@etheralm etheralm force-pushed the etheralm:dyson_sensors branch from c6dbb87 to ce5a27d Apr 10, 2019

@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@MartinHjelmare any idea what I have to do to clean up the core entity registry file after the tests? I guess this has something to do with the unique_id.

EDIT: nevermind I think I found the issue. I'll have to rewrite several of the tests for the old devices.

@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@MartinHjelmare I think I covered all the issues in the review, let me know if I missed something.

homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/dyson/sensor.py Outdated Show resolved Hide resolved
@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@MartinHjelmare thx for the feedback I'll do the requested changes tomorrow.

In the meantime, yesterday I discovered that I missed one of the dyson fan speeds in the speed dictionary. Since that pr is already merged which one would be better, to open a new pr with that change or to add it here? It's a single line change.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

You can add it here if you like.

@etheralm etheralm force-pushed the etheralm:dyson_sensors branch from b73b6e1 to fb3b878 Apr 29, 2019

@MartinHjelmare
Copy link
Member

left a comment

Great work! Can be merged when last comment is addressed and build passes.

@codecov

This comment has been minimized.

Copy link

commented Apr 29, 2019

Codecov Report

Merging #22578 into dev will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22578      +/-   ##
==========================================
+ Coverage   93.81%   93.84%   +0.03%     
==========================================
  Files         461      450      -11     
  Lines       37571    36844     -727     
==========================================
- Hits        35246    34578     -668     
+ Misses       2325     2266      -59
Impacted Files Coverage Δ
homeassistant/components/dyson/sensor.py 100% <100%> (ø) ⬆️
homeassistant/components/dyson/air_quality.py 100% <100%> (ø)
homeassistant/components/mqtt/fan.py 74.33% <0%> (-23.64%) ⬇️
homeassistant/components/mqtt/light/schema_json.py 73.06% <0%> (-20.65%) ⬇️
homeassistant/bootstrap.py 58.78% <0%> (-15.62%) ⬇️
homeassistant/components/hassio/ingress.py 67.76% <0%> (-10.29%) ⬇️
homeassistant/components/mqtt/lock.py 91.83% <0%> (-8.17%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
homeassistant/components/deconz/config_flow.py 97.77% <0%> (-2.23%) ⬇️
homeassistant/components/hue/bridge.py 73.07% <0%> (-1%) ⬇️
... and 186 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aa9a1a...6572caa. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Apr 29, 2019

Codecov Report

Merging #22578 into dev will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22578      +/-   ##
==========================================
+ Coverage   93.81%   93.84%   +0.03%     
==========================================
  Files         461      450      -11     
  Lines       37571    36844     -727     
==========================================
- Hits        35246    34578     -668     
+ Misses       2325     2266      -59
Impacted Files Coverage Δ
homeassistant/components/dyson/sensor.py 100% <100%> (ø) ⬆️
homeassistant/components/dyson/air_quality.py 100% <100%> (ø)
homeassistant/components/mqtt/fan.py 74.33% <0%> (-23.64%) ⬇️
homeassistant/components/mqtt/light/schema_json.py 73.06% <0%> (-20.65%) ⬇️
homeassistant/bootstrap.py 58.78% <0%> (-15.62%) ⬇️
homeassistant/components/hassio/ingress.py 67.76% <0%> (-10.29%) ⬇️
homeassistant/components/mqtt/lock.py 91.83% <0%> (-8.17%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
homeassistant/components/deconz/config_flow.py 97.77% <0%> (-2.23%) ⬇️
homeassistant/components/hue/bridge.py 73.07% <0%> (-1%) ⬇️
... and 186 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aa9a1a...e788176. Read the comment docs.

@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Great, thank you for the assistance!

@etheralm etheralm force-pushed the etheralm:dyson_sensors branch from e788176 to 9ec3ffa Apr 29, 2019

@format80

This comment has been minimized.

Copy link

commented Apr 29, 2019

Hi guys,
thank you all for the great work that you do every day for this community.

@etheralm: I can do some user tests for you if you want; I have a TP04 model. If yes, let me know how, please. I'm new on HA.

@etheralm etheralm force-pushed the etheralm:dyson_sensors branch from 9ec3ffa to efb6d86 Apr 29, 2019

add sensor support for dyson 2018 models
fix check for already created entities

remove hepa and carbon filter

add AQI attribute

initial commit

fix check for already created entities

remove hepa and carbon filter

add AQI attribute

add air quality component tests

fix method call tests

fix line lengths

fix pylint issues

fix docstrings

revert fan related changes

remove whitespace change

fix fan update state test

add for loop for platform initialization

add requested changes to aiq platform

change string concatenation to new style string formatting

update air quality tests

update air quality tests

refactor sensor component changes

fix pylint issues

fix debug string in the air quality component

replace failing tests for older devices

fix line length fan tests

remove dependencies const and move imports

move back imports to methods

remove whitespace from blank line

@etheralm etheralm force-pushed the etheralm:dyson_sensors branch from efb6d86 to 1b8d29c Apr 29, 2019

@MartinHjelmare MartinHjelmare merged commit 1d70005 into home-assistant:dev Apr 30, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch No report found to compare against
Details
codecov/project 93.82% (target 90%)
Details
@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@format80 Thanks.

You can copy the dyson folder from the dev branch here in the custom_components folder(create it if it doesn't exist) inside of your home assistant config folder.

@format80

This comment has been minimized.

Copy link

commented May 1, 2019

@etheralm: humidity, temperature and air quality sensor are working well.
Can you manage the informations related to air quality creating multiple sensor for AQI, PM2.5, PM10, NO2 and VOC, please?
I tried by my self with bad results ;)
I see in file air_quality.yaml some mistyping of AIQ instead of AQI :) I replaced all AIQ with AQI in my local custom component and it is working fine.

I have some little issues on the fan. Can I open these issues in the other topic related to the fan integration?

@etheralm

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@format80 Thanks for trying it out.

Good catch about the misspellings, it's not easy being dyslexic 😄 . It's jut an internal constant that is not stored or displayed anywhere, but still I have to sort it out. I'll fix it when I refactor the older models.

About the air quality related values as separate sensors, you can use template sensors:
https://www.home-assistant.io/components/template/

for example:

sensor:
  - platform: template
    sensors:
      pm25:
        friendly_name: "Particulate Matter 2.5"
        unit_of_measurement: 'µg/m3'
        value_template: "{{ state_attr(' air_quality.living_room', 'particulate_matter_2_5') }}"

About the other issues, I expect that they are with the interface? For example the oscillation toggle. The frontend for home assistant is in another project:
https://github.com/home-assistant/home-assistant-polymer
I plan to update the frontend component next adding controls for everything that you can control from the app right now, but I'm extremely busy until the 20th and won't be able to get around to it until the end of the month. That shouldn't discourage you from opening issues though, feel free to do so.
On the other side if you have problems with with the services that the fan registers, or if you see that some of the information that it shows is wrong, let me know I'll try to fix them as fast as possible.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Please continue the conversation somewhere but not in this merged PR. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators May 1, 2019

@etheralm etheralm deleted the etheralm:dyson_sensors branch May 1, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.