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 MELCloud integration #30712

Merged
merged 35 commits into from Feb 10, 2020
Merged

Conversation

@vilppuvuorinen
Copy link
Contributor

vilppuvuorinen commented Jan 12, 2020

Description:

This stuff is not supported nor in any way affiliated with Mitsubishi Electric.

Provides climate and sensor platforms for Mitsubishi Electric heat pumps connected to MELCloud service. Multiple platforms on one go is not the best option, but it does not make sense to remove them and commit them later either. This thing started as a custom_component.

Email and access token are stored to the ConfigEntry. The token can be updated by adding the integration again with the same email address. The config flow is aborted and the update is performed on the background. This is a bit unorthodox, but at least I like it better than storing the plain-text password.

Additional information

Link to documentation pull request: home-assistant/home-assistant.io#11930

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.
@vilppuvuorinen

This comment has been minimized.

Copy link
Contributor Author

vilppuvuorinen commented Jan 13, 2020

@frenck Do I need to provide docs PR before any review is done? I can do the docs PR, but I don't want to commit time to doing that without knowing if this is something that can/will be merged.

This comment has been minimized.

Copy link
Member

frenck commented Jan 13, 2020 — with Octobox

Docs are required before being able to merge. I've added it to ensure we do not merge it accidentally without documentation.

@vilppuvuorinen vilppuvuorinen changed the title WIP: Add MELCloud integration Add MELCloud integration Jan 14, 2020
@vilppuvuorinen vilppuvuorinen requested review from MartinHjelmare and fabaff Jan 16, 2020
@rudizl

This comment has been minimized.

Copy link

rudizl commented Jan 25, 2020

Excellent work, fingers crossed!

@gdude2002

This comment has been minimized.

Copy link

gdude2002 commented Jan 27, 2020

I attempted to test this component on my own HA install, but it appears to assume that MELCloud is only used for HVAC/AC systems. When tested with my system (simply an air-to-water system), I noted that the component just presumed that I had an HVAC system. It also doesn't show any zones, and only the first sensor (the power one) reports anything.

image

Is this out of scope of this PR, or is this unintentional?

@vilppuvuorinen

This comment has been minimized.

Copy link
Contributor Author

vilppuvuorinen commented Jan 28, 2020

I think the air-to-water systems are out of scope, but the lack of filtering is not. I also noticed I'm not filtering out empty model numbers for that device info like I though I did.

I have some read-only stuff for air-to-water systems here, but it has just temperatures for now. I would need help with that because I'm doing this with a blackbox or record/replay type approach (just in case) and therefore I don't have access to the source code of the original application. Help would be appreaciated. Right now it would be awesome to have captures of /Device/Get response body and bodies of individual parameter updates requests (SetAta) done at least 1 minute apart. That data is awful to anonymize. You can find me on home-assistant Discord.

@gdude2002

This comment has been minimized.

Copy link

gdude2002 commented Jan 28, 2020

While working with @vilppuvuorinen on Discord to get some testing data, I produced a Gist with some dumped data. To keep things together and to provide a resource for random internet wanderers, I'm linking it here.

Thanks for looking into this!

homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
speed = self._api.device.fan_speed
if speed is None:
return None
return speed.replace("-", " ")

This comment has been minimized.

Copy link
@rytilahti

rytilahti Jan 29, 2020

Contributor

Maybe this replace belongs to the backend library?

This comment has been minimized.

Copy link
@vilppuvuorinen

vilppuvuorinen Jan 29, 2020

Author Contributor

There's a supposedly dynamic amount of fan speeds available in these devices. The backend lib doing this speed-%d mapping for them and this is just to turn that "constant-y" string to "human readable" variant. What I'm trying to say is that I think the replace belongs here, but it just sucks.

Would it be better to use the FAN_* fan mode constants here instead of sequentially numbered speeds? I should be possible at least, but I don't know if it has any benefits.

This comment has been minimized.

Copy link
@rytilahti

rytilahti Jan 30, 2020

Contributor

Sorry, I don't know about the details, but maybe using those constants will help when it comes to consistency (think: usage via voice controls)?


async def async_set_fan_mode(self, fan_mode: str) -> None:
"""Set new target fan mode."""
await self._api.device.set({"fan_speed": fan_mode.replace(" ", "-")})

This comment has been minimized.

Copy link
@rytilahti

rytilahti Jan 29, 2020

Contributor

See above wrt. replace.

This comment has been minimized.

Copy link
@vilppuvuorinen

vilppuvuorinen Jan 29, 2020

Author Contributor

FAN_* built-in mode mappings could work here too.

homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/config_flow.py Outdated Show resolved Hide resolved
@vilppuvuorinen vilppuvuorinen requested a review from rytilahti Jan 29, 2020
@MuppetOwl

