Disable schedule on Tuya/Saswell TRV (TS601/_TZE200_c88teujp) #1816
Conversation
Codecov ReportBase: 80.00% // Head: 80.18% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## dev #1816 +/- ##
==========================================
+ Coverage 80.00% 80.18% +0.18%
==========================================
Files 239 240 +1
Lines 7430 7428 -2
==========================================
+ Hits 5944 5956 +12
+ Misses 1486 1472 -14
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Pull Request Test Coverage Report for Build 3255727846
💛 - Coveralls |
blazewicz
force-pushed
the
tuya-trv-schedule-fix
branch
from
acfc013 to
caaf4a9
Compare
blazewicz
force-pushed
the
tuya-trv-schedule-fix
branch
from
caaf4a9 to
9ed1c12
Compare
zhaquirks/tuya/ts0601_trv_sas.py
Outdated
| if "system_mode" in attributes: | ||
|
|
||
| async def _disable_schedule(): | ||
| await asyncio.sleep(3) |
There was a problem hiding this comment.
I think that Z2M adds a delay because there is 2 writes. Have you tried without the sleep?.
There was a problem hiding this comment.
I think they added it to work-around some race condition in device firmware. I've tried to reproduce this condition but I've failed to. For now I'll remove this delay.
zhaquirks/tuya/ts0601_trv_sas.py
Outdated
| if "system_mode" in attributes: | ||
|
|
||
| async def _disable_schedule(): | ||
| await asyncio.sleep(3) | ||
| await self.write_attributes( | ||
| {"schedule_enabled": 0}, manufacturer=manufacturer | ||
| ) | ||
|
|
||
| asyncio.get_running_loop().create_task(_disable_schedule()) |
There was a problem hiding this comment.
Maybe you can do the same with:
if "system_mode" in attributes:
await self.write_attributes({"schedule_enabled": 0}, manufacturer=manufacturer)
return await super().write_attributes(attributes, manufacturer)(Despite I don't know the relationship between the system_mode and schedule_enabled attributes) 🤷🏻♂️
There was a problem hiding this comment.
Changed to send state and schedule mode simultaneously.
zhaquirks/tuya/ts0601_trv_sas.py
Outdated
| def schedule_state_reported(self, value): | ||
| """Handle reported schedule state.""" | ||
| if value == 1: | ||
| _LOGGER.debug("reported schedule state: enabled") | ||
| else: | ||
| _LOGGER.debug("reported schedule state: disabled") |
There was a problem hiding this comment.
The implementation is updating the schedule_enabled attribute when the system_mode is updated. Why don't just let the user write the attribute here?
def schedule_state_reported(self, value):
"""Handle reported schedule state."""
self._update_attribute(self.attributes_by_name["schedule_enabled"].id, value)There was a problem hiding this comment.
This quirk does not implement schedule programming therefore IMO it should automatically force schedule mode to be off, without involving the user.
There was a problem hiding this comment.
It's fine to me.
Just to say that the SASSWEL_SCHEDULE_ENABLE_ATTR: 0 is performed here:
def map_attribute(self, attribute, value):
"""Map standardized attribute value to dict of manufacturer values."""
if attribute == "occupied_heating_setpoint":
# centidegree to decidegree
return {SASSWEL_HEATING_SETPOINT_ATTR: round(value / 10)}
if attribute == "system_mode":
# this quirk does not support programmig modes so we force the schedule mode to be always off
# more details: https://github.com/zigpy/zha-device-handlers/issues/1815
if value == self.SystemMode.Off:
return {SASSWEL_STATE_ATTR: 0, SASSWEL_SCHEDULE_ENABLE_ATTR: 0}
if value == self.SystemMode.Heat:
return {SASSWEL_STATE_ATTR: 1, SASSWEL_SCHEDULE_ENABLE_ATTR: 0}and takes place every time that the system_mode is written to the Off or Heat values. I don't know if there can be other values for the system_mode.
There was a problem hiding this comment.
This device has only Off and Heat modes. Disabling the builtin schedule on each state change assures that it always stays off.
blazewicz
force-pushed
the
tuya-trv-schedule-fix
branch
2 times, most recently
from
66981bf to
8834d52
Compare
blazewicz
force-pushed
the
tuya-trv-schedule-fix
branch
from
8834d52 to
77e45ba
Compare
|
For future references, these are the relevant lines: |
There was a problem hiding this comment.
Just to be sure.
Are that change indented to be applied to all the devices in that quirk?
That will be all these devices:
("_TZE200_c88teujp", "TS0601"),
("_TZE200_azqp6ssj", "TS0601"),
("_TZE200_yw7cahqs", "TS0601"),
("_TZE200_9gvruqf5", "TS0601"),
("_TZE200_zuhszj9s", "TS0601"),
("_TZE200_2ekuz3dz", "TS0601"),
("_TYST11_KGbxAXL2", "GbxAXL2"),
("_TYST11_c88teujp", "88teujp"),
("_TYST11_azqp6ssj", "zqp6ssj"),
("_TYST11_yw7cahqs", "w7cahqs"),
("_TYST11_9gvruqf5", "gvruqf5"),
("_TYST11_zuhszj9s", "uhszj9s"),
Fair point, I've assumed these were all clones. All except these three share the same configuration in Z2M:
Here's the list: The I couldn't find any information regarding The I have no way of testing them all of course. |
|
Here's the configuration for |
|
But... according to Z2M, your device also have some kind of scheduler: and: toZigbee: [tz.saswell_thermostat_current_heating_setpoint, ... , tz.tuya_thermostat_weekly_schedule], |
|
You have said that the scheduled was setted in the Tuya application, right? But as the quirk can not manage the scheduled for the device you need to disable it always to prevent that undesired behavior when HA interacts with the device. If HA could manage the scheduler (in a good manner) that quirk will not be needed. Would that be more or less the purpose of the quirk? |
|
How to i disable schedule??? |
Yes, that is correct. Whenever someone finds time and will to implement the full support for the schedules this quirk may be upgraded. Until then my patch makes it usable in the basic form. |
|
Mvh.Jørgen HansenDen 19. okt. 2022 kl. 16.43 skrev Krzysztof Błażewicz ***@***.***>:
You have said that the scheduled was setted in the Tuya application, right?
But as the quirk can not manage the scheduled for the device you need to disable it always to prevent that undesired behavior when HA interacts with the device.
If HA could manage the scheduler (in a good manner) that quirk will not be needed.
Would that be more or less the purpose of the quirk?
Yes, that is correct.
Whenever someone finds time and will to implement the full support for the schedules this quirk may be upgraded. Until then my patch makes it usable in the basic form.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>
|
| LOCAL_TEMP_COMMAND_ID: ("local_temperature", t.uint32_t, True), | ||
| BATTERY_STATE_COMMAND_ID: ("battery_state", t.uint8_t, True), | ||
| SASSWEL_STATE_ATTR: ("state", State, True), | ||
| SASSWEL_HEATING_SETPOINT_ATTR: ("heating_setpoint", t.uint32_t, True), |
There was a problem hiding this comment.
Is that correct?
In TEMPERATURE_ATTRS is occupied_heating_setpoint, also in line 129 (if attribute == "occupied_heating_setpoint":)
There was a problem hiding this comment.
I see how that's tricky, let me try to explain.
ZCL Thermostat class uses occupied_heating_setpoint this is the attribute name which zigpy sends to the quirk when user requests setpoint change. Unit used by ZCL is centidegree Celsius (22,4 °C = 2240)
Tuya thermostat uses different set of attributes and the name used by Tuya developers is simply "heating setpoint", I've used different name to avoid confusion between ZCL and Tuya attributes which use different units. Unit used by Tuya is decidegree Celsius (22,4 °C = 224).
When zigpy updates the value for ZCL Thermostat, (i.e. when user changes setpoint in HA dashboard) it calls ThermostatCluster.map_attribute() with attribute = "occupied_heating_setpoint". This method converts the value from centidegree to decidegree and translates ZCL's occupied_heating_setpoint to Tuya's SASSWEL_HEATING_SETPOINT_ATTR.
On the other hand device reports two temperature values with attrids: SASSWEL_HEATING_SETPOINT_ATTR and SASSWEL_LOCAL_TEMP_ATTR. First is the current heating setpoint and the second is the local temperature as measured by the temperature sensor on the device. Both reported values are handled by the same event listener implemented in TuyaThermostatCluster.temperature_change(). To make code simpler both reports are handled by lines 78:83 and Tuya->ZCL attribute name translation is handled by the TEMPERATURE_ATTRS mapping, both values require the same *10 conversion from Tuya to ZCL standard.
| if attribute == "occupied_heating_setpoint": | ||
| # centidegree to decidegree | ||
| return {OCCUPIED_HEATING_SETPOINT_COMMAND_ID: round(value / 10)} | ||
| return {SASSWEL_HEATING_SETPOINT_ATTR: round(value / 10)} |
There was a problem hiding this comment.
I feel like there is something wrong here.
At first sign there is an asymetric behavior with the temp attributes (local_temperature and occupied_heating_setpoint).
The _update_attribute is updating both with value * 10.
And then here in map_attribute is only updating the occupied_heating_setpoint (value/10)`.
Are you able to check if this part is fine?
There was a problem hiding this comment.
There is no need to handle local temperature in map_attribute because this value is a read only property of the device. Only the heating setpoint can be changed by the user (HA dashboard) and only this value requires translation from ZCL to Tuya format.
|
Would be really great if this could be merged soon. We just hit 0°C and my thermostats are not turning on because of this. |
|
I feel you buddy, winter has come to Poland as well. I hope you'll be able to enjoy this patch soon, I've been using it ever since I've published this PR and it works fine. |
There was a problem hiding this comment.
If it has been working for you, I have no objections to the change.
The explanations are convincing, but I think the change may have enough impact that I can't assess by myself.
Simply indicate that if the change presents serious problems for users, it is likely that I will ask you for support to fix it or to go back.
javicalle
added
the
needs final review
|
@blazewicz one question, the disable schedule is a 'one time' action, or is needed to call every time? I was thinking that we can update the attribute in the device binding? That will be enough? |
|
@javicalle I understand your concerns and of course I'll try to help if any issue arise from my changes.
Thanks for the suggestion, It might be similar to the issue handled in async def bind(self):
"""
Bind cluster.
When adding this device tuya gateway issues factory reset,
we just need to reset the frost lock, because its state is unknown to us.
"""
result = await super().bind()
await self.write_attributes({self.attributes_by_name["frost_lock_reset"].id: 0})
return resultI've moved schedule disable command to diff --git a/zhaquirks/tuya/ts0601_trv_sas.py b/zhaquirks/tuya/ts0601_trv_sas.py
index a5304a2..4cd5249 100644
--- a/zhaquirks/tuya/ts0601_trv_sas.py
+++ b/zhaquirks/tuya/ts0601_trv_sas.py
@@ -73,6 +73,15 @@ class ManufacturerThermostatCluster(TuyaManufClusterAttributes):
SASSWEL_LOCAL_TEMP_ATTR: "local_temperature",
}
+ async def bind(self):
+ """Bind cluster."""
+ # this quirk does not support programmig modes so we force the schedule mode to be always off
+ # more details: https://github.com/zigpy/zha-device-handlers/issues/1815
+ result = await super().bind()
+ _LOGGER.warning("ManufacturerThermostatCluster.bind()")
+ # await self.write_attributes({SASSWEL_SCHEDULE_ENABLE_ATTR: 0})
+ return result
+
def _update_attribute(self, attrid, value):
super()._update_attribute(attrid, value)
if attrid in self.TEMPERATURE_ATTRS:
@@ -131,12 +140,10 @@ class ThermostatCluster(TuyaThermostatCluster):
return {SASSWEL_HEATING_SETPOINT_ATTR: round(value / 10)}
if attribute == "system_mode":
- # this quirk does not support programmig modes so we force the schedule mode to be always off
- # more details: https://github.com/zigpy/zha-device-handlers/issues/1815
if value == self.SystemMode.Off:
- return {SASSWEL_STATE_ATTR: 0, SASSWEL_SCHEDULE_ENABLE_ATTR: 0}
+ return {SASSWEL_STATE_ATTR: 0}
if value == self.SystemMode.Heat:
- return {SASSWEL_STATE_ATTR: 1, SASSWEL_SCHEDULE_ENABLE_ATTR: 0}
+ return {SASSWEL_STATE_ATTR: 1}
class Thermostat_TYST11_c88teujp(TuyaThermostat):Here's my testing procedure:
I've also added extra debug logs to confirm that request is sent to the device and proper response is received - it is. As strange as it looks, all devices properly report schedule state to be off, but the schedule is still active. Temperature setpoint eventually changes to 16 °C and a clock icon is displayed on the LED display, presumably indicating that current value is set by the schedule.
I've tried adding a 3 second delay between call to diff --git a/zhaquirks/tuya/ts0601_trv_sas.py b/zhaquirks/tuya/ts0601_trv_sas.py
index 4cd5249..179ad1c 100644
--- a/zhaquirks/tuya/ts0601_trv_sas.py
+++ b/zhaquirks/tuya/ts0601_trv_sas.py
@@ -1,5 +1,6 @@
"""Saswell (Tuya whitelabel) 88teujp thermostat valve quirk."""
+import asyncio
import logging
from zigpy.profiles import zha
@@ -77,9 +78,16 @@ class ManufacturerThermostatCluster(TuyaManufClusterAttributes):
"""Bind cluster."""
# this quirk does not support programmig modes so we force the schedule mode to be always off
# more details: https://github.com/zigpy/zha-device-handlers/issues/1815
+ _LOGGER.warning("ManufacturerThermostatCluster.bind(): starting bind procedure")
result = await super().bind()
- _LOGGER.warning("ManufacturerThermostatCluster.bind()")
- # await self.write_attributes({SASSWEL_SCHEDULE_ENABLE_ATTR: 0})
+
+ async def disable_schedule():
+ await asyncio.sleep(3)
+ _LOGGER.warning("ManufacturerThermostatCluster.bind(): requesting schedule disable")
+ await self.write_attributes({SASSWEL_SCHEDULE_ENABLE_ATTR: 0})
+
+ asyncio.create_task(disable_schedule())
+
return result
def _update_attribute(self, attrid, value):
@@ -100,6 +108,8 @@ class ManufacturerThermostatCluster(TuyaManufClusterAttributes):
self.endpoint.device.battery_bus.listener_event(
"battery_change", 0 if value == 1 else 100
)
+ elif attrid == SASSWEL_SCHEDULE_ENABLE_ATTR:
+ _LOGGER.warning("Reported schedule state change: %s", value)
class ThermostatCluster(TuyaThermostatCluster):Then I've rolled back to the version from this PR, restarted HA (reconfigure is not required) and confirmed schedule is disabled on all TRV's.
|
|
Thanks for the work here! |
|
Thanks for testing. |
|
Thanks from me as well. Can't wait! :) |
Unfortunately 2ekuz3dz is not working at all using this quirk. Not a single entity is available. |
@Hilpas thats probably because of missing output clusters, like I said, this one should have a separate quirk on its own since it's not a TRV. Could you provide a list of supported clusters for this device? |
This might help, otherwise i could try to provide some Information later. |
|
But #1721 was opened on August 30-th, almost 3 months before this PR was merged in. |
|
My device signaturen is exactly the same though. |
This work-around is intended to fix #1815. Its based on solution used in Zigbee2MQTT linked below:
https://github.com/Koenkk/zigbee-herdsman-converters/blob/master/converters/toZigbee.js#L5877:L5879
I've tested it locally with 6 devices and it fixed the issue.