domoticz / domoticz

Open source Home Automation System
http://www.domoticz.com
GNU General Public License v3.0
3.49k stars 1.13k forks source link

ZWave thermostat mode "Manufacturer Specific" not working correctly #4682

Closed rrozema closed 2 years ago

rrozema commented 3 years ago

Multiple errors exist in zwave Thermostat Mode, Thermostat Fan Mode, Thermostat Operating State and Thermostat Fan Operating State. Some are due to OpenZwave, others are in Domoticz' source code. I have tried to fix this myself, but with my limited experience I keep running into new issues every time. We need a coordinated action between openzwave and domoticz to fix the problems. Problems are:

  1. not all modes available in some devices can be set (for example Manufacturer Specific (31), mode can not be set properly)
  2. selected mode/state is often not shown (correctly) in the big text. Modes 0 and 1 are often correct. 4 and 31 are not because the value does not match the index.
  3. selected mode/state is never shown in the dashboard.
  4. selected mode/state is often not available inside the edit dialog: the selection box is empty. Depending on the mode value, most of the times an emtpy selection is shown because the mode's value is used an index into the list of available options.
  5. selected mode/state is not properly updated for scripts to retrieve in dzvents.
  6. not all modes/states supported by the device may be shown in Domoticz.
  7. Operating state and Fan Operating state are not showing the actual state of the device (Domoticz does not retrieve the supported options from OpenZwave, but tries to interpret the value by itself).

There are multiple reasons for these problems:

gizmocuz commented 3 years ago

not quite true. openzwave.cpp, line 2143 for example

rrozema commented 3 years ago

Yes, and then look at line 2158.

Also look at line 4395. The tmode value passed in here is sometimes an index value and sometimes a value, depending from where the call was made. For example, when freshly retrieved from the xml file tmode will be an item value. But when called from webserver.cpp it is an index. Now look at webserver.cpp, line 11297. Here it is for sure used as an index, otherwise why compare it to vmodes.size()? In the loop above there (line 11290), even a counter (smode) is used to fill the option value. the sprintf( "%d;%s;", ...) should set "[value];[description];", not "[index];[description];". And these are only some examples, this sort of mistakes happen in multiple places. Both in Domoticz and OpenZwave.

As I mentioned, I can see where the confusion is coming from: the openzwave API is very inconsistent in the ValueList area. Plus the implementation is just dead wrong at some points. In any case it is over complex trying to do the translations from index into values and the other way around. The much simpler way is to just treat the Thermostat mode, Theremostat Fan mode, etc values like a ValueInt plus a list of supported values for the UI to build a gui from. All the other logic in there is unnecessary and far to complicated.

Why make it so complicated? the mode is a uint8. If you send an unsupported value to the device, the device will return an error or it will gracefully accept it and decide for itself what to set the mode to. That is up to the device manufacturer and we can't predict that behavior anyway. So wht try validating it at all? Just treat the thermostat mode, thermostat fan mode, thermostat state and thermostat fan state as a normal uint8. Only when displaying a GUI for the user to select a value we need the supported modes list to build ourselves an <option />-list instead of an <input />-control. Otherwise it is just another uint8 value.

rrozema commented 3 years ago

Also, shouldn't there be an update somewhere to set DeviceStatus.intValue to the value of the newly selected tmode? How will we -for example- be able to push the mode into the influxdb if the value is not written into the database?

gizmocuz commented 3 years ago

Before asking any of these questions, setup a build environment under windows with Visual Studio, and set some breakpoints

rrozema commented 3 years ago

That's exactly what I did: I bought an aeotec usb z-stick, and excluded one of my 'production' eurotronic spiritz nodes and included that with my windows machine. I opened the Domoticz solution and added the openzwave project to it. I then set the Configuration on the openzwave project to DebugDLL and set the Output Directory for openzwave to "$(SolutionDir)Debug\" so I can build both projects together and start the debugger to run them together too.

I stepped through lots of code many times and tried a lot of changes. It's just not enough: I am not an experienced c++ programmer and I don't have any experience debugging javascript. There are to many errors, the errors are structural (correcting incorrect code is easy; adding missing code is a lot harder) and the bit where it goes through javascript is not debuggable, and this is a big part of where things go wrong. On top of all that, when trying to work around the problems I find that the API openzwave doesn't provide the proper methods to get the information I need. f.e. there is no means to get an item's m_description given the item's m_value from a ValueList, I need to add new methods in both valuelist.cpp and manager.cpp (OZW) plus in openzwave.cpp (Domoticz) to get those values in webserver.cpp.

Symptoms of the problems are: setting the thermostat mode most of the times works correctly (not always!) since the changes you made after my earlier suggestions. The device does switch to the new mode for all of the values 0, 1, 11, 15 and 31. This can be verified from the display on the spirit Z and the sound of the motor opening and closing the valve in response to the new mode. But, the new value set is NOT shown on the device in Domoticz if the new mode is not one of 'Off' or 'Heat' (= 0 or 1). i.e. when the value does not match the index. Modes "Heat Econ" (value = 11) and "Full Power" (value = 15) and "Manufacturer Specific" (value = 31) are never selected in the drop down in the edit dialog (selection comes up empty), and also the big text is empty when one of these modes gets set. Also the results can vary, depending on where the information of the value came from. If it's from the webserver the mode to be set will be one of 0, 1, 2, 3, 4. And if it's from the notification message from domoticz it's 0, 1, 11, 15, or 31. BUT, since there are in many places (incorrect) attempts to validate the value as if it's an index, values 11, 15 and 31 are never allowed in because they do not match (for example) <= m_modes.size()., though they are perfectly valid mode values. These incorrect checks are both in Domoticz and in OpenZwave and in both they should be removed.

Via RType_setused() the tmode is received from the webserver, and this is sometimes a value (0, 1, 11, 15 or 31) and other times it's an index (0, 1, 2, 3, 4), depending on which code filled the options list.

Another example: dzvents needs to retrieve the currently set mode value from the database, as do the data pushers like the influxdb. But there is nowhere any update to be found that sets the new mode into DeviceStatus table's intValue column. Again, I can fix problems like a range error or something similar, but with little oversight on the both projects and little experience in c++ it's hard to think up new code for logic that's missing.

nebuohyrrah commented 3 years ago

I recently found out that z-wave devices don't always report the 'manufacturer specific' mode in the THERMOSTAT_MODE_SUPPORTED_REPORT. At least this is true for the Fibaro heat controller FGT-001 and Eurotronic Spiritz. Therefore it is not added in the supported modes list in openzwave and when this mode is reported by the node it is discarded as not supported.

I have added a few lines of code that checks if mode 31 ('manufacturer specific') is reported and if not add it to the list. With the mode added manually to the list I have no problems anymore, but it is not a final solution as not all thermostat devices have the manufacturer specific mode. You can also add it to the ozwcache file when domoticz is not running, but this only last untill the node information is updated and overwritten. I don't have a good solution for this as I am not an experienced c++ programmer either.

rrozema commented 3 years ago

@nebuohyrrah While debugging I too have observed incorrect results when getting the supported modes (THERMOSTAT_MODE_SUPPORTED_REPORT). What I noticed is that the length of the report message is not always correctly passed in. Sometimes the length is (correctly) reported as 6 bytes, whereas the actual content is only 4 bytes long: this causes the message's crc code to be interpreted as a byte of the bit flags, resulting in my eurotronic spirit Z reporting to support some 'unknown' modes below 31 and not 31 itself.. I am however not sure I did not cause this myself by one of the small modifications I made, or that it is a result of openzwave not completely/correctly supporting V2 and higher of the Thermostat Mode Command Class: the message was smaller in V1 than it is now in V2 and higher.

nebuohyrrah commented 3 years ago

I did not see this. I have 'decoded' the received message from the node by hand according to the command class specification provided by Silabs. Everything checks out, the length of the message and the reported modes correspond with the modes described in the manual EXECPT for the manufacturer specific mode. This is not reported.

The command class version reported by the node and openzwave is v3. Even the 'Full power' mode, that is introduced in version 3 of the command class specification, is reported correctly by the node and openzwave adds it to the supported_modes list.

I use DzVents to control the modes of the radiator valves and this works fine. However if I add the manufacturer specific mode manually in the ozwcache, the correct mode is recognized during startup and with a poll/refresh as well.

rrozema commented 3 years ago

It doesn't happen all the time. I saw 2 THERMOSTAT_MODE_SUPPORTED_REPORT messages coming in, both had a specifed length of 6 bytes, however the 1st was sometimes incorrect in that the last 2 bytes were sometimes missing/overwritten by the messages crc value, the 2nd received message was always correct.

waltervl commented 2 years ago

Still actual now OpenZwave is being deprecated and ZwaveJs2MQTT being promoted?

no-response[bot] commented 2 years ago

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.