This comment has been minimized.

Copy link

MuppetOwl commented Jan 30, 2020

Swing modes (vertical vane adjusting) is missing Auto, Top, MiddleTop, Middle, MiddleBottom, Bottom and Swing
Also Heat/cool should maybe be renamed to Auto? Like it was in o0Zz version and is also what its called in Melcloud.

homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
speed = self._api.device.fan_speed
if speed is None:
return None
return speed.replace("-", " ")

This comment has been minimized.

Copy link
@rytilahti

rytilahti Jan 30, 2020

Contributor

Sorry, I don't know about the details, but maybe using those constants will help when it comes to consistency (think: usage via voice controls)?

homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
@vilppuvuorinen

This comment has been minimized.

Copy link
Contributor Author

vilppuvuorinen commented Jan 30, 2020

@MuppetOwl I'll have to backtrack on my comments on the forum. HVAC_MODE_AUTO is described as "The device is set to a schedule, learned behavior, AI." and that's definitely not the case here. I'll stick with HVAC_MODE_HEAT_COOL.

@vilppuvuorinen vilppuvuorinen mentioned this pull request Jan 30, 2020
3 of 7 tasks complete
@vilppuvuorinen vilppuvuorinen requested a review from rytilahti Jan 30, 2020
Copy link
Contributor

rytilahti left a comment

This looks fine to me, I left a couple of comments on very minor issues, maybe someone else can also take a quick look before getting this merged?

homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
@vilppuvuorinen vilppuvuorinen requested a review from rytilahti Feb 8, 2020
Copy link
Contributor

rytilahti left a comment

Looks good to me, but considering the PRs merged by myself often get commented after the merge, I hope someone else will also take a quick look before we proceed with merging :-) (ping @MartinHjelmare ;-))

Dev automation moved this from Needs review to Reviewer approved Feb 8, 2020
homeassistant/components/melcloud/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/climate.py Outdated Show resolved Hide resolved
@vilppuvuorinen vilppuvuorinen requested a review from springstan Feb 9, 2020
Copy link
Member

MartinHjelmare left a comment

Don't we want to set up a way to refresh the token automatically?

.coveragerc Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
tests/components/melcloud/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/melcloud/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/melcloud/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/melcloud/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/melcloud/test_config_flow.py Outdated Show resolved Hide resolved
Dev automation moved this from Reviewer approved to Review in progress Feb 9, 2020
* any -> Any

* Better names for dict iterations

* Proper dict access with mandatory/known keys

* Remove unused 'name' argument

* Remove unnecessary platform info from unique_ids

* Remove redundant methods from climate platform

* Remove redundant default value from dict get

* Update ConfigFlow sub-classing

* Define sensors in a dict instead of a list
@vilppuvuorinen vilppuvuorinen force-pushed the vilppuvuorinen:integration-melcloud branch from ca414fd to 1acac8b Feb 9, 2020
homeassistant/components/melcloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/melcloud/sensor.py Outdated Show resolved Hide resolved
tests/components/melcloud/test_config_flow.py Outdated Show resolved Hide resolved
@vilppuvuorinen

This comment has been minimized.

Copy link
Contributor Author

vilppuvuorinen commented Feb 9, 2020

That test failure does not make sense to me. As if pylint didn't see the usage at homeassistant/components/melcloud/config_flow.py:19.

@vilppuvuorinen vilppuvuorinen requested a review from MartinHjelmare Feb 10, 2020
Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>
Copy link
Member

MartinHjelmare left a comment

Nice!

Dev automation moved this from Review in progress to Reviewer approved Feb 10, 2020
@vilppuvuorinen

This comment has been minimized.

Copy link
Contributor Author

vilppuvuorinen commented Feb 10, 2020

Should I squash the commits now?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 10, 2020

No, please don't do that. We'll squash when merging.

@codecov

This comment was marked as off-topic.

Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #30712 into dev will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev   #30712   +/-   ##
=======================================
  Coverage   94.64%   94.64%           
=======================================
  Files         758      758           
  Lines       54882    54882           
=======================================
  Hits        51943    51943           
  Misses       2939     2939           

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 f95dd32...f95dd32. Read the comment docs.

Copy link
Member

springstan left a comment

LGTM 🎉

@rytilahti

This comment has been minimized.

Copy link
Contributor

rytilahti commented Feb 10, 2020

Considering we have already three approvals, let's get this merged. Thanks @vilppuvuorinen! 🎉

@rytilahti rytilahti merged commit b78d156 into home-assistant:dev Feb 10, 2020
10 checks passed
10 checks passed
CI Build #20200210.34 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 Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 98% of diff hit (target 94.65%)
Details
codecov/project 94.64% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Feb 10, 2020
@lock lock bot locked and limited conversation to collaborators Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.