OpenZWave / open-zwave

a C++ library to control Z-Wave Networks via a USB Z-Wave Controller.
http://www.openzwave.net/
GNU Lesser General Public License v3.0
1.05k stars 915 forks source link

Unable to set Heatit Z-TRM3 thermostat setpoint to 25.5C or lower #2435

Open robinsmidsrod opened 3 years ago

robinsmidsrod commented 3 years ago

After quite a lot of troubleshooting with my Thermofloor Heatit TF 058 (aka. Z-TRM3) thermostat I think I've found a bug, and I'm unsure how to approach the issue.

As mentioned in the headline, I'm unable to set the thermostat (heating) setpoint to a value below 26C. By looking carefully at the ThermostatSetpointCmd_Set lines, I was able to understand that it seems to be related to encoding of integers below 256.

These three commands work (i.e. the setpoint is reported back as changed to the new value):

ThermostatSetpointCmd_Set (Node=10): 0x01, 0x0d, 0x00, 0x13, 0x0a, 0x06, 0x43, 0x01, 0x01, 0x22, **0x01, 0x0e**, 0x25, 0xa6, 0x00 # 27.0C=270=0x01, 0x0e
ThermostatSetpointCmd_Set (Node=10): 0x01, 0x0d, 0x00, 0x13, 0x0a, 0x06, 0x43, 0x01, 0x01, 0x22, **0x01, 0x09**, 0x25, 0xac, 0x0d # 26.5C=265=0x01, 0x09
ThermostatSetpointCmd_Set (Node=10): 0x01, 0x0d, 0x00, 0x13, 0x0a, 0x06, 0x43, 0x01, 0x01, 0x22, **0x01, 0x04**, 0x25, 0xb2, 0x1e # 26.0C=260=0x01, 0x04

This command fails (i.e. the old setpoint is reported back unchanged):

ThermostatSetpointCmd_Set (Node=10): 0x01, 0x0c, 0x00, 0x13, 0x0a, 0x05, 0x43, 0x01, 0x01, 0x21, **0xff**, 0x25, 0xb8, 0xef # 25.5C=255=0xff

First of all, is there a compatibility flag available to force the setpoint value to always be sent using 2 bytes, even when the value is below 256? I'm fairly certain that should solve the immediate issue, but not the root cause.

I'm not exactly sure what you need from the log file to debug this further. I was trying out a lot of different configurations in thermofloor/heatit058.xml, so my log file is full of trial-and-error issues.

Using ozwdaemon docker build 175 together with Home Assistant 0.116.2. Aeotec Z-Stick Gen5 USB controller.

Cyberwizzard commented 3 years ago

I have a few of the same thermostats and same issue.

For completeness, the firmware version of the HeatIt Z-TRM3 is 4.00.

Link to a Norwegian forum where the same behavior and issue was observed: https://www.hjemmeautomasjon.no/forums/topic/6330-z-trm3-target-temperature-f%C3%A5r-ikke-satt People on there are testing unofficial firmware versions to work around this issue but as far as I can establish, any workaround requires a programming cable from HeatIt to physically reprogram the thermostat, so a software workaround to just send 2 bytes from the OZW controller is the easiest solution I think.

Cyberwizzard commented 3 years ago

Some scanning of the OZW source for suggesting a fix: https://github.com/OpenZWave/open-zwave/blob/b0afd4c60b38065fdbc57835cb3eb6035afcc00f/cpp/src/command_classes/CommandClass.cpp#L600 could have a minSize input with a default 0 value which leaves it on automatic sizing as it is now.

https://github.com/OpenZWave/open-zwave/blob/b0afd4c60b38065fdbc57835cb3eb6035afcc00f/cpp/src/command_classes/ThermostatSetpoint.cpp#L281 would then need to set minSize to 2 in this specific case.

