Support for TadoX thermostatic radiator valves #129600

Moritz-Schmidt · 2024-10-31T20:14:46Z

Proposed change

Add support for TadoX thermostatic radiator valves (TRV) which has some API changes to the old Tado V3.
This doesn't change any functionality for Tado V3 systems.

This PR will only add support for the TadoX radiator valve, it may work with the Smart Thermostat X and the Wireless Temperature Sensor X but not tested, will probably not work with Heat Pump Optimizer X.

~~This also changes the PyTado dependency from 0.17.6 to 0.17.7~~ Library version bump is in PR #129842 which has to be merged first

Since – with the heating season having started already – people are waiting for the support for TadoX it would be nice to get this released this Month.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes Valves not present on Home Assistant #129097

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

Co-authored-by: Karl Beecken <[email protected]>

home-assistant

Hi @Moritz-Schmidt

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

home-assistant · 2024-10-31T20:14:54Z

Hey there @chiefdragon, @erwindouna, mind taking a look at this pull request as it has been labeled with an integration (tado) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of tado can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign tado Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

liudger

LGTM

home-assistant

Hi @karlbeecken

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Co-signed-by: Moritz Schmidt <[email protected]>

karlbeecken · 2024-11-02T09:06:14Z

It says changes are requested by the bot, but I do not understand what the bot wants to be changed. Both contributors have signed the CLA, is anything else missing?

erwindouna

Thanks for taking the effort in bringing in TadoX! I have a few remarks and questions left.

homeassistant/components/tado/climate.py

homeassistant/components/tado/entity.py

erwindouna · 2024-11-02T10:08:43Z

It says changes are requested by the bot, but I do not understand what the bot wants to be changed. Both contributors have signed the CLA, is anything else missing?

It seems... Nothing. No worries, it can be overruled once a Core Member did the review.

homeassistant/components/tado/binary_sensor.py

homeassistant/components/tado/climate.py

homeassistant/components/tado/entity.py

homeassistant/components/tado/manifest.json

homeassistant/components/tado/sensor.py

homeassistant/components/tado/tado_connector.py

erwindouna · 2024-11-13T08:36:00Z

@Moritz-Schmidt I also want to let you know that there is an ongoing effort to try and get a new library from scratch (#121503), which would be async fully typed.

Thanks for the info! I think I'll try to implement Tado X support there and help to get it working with this integration. But for the time being I think it would be good to merge this PR to get Tado X running with the December release if tadoasync isn't ready yet.

That would be much appreciated! I have a new branch working on it. Your contribution to get TadoX in would really help me, whereas I don't have TadoX, yet.
Feel free to create a PR here: https://github.com/erwindouna/python-tado to integrate TadoX as well.

jelmerkk · 2024-11-18T09:06:16Z

Anything that can be done still to be able to include this in the .12 release?

edenhaus

You added a lot of complexity with the if tado.is_x statements. Can you please check if it's possible to create two classes for it?

Some changes like identifying if the device is a TadoX should go directly in the library

edenhaus · 2024-11-26T14:44:59Z

homeassistant/components/tado/climate.py

+            if self._tado.is_x and self._tado_geofence_data["presence"] == "HOME":
+                return PRESET_HOME
+            if self._tado.is_x and self._tado_geofence_data["presence"] == "AWAY":
+                return PRESET_AWAY


Suggested change

if self._tado.is_x and self._tado_geofence_data["presence"] == "HOME":

return PRESET_HOME

if self._tado.is_x and self._tado_geofence_data["presence"] == "AWAY":

return PRESET_AWAY

if self._tado.is_x:

if self._tado_geofence_data["presence"] == "HOME":

return PRESET_HOME

if self._tado_geofence_data["presence"] == "AWAY":

return PRESET_AWAY

edenhaus · 2024-11-26T14:49:34Z

homeassistant/components/tado/entity.py

+                sw_version=device_info["currentFwVersion"],
+                model=device_info["deviceType"],
+                via_device=(
+                    DOMAIN,
+                    device_info["serialNo"],
+                ),


To deduplicate code please create local variables for these attributes and have a single DeviceInfo

edenhaus · 2024-11-26T14:50:59Z

homeassistant/components/tado/climate.py

+    cool_max_temp: float | None = None
+    cool_step: float | None = None
+
+    if tado.is_x:


Instead of having a lot of these if statements, can we have separate classes for it?

edenhaus · 2024-11-26T14:54:48Z

homeassistant/components/tado/tado_connector.py

+        self.is_x = self.tado.http.isX
+        [device.update(is_x=self.is_x) for device in self.devices]


This code is strange...
Why can't the library identify if the device is a TadoX or not??? I recommend to extend the library to add support to identify if a device is a TadoX one instead of doing here this hacky workaround

Also, you are creating a list here, which is never to be used...

The library identifies if the device is a TadoX. The isX attribute comes from it.

The smell here is the leak of that http object. HA shouldn't be aware of the libraries internals.

edenhaus · 2024-11-26T14:56:10Z

homeassistant/components/tado/tado_connector.py

@@ -221,6 +240,8 @@ def update_home(self):

    def get_capabilities(self, zone_id):
        """Return the capabilities of the devices."""
+        if self.is_x:
+            return {"type": TYPE_HEATING}


Why does the library not provide this information for the TadoX too?

That is because the Tado X API does not return any comparable capabilities. The endpoint that did this in the old API is not existing anymore.

Same issue as above. get_capabilities is an Endpoint of the Tado pre-X devices.

The library should abstract this implementation detail away, so the HA integration wouldn't have to know about this.

The issue is that there is a complete rewrite being done by @erwindouna at the time anyway, so from our point of view it isn't really a good use of time to make everything "beautiful" in the old library. This here is intended as a rather quick fix so users can get their Tado X working with the beginning of the heating season in the northern hemisphere.

I understand.

This is a decision that needs to be done and communicated by @edenhaus, as he would take the responsibility, that the "temporary" solution really is just temporary.

@edenhaus Is there anything you need help with to get rewrite shippable?

Moritz-Schmidt · 2024-11-27T14:16:13Z

I've decided that I'll hereby close this PR and not to pursue completion any further. I've always wondered why it seems to be more popular to contribute via HACS rather than contributing to core directly, but now I think I understand.

I can understand all the feedback in the reviews and for the most part I agree with it. It's not beautiful code, it's not a nice solution, but it still works, and it would have been a sufficient temporary fix. Perhaps I should have made it clearer and earlier that it was intended as a temporary fix to get Tado X working as the heating season has already started in most parts of the Northern Hemisphere. My problem is that at first it seems like everything is okay, the PR is close to merging and added to the beta milestone. Then the PR is drafted again with no realistic timeframe remaining to fix the issues in time to get it out in 2024.12. Those Reviews could have been written weeks earlier with enough time remaining to get things done. Furthermore, it's not really transparent to newbies like me how the release cycle works, when cutoff dates are etc.

A good solution will have to come with the rewrite of the upstream tado library (for which I have already started implementing Tado X support).

I'm really sorry for everyone waiting patiently for Tado X support (you can still add the changes in this PR as a custom component). I still want to help to get this working, but I'll have to reconsider if it's really worth the time and hassle. Anyway it'll probably take some time.

modem7 · 2024-11-27T14:57:48Z

@Moritz-Schmidt - whilst I'm not affiliated with HA bar being a user and paying my subscription to NabuCasa, I'd like to thank you for doing what you've done so far on this PR.

I understand the frustrations you've gone through as I've gone through them myself, and it's rare to get at least a sense of "why bother?" at times, so I'd like to chip in and offer you my thanks for the work you've done integrating this so far, and I look forward to a resolution if and when there is one as I'm about to purchase said Tado system myself.

jelmerkk · 2024-11-27T16:03:16Z

Thanks @Moritz-Schmidt and @karlbeecken for your contributions. I hope we can find a way to update the integration soon. And don't loose faith in contributing to HA. For us non-developers we can only offer support in testing, and your contributions are invaluable to make HA as great as it is.

In the meanwhile I'm pretty sure that an updated version of the Node Red Tado node will be released soon (0.12) that will have basic TadoX support. Hopefully that will get us through the winter :)

MrEbbinghaus · 2024-11-27T16:24:35Z

@Moritz-Schmidt At first, let me say, that I can understand your position. It can get frustrating at times, working on a large project with so many moving parts and people. I experienced it with my first PR, which I then recommended being reverted, because of some issues in HA core.

I especially understand your frustration with "turbulent" communication. I don't understand why @frenck added the smash or beta release tag. Most likely it's an internal organization thing, not necessarily meant for us? (@frenck Correct me if this is explained somewhere?)

But: I have to say, I find some of your statements unjustified.
It is the responsibility of the codeowners that the project remains in a maintainable state, and so it is on them to decide if this temporary fix is sufficient. If it's not, it can be more work in the future (for them and other contributors, not necessarily you). Expecting them to do this because you think it's right is not fair.

Keep in mind, that you are submitting code, that gets installed in hundred of thousands of homes and which has access to critical things like heating.

This PR adds yourself to the CODEOWNERS file (767af19), with a commit message that is: set correct python-tado version. Codeowners are trusted people, that have responsibility and authority for part of the code. (See Github Docs) Inserting yourself this way can be seen as impolite in the simplest case, in the worst as an intrusion attempt (see the recent XZ Utils intrusion)
(To be clear: I'm not trying to accuse you of malicious intent here.)

Those Reviews could have been written weeks earlier with enough time remaining to get things done.

I assume that you meant this differently than you expressed it. (I can understand frustration with long dev cycles)
You don't expect volunteers to follow your timeline and use their spare time to review your proposals, right?

Roedandan · 2024-11-27T16:43:20Z

It's sad to see this PR being closed prematurely. We were so close! @Moritz-Schmidt Thanks for your tireless contributions!

With this PR being closed, it's time to look ahead and plan a more permanent solution. Altough @edenhaus is making big steps, the timeline for Tado X being supported will probably be pushed back substantially. With this temporary solution being pulled, it might be nice looking ahead at that timeline.

Moritz-Schmidt · 2024-11-27T17:20:38Z

@MrEbbinghaus thanks for your reply. I may have been too emotional and impolite earlier, I apologise for that.

It is the responsibility of the codeowners that the project remains in a maintainable state, and so it is on them to decide if this temporary fix is sufficient. If it's not, it can be more work in the future (for them and other contributors, not necessarily you). Expecting them to do this because you think it's right is not fair.

That's correct, my problem is not with the disagreements in terms of content, but primarily with the turbulent communication. I don't expect anyone to have the same opinion as me. On the contrary – a project like this thrives on discussion and mutual feedback.

This PR adds yourself to the CODEOWNERS file (767af19), with a commit message that is: set correct python-tado version. Codeowners are trusted people, that have responsibility and authority for part of the code. (See Github Docs) Inserting yourself this way can be seen as impolite in the simplest case, in the worst as an intrusion attempt (see the recent XZ Utils intrusion)
(To be clear: I'm not trying to accuse you of malicious intent here.)

I'm sorry, this was impolite of me, I should have at least made it a separate commit. I was following the development checklist and didn't give it a second thought which I should have done. However, it was my intention to also take responsibility for the code I submitted.

I assume that you meant this differently than you expressed it. (I can understand frustration with long dev cycles)
You don't expect volunteers to follow your timeline and use their spare time to review your proposals, right?

No of course I don't expect volunteers to do that. But I expect to have somewhat of a clear timeline, especially when it comes from people who are presumed to be Nabu Casa employees.

Moritz-Schmidt · 2024-11-27T17:30:03Z

With this PR being closed, it's time to look ahead and plan a more permanent solution.

I think the best way would be to get tadoasnyc ready for use in the Tado integration and implement Tado X Support in a way that is not as hacky as in the old library. After the upstream library is ready the Integration has to be updated to use the new Library and the Tado X devices. As I've wrote earlier I've already started implementing Tado X in the tadoasync library and will probably continue my work there. I think any help on tadoasync will be appreciated, right @erwindouna ?

maxcerny · 2024-11-27T21:13:16Z

I'm really sorry for everyone waiting patiently for Tado X support (you can still add the changes in this PR as a custom component). I still want to help to get this working, but I'll have to reconsider if it's really worth the time and hassle. Anyway it'll probably take some time.

I've added all the files as a custom component, but i still get handed the built in integration when trying to add it

Edit: NVM figured it out, the manifest was missing a version, added one and now it's working

jelmerkk · 2024-11-28T12:18:23Z

Can you share how you did this? I could not get this to work yet...

maxcerny · 2024-11-28T13:00:33Z

Can you share how you did this? I could not get this to work yet...

Do you already have the files in custom components?

Moritz-Schmidt · 2024-11-28T13:39:00Z

For anyone waiting for Tado X Support: Threre are 2 Ways to get Tado X working right now

1. Custom Integration:

Copy the files from https://github.com/Moritz-Schmidt/hass-core/tree/dev/tests/components/tado into homeassistant/custom_components/tado
add "version": "v1.0.0", to manifest.json (it doesn't have to be 1.0.0, just anything would be fine afaik)
restart home assistant and you should be good to go
There may or may not be breaking changes when switching back to the core integration when it has Tado X support

OR

2. via Matter and Thread
If you have a compatible thread border router you can add the thermostats with very basic support via thread and matter. The Thremostats have to be added to the Tado App befor adding them to Home Assistant, otherwise it's not possible to add them to Tado later afaik.

Doing both does not really make sense since you would have them twice in Home Assistant.

jelmerkk · 2024-11-28T14:22:08Z

Can you share the manifest.json for your custom integration? Just having the version there is apparently not enough :)

maxcerny · 2024-11-28T14:44:02Z

{
  "domain": "tado",
  "name": "Tado",
  "codeowners": ["@chiefdragon", "@erwindouna", "@Moritz-Schmidt"],
  "config_flow": true,
  "dhcp": [
    {
      "hostname": "tado*"
    }
  ],
  "documentation": "https://www.home-assistant.io/integrations/tado",
  "homekit": {
    "models": ["tado", "AC02"]
  },
  "iot_class": "cloud_polling",
  "loggers": ["PyTado"],
  "requirements": ["python-tado==0.17.7"],
  "version": 2024.12
}

thomasbeaumont · 2024-11-29T10:24:57Z

Hi and thanks for the hard work! @maxcerny I copied your conf and custom integration overrides default as expected, but I'm getting "Config flow could not be loaded: {"message":"Invalid handler specified"}"

Does anyone else experience this?

Much appreciated

maxcerny · 2024-11-29T10:46:44Z

Strange, I didn't get that error on 3 separate HA installations. One thing I did have on all of them was the base integration was already set up. Maybe try removing the custom component, set up the core tado integration, then add the custom component back.

thomasbeaumont · 2024-11-29T11:01:10Z

thanks @maxcerny for your quick answer and help

Did that, could reconfigure cloud access, but now the integration won't load

Logger: homeassistant.setup
Source: setup.py:269
First occurred: 11:59:07 AM (1 occurrences)
Last logged: 11:59:07 AM

Setup failed for custom integration 'tado': No setup or config entry setup function defined.

maxcerny · 2024-11-29T11:31:02Z

Then I'm sorry, I can't help you. :(

Those the steps I took, and it all worked for me.

thomasbeaumont · 2024-11-29T12:05:58Z

thank you, will stick to matter for now :)

Support for TadoX thermostatic radiator valves

08734c5

Co-authored-by: Karl Beecken <[email protected]>

home-assistant bot previously requested changes Oct 31, 2024

View reviewed changes

home-assistant bot added cla-needed Hacktoberfest integration: tado new-feature Quality Scale: No score labels Oct 31, 2024

home-assistant bot added cla-recheck cla-signed and removed cla-recheck cla-needed labels Oct 31, 2024

Merge branch 'home-assistant:dev' into dev

b5544e4

liudger approved these changes Oct 31, 2024

View reviewed changes

karlbeecken mentioned this pull request Nov 1, 2024

Tado integration not working with Heat Pump Optimizer X: no devices found #128558

Open

Merge branch 'dev' into dev

c9b2c4c

home-assistant bot added the cla-needed label Nov 1, 2024

home-assistant bot previously requested changes Nov 1, 2024

View reviewed changes

home-assistant bot added cla-recheck and removed cla-recheck cla-needed labels Nov 1, 2024

karlbeecken added 2 commits November 1, 2024 20:53

set correct python-tado version

767af19

Co-signed-by: Moritz Schmidt <[email protected]>

fix tests

bbb0105

Moritz-Schmidt marked this pull request as ready for review November 1, 2024 21:54

erwindouna suggested changes Nov 2, 2024

View reviewed changes

homeassistant/components/tado/climate.py Outdated Show resolved Hide resolved

homeassistant/components/tado/entity.py Outdated Show resolved Hide resolved

Merge branch 'dev' into dev

116811d

zweckj reviewed Nov 2, 2024

View reviewed changes

code quality improvements

42856eb

Moritz-Schmidt mentioned this pull request Nov 11, 2024

Tado configuration error #130274

Open

frenck added this to the 2024.12.0b0 milestone Nov 25, 2024

Merge branch 'dev' into dev

a461545

edenhaus requested changes Nov 26, 2024

View reviewed changes

home-assistant bot marked this pull request as draft November 26, 2024 14:59

edenhaus removed the smash Indicator this PR is close to finish for merging or closing label Nov 26, 2024

edenhaus removed this from the 2024.12.0b0 milestone Nov 26, 2024

Moritz-Schmidt closed this Nov 27, 2024

home-assistant bot removed the Hacktoberfest label Nov 27, 2024

github-actions bot locked and limited conversation to collaborators Nov 30, 2024

		self.is_x = self.tado.http.isX
		[device.update(is_x=self.is_x) for device in self.devices]

Support for TadoX thermostatic radiator valves #129600

Support for TadoX thermostatic radiator valves #129600

Conversation

Moritz-Schmidt commented Oct 31, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot left a comment

Choose a reason for hiding this comment

home-assistant bot commented Oct 31, 2024

liudger left a comment

Choose a reason for hiding this comment

home-assistant bot left a comment

Choose a reason for hiding this comment

karlbeecken commented Nov 2, 2024

erwindouna left a comment

Choose a reason for hiding this comment

erwindouna commented Nov 2, 2024

erwindouna commented Nov 13, 2024 • edited Loading

jelmerkk commented Nov 18, 2024

edenhaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrEbbinghaus Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Moritz-Schmidt commented Nov 27, 2024

modem7 commented Nov 27, 2024 • edited Loading

jelmerkk commented Nov 27, 2024

MrEbbinghaus commented Nov 27, 2024

Roedandan commented Nov 27, 2024

Moritz-Schmidt commented Nov 27, 2024

Moritz-Schmidt commented Nov 27, 2024

maxcerny commented Nov 27, 2024 • edited Loading

jelmerkk commented Nov 28, 2024

maxcerny commented Nov 28, 2024

Moritz-Schmidt commented Nov 28, 2024 • edited Loading

jelmerkk commented Nov 28, 2024

maxcerny commented Nov 28, 2024 • edited Loading

thomasbeaumont commented Nov 29, 2024

maxcerny commented Nov 29, 2024

thomasbeaumont commented Nov 29, 2024

maxcerny commented Nov 29, 2024

thomasbeaumont commented Nov 29, 2024

Moritz-Schmidt commented Oct 31, 2024 •

edited

Loading

erwindouna commented Nov 13, 2024 •

edited

Loading

MrEbbinghaus Nov 26, 2024 •

edited

Loading

modem7 commented Nov 27, 2024 •

edited

Loading

maxcerny commented Nov 27, 2024 •

edited

Loading

Moritz-Schmidt commented Nov 28, 2024 •

edited

Loading

maxcerny commented Nov 28, 2024 •

edited

Loading