zigpy / zha-device-handlers

ZHA device handlers bridge the functionality gap created when manufacturers deviate from the ZCL specification, handling deviations and exceptions by parsing custom messages to and from Zigbee devices.
Apache License 2.0
741 stars 675 forks source link

[BUG] Danfoss Ally valve: Slow actuator response [solved already in other solutions] #1005

Closed WolfRevo closed 2 years ago

WolfRevo commented 3 years ago

Describe the bug The problem is well described here: https://community.home-assistant.io/t/danfoss-ally-valve-slow-actuator-response-solved/254939

So if you issue a new temperature target setpoint for the thermostat using HA+ZHA the thermostat uses the internal PID mechanism to control the valve which results in slow (and sometimes unexpected) valve control movements.

# To Reproduce Steps to reproduce the behavior:

  1. Go to your Danfos Ally thermostat connected using ZHA
  2. Change the desired temperature
  3. Watch the pi_heating_demand changing over time (far to slow)

Expected behavior The pi_heating_demand should change much faster

Screenshots grafik

Additional context The issue is already fixed in deconz (https://github.com/dresden-elektronik/deconz-rest-plugin/pull/4408) as well as in the Koenkk/zigbee-herdsman-converters (https://github.com/Koenkk/zigbee-herdsman-converters/pull/2592)

So mainly the solution is well described in the official document (https://assets.danfoss.com/documents/176987/AM375549618098en-000101.pdf):

grafik

The zigbee coordinator needs to send a Setpoint Command including the HeatingSetpoint itself aswell as a bitmask (enum8 set to 1) to circumvent the thermostats PID mechism.

Based on that I started to read carefully all instructions of the zha quirks because I wanted to sort out if I can create a custom zhaquirk (based on https://github.com/zigpy/zha-device-handlers/tree/dev/zhaquirks/danfoss) fixing this issue.

While I already identified that I need to add a custom output cluster in the replacement (and I guess for that I also need to add the thermostat output cluster to the signature itself?) I didn't find any good example (first I thought the tuya custom quirks were good to understand but I honestly gave up) on how to understand: 1) how do I implement a custom command in the quirk? 2) (in this specific case) how can I access the occupied_heating_setpoint sent by HA+ZHA to the thermostat (which is based on the solution above an int16) 3) how can I create a custom command like "setpointType (enum8) + HeatingSetpoint (16bit)" where the HeatingSetpoint is occupied_heating_setpoint from 2) and the setPointType is set to 1 (the final command input needs to be masked in hex I guess)?

Also just to recap how it works: ZHA is using the climate.py integration in HA and calls the function

async def async_set_temperature(self, **kwargs):
        """Set new target temperature."""

which eventually calls (in the case of the Danfoss Ally):

            elif self.hvac_mode == HVAC_MODE_HEAT:
                success = await thrm.async_set_heating_setpoint(
                    temp, self.preset_mode == PRESET_AWAY
                )

which is implemented in core/homeassistant/components/zha/core/channels/hvac.py

so there in normal operation the following code should be executed:

        if not await self.write_attributes(data):
            self.debug("couldn't set heating setpoint")
            return False

That's where I got lost to find the appropriate calls into zigpy and how the zha quirks overwrite the zigpy code to be executed (or is this assumption already wrong?).

Any help appreciated. Surely you also can fix this yourself if you think this is faster than to teach me ;) (because I won't have time within the next 3 weeks). If you fix it yourself I can learn from your implementation to be able to handle such requests myself in the future.

WolfRevo commented 3 years ago

Hi,

I tried to understand other custom quirks to be able to adapt them to my issue. Prior to start testing I would really ask you to look at my first Idea of enhancing the existing quirk for Danfoss to see if I am on the right track or completely misunderstood the concept of ZHA custom quirks.

"""Module to handle quirks of the  Fanfoss thermostat.

manufacturer specific attributes to control displaying and specific configuration.
"""

import zigpy.profiles.zha as zha_p
from zigpy.quirks import CustomCluster, CustomDevice
import zigpy.types as t
from zigpy.zcl.clusters.general import (
    Basic,
    Identify,
    Ota,
    PollControl,
    PowerConfiguration,
    Time,
)
from zigpy.zcl.clusters.homeautomation import Diagnostic
from zigpy.zcl.clusters.hvac import Thermostat, UserInterface
from zigpy.zcl import foundation

from zhaquirks.const import (
    DEVICE_TYPE,
    ENDPOINTS,
    INPUT_CLUSTERS,
    MODELS_INFO,
    OUTPUT_CLUSTERS,
    PROFILE_ID,
)
from zhaquirks.danfoss import DANFOSS

COMMAND_SETPOINT_COMMAND = "setpoint_command"
MANUFACTURER = 0x1246
OCCUPIED_HEATING_SETPOINT_ATTR = 0x0012

class DanfossThermostatCluster(CustomCluster, Thermostat):
    """Danfoss custom cluster."""

    manufacturer_server_commands = {
        0x40: ("setpoint_command", (t.enum8, t.int16s), False),
    }

    manufacturer_attributes = {
        0x4000: ("etrv_open_windows_detection", t.enum8),
        0x4003: ("external_open_windows_detected", t.Bool),
        0x4010: ("exercise_day_of_week", t.enum8),
        0x4011: ("exercise_trigger_time", t.uint16_t),
        0x4012: ("mounting_mode_active", t.Bool),
        0x4013: ("mounting_mode_control", t.Bool),
        0x4014: ("orientation", t.Bool),
        0x4015: ("external_measured_room_sensor", t.int16s),
        0x4016: ("radiator_overed", t.Bool),
        0x4020: ("control_algorithm_scale_factor", t.uint8_t),
        0x4030: ("heat_available", t.Bool),
        0x4031: ("heat_supply_request", t.Bool),
        0x4032: ("load_balancing_enable", t.Bool),
        0x404A: ("load_estimate_radiator", t.uint16_t),
        0x404B: ("regulation_setPoint_offset", t.int8s),
        0x404C: ("adaptation_run_control", t.enum8),
        0x404D: ("adaptation_run_status", t.bitmap8),
        0x404E: ("adaptation_run_settings", t.bitmap8),
        0x404F: ("preheat_status", t.Bool),
        0x4050: ("preheat_time", t.uint32_t),
        0x4051: ("window_open_feature_on_off", t.Bool),
        0xFFFD: ("cluster_revision", t.uint16_t),
    }

        async def write_attributes(self, attributes, manufacturer=None):
        """Send SETPOINT_COMMAND after setpoint change"""

            if "occupied_heating_setpoint" in attributes:
                setpoint = foundation.ReadAttributeRecord(
                    OCCUPIED_HEATING_SETPOINT_ATTR, foundation.Status.SUCCESS, foundation.TypeValue()
                )

                cmd_payload = super().Command()
                cmd_payload.status = 0
                cmd_payload.tsn = self.endpoint.device.application.get_sequence()
                cmd_payload.command_id = 0x40
                cmd_payload.function = 0
                cmd_payload.data = [1, setpoint]

                await super().command(
                    COMMAND_SETPOINT_COMMAND,
                    cmd_payload,
                    manufacturer=MANUFACTURER,
                    expect_reply=True,
                    tsn=cmd_payload.tsn,
                )

            return super().write_attributes(attributes, manufacturer)

class DanfossUserInterfaceCluster(CustomCluster, UserInterface):
    """Danfoss custom cluster."""

    manufacturer_attributes = {
        0x4000: ("viewing_direction", t.enum8),
        0xFFFD: ("cluster_revision", t.uint16_t),
    }

class DanfossDiagnosticCluster(CustomCluster, Diagnostic):
    """Danfoss custom cluster."""

    manufacturer_attributes = {
        0x4000: ("sw_error_code", t.bitmap16),
        0x4001: ("wake_time_avg", t.uint32_t),
        0x4002: ("wake_time max duration", t.uint32_t),
        0x4003: ("wake_time min duration", t.uint32_t),
        0x4004: ("sleep_Postponed_count_avg", t.uint32_t),
        0x4005: ("sleep_Postponed_count_max", t.uint32_t),
        0x4006: ("sleep_Postponed_count_min", t.uint32_t),
        0x4010: ("motor_step_counter", t.uint32_t),
        0xFFFD: ("cluster_revision", t.uint16_t),
    }

class DanfossThermostat(CustomDevice):
    """DanfossThermostat custom device."""

    signature = {
        # <SimpleDescriptor endpoint=1 profile=260 device_type=769
        # device_version=0 input_clusters=[0, 1, 3, 10,32, 513, 516, 1026, 2821]
        # output_clusters=[0, 25]>
        MODELS_INFO: [(DANFOSS, "eTRV0100")],
        ENDPOINTS: {
            1: {
                PROFILE_ID: zha_p.PROFILE_ID,
                DEVICE_TYPE: zha_p.DeviceType.THERMOSTAT,
                INPUT_CLUSTERS: [
                    Basic.cluster_id,
                    PowerConfiguration.cluster_id,
                    Identify.cluster_id,
                    Time.cluster_id,
                    PollControl.cluster_id,
                    Thermostat.cluster_id,
                    UserInterface.cluster_id,
                    Diagnostic.cluster_id,
                ],
                OUTPUT_CLUSTERS: [Basic.cluster_id, Ota.cluster_id],
            }
        },
    }

    replacement = {
        ENDPOINTS: {
            1: {
                INPUT_CLUSTERS: [
                    Basic,
                    PowerConfiguration,
                    Identify,
                    Time,
                    PollControl,
                    DanfossThermostatCluster,
                    DanfossUserInterfaceCluster,
                    DanfossDiagnosticCluster,
                ],
                OUTPUT_CLUSTERS: [Basic, Ota],
            }
        }
    }

In short I added some imports and three constants at the beginning to work with. In the class DanfossThermostatCluster I added the new command to manufacturer_server_commands (hopefully correctly interpreted) and last but not least I replaced the write_attributes function to issue the custom command every time the occupied_heating_setpoint for an thermostat is updated. I struggle a bit as I don't know how the corresponding thermostat should be adressed for which write_attributes is called.

So could someone please tell me if this is completely rubbish or if this should be the way to go?

Thanks in advance.

Adminiuga commented 3 years ago

Do you have your changes on GitHub?

 I struggle a bit as I don't know how the corresponding thermostat should be adressed for which write_attributes is called.

What do you mean by addressed? Is this setpoint command sent through the manufacturer specific cluster? In this case you can send the commands or access other cluster through the endpoint. E.g.

other_cluster = self.endpoint.in_clusters[0xff01]
await other_cluster.command(...

If you know the cluster's ep_attribute, then you can retrieve that cluster by that name e.g.

on_off_cluster = self.endpoint.on_off
WolfRevo commented 3 years ago

Do you have your changes on GitHub?

Freshly forked your repo.

I struggle a bit as I don't know how the corresponding thermostat should be adressed for which write_attributes is called.

What do you mean by addressed?

Maybe a misunderstanding in my view. The command needs to be send to the thermostat where the occupied heating setpoint is changed.

Is this setpoint command sent through the manufacturer specific cluster? In this case you can send the commands or access other cluster through the endpoint. E.g.

other_cluster = self.endpoint.in_clusters[0xff01]
await other_cluster.command(...

If you know the cluster's ep_attribute, then you can retrieve that cluster by that name e.g.

on_off_cluster = self.endpoint.on_off

Based on the official documentation the setpoint command is in the 0x0201 Thermostat Cluster:

grafik grafik

Adminiuga commented 3 years ago

seems like you already defined the command correctly. Do they tell you what behavior is going to be if you do both the "write_attributes" and "setpoint" command? In other words, do you need to filter out the setpoint attribute write?

Try the following instead, which would do both attr write and setpoint command:

    async def write_attributes(self, attributes, manufacturer=None):
    """Send SETPOINT_COMMAND after setpoint change"""

        write_res = await super().write_attributes(attributes, manufacturer=manufacturer)

        if "occupied_heating_setpoint" in attributes:
            await self.setpoint_command(0x01, attributes["occupied_heating_setpoint"], manufacturer=manufacturer)

        return write_res

it would do the write attributes and then the setpoint command, it the heating setpoint was in the attributes

WolfRevo commented 3 years ago

Hey @Adminiuga I have to admit that I really thought it is much more complex to create a manufacturer custom command but after reading your short explanation again and again I realized that the framework is really that dynamically created to just take the definition of a custom command to create the corresponding internal data structures and so on (is my understanding right?). This helped a lot to understand how the whole zha quirks integration is working.

I made the proposed change as your suggestion looks legit. I am now at the point to give it a hot try at my home. For that I took over the thermostat.py file to my local HA installation.

The custom quirk is loaded correctly:

2021-09-27 11:42:38 DEBUG (MainThread) [zhaquirks] Loading custom quirks from /config/zha_custom_quirks
2021-09-27 11:42:38 DEBUG (MainThread) [zhaquirks] Loading custom quirks module danfoss
2021-09-27 11:42:38 DEBUG (MainThread) [zhaquirks] Loading custom quirks module danfoss.thermostat

Unfortunately when I change the temperature of the thermostat I see the write_attributes but not the setpoint command:

2021-09-27 11:52:14 DEBUG (MainThread) [homeassistant.components.zha.core.channels.base] [0x6045:1:0x0201]: Attribute report 'DanfossThermostatCluster'[occupied_heating_setpoint] = 2500
2021-09-27 11:52:14 DEBUG (MainThread) [homeassistant.components.zha.core.channels.base] [0x6045:1:0x0201]: Attribute report 'DanfossThermostatCluster'[setpoint_change_source] = SetpointChangeSource.External
2021-09-27 11:52:15 DEBUG (MainThread) [homeassistant.components.zha.core.channels.base] [0x6045:1:0x0201]: wrote {'occupied_heating_setpoint': 2500} attrs, Status: [[WriteAttributesStatusRecord(status=<Status.SUCCESS: 0>)]]
2021-09-27 11:52:15 DEBUG (MainThread) [homeassistant.components.zha.core.channels.base] [0x6045:1:0x0201]: set heating setpoint to 2500
2021-09-27 11:52:15 DEBUG (MainThread) [homeassistant.components.zha.core.channels.base] [0x6045:1:0x0201]: Attribute report 'DanfossThermostatCluster'[setpoint_change_source] = SetpointChangeSource.External

Or do I need further debug code to see if the

        if "occupied_heating_setpoint" in attributes:
            await self.setpoint_command(0x01, attributes["occupied_heating_setpoint"], manufacturer=manufacturer)

is executed?

Anyway thanks for all your help and insights 😊 I will create a pull request as soon I have tested it sucessfully.

Adminiuga commented 3 years ago

Hrm, you won't see specific call outs that the command was called. Enable logging for quirks and add a debug logging before the command. E.g. self.debug("sending setpoint command: %s", attributes ["occupied_heating_setpoint"])

You could see the command calling in radio lib debug, but that is a tad more complicated to parse

WolfRevo commented 3 years ago

Thanks again. debug logging is enabled this way:

logger:
  default: info
  logs:
    homeassistant.components.zha.core.channels.base: debug
    zhaquirks: debug
    zhaquirks.danfoss: debug
    custom_components: debug

Looks like I am missing the corresponding logger call as there is no quirks output in the logfile. I also added another self.debug call into write_attributes to see if the debug call is logged but it seems I have a wrong logger configuration?

Adminiuga commented 3 years ago

Add zigpy: debug ? Add another self.debug before the if jus to make sure you are hitting your quirk

WolfRevo commented 3 years ago

Got it ... my debug config was to specific. Needed to change zigpy.zcl back to debug 😉 (disabled it as this is really noisy and homeassistant.components.zha mostly tells enough).

2021-09-27 14:13:30 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=GLOBAL_COMMAND manufacturer_specific=False is_reply=True disable_default_response=False> manufacturer=None tsn=155 command_id=Command.Write_Attributes_rsp>
2021-09-27 14:13:30 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] sending setpoint command: 2800
2021-09-27 14:13:31 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=GLOBAL_COMMAND manufacturer_specific=True is_reply=True disable_default_response=False> manufacturer=4678 tsn=157 command_id=Command.Default_Response>
2021-09-27 14:13:31 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=GLOBAL_COMMAND manufacturer_specific=False is_reply=True disable_default_response=False> manufacturer=None tsn=124 command_id=Command.Report_Attributes>
2021-09-27 14:13:31 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL request 0x000a: [[Attribute(attrid=48, value=<TypeValue type=enum8, value=enum8.undefined_0x02>)]]
2021-09-27 14:13:31 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] Attribute report received: setpoint_change_source=2
2021-09-27 14:13:31 DEBUG (MainThread) [homeassistant.components.zha.entity] climate.thermostat_dj_zimmer_danfoss_thermostat: Attribute 'setpoint_change_source' = SetpointChangeSource.External update
2021-09-27 14:13:33 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=GLOBAL_COMMAND manufacturer_specific=False is_reply=True disable_default_response=False> manufacturer=None tsn=123 command_id=Command.Report_Attributes>
2021-09-27 14:13:33 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL request 0x000a: [[Attribute(attrid=48, value=<TypeValue type=enum8, value=enum8.undefined_0x02>)]]
2021-09-27 14:13:33 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] Attribute report received: setpoint_change_source=2
2021-09-27 14:13:33 DEBUG (MainThread) [homeassistant.components.zha.entity] climate.thermostat_dj_zimmer_danfoss_thermostat: Attribute 'setpoint_change_source' = SetpointChangeSource.External update
2021-09-27 14:13:37 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=GLOBAL_COMMAND manufacturer_specific=False is_reply=True disable_default_response=False> manufacturer=None tsn=125 command_id=Command.Report_Attributes>
2021-09-27 14:13:37 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL request 0x000a: [[Attribute(attrid=18, value=<TypeValue type=int16s, value=2800>)]]
2021-09-27 14:13:37 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] Attribute report received: occupied_heating_setpoint=2800
2021-09-27 14:13:37 DEBUG (MainThread) [homeassistant.components.zha.entity] climate.thermostat_dj_zimmer_danfoss_thermostat: Attribute 'occupied_heating_setpoint' = 2800 update

The command itself looks good (manufacturer_specific=True and manufacturer=4678 set correctly). The only thing I see is the two times Attribute report received: setpoint_change_source=2 which looks like a command is issued twice? If not I will take over the code and create the pull request.

Adminiuga commented 3 years ago

The command was issued once, and you've got a default response for it:

2021-09-27 14:13:30 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] sending setpoint command: 2800
2021-09-27 14:13:31 DEBUG (MainThread) [zigpy.zcl] [0x6045:1:0x0201] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=GLOBAL_COMMAND manufacturer_specific=True is_reply=True disable_default_response=False> manufacturer=4678 tsn=157 command_id=Command.Default_Response>