From the linked forum thread it seems HeatIt claims the thermostat reports that it needs 2 bytes for the setpoint. If this is correct, I assume the ThermostatSetpointCmd_CapabilitiesReport call should have this information? Perhaps the minSize for the setpoint can then be set once this report is parsed. I have not had the time yet to see if this is indeed the case.

Disclaimer: I have never looked at the OZW source before today, so perhaps I am way off 😃

Edit: I was hoping the OZW logs have "Received capabilities of thermostat setpoint" to debug what is the actual reported min and max (https://github.com/OpenZWave/open-zwave/blob/b0afd4c60b38065fdbc57835cb3eb6035afcc00f/cpp/src/command_classes/ThermostatSetpoint.cpp#L244) but either it is not reported at all or I missed it as the OZW integration in HomeAssistant only shows a few lines of the log at a time with no way of getting the entire log... Is this capability report something which must be requested?

Hatzl commented 3 years ago

I know it‘s not the smartest way, but I flashed my thermostat with the unofficial firmware. You don‘t need a flashing cable, updating works with a zwave usb-stick. I wrote a litte bit more information in this PR: #2364

robinsmidsrod commented 3 years ago

For those that don't understand Norwegian, I'd like to point out that the firmware is not "unofficial", per se. It is delivered from Thermofloor, but it doesn't pass Z-Wave certification, which is why we shouldn't rely on it. We should instead try to fix open-zwave to work with the setpoint capabilities report that defines how the setpoint should be sent to the device.

Cyberwizzard commented 3 years ago

Yes, since this seems to happen with a number of thermostats, it seems OZW is missing this feature.

I have a PDF and alternative firmware from Thermofloor where they document their findings on the problem.

In a nutshell it seems that the capabilities report where the min- and maximum temperature are reported, this information is passed with a field size for the minimum and maximum temperature. According to them, since the minimum temperature field has a size of 2 bytes, this is the minimum size for the setpoint command as well.

So in essence, this size information needs to be cached per thermostat node and then applied when building the setpoint payload.

I am willing to help out, but since I never worked on OZW I'm still in the exploration phase and I need to get a development platform as the HomeAssistant docker is not the right way of altering the source. Unless someone is already working on caching the minimum and maximum temperature bounds for the setpoint command, in which case retaining the field size should be a breeze - but I did not figure out if that is implemented yet.

On Sat, Oct 31, 2020 at 9:52 AM Robin Smidsrød notifications@github.com wrote:

For those that don't understand Norwegian, I'd like to point out that the firmware is not "unofficial", per se. It is delivered from Thermofloor, but it doesn't pass Z-Wave certification, which is why we shouldn't rely on it. We should instead try to fix open-zwave to work with the setpoint capabilities report that defines how the setpoint should be sent to the device.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/OpenZWave/open-zwave/issues/2435#issuecomment-719905301, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOMT6JA5UWHQILMYBW536TSNPF4NANCNFSM4TAQVZMA .

kpine commented 3 years ago

In a nutshell it seems that the capabilities report where the min- and maximum temperature are reported, this information is passed with a field size for the minimum and maximum temperature. According to them, since the minimum temperature field has a size of 2 bytes, this is the minimum size for the setpoint command as well.

I'm legitimately curious how this interpretation can be derived from the spec (section 4.114.9 Thermostat Setpoint Capabilities Report Command).

Min Value Size (3 bits) This field is used to indicate the length in bytes of the Min Value field. This field MUST be set to 1, 2 or 4.

How does it relate to the size in the Set command? It literally says the Min Value Size field is the size of the Min Value field. How would a developer (e.g. OpenZWave) know to interpret it the same way Heat-it says? Why is it the minimum value size field and not the maximum? Are they guaranteed to be the same so it's OK to pick either?

There is this ambiguous statement in section 4.114.3 Thermostat Setpoint Set Command:

A supporting node MUST support the same format of Precision, Scale and Size fields values as it sends in the Thermostat Setpoint Supported Report Command or Thermostat Setpoint Capabilities Report Command.

First of all, Thermostat Setpoint Supported Report Command does not report any precision, scale or size fields. Secondly, the phrase MUST means the thermostat is required to accept the same format it reports, but it doesn't say anything about not supporting other valid formats, and OZW is sending a valid command.

I could see that statement, along with the Capabilities Report, being interpreted to mean what Heat-it is saying. However, it doesn't say which field, min or max, is supposed to be used. I guess you hope they are the same? Assuming this is the interpretation being used, and it is in fact required for certification, it would be safer for OZW to use these fields as has been stated.

I am not privy to the certification process and what Heat-it has been told, so I am not at all saying they are wrong, just trying to understand where this requirement came from. From an amateur perspective looking strictly at the spec docs, I don't see where.

I tried to follow the thread via Google Translate. What was the consensus on support from other hubs? I thought I saw only the Fibaro Home Center was working and no other hubs were, or is it just OpenZWave that doesn't work? I know SmartThings has a 3rd party device handler (RBoy Apps) which seems to be an official handler developed in coordination with the vendor. Does it work with vanilla SmartThings?

Cyberwizzard commented 3 years ago

The Norwegian thread and goes all over the place but since OpenHab and HA both use OZW under the hood, both are affected.

I wonder if some other thermostats with issues around the setpoint command have the same problem, or that Heatit is the only one (I thought I found a few others but I went through so many Google results I've lost overview).

In any case I think Heatit is right that, as you also pointed out, that the capability report is supposed to denote something about the field size, and the only two sizes in there are the min and max.

It's not specified explicitly that the max temp size has to be equal or larger than the min temp size but I guess it's safe to assume this is the case. In case it is desired to handle that, doing a min(sizemin, sizemax) should cover that.

Reversing the argument, what harm would there be in using the min temp field size for the setpoint command? Worst case scenario would be that a zero byte is added to the payload of thermostats that would have accepted a smaller field size, right? Or are you worried it would break this command for other thermostats?

Alternatively this new behavior could be placed in the device xml behind an optional switch to handle the setpoint field width like heatit claims its supposed to be.

On a side note, their min temp size and max temp size is both 2 bytes in that report.

On Sat, 31 Oct 2020, 18:12 Keith Pine, notifications@github.com wrote:

In a nutshell it seems that the capabilities report where the min- and maximum temperature are reported, this information is passed with a field size for the minimum and maximum temperature. According to them, since the minimum temperature field has a size of 2 bytes, this is the minimum size for the setpoint command as well.

I'm legitimately curious how this interpretation can be derived from the spec (section 4.114.9 Thermostat Setpoint Capabilities Report Command).

Min Value Size (3 bits) This field is used to indicate the length in bytes of the Min Value field. This field MUST be set to 1, 2 or 4.

How does it relate to the size in the Set command? It literally says the Min Value Size field is the size of the Min Value field. How would a developer (e.g. OpenZWave) know to interpret it the same way Heat-it says? Why is it the minimum value size field and not the maximum? Are they guaranteed to be the same so it's OK to pick either?

There is this ambiguous statement in section 4.114.3 Thermostat Setpoint Set Command:

A supporting node MUST support the same format of Precision, Scale and Size fields values as it sends in the Thermostat Setpoint Supported Report Command or Thermostat Setpoint Capabilities Report Command.

First of all, Thermostat Setpoint Supported Report Command does not report any precision, scale or size fields. Secondly, the phrase MUST means the thermostat is required to accept the same format it reports, but it doesn't say anything about not supporting other valid formats, and OZW is sending a valid command.

I could see that statement, along with the Capabilities Report, being interpreted to mean what Heat-it is saying. However, it doesn't say which field, min or max, is supposed to be used. I guess you hope they are the same? Assuming this is the interpretation being used, and it is in fact required for certification, it would be safer for OZW to use these fields as has been stated.

I am not privy to the certification process and what Heat-it has been told, so I am not at all saying they are wrong, just trying to understand where this requirement came from. From an amateur perspective looking strictly at the spec docs, I don't see where.

I tried to follow the thread via Google Translate. What was the consensus on support from other hubs? I thought I saw only the Fibaro Home Center was working and no other hubs were, or is it just OpenZWave that doesn't work? I know SmartThings has a 3rd party device handler (RBoy Apps) which seems to be an official handler developed in coordination with the vendor. Does it work with vanilla SmartThings?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/OpenZWave/open-zwave/issues/2435#issuecomment-719961778, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOMT6LS52EAUQFY5JNCP23SNRAQPANCNFSM4TAQVZMA .

kpine commented 3 years ago

The Norwegian thread and goes all over the place but since OpenHab and HA both use OZW under the hood, both are affected.

OpenHAB is an independent implementation of Z-Wave in Java, no relation to OZW. The thread also says Homeseer doesn't work, so I guess they've all independently done it wrong. Are the working controllers actually going "by the spec", or do they implement a workaround for Heat-it? That I'm not clear on.

SDS14223 lists the interview process for the setpoint class, and has a pretty non-ambiguous table for the Set command.

image

Does "user defined" have a different meaning than I would think of?

robinsmidsrod commented 3 years ago

I've sent an email to Heat-it/Thermofloor making them aware of this bug report, asking them to clarify how their thermostats implement the thermostat setpoint specification mentioned above. Hopefully someone there will take the time to actually respond here with some more details, so the problem can be solved once and for all, in a way that is consistent with the ZWave specification.

Cyberwizzard commented 3 years ago

They sent me a document containing an alternative firmware, instructions to patch and most importantly a document detailing what they concluded is wrong and how to read the spec according to them.

@kpine sorry I thought I inferred from that thread that OpenHAB used OZW as well; which means whenever we figure out what is wrong that still leaves other stacks to be fixed.

About that screenshot: I was linked the Z-wave specification before which is the same document Heat-It seemed to use but it differs from yours: the capabilities report apparently defines the format which is needed for the setpoint command, so while there is some freedom at the setpoint format, demanding a specific number of bytes or precision is also possible. See: Test of thermostat Setpoint Command Class.pdf

In my fork I now implemented this, where the capability report is parsed to retain the number of bytes and precision and applied at the setpoint command. The thermostat is actually reacting to these payloads, but unfortunately its setting the setpoint to wildly different values so something is still wrong. Edit: link to my fork: https://github.com/Cyberwizzard/open-zwave

kpine commented 3 years ago

About that screenshot: I was linked the Z-wave specification before which is the same document Heat-It seemed to use but it differs from your

My screenshot is from document SDS14223 - Z-Wave Command Class Control Specification. The Control Specification defines how controlling nodes (i.e. openzwave) are supposed to act and the table I listed shows how the Setpoint Set command is supposed to be formatted. According to that, the size argument can be set by the user, but the scale value cannot.

The screenshot with the Set and Report payloads is from SDS13781 - Z-Wave Application Command Class Specification.

Regarding the test report, I'm not sure what it's supposed to be showing. Is this from Si Labs or Heat-it? I don't have any disagreements with the data it shows. The statement is that OZW is not following the spec (using a size of 1), but the report does not show anything relevant to that. Did I miss something?

robinsmidsrod commented 3 years ago

Thermofloor has responded back by email, and they only state that they've talked with several people from Open-ZWave project in the past, so they aren't interested in spending time responding to this again. I tried getting the names of who they spoke to, but they weren't interested in providing this.

Cyberwizzard commented 3 years ago

@kpine from SDS14223 v11, 4.19.1 shows the diagram for the interview for a v3 node, where setpoint capabilities report is first issued before issuing the setpoint. In Table 30 is says for the Setpoint Value: If nodes are version 3 or newer: User defined among supported values, If nodes are version 1 or 2: User defined. So currently the behavior is for version 1 or 2 while HeatIt implemented v3: they specify 2 bytes and 1 decimal precision for both min and max temperatures and as such the thermostat is only accepting that.

I am not saying this is the only correct interpretation of the spec (which is fairly ambiguous on how those capabilities are to be used, for example if max uses less bytes than min, or the precision differs between min and max) but I do think its implementable, and I assume its backwards compatible if the older versions just allowed any user defined format.

HeatIt mailed me back on my trace and suggested that this command works for them: "This works for setting setpoint to 9.5 degrees. Hex data: 60 0D 00 01 43 01 01 22 00 5F "

Below is a trace using my fork I made for 14 degrees for reference: 0x60, 0x0d, 0x01, 0x01, 0x43, 0x01, 0x01, 0x22, 0x00, 0x8c ... This one is not working: the actual interpreted value is wrong, but the thermostat is reacting to this, so thats progress.

Note how the 3rd byte in this sequence is 0x00 for them and 0x01 for mine - this seems to be the layer below the command layer and I have not yet found the protocol specification to explain to me what that one does. @kpine perhaps you know?

kpine commented 3 years ago

For the interview, in v1 and v2, the controller must enumerate all of the "ambiguous" setpoint values. In V3 it only needs to enumerate the setpoints that it says are supported. This is because all thermostats do not implement the setpoints the same for V1/2. Yes, OZW can use the same size/scale/precision during the Get phase of the interview and use the same values, as the device is required to support those. I have not disputed that.

For the table I quoted, it clearly says the size value is user defined; it makes no distinction between v1/2 and 3. The only one that is clarified is for the "Setpoint Value" which means the user can only select from the setpoints that are supported in V3, otherwise it's completely user defined.

Notice in that same section there is a specific mention of the scale argument:

The Scale field value MUST be identical to the value received in the Thermostat Setpoint Report for the actual Setpoint Type during the node interview.

Why doesn't it say that for the size value if it's also true? Of course there is nothing about the min/max values.

No one is required to listen to me, certainly not Heat-it. As I said, I'm just trying to learn and so far I'm not convinced that OZW is doing anything wrong. Hopefully sometime soon Fishwaldo will eventually correct me or not. 😃

60 0D 00 01 43 01 01 22 00 5F

That's a multichannel encapsulated command (SDS13783).

image

60 - COMMAND_CLASS_MULTI_CHANNEL 0D - MULTI_CHANNEL_CMD_ENCAP 00 - Source endpoint 0 01 - Destination endpoint 1 43 - Command Class - Setpoint 01 - Command Class - Set Setpoint, etc. ...

There are a couple reasons why the value could be 0. I don't know why it needs to be 0 in this scenario (SDS13783 describes it).

If you use the PC-Controller software, there is an option to encapsulate the command and you can tweak the source and destination endpoints to experiment if you want.

kpine commented 3 years ago

FYI, my older tstat with V2 Setpoint and multi-channel association has no problem accepting an encapsulated command that comes from source endpoint 1. Not sure if there's a requirement one way or the other.

2020-11-08 09:24:56.230627583  [20201108 9:24:56.230 PST] [ozw.library] [info]: Info - Node: 5 Sending (Send) message (Callback ID=0xb8, Expected Reply=0x13) - MultiChannel Encapsulated (instance=1): ThermostatSetpointCmd_Set (Node=5): 0x01, 0x10, 0x00, 0x13, 0x05, 0x09, 0x60, 0x0d, 0x01, 0x01, 0x43, 0x01, 0x01, 0x09, 0x47, 0x25, 0xb8, 0x0d 

0x60, 0x0d, 0x01, 0x01, 0x43, 0x01, 0x01, 0x09, 0x47

2020-11-08 09:24:57.874438255  [20201108 9:24:57.874 PST] [ozw.library] [debug]: Detail - Node: 5   Received: 0x01, 0x11, 0x00, 0x04, 0x00, 0x05, 0x09, 0x60, 0x0d, 0x01, 0x01, 0x43, 0x03, 0x01, 0x09, 0x47, 0xca, 0x00, 0x4e 
2020-11-08 09:24:57.874549177  [20201108 9:24:57.874 PST] [ozw.library] [info]: Info - Node: 5 Response RTT 368 Average Response RTT 399 
2020-11-08 09:24:57.874587271  [20201108 9:24:57.874 PST] [ozw.library] [info]: Info - Node: 5 Received a MultiChannelEncap from node 5, endpoint 1 for Command Class COMMAND_CLASS_THERMOSTAT_SETPOINT 
2020-11-08 09:24:57.874666065  [20201108 9:24:57.874 PST] [ozw.library] [debug]: Detail - Node: 5 Value Updated: old value=70, new value=71, type=decimal 
Cyberwizzard commented 3 years ago

I discovered the issue, and it was my own fault. I messed with the size and precision but I did not patch the function which computes the size of the value.

After fixing this, I can now control the z-trm3 from openzwave as intended.

I will put this behavior behind a compat flag to limit the impact to only thermostats which need it.

On Sun, 8 Nov 2020, 18:32 Keith Pine, notifications@github.com wrote:

FYI, my older tstat with V2 Setpoint and multi-channel association has no problem accepting an encapsulated command that comes from source endpoint

  1. Not sure if there's a requirement one way or the other.

2020-11-08 09:24:56.230627583 [20201108 9:24:56.230 PST] [ozw.library] [info]: Info - Node: 5 Sending (Send) message (Callback ID=0xb8, Expected Reply=0x13) - MultiChannel Encapsulated (instance=1): ThermostatSetpointCmd_Set (Node=5): 0x01, 0x10, 0x00, 0x13, 0x05, 0x09, 0x60, 0x0d, 0x01, 0x01, 0x43, 0x01, 0x01, 0x09, 0x47, 0x25, 0xb8, 0x0d

0x60, 0x0d, 0x01, 0x01, 0x43, 0x01, 0x01, 0x09, 0x47

2020-11-08 09:24:57.874438255 [20201108 9:24:57.874 PST] [ozw.library] [debug]: Detail - Node: 5 Received: 0x01, 0x11, 0x00, 0x04, 0x00, 0x05, 0x09, 0x60, 0x0d, 0x01, 0x01, 0x43, 0x03, 0x01, 0x09, 0x47, 0xca, 0x00, 0x4e 2020-11-08 09:24:57.874549177 [20201108 9:24:57.874 PST] [ozw.library] [info]: Info - Node: 5 Response RTT 368 Average Response RTT 399 2020-11-08 09:24:57.874587271 [20201108 9:24:57.874 PST] [ozw.library] [info]: Info - Node: 5 Received a MultiChannelEncap from node 5, endpoint 1 for Command Class COMMAND_CLASS_THERMOSTAT_SETPOINT 2020-11-08 09:24:57.874666065 [20201108 9:24:57.874 PST] [ozw.library] [debug]: Detail - Node: 5 Value Updated: old value=70, new value=71, type=decimal

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/OpenZWave/open-zwave/issues/2435#issuecomment-723641203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOMT6KZPVWLWEQA3ISQQTTSO3IZVANCNFSM4TAQVZMA .

robinsmidsrod commented 3 years ago

@Cyberwizzard What's left to get this pushed to the container on Docker Hub and close this issue?

Cyberwizzard commented 3 years ago

@robinsmidsrod It is essentially done and I have it up and running in my house; the only problem is that on the OZW code I need an instance number in a function which does not have it right now, so I hard-coded it. For anyone with a Z-TRM3 this will not matter, but it needs to be fixed before I can make a pull-request.

@kpine or @Fishwaldo - at https://github.com/Cyberwizzard/open-zwave/blob/master/cpp/src/command_classes/ThermostatSetpoint.cpp#L284 I hard-code the instance index to 1 as SetValue on that class does not get an instance ID. How would I get this in a neat manner?

Cyberwizzard commented 3 years ago

Created pull request #2458 with last cleanups with some help from @kpine and others.