Ts0601 trv

[gmsoft-tuxicoman]

Breaking change

This PR moves all the hacky OnOff/Number clusters for the ZonnSmart TRV into proper HA components. As such, all the entities created by those clusters will be gone and replaced by the new ones from the HA components.

Proposed change

This PR creates new components for the ZonnSmart TRV which were handled by custom clusters.

Type of change

• [ ] Dependency upgrade
• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New integration (thank you!)
• [x] 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 is to be merged alongside https://github.com/zigpy/zha-device-handlers/pull/1961 . The above PR for zha-device-handlers add some functionalities but also cleanup the hacky clusters. The clusters are then replaced by proper HA components.

Checklist

• [x] The code change is tested and works locally.
• [x] Local tests pass. Your PR cannot be merged unless tests pass
• [x] There is no commented out code in this PR.
• [x] I have followed the development checklist
• [x] The code has been formatted using Black (black --fast 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.
• [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

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

[home-assistant[bot]]

Hi @gmsoft-tuxicoman

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[bot]]

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

Code owner commands

Code owners of zha can trigger bot actions by commenting:

• @home-assistant close Closes the issue.
• @home-assistant rename Awesome new title Change the title of the issue.
• @home-assistant reopen Reopen the issue.
• @home-assistant unassign zha Removes the current integration label and assignees on the issue, add the integration domain after the command.

[MattWestb]

Is it possible doing the same here so not need adding devices in ZHA every time new TRVs is showing up ? https://github.com/home-assistant/core/blob/746afb70badb8772648d49c912f83719fcbc30f7/homeassistant/components/zha/climate.py#L760-L774

[javicalle]

Is it possible doing the same here so not need adding devices in ZHA every time new TRVs is showing up ?

Not the same case. There can be (and will be) a lot of devices with the channel_names=CHANNEL_THERMOSTAT that wouldn't have the ZONNSMART presets, at least all the not quirked devices.

@gmsoft-tuxicoman code points to channel_names="tuya_manufacturer" devices. Not sure if only the quirked ones will match here and that is a question that I will ask to him. Have you tested the PR against devices with the matching signature (channel_names="tuya_manufacturer", models={"TS0601"}) but without a quirk applied?

[MattWestb]

@javicalle Is it possible overriding the manufacture "TS0601" in the quirk to somthing like "TS0601-TRVZS" and using it in ZHA ? Then is only quirked devices that is using it.

[jacekk015]

This will probably break all TS0601 devices. Any problems in this area will touch all HA users.

[javicalle]

Not sure if the current approach will break TS0601 devices or just give some errors in the logs, but IMHO is a too much generic definition.

@javicalle Is it possible overriding the manufacture "TS0601" in the quirk to somthing like "TS0601-TRVZS" and using it in ZHA ? Then is only quirked devices that is using it.

I would prefer my other approach: to overwrite the ep_attribute for that devices like that:

class SiterwellManufCluster(TuyaManufClusterAttributes):
    """Manufacturer Specific Cluster of some thermostatic valves."""

    ep_attribute = "tuya_trv"

    attributes = TuyaManufClusterAttributes.attributes.copy()
    attributes.update(
        {
            .../...
    )

This way you can ensure that all the channel_names="tuya_trv" are quirked and are of type TRV (TS0601 is plenty of diferents device types).

But this approach has not been approved and not sure if is the way to go.

In any case, I'll be against any approach that involves keeping the list of manufacturers on both sides (ZHA and the quirks).

[MattWestb]

In any case, I'll be against any approach that involves keeping the list of manufacturers on both sides (ZHA and the quirks).

Most of tuya MCU devices is first coming one and later its coming more versions with the same functionality and its one PITA patching quirks and ZHA. If getting one "master" quirk and one handle in ZHA for the device type /class and only need updating the quirk is being much better then today.

Sadly we was not knowing what was coming then tuya was starting falling down on us and we shall have doing this much earlier.

[gmsoft-tuxicoman]

From my testing, the additional entities only show up when the corresponding zcl attribute exists. This means that other TS0601 devices without the attributes I'm creating entitites for will be left untouched.

I currently create entities for the folliowing attributes:

• window_detection
• online_set
• temperature_calibration
• opened_window_temperature
• child_lock
• boost_duration_seconds

TRVs with the same model as ZonnsmartTV01_ZG :

• SiterwellGS361_Type2
• MoesHY368_Type1
• MoesHY368_Type1new

Right now it seems that the new components would benefit the other TRV as well but the window_detection component. The window_detection attribute is an array for the Moes TRVand it would definitely break.

So I can update my zha-device-handler PR and add zonnsmart_trv as the ep_attribute as it seems like a sensible approach but other TRVs will not benefit from the new components.

There is also another approach. We could modify HA to be able to match the quirk_class. Maybe it would be best and solve the matching even for different devices. @javicalle I can probably work on a PR in that way if you feel like it would be beneficial.

Let me know what you think.

[javicalle]

This means that other TS0601 devices without the attributes I'm creating entitites for will be left untouched.

But this generates error logs? I think that some issues had been made because of something similar.

There is also another approach. We could modify HA to be able to match the quirk_class.

That would be an alternative approach to my ep_attribute overwrite with the same benefits (or even more). @dmulcahey any thoughts on this topic? Let me know if you need more context.

[gmsoft-tuxicoman]

This means that other TS0601 devices without the attributes I'm creating entitites for will be left untouched.

But this generates error logs? I think that some issues had been made because of something similar.

None that I can see.

[MattWestb]

We need getting it working for the first gen TRVs with the old Zigbee module _TYST11_ or we is loosing support for 1/4 of all TRVs.

If possible using the device class of the quirk its shall not being any problems. Also adding new devices in the quirk its getting all the function in ZHA without adding devices in ZHA = +++.

[gmsoft-tuxicoman]

I created the PR anyway : https://github.com/home-assistant/core/pull/87653

[MattWestb]

If that working OK i think its the best we have done for getting better support for strange tuya MCU devices !!! Also easy adding more versions of one device with different EP / cluster setting in one new class its only adding the class in ZHA and it working OK.

Also if some like doing one complete new device in ZHA its only making one quirk with one device class and piping all tuya MCU commands to ZHA that is using the new device type in ZHA..

Great work !!!

[TheJulianJES]

But this generates error logs? I think that some issues had been made because of something similar.

Warnings should only show up when you try to initialize an attribute that does not exist. If it returns "unsupported attribute" (but the attribute can be found), no warning is printed.

[gmsoft-tuxicoman]

If PR https://github.com/home-assistant/core/pull/87653 gets approved, I will probably change the names of the component's class I created to something more generic like SwitchTuyaTRVChildLock() and apply that to the ZonnSmart TRV. Then people who have the Moes or other TRV might just add their quirk class there after confirming it works.

[javicalle]

I will probably change the names of the component's class I created to something more generic

Yes please. This way we can match the same HA entity just with the attribute name, the cluster and the quirk class. I believe that this would be a much more versatile approach.

Thanks for your work.

[gmsoft-tuxicoman]

PR https://github.com/home-assistant/core/pull/87653 has been merged !

I"ve updated this one to use quirk_classes and it works flawlessly. I wasn't confident about merging all the child lock switch for examples so I didn't do that. I'd rather someone who owns those devices to add the right quirk_class.

This PR should be merged alongside https://github.com/zigpy/zha-device-handlers/pull/1961 as they both depend on each other.

[gmsoft-tuxicoman]

This PR along with https://github.com/zigpy/zha-device-handlers/pull/1961 are both ready to be merged. I can rebase them if needed.

The failing codecov issue can be fixed but that means writing really long unit test for a just a few lines of trivial code. Let me know what you think.

[gmsoft-tuxicoman]

The failure for the test above is because I specify "ts0601_trv.ZonnsmartTV01_ZG" for the quirk class while the unit test expects "zhaquirks.tuya.ts0601_trv.ZonnsmartTV01_ZG". However, if I use that last value, the elements don't match and don't appear in the UI.

Has https://github.com/home-assistant/core/pull/90140 been tested correctly while checking that the elements appear in the UI ?

[gmsoft-tuxicoman]

I have used the full path for the quirk_class and it works provided https://github.com/home-assistant/core/pull/93268 is merged I can test it with custom quirks.

[home-assistant[bot]]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

[gmsoft-tuxicoman]

I fixed the unit test issue. pylint seems to have timed out because of a network issue in github,

[puddly]

CI is currently failing because the patch doesn't have 100% coverage. Once https://github.com/home-assistant/core/pull/98421 is merged you can remove all of the try blocks, attribute writing SUCCESS checks, and then add a quick test for the 10x attribute scaling. It should be good to merge after that. Thanks!

[edenhaus]

Converting this PR to draft as it requires #98421 to be merged first.

[TheJulianJES]

For reference, my comment in the quirk PR: https://github.com/zigpy/zha-device-handlers/pull/1961#issuecomment-1777734802 So, I'd wait until quirk IDs have replaced quirk class path matching, then merge the quirks PR, bump quirks in HA, then update this, then merge this finally.

This PR should not be merged until the quirks PR is merged. (so, should not be merged for 2023.11.x)

[emontnemery]

@gmsoft-tuxicoman please respond to the review by @TheJulianJES and mark the PR as ready for review when done 👍

[gmsoft-tuxicoman]

Unfortunately there is no way the checks will work until https://github.com/zigpy/zha-device-handlers/pull/1961 is merged. It requires the quirk_id from the new quirks.

For the rest, the translations are now working. All the components are added correctly when adding the device.

[gmsoft-tuxicoman]

Rebased. Still pending on https://github.com/zigpy/zha-device-handlers/pull/1961 for the self tests to pass.

[TheJulianJES]

As a note, quirks v2 is currently being worked on and should hopefully land relatively soon. If everything works out as expected, we're able to expose enough metadata from zha-quirks to have ZHA automatically create the correct custom/configuration entities (without them needing to be defined in ZHA).

Since this PR has been stuck for quite some time, we could still merge this probably (as soon as reviewed/approved + quirks bumped), but we eventually want to move all/most custom entities to quirks directly. That should also make it way easier to maintain stuff like this without having to jump between quirks and ZHA all the time.