The two attribute updates are likely because setpoint change was from the attribute write and the setpoint command. Question is: does it work as expected now?

Otherwise could filter just that specific attribute. Just complicates logic a little bit, depending whether that was the only attribute in the write request or not.

WolfRevo commented 3 years ago

The two attribute updates are likely because setpoint change was from the attribute write and the setpoint command.

That would indeed make sense (as I guess the thermostat reports the attribute update both on the attribute write and the setpoint command).

Question is: does it work as expected now?

I will monitor that. Needs some time to check the response of the valve in regards to setpoint changes.

WolfRevo commented 3 years ago

Great 😊 I tested it with all my eight thermostats. The behaviour is really nice:

grafik

The first one is the setpoint (not quite right as it also changed instantly through write_attributes) and the second one is the actual pi heating demand of the thermostat. As you can see the slope is much steeper (have a look into the original bug post above for reference how it worked before).

I will create the pull request accordingly. Cheers and thanks!

Stefano0042 commented 3 years ago

On the basis of a direct feedback from Danfoss R&D I would like to point out that is certainly good to have the possibility to use Setpoint type = 1 on request (normally, in consequence of a manual increase of temperature), to boost the PI value, but pay attention, this situation can lead to a temperature overshoot and economy losses. Setpoint type = 0 is the normal setup in scheduled mode (almost all the time). Massive Type 1 calls will disturb the self-learning activity of the actuator itself. Hopefully you will take care of this suggestions during deployment of the PR. Definitely this is not a bug but the result of a well engineered project.

Adminiuga commented 3 years ago

When HA Climate sets the setpoint, that should count as a manual setpoint change, correct?

Stefano0042 commented 3 years ago

That is really a tricky part, my Allies in HA environment are normally driven all the time by a scheduled activity, as result the temperature control in the rooms is smooth and stable (Standard setpoint = Ss, Eco setpoint (night) = Ss -1 or maximum -2 °C). so let's call it the basic functioning. Type 1 play his role if you manually rotate the actuator dial. Ideally it would be optimal if only the scheduled temperature changes would be followed by a type 0 setpoint command.

Stefano0042 commented 3 years ago

Normally it takes a couple of days before the Ally is properly learning the room response.

WolfRevo commented 3 years ago

Yes that sounds logically. So is there a way to filter if the setpoint change from HA is triggered by schedule or manually?

Adminiuga commented 3 years ago

But this is actually what i mean, by definition, commands originated from ZHA Climate entity count (or should count) as manual setpoint change, because it cannot come from Thermostat scheduling. The scheduled thermostat changes are pushed from Thermostat to HA?

Maybe should ask Dandies support? As is the OP problem that thermostat hasn't learned correctly the room response and that's why it behaves the way it behaves? In my POV changes pushed from HA to thermostat are equivalent to user changing the dial on the thermostat manually.

