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 support for zha thermostat #24549

Closed
wants to merge 4 commits into from
Closed

Add support for zha thermostat #24549

wants to merge 4 commits into from

Conversation

ckpt-martin
Copy link

zha fan now also reads available speeds from device

Breaking Change:

Description:

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

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.

If the code does not interact with devices:

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

zha fan now also reads available speeds from device
@ghost
Copy link

ghost commented Jun 15, 2019

Hey there @dmulcahey, @Adminiuga, mind taking a look at this pull request as its been labeled with a integration (zha) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@Adminiuga
Copy link
Contributor

IIRC merging new climate platforms if blocked until #23899 goes in. Can you split FAN changes into a separate PR?

Copy link
Contributor

@Adminiuga Adminiuga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the FAN changes into a separate PR so we could get those on.
The Climate however needs to be based on https://github.com/home-assistant/home-assistant/tree/climate-1.0/

@frenck
Copy link
Member

frenck commented Jun 20, 2019

⚠️ I was unable to find a documentation PR (matching this code change) in our documentation repository.

Please, update the documentation and open a PR for it (be sure to create a documentation PR against the next branch) and link 🔗 this PR and the new documentation PR by mentioning the link to the PR's in both openings posts/descriptions.

🏷 I am adding the docs-missing label until this has been resolved.

@pvizeli
Copy link
Member

pvizeli commented Jul 8, 2019

[ SPEED_OFF, SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH, SPEED_SMART ],
[ SPEED_OFF, SPEED_LOW, SPEED_HIGH, SPEED_SMART ],
[ SPEED_OFF, SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH, SPEED_AUTO, SPEED_SMART ],
[ SPEED_OFF, SPEED_LOW, SPEED_HIGH, SPEED_AUTO, SPEED_SMART ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only speed constants in the base fan component is allowed to be returned in the speed list or accepted as set speed argument.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we go about when platform has more options than allowed by HA component? Like in this case, Zigbee "fan mode" has two more modes/speeds, values 5 and 6
image

and in the same time, climate component has a few more fan states (BTW what does FAN_MIDDLE means in climate?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either leave some modes out, or have a non 1:1 mapping, ie map eg two device speeds to the same home assistant speed and pick one of the two device speeds to map to the home assistant speed the other way.

There's no really good solution for this problem currently. We have to solve it as best possible while staying in line with current architecture.

@MartinHjelmare MartinHjelmare added this to Review in progress in Dev Jul 23, 2019
@presslab-us
Copy link
Contributor

Interesting! I didn't realize someone could open a pull request from someone else's repo.

@MartinHjelmare
Copy link
Member

Sorry for not noticing this earlier @presslab-us. I'll close this now.

Dev automation moved this from Review in progress to Cancelled Jul 25, 2019
@presslab-us
Copy link
Contributor

It's no problem! I figured @Adminiuga would be the one to do ZHA thermostats because he has a branch working on that already. I am happy to update mine for those that want to use it in the mean time but I don't expect it would become the official one.

@Adminiuga
Copy link
Contributor

@presslab-us I have one tracking/rebasing against current dev constantly at https://github.com/Adminiuga/home-assistant/tree/ac/new-zha-climate-1.0

Feel free to give it a try. Don't have FAN support currently as my "production" climate is baseboard heating only, but I'm building a testbed for a regular heat/cool/fan thermostat so going to add it relatively soon. Primarily I wanted to have "away" support and while ZCL does rudimentary support it, implementation methods between vendors differs.

@Hedda
Copy link
Contributor

Hedda commented Jan 15, 2020

@Adminiuga & @presslab-us any news on ZHA Climate support since @pvizeli merged #23899 ?

Climate is the main device type missing from ZHA: https://www.home-assistant.io/integrations/zha/

Update: Noticed that @presslab-us and @Adminiuga have branches so I reposted my question as a new architecture for discussion there instead => home-assistant/architecture#336

@presslab-us
Copy link
Contributor

I have been maintaining and using my fork since May and it has been working just fine. I just merged in the latest dev just yesterday in fact.
https://community.home-assistant.io/t/zha-thermostat/117851

@iamJoeTaylor
Copy link

@MartinHjelmare @Adminiuga @presslab-us
Thank you all so much for your work here. I just installed my Zen Zigbee Thermostat and found my way here.

I would love to find a way to get this into ZHA master. I'd be willing to put some time into getting @Adminiuga 's branch fixed up to a state where it can be merged in.

@MartinHjelmare If I prepared a PR, what is blocking this from being considered for Master, just the fan speed changes?

@Hedda
Copy link
Contributor

Hedda commented Mar 10, 2020

@dmulcahey & @Adminiuga & @presslab-us & & @iamJoeTaylor @pvizeli & @DB-CL would it be a good idea to base a ZHA thermostat device type configuration on the new PR #31799 to help as a reference concept for this? PR #31799 is DB-CL refactoring and improvement of the Generic Thermostat (generic_thermostat climate platform) for climate 1.0

@presslab-us
Copy link
Contributor

@Hedda there's no need to tag me in any of this, I am no longer contributing to ZHA or zigpy.

@Hedda
Copy link
Contributor

Hedda commented May 18, 2020

FYI, Adminiuga has just now submitted a first pull request for "ZHA Climate" (beta) with thermostats

#35682

Note! That mentions that currently only a Sinope thermostat and a Zen thermostat has been tested.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators May 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

10 participants