zha-device-handlers icon indicating copy to clipboard operation
zha-device-handlers copied to clipboard

Maxsmart, Beca, Siterwell and Moes TRV support/update

Open jacekk015 opened this issue 3 years ago • 33 comments

According to matrix https://github.com/zigpy/zigpy/discussions/653 new TRVs has support for ZHA and some got update of functionality.

Siterwell tested locally by @MattWestb https://github.com/zigpy/zha-device-handlers/discussions/1084#discussioncomment-1701973

Beca tested locally by @iedex and @Youpimatin https://github.com/zigpy/zha-device-handlers/issues/1123#issuecomment-980314632

Maxsmart written and tested locally by me - I own 12pcs of them Maxsmart -> Silvercrest Tuya Lidl clone tested by @JoostvdB94 @lukasf98 @baldisos https://github.com/zigpy/zha-device-handlers/issues/1061#issuecomment-981156618

Moes TRV extended using same approach as above Tuyas, split to separate file Zonnsmart not touched code, spitted to separate file

Pull request made by @MattWestb should be Closed, newer code version is here https://github.com/zigpy/zha-device-handlers/pull/1129

jacekk015 avatar Nov 29 '21 12:11 jacekk015

Are you planning on finishing this in the next couple of days? The beta for HA is Friday and I want to cut a release and get it into HA before then. I'll wait if you have the bandwidth to finish this before then. Just LMK.

dmulcahey avatar Dec 01 '21 18:12 dmulcahey

If jacekk015 can do all test we also need adding more presets for all new devices in ZHA that is needing presets for working OK but i can helping with that if needed.

MattWestb avatar Dec 01 '21 18:12 MattWestb

Unfortunately I don't know the Python test approach so that parts is out of my knowledge. I've seen plenty of bytes sent RAW which I do not understand. Isort check is wrong itself here - I've got black and isort on Visual Studio Code installed and my Python 3.9.4 on Ubuntu Linux doesn't complain at all. Maybe there's a switch for isort to make them compliant.

jacekk015 avatar Dec 01 '21 18:12 jacekk015

I have tested the Maxsmart implementation with _TZE200_thbr5z34 (https://github.com/zigpy/zha-device-handlers/issues/978) and it is working.

@jacekk015 Would it be possible to make the HVAC mode off work to disable heating?

chaptergy avatar Dec 12 '21 18:12 chaptergy

Not sure if Off mode on TRV means totally Off. It could be that Freeze prevention is still enabled - I think I've read that in TRV manual. Is it possible? Of course. Bear in mind that you can make that TRV off only in Manual mode, so you need working Presets patch to make use of it. If not you won't be able to switch from Manual mode to Schedule.

jacekk015 avatar Dec 12 '21 18:12 jacekk015

Yes, I was talking about that mode as well, so no heating except when it is preventing things from breaking due to freezing or calcification.
But you're right, switching it to off is only possible in manual mode, I did not notice that before. But I'm not sure I understand why it would prevent a switch to auto mode? It seems internally in the trv manual temperature and auto temperature are saved separately, so when setting it to off in manual mode and switching back to auto mode, the last used auto temperature is restored.
But I'm not sure what should happen when the device is in auto mode and the command to set it to off is received...

chaptergy avatar Dec 12 '21 18:12 chaptergy

TRVs is normally not have any off mode like electrical thermostats is having and and they have normal different working modes like away or holyday for not losing antifreeze protection. Some have making it so that you is manual making max is the valve force 100% open and min its completely closed but with antifreeze protection still working.

The system more off is the same the TRV is dead and cant / shall not do anything (only sending one new system mode to the system that is not off).

If making one preset with off then it shall being the lowest possible inforced in manual mode.

If cant using working mode in the TRV / presets its better using HA using manual mode and setting one very low setpoint temperature by adding one template switch that is doing that operation.

MattWestb avatar Dec 12 '21 18:12 MattWestb

@chaptergy Use a local quirk from this post. https://github.com/zigpy/zha-device-handlers/issues/1061#issuecomment-992905875 This quirk probably won't be published because it doesn't pass the tests. Waste of time waiting for that. It's almost winter.

jacekk015 avatar Dec 13 '21 21:12 jacekk015

Hi, @MattWestb asked me for help with the tests here.

I have rebased this branch against current dev and added a fix for the cause of these errors in the tests:

E   ModuleNotFoundError: No module named 'zhaquirks.tuya.ts0601_trv

There is now another error in the tests showing up. I think this time it could be a genuine problem:

tests/test_tuya.py:430: in test_valve_state_report
    tuya_cluster.handle_message(hdr, args)
../zigpy/zigpy/zcl/__init__.py:205: in handle_message
    self.handle_cluster_request(hdr, args, dst_addressing=dst_addressing)
zhaquirks/tuya/__init__.py:417: in handle_cluster_request
    self._update_attribute(tuya_cmd, zvalue)
zhaquirks/tuya/ts0601_trv_siterwell.py:141: in _update_attribute
    (value + temp_calibration * 10) * 10,
E   TypeError: unsupported operand type(s) for *: 'NoneType' and 'int'

The error is happening here:

        if attrid in (SITERWELL_TEMPERATURE_ATTR, SITERWELL_TARGET_TEMP_ATTR):
            if attrid == SITERWELL_TEMPERATURE_ATTR:
                temp_calibration = (
                    self.endpoint.device.SiterwellTempCalibration_bus.listener_event(
                        "get_value"
                    )[0]
                )
                self.endpoint.device.thermostat_bus.listener_event(
                    "temperature_change",
                    "local_temp",
>                   (value + temp_calibration * 10) * 10,
                )
E               TypeError: unsupported operand type(s) for *: 'NoneType' and 'int'

zhaquirks/tuya/ts0601_trv_siterwell.py:141: TypeError

It is because temp_calibration is uninitialised in SiterwellTempCalibration_bus so the value returned is None. This will be because the test has not simulated a calibration value message beforehand. It should be possible to do that, but I'm thinking that this condition should have been handled in the code somehow. It's not clear to me what is initialising this and how. @jacekk015 what do you think?

challs avatar Feb 05 '22 00:02 challs

@MattWestb Do we still need this PR and this one: https://github.com/home-assistant/core/pull/59289 What needs to be done to move these forward?

dmulcahey avatar Mar 14 '22 18:03 dmulcahey

@jacekk015 and i and many user have getting all TRV working with this quirk but we dont have the knowledge writing the tests for getting it merged.

@challs have the knowledge but i cant do the coding :-((

I dont knowing if @jacekk015 still is interested writing the tests if hi is getting help or if some one other can helping doing that.

I think its some more devices that can being added that have showing up later and is using local quirks but i can catching them from the tuya TRV matrix.

I think we need adding some more of this in clima.py in ZHA for getting all function but its one smal thing.

MattWestb avatar Mar 14 '22 18:03 MattWestb

so the linked PR in HA: can you rebase it to remove the conflicts? then I'll take a look at both to see what we can do?

dmulcahey avatar Mar 14 '22 19:03 dmulcahey

I is doing it but not sure i have time today (its later in Europe) but remember my is i forgetting it.

Its one half braking warning on all devices that is already have one quirk then the function is changed and using presets and getting more functions in ZHA GUI..

MattWestb avatar Mar 14 '22 19:03 MattWestb

https://github.com/home-assistant/core/pull/59289 is now OK and its not one braking thing then the devices is not having one quirk in ZHA. It can being merged without problems also if the quirks is not merged.

But Siterwell also need presets but and that is braking for users that is using there devices with on/off mode (then presets was not implanted) that is wrong method (then the TRV is never off). I have doing one old PR for it but its closed with the no go for the first update that is closed and the presets PR 2.

I doing one new PR for Stairwell presets tomorrow but that shall being merged with this quirk for making all working OK with presets.

Thanks for helping David !!

Ed: Stairwell presets presets PR in ZHA is made and must being merged with this PR for not loosing control function of the TRV.

MattWestb avatar Mar 14 '22 19:03 MattWestb

I can write write test for ts0601_trv_beca.py, because I own it. But I think that will be easier to break this mega PR into smallers ones. For every TRV one PR.

rforro avatar Mar 27 '22 17:03 rforro

I can write write test for ts0601_trv_beca.py, because I own it. But I think that will be easier to break this mega PR into smallers ones. For every TRV one PR.

Yes I agree with you on that and I had already thought about doing the same for the SEA802 which I own.

challs avatar Mar 27 '22 17:03 challs

Also possible copy the Beca code and doing the test code for it in one new PR then currently the devices its using one "Moes" code that is not optimal for the device.

I think present in ZHA is already added for it.

The current test code is using Moes and Siterwell so also need updating the old test for this classes.

The PR is splitting all device families so shall being easy only doing new PR and deleting the code in the old quirk.

I can producing the nervelessly debut logging for Siterwell test code but i cant doing coding.

MattWestb avatar Mar 27 '22 17:03 MattWestb

Also possible copy the Beca code and doing the test code for it in one new PR then currently the devices its using one "Moes" code that is not optimal for the device.

Yes, that would be possible.

The current test code is using Moes and Siterwell so also need updating the old test for this classes.

Yes, there seems to be a problem with the new implementation (see my earlier comment). temp_calibration is not initialised and can cause an error.

The PR is splitting all device families so shall being easy only doing new PR and deleting the code in the old quirk.

I don't think this is a good idea, because it makes a very big PR. If you have a separate PR for a single device class, it is easier to review, and any fixes for common problems only need to be discussed and implemented for one device to start with. Once the first device PR is ready and can be merged, the next PRs will need less work to be merged.

I can producing the nervelessly debut logging for Siterwell test code but i cant doing coding.

Okay, if you have time to collect some logs for the Siterwell device that would be great, and I can spend the time I have looking at how to write the tests.

Maybe you could collect about 5 messages? That would be enough for me to start writing the tests for now, and you can collect further logs later, while I am working on the first tests.

challs avatar Mar 27 '22 21:03 challs

@challs thanks for the offers !! The Siterwell is having most command in the original test implementation with the "Moes" but it can being somthing is missing and its not having local calibration its only made in ZHA and the device is running in its own temperature and we is correcting it in ZHA then its not having it in the firmware. I must taking some time and moving one to my RCP test HA and doing some changes of mode and switches and posing it but it can taking some time before i can doing it.

For the MOES we need some user that can making the same and the most important is the temp_calibration so its being possible maning one working test for the device sooner or later.

MattWestb avatar Mar 28 '22 15:03 MattWestb

One fast mode switching and un/setting the functions of the TRV (I is not having the presets in my new test setup but have doing all mode changed local and all other with ZHA) Sitew.txt

Not logged is open window and valve jammed alarms then need more work for triggering them (deep freezer and unrealistic setpoint.temp).

MattWestb avatar Mar 28 '22 17:03 MattWestb

Just to let you know, none of this code will be working in 2022.04 The never version of zigpy rejects manufacturer_attributes and because we have to rewrite all four quirks, we should split them in to four separate PRs.

BTW I'm currently working on exposing AnalogInput clusters, just give me few days. https://github.com/home-assistant/core/pull/69063

rforro avatar Apr 01 '22 11:04 rforro

Thais sad but nothing we can doing more then keep trying getting the code fixed and the tests. If getting one device class fixed and merges is better then zero and in the end i hope we can getting all merged but as sad one by one so being easier working with.

Thanks for info and you work @rforro !!!

MattWestb avatar Apr 01 '22 11:04 MattWestb

Sitew.txt

Thanks @MattWestb - that's very helpful. I'll start working on a new branch for just that device for now. I'll let you know if I have any more questions about the logs.

challs avatar Apr 03 '22 10:04 challs

Sevus @challs and thanks for you work !! Only saying if you like more info or testing being made and i doing it SAP if not being at work.

MattWestb avatar Apr 03 '22 10:04 MattWestb

Updated Maxsmart/Lidl quirk to work with HA 2022.4 https://github.com/zigpy/zha-device-handlers/issues/1061#issuecomment-1090855072

jacekk015 avatar Apr 06 '22 22:04 jacekk015

@MattWestb Your Siterwell quirk updated also - should work: ts0601_trv_siterwell.py.zip

jacekk015 avatar Apr 06 '22 22:04 jacekk015

Great thanks @jacekk015 then i can upgrading my production system 2 (i have done 2 test setups with RCP coordinator for testing "IKEA remote problems") and need doing the last ones SAP but now in knowing the TRV problem is fixed !!!.

MattWestb avatar Apr 07 '22 03:04 MattWestb

2 HA test setups with RCP EZSP coordinator (Radio Co Processor firmware for Zigbee and Thread) and Open Thread Boarder routers is updated yesterday evening and working OK.

For dinner this evening 2 HA test system with normal EZSP coordinator updated and also one test TRV (without presets) is up and running OK.

Production network updated and all TRV looks working OK (presets is not updated) !!

One more great thanks Jakke !!!!!

MattWestb avatar Apr 07 '22 18:04 MattWestb

Hello @jacekk015 I updated to the latest qirk after my home assistant configuration went into errors. What I realized now, but I think it was already an issue before, was that the time set on my thermostat is not kept. Two days ago I went through the flat and updated date and time on all thermostates. One day later I was wondering, why my thermostat in the bathroom created a temperature of 23°C in the middle of the day. Today I went again through all rooms and checked the time. The date is still fine. I did this on 16.04.2022 at 12:24 german local time.

  • 03:53 Bathroom
  • 11:24 Bedroom
  • 11:24 Living room
  • 11:24 Office

Strange in this case: All rooms except the bathroom have one hour difference, where I guess that this might be an issue between winter / summer time. The bathroom is totally wrong and was set to 03:53.

EDIT: One hour difference was caused by a wrongly set timezone. The bathroom thermostat still has the same behaviour.

Ja-Ju avatar Apr 16 '22 10:04 Ja-Ju

Is this still needed?

dmulcahey avatar Aug 30 '22 12:08 dmulcahey