Alternatively, could bring this logic into zha climate entity and scope it to specific manufacturers and/or models: the logic being: if new setpoint is not far off from the current temperature than do the usual attribute write and of above the difference, then do the setpoint command with type of 0x01? The question then becomes what delta triggers the setpoint command

Stefano0042 commented 3 years ago

We are on the core, type 0x01 make use of PI control only with aggressive tuning, while type 0x00 belong to occupied_heating_setpoint attribute set changes with high conservative PID tuning control (PID inside) to be sure to operate in so many unpredictable situations without causing an overshoot in room temperature and making use of a more sophisticated control way. In my previous comment I refer to scheduled mode as 100% HA scheduling method (by chrono programmed occupied_heating _setpoint changes). The original Danfoss App is making use of the thermostat (Ally) scheduling capabilities, so it is possible to separates manual changes of setpoint in the app (that is the main difference) and on the mechanical dial from scheduled operation, thus assigning different Setpoint type to both. Going straight into Type 1 mode it mean loose PID functionalities, so it's really difficult take a decision.

WolfRevo commented 3 years ago

Both of you have a point.

I am still checking total behaviour of the thermostat with setpoint 0x01. I can confirm that I had an overshoot in temperature after the change yesterday in one case. As it is not heating season I didn't had a chance to test this on more occasions.

I also can confirm that the "conservative PID tuning control" is quite good in flatening the temperature curve. So I also tend to revert the setpoint change to the less agressive way. That's because after the (also mentioned by Stefano) initial learning period in the first week the pi_heating_demand changes faster and more in a way someone wouldn't acknowledge at all if he does not look at the raw values.

So as Stefano said ...

it's really difficult take a decision.

This field offers a lot to learn for me ... 😉

But this is actually what i mean, by definition, commands originated from ZHA Climate entity count (or should count) as manual setpoint change, because it cannot come from Thermostat scheduling. The scheduled thermostat changes are pushed from Thermostat to HA?

So I know people are using HA for scheduling their heating. This might not be the intention I am aware of this.

On the other side ... as I said ... if no one complains with "setpoint 0x00" because the PID improves over time and the reaction time comes close to "normal behaviour" after one week the decision could be to stay as it was... or in best case to have a config parameter in configuration.yaml for the danfoss quirk to enable users choosing the behaviour on their own. Based on this thread a short description could be easily achieved.

MattWestb commented 3 years ago

One PM how some users is using there TRVs (at least tuya ones) in HA as manual or scheduling on / off from HA then thinking its more economic then letting the TRV doing the work and getting one stable and more economical heating in the room.

My goal is that the TRV shall doing the work and only changing the set point temperature if changing somthing but normally leaving it doing its work as good it can (if not getting the valve jammed and need little hands on).

albalaing commented 3 years ago

You could take advantage of it to include compatibility with the Popp Zigbee thermostat (POPZ701721) that has the same characteristics. Changing the line: MODELS_INFO: [(DANFOSS, "eTRV0100"),("D5X84YU", "eT093WRO")],

http://manuals-backend.z-wave.info/make.php?lang=en&sku=POPZ701721&cert=ZIG21051ZB330911-24&type=mini

WolfRevo commented 3 years ago

You could take advantage of it to include compatibility with the Popp Zigbee thermostat (POPZ701721) that has the same characteristics. Changing the line: MODELS_INFO: [(DANFOSS, "eTRV0100"),("D5X84YU", "eT093WRO")],

http://manuals-backend.z-wave.info/make.php?lang=en&sku=POPZ701721&cert=ZIG21051ZB330911-24&type=mini

As I don't own this one I can't test this change. Besides that my PR already has been approved and integrated into DEV.

One PM how some users is using there TRVs (at least tuya ones) in HA as manual or scheduling on / off from HA then thinking its more economic then letting the TRV doing the work and getting one stable and more economical heating in the room.

My goal is that the TRV shall doing the work and only changing the set point temperature if changing somthing but normally leaving it doing its work as good it can (if not getting the valve jammed and need little hands on).

As I already mentioned I see that both arguments are legit. I also understand the statement from @Adminiuga

But this is actually what i mean, by definition, commands originated from ZHA Climate entity count (or should count) as manual setpoint change, because it cannot come from Thermostat scheduling.

Checking the code at the moment there is no quirk using a parameter from the configuration.yaml of HA. The zha-device-handler on the other hand already checks for custom quirks path in the config file. So I guess it wouldn't be hard to extend the setup() function in zhaquirks/__init__.py for additional optional parameters and make them available to the single quirks for further usage?

BTW I reverted back to the original code and disabled the setpoint command again. I have to admit it feels more natural and comfortable this way (knowing that the valve needs some time to react; the first on is the occupied heating setpoint, the second one the pi heating demand):

grafik

This is mostly based on the easy fact that it always needs some time to heat up the air in a room so "feeling" the warm air is never immediately after the valve has fully openend and the time the PID takes is close to normal warm up of our rooms so from a external perspective the effect is not really relevant but the thermostat is much more precise in getting and holding the desired temperature. Nevertheless the PR still makes sense from the ZHA point of view but integrating an parameter would be nice to have. Objections?

Stefano0042 commented 3 years ago

@WolfRevo exactly, what really matter is the controlled parameter: air temperature. I've got further details from eTRV department head engineer about the valve movement in scheduling mode, thus is not a pure PID but builded-up on a proprietary algorithm, that has not obviously been unveiled, based on a mix of adaptive and statistical data quite complicated they say, one reason more to use the factory set I would say.

WolfRevo commented 3 years ago

Just a question as you have direct contact with Danfoss R&D. Is Danfoss the OEM of the Thermostat mentioned earlier (Popp Zigbee thermostat (POPZ701721))? It looks like that besides the model code the firmware and hardware is the same? Or is there a unbranded OEM behind both and Danfoss is "just" adjusting the firmware for their needs?

Besides that ... so far no feedback in regards to the config file idea. Anyone?

Stefano0042 commented 3 years ago

@WolfRevo, yes Danfoss is the OEM, my personal suggestion is to enrich the current working quirks zhaquirks.danfoss.thermostat.DanfossThermostat with command cluster Command type 0x01 and 0x00 to be used in automations as service calls "Zigbee Home Automation: Issue zigbee cluster command", if technically possible...

WolfRevo commented 3 years ago

I'm still n00b in understanding the whole architecture. In my understanding filtering on quirk level is far too low for your idea. So only HA itself knows if the setpoing change is send by intentional user interaction or by an automation. The setpoint command itself does not include such information as arguments you could use to filter. So I think the only easier solution would be really to turn the command type 0x01 on and off for all danfoss thermostats at once via configuration file. Maybe the more advanced coders could implement a filtering on NWK level to be able to configure the behaviour for every thermostat differently? @Adminiuga correct me if I am wrong.

Adminiuga commented 3 years ago

You are correct, this should be done in zha. But even ZHA climate entity won't know if it is an automation or user request. Well, maybe it could, need to look

Stefano0042 commented 3 years ago

In other words I would be happy to get Command setpoint type in the list... but I do not have any clue about feasibility.

Screenshot 2021-09-30 205027

Adminiuga commented 3 years ago

With the quirk you do get the command in the manage cluster panel. Do you have quirk installed?

WolfRevo commented 3 years ago

The code change I've done is rather small. So for the moment: if you don't like to have the command 0x01 send out automatically you can create a custom quirk based on the danfoss/thermostat.py and comment out the write_attributes() substitution in the thermostat cluster.

from:

class DanfossThermostatCluster(CustomCluster, Thermostat):
    """Danfoss custom cluster."""

    manufacturer_server_commands = {
        0x40: ("setpoint_command", (t.enum8, t.int16s), False),
    }

    manufacturer_attributes = {
[...] # just collapsed for better view here
    }

    async def write_attributes(self, attributes, manufacturer=None):
        """Send SETPOINT_COMMAND after setpoint change."""

        write_res = await super().write_attributes(
            attributes, manufacturer=manufacturer
        )

        if "occupied_heating_setpoint" in attributes:
            self.debug(
                "sending setpoint command: %s", attributes["occupied_heating_setpoint"]
            )
            await self.setpoint_command(
                0x01, attributes["occupied_heating_setpoint"], manufacturer=manufacturer
            )

        return write_res

to:

class DanfossThermostatCluster(CustomCluster, Thermostat):
    """Danfoss custom cluster."""

    manufacturer_server_commands = {
        0x40: ("setpoint_command", (t.enum8, t.int16s), False),
    }

    manufacturer_attributes = {
[...] # just collapsed for better view here
    }

#    async def write_attributes(self, attributes, manufacturer=None):
#        """Send SETPOINT_COMMAND after setpoint change."""
#
#        write_res = await super().write_attributes(
#            attributes, manufacturer=manufacturer
#        )
#
#        if "occupied_heating_setpoint" in attributes:
#            self.debug(
#                "sending setpoint command: %s", attributes["occupied_heating_setpoint"]
#            )
#            await self.setpoint_command(
#                0x01, attributes["occupied_heating_setpoint"], manufacturer=manufacturer
#            )
#
#        return write_res

With that you still have the command definition and the final result looks like:

grafik

but with the original bevaviour.

Stefano0042 commented 3 years ago

Ok it works as you mentioned. Thank you. @WolfRevo when you say that the PR is already in dev it mean that the next major upgrade of HA is going to implement it?

WolfRevo commented 3 years ago

I'm not part of the dev team. But HA dev already includes this PR so yes one of the next HA releases will have it. Important: this inlcudes the setpoint command by default so to deactivate it you need to maintain your custom quirk.

I've already checked into the code to find out how easy it would be possible to include another config parameter for this. Unfortunately it looks like this also needs changes in HA itself. I would prefer this method but I need more time to see if I can enhance this idea further.

MattWestb commented 3 years ago

I was looking around and its looks being 2 more clones of Ally. From Z2M:

        zigbeeModel: ['eTRV0100'],
        model: '014G2461',
        vendor: 'Danfoss',

        zigbeeModel: ['TRV001'],
        model: 'UK7004240',
        vendor: 'Hive',

        zigbeeModel: ['eT093WRO'],
        model: '701721',
        vendor: 'Popp',

I was thinking adding the 2 new MODELS_INFO IDs in the danfoss quirk but is some knowing if the firmware is having the same functional or is its not compatible ? I have fast looking in Z2M and its looks both is using danfoss handles but its not the same that all is 110% the same as the "original" device is having.

But in the end if user have baying the devcie they can testing and if its working for 80% is better then not working at all and then some can doing one new quirk that is working for there device.

Feedback is wanted !!!

WolfRevo commented 3 years ago

On the other hand if users buy these devices and miss the support they either just send them back or they will try to find out what's going on and will land in this thread in the end. They could then request help in creating a new quirk they successfully can test on their own prior to creating the corresponding PR. I don't know if it is a good idea from the perspective of quality to add support for untested devices.

MattWestb commented 3 years ago

I agree then we is not knowing is the 3 device types is behaving the same way from Zigbee point of view. And then some is getting the device is fast done patching for testing if is working OK and if they is searching the git they finding the relevant posting so i dont do "ghost" that is not being tested in real.

Thanks for feed back !!

albalaing commented 3 years ago

I have the Popp and it works without problems. it is exactly the same. The only difference from Danfoss is that it has a higher firmware version (1.12). On the Danfoss support page the latest version is 1.08 but in the documentation they refer to 1.12.

https://www.danfoss.com/en/products/dhs/smart-heating/smart-heating/danfoss-ally/danfoss-ally-support/#tab-faq

Basic Cluster Popp sw_build_id: 01.12.0008 01.12 hw_version: 69

Danfoss sw_build_id: 01.08.0008 01.08 hw_version: 69

MattWestb commented 3 years ago

Sorry for hijacking the issue ! @albalaing shall I making one PR for adding the Popp in the quirk so it working out of the box ? Then i need you testing it so its being OK.

Looks like its the last firmware for the Ally and some have problems in Z2M getting the devices updated with it but is not reported in ZHA and the firmware can being downloaded from the Danfoss support page as you was saying :-))

albalaing commented 3 years ago

I have been using the Popp for quite some time and it works exactly the same as the Danfos. Danfos updated it to 1.08 by downloading the firmware from the support page and using ZHA. I had no problem

sarangnemo commented 2 years ago

@WolfRevo exactly, what really matter is the controlled parameter: air temperature. I've got further details from eTRV department head engineer about the valve movement in scheduling mode, thus is not a pure PID but builded-up on a proprietary algorithm, that has not obviously been unveiled, based on a mix of adaptive and statistical data quite complicated they say, one reason more to use the factory set I would say.

Hi, thanks for your hard work here! One mystery that is being discussed at length over in the HA Community forum is how the Ally makes use of data pushed from an external room temperature sensor, and how to improve its performance with that configuration.

Many observe that the Ally aims to get the average of the onboard sensor and external room sensor to the setpoint temperature, and – related to the topic here – that the actuator can take several hours to get there. (For example, if the setpoint is 21°, and the onboard sensor goes to 22°, then it seems to slowly heat the room to 20° but maintain it once there.) It also seems odd that the documentation recommends that the external temperature reading gets pushed at a minimum interval of 30 minutes, instead of more often.

@WolfRevo do you have any experience with how to handle an external sensor, or maybe your contact with Danfoss R&D can provide some clues?

WolfRevo commented 2 years ago

For feedback from Danfoss we need to ask @Stefano0042, I don't have any connections with them.

I have one example from my daily used room:

grafik

Purple is the requested setpoint, dark blue is the temperature from the thermostat itself and light blue is the actual room temperature. Yes, with activated PID (for that you need to use a custom integration right now as the standard ZHA integration disables the PID by intention) it really needs time to reach the desired temperature. But you also can see that the temperature of the thermostat still rises as long as the external temperature is not reached (so in my experience the average fits really well).

I use scripts and an automation in HA that triggers as often as the external room temperature sensor changes. This happens more often than 30 minutes at daytime but only very view times at night. I can't answer why the documentation suggests to only send the temperature at a minimum of 30 minutes. It works well with more often changes also.

sarangnemo commented 2 years ago

Thanks @WolfRevo, and sorry I misread the thread above and thought you were the one in contact with Danfoss. In the meantime, my own experience has changed, and now my Allys do in fact maintain the externally measured room temperature at the desired setpoint (without any apparent averaging with the onboard sensor). Apparently they do "learn" over time.

@Stefano0042 if you have a chance to ask Danfoss, it is still a mystery why their documentation suggests sending temperature updates only every 30 minutes.

Stefano0042 commented 2 years ago

@sarangnemo Hello, there is not any special calculation behind the use of an external sensor, simply it will become the new temperature feedback for the output calculation (PI), Ally thermostat will continue to report his "current_temperature", e.g. if the remote sensor location is 2 degree lower than the Ally sensors location it will report, once stable, 2 degree more than the active setpoint. Regarding the max 30 min update rate I do not have a precise clue about that, but I will simply accept the recommendation ..by now :) Nowadays the standard ZHA integration disables the predictive/adaptive part (based on a long-term temp. data acquisition) of the whole algorithm, and let run the "light" PI or PID calculation, these are details that will never be unveiled, we can fully understand, but this is not a bad solution especially if you have the idea to use the TRVs status e.g. to trigger an external boiler relay (on-off) as result of TRVs heating demand condition, as you need a more clear, reactive and stable valve position.

Hedda commented 2 years ago

@WolfRevo can this issue be closed now that https://github.com/zigpy/zha-device-handlers/pull/1148 by @hertleinj for Popp D5X84YU (POPZ701721) has been merged into zha-device-handlers as part of 0.0.64 release? -> https://github.com/zigpy/zha-device-handlers/releases or did that not fix all issues?

WolfRevo commented 2 years ago

@Hedda I never had an issue after my initial PR has been taken over as I use a custom quirk to disable my own PR. As noone was able to propose an easy way how the manufacturer custom command can be filtered I am fine with this.

Still if someone has a smart idea how to do so (e.g. like adding parameters in the configuration.yaml to be able to set entity specific options for zha quirks) I'm interested in following this approach. I will close this topic within the next days if there is no new idea posted here.

WolfRevo commented 2 years ago

@Stefano0042 is there any way to directly contact you? I have some questions regarding the specific behaviour for following topics for Danfoss Ally and there is no sufficient documentation on it: 1) Attr 0x4051 Window Open Feature ON/OFF in conjunction with Attr 0x4003 External Open Window Detected 2) general understanding of the adaptation run, as the automatic adaptation run is not running in the night anymore but in the morning where the valve should not close. I need to understand if the adaptation run is necessary for the function so that I can create my own automation when to run. If you want you coud write to me using my username and adding the gmail suffix for reaching me. That would be really nice. Or is there a place to discuss such topics?

github-actions[bot] commented 2 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.