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

Maxsmart, Beca, Siterwell and Moes TRV support/update #1192

Closed
wants to merge 15 commits into from
Closed

Maxsmart, Beca, Siterwell and Moes TRV support/update #1192

wants to merge 15 commits into from

Conversation

jacekk015
Copy link
Contributor

According to matrix
zigpy/zigpy#653
new TRVs has support for ZHA and some got update of functionality.

Siterwell tested locally by @MattWestb
#1084 (reply in thread)

Beca tested locally by @iedex and @Youpimatin
#1123 (comment)

Maxsmart written and tested locally by me - I own 12pcs of them
Maxsmart -> Silvercrest Tuya Lidl clone tested by @JoostvdB94 @lukasf98 @baldisos
#1061 (comment)

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
#1129

@dmulcahey
Copy link
Collaborator

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.

@MattWestb
Copy link
Contributor

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.

@jacekk015
Copy link
Contributor Author

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.

@chaptergy
Copy link

I have tested the Maxsmart implementation with _TZE200_thbr5z34 (#978) and it is working.

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

@jacekk015
Copy link
Contributor Author

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.

@chaptergy
Copy link

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

@MattWestb
Copy link
Contributor

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.

@jacekk015
Copy link
Contributor Author

@chaptergy Use a local quirk from this post.
#1061 (comment)
This quirk probably won't be published because it doesn't pass the tests.
Waste of time waiting for that. It's almost winter.

@challs
Copy link
Contributor

challs commented Feb 5, 2022

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?

@dmulcahey
Copy link
Collaborator

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

@MattWestb
Copy link
Contributor

@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
Copy link
Contributor

MattWestb commented Mar 14, 2022

home-assistant/core#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.

@rforro
Copy link
Contributor

rforro commented Mar 27, 2022

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.

@challs
Copy link
Contributor

challs commented Mar 27, 2022

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.

@MattWestb
Copy link
Contributor

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.

@challs
Copy link
Contributor

challs commented Mar 27, 2022

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.

@MattWestb
Copy link
Contributor

@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
Copy link
Contributor

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).

@rforro
Copy link
Contributor

rforro commented Apr 1, 2022

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. home-assistant/core#69063

@MattWestb
Copy link
Contributor

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 !!!

@challs
Copy link
Contributor

challs commented Apr 3, 2022

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.

@MattWestb
Copy link
Contributor

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.

@jacekk015
Copy link
Contributor Author

Updated Maxsmart/Lidl quirk to work with HA 2022.4
#1061 (comment)

@jacekk015
Copy link
Contributor Author

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

@MattWestb
Copy link
Contributor

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
Copy link
Contributor

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 !!!!!

@Ja-Ju
Copy link

Ja-Ju commented Apr 16, 2022

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.

@dmulcahey
Copy link
Collaborator

Is this still needed?

@Ja-Ju
Copy link

Ja-Ju commented Aug 30, 2022

Is this still needed?

Hello @dmulcahey!
From my point of few: yes, definitely!
I am using this quirk to be able to control my devices from lidl. It does not work without this quirk.
It would be great if it can be merged into the main repository.

@MattWestb
Copy link
Contributor

Nearly all tuya TRVs have very basic functionality and not working in ZHA GUI and we have spending weeks of testing and coding so the user can getting there devices working OK in ZHA.

Still If no one can doing it better this PR is much better then no or very bad support for tuya TRVs and more user is getting them and moving to other system (normaly not to de(F)CON) then is not getting any functionality in ZHA GUI.

Also pleas say what is wrong and trying getting it fixed instead letting it dying jacekk015] on Nov 29, 2021 and in some week the PR is needed then its being cold in the north.

@dcoder42
Copy link

dcoder42 commented Oct 3, 2022

I have updated moes quirk based on the changes from the Beca PR #1459 to work with the current code base. Just in case someone needs it...

ts0601_trv_moes.zip

@dmulcahey
Copy link
Collaborator

@jacekk015 any chance we can get this cleaned up and merged if it is still necessary?

@TheJulianJES TheJulianJES added the Tuya Request/PR regarding a Tuya device label Jan 16, 2023
@jacekk015 jacekk015 closed this by deleting the head repository Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tuya Request/PR regarding a Tuya device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants