search

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 916 forks source link

Thermostat Fan State - Convert to ValueList (and keep ValueString as backwards compatible) #1942

Open kdschlosser opened 5 years ago

kdschlosser commented 5 years ago

ok so this is the issue.

There are 2 versions of the thermostat fan state command class.. Here are the available states per version

version 2

Can you see the problem?

the problem is 0x1. It can either be Running or Low. This all depends on the device. if it supports only on and off. then it is going to report 0x1 as being on. but if it supports multiple speeds it is going to report 0x1 as Low. But with a single speed 0x1 is in fact not Low.. It is High. because the thing only has on and full speed. and because they did not add supported get and supported report there is no way to determine what label to provide to the user. I sent Silabs a message about it.

Now.. we can reduce the issue some by using THERMOSTAT_FAN_STATE_VERSION (0x1) so that the data list the value has is only going to return "Idle, and "Running" if the version is 1 and the full list of them for version 2.

There is no way to fix this other then setting the label to Low if the node ever reports being on high or medium speed. But this is not going to be 100% because if the user never changes the fan off of low speed it will never know. But at least the first addition will reduce the impact of the bug in the specification.

kdschlosser commented 5 years ago

I am making python-openzwave extensible so that addon modules can be coupled with it. I am working on an addon module that will spit out histogram data for energy use. So the user would be able to supply a max amount of energy consumed while running and the addon would monitor for changes to values to know the state and set the consumption according to the state and time stamp when the state changed to that and then time stamp when it ended.

This would allow an application to provide nifty features like bill estimation and also find out where energy is being used so they can reduce their consumption if they want.

petergebruers commented 5 years ago

Now.. we can reduce the issue some by using THERMOSTAT_FAN_STATE_VERSION (0x1) so that the data list the value has is only going to return "Idle, and "Running" if the version is 1 and the full list of them for version 2.

I see what you mean. But. The spec actually says this for V1 and V2. According to this, you cannot know for sure if V1 state 2 aka "Running High" exists without looking at the manufacturer's device manual/specification. So returning the full list for V2 seems correct. But --only going to return "Idle, and "Running" if the version is 1-- as you write seems incorrect to me...

Screen Shot 2019-09-16 at 09 58 07
kdschlosser commented 5 years ago

i believe that command class list to be incorrect. I can't seem to remember for the life of me why that is. I know I saw something that conflicted It should be in the specification somewhere. I would have to go poking about it...

I think I remember what it is.

let me go and look.

kdschlosser commented 5 years ago

yes that is it..

https://www.silabs.com/documents/login/software/zw-classcmd-20171229.zip

download the definition files. and in the header you will see this for the fan states.


/* Thermostat Fan State command class commands */
#define THERMOSTAT_FAN_STATE_VERSION                                                     0x01
#define THERMOSTAT_FAN_STATE_GET                                                         0x02
#define THERMOSTAT_FAN_STATE_REPORT                                                      0x03
/* Values used for Thermostat Fan State Report command */
#define THERMOSTAT_FAN_STATE_REPORT_LEVEL_FAN_OPERATING_STATE_MASK                       0x0F
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_IDLE                             0x00
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_RUNNING                          0x01
#define THERMOSTAT_FAN_STATE_REPORT_LEVEL_RESERVED_MASK                                  0xF0
#define THERMOSTAT_FAN_STATE_REPORT_LEVEL_RESERVED_SHIFT                                 0x04

/* Thermostat Fan State command class commands */
#define THERMOSTAT_FAN_STATE_VERSION_V2                                                  0x02
#define THERMOSTAT_FAN_STATE_GET_V2                                                      0x02
#define THERMOSTAT_FAN_STATE_REPORT_V2                                                   0x03
/* Values used for Thermostat Fan State Report command */
#define THERMOSTAT_FAN_STATE_REPORT_LEVEL_FAN_OPERATING_STATE_MASK_V2                    0x0F
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_IDLE_V2                          0x00
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_RUNNING_V2                       0x01
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_RUNNING_HIGH_V2                  0x02
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_RUNNING_MEDIUM_V2                0x03
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_CIRCULATION_V2                   0x04
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_HUMIDITY_CIRCULATION_V2          0x05
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_RIGHT_LEFT_CIRCULATION_V2        0x06
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_UP_DOWN_CIRCULATION_V2           0x07
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_QUIET_CIRCULATION_V2             0x08
#define THERMOSTAT_FAN_STATE_REPORT_LEVEL_RESERVED_MASK_V2                               0xF0
#define THERMOSTAT_FAN_STATE_REPORT_LEVEL_RESERVED_SHIFT_V2                              0x04

now you see why I think that table is a typo. these are the only 2 defines for version 1 for the state


#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_IDLE                             0x00
#define THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_RUNNING                          0x01```
kdschlosser commented 5 years ago

I have an issue opened with silabs about this problem I am hoping to hear from them soon. so we will know 100% what way it is supposed to be. But for the nodes that are version 2 it is still a crap shoot about the fan state. I do not know when the update was made to the specification for version 2. I also sent in a request to give the thermostat command classes another revision.

they added multistage support but only for reporting and not for the user being able to set it. and it does not support multi speed fans either. both of which have been around for 30+ years and are common place. I personally have a modulation furnaces with an EC fan and a modulating split system (central air) the furnace has variable btu output from 20% to 100% the fan is variable from 20% to 100% and the air conditioning is variable from 40% to 100% all of them adjustable in 1% increments.

There are no zwave thermostats made to handle this.. the reason is there is no specification for it. But 1000's of systems like this get installed daily and simply get operated in multi stage instead of variable. and this technology is close to 20 years old..

petergebruers commented 5 years ago

I see, that table is from latest spec and indeed in SDK 7.11 there is (still) only state 0x02 defined as THERMOSTAT_FAN_STATE_REPORT_FAN_OPERATING_STATE_RUNNING_HIGH_V2 suggesting indeed V1 only supports 0 and 1. May not solve the problem at all if someone implemented a 2 or 3 speed using V2 of the CC because then it is still ambiguous. Don't know much about modern heat/cool system except that some have a (dedicated) TCP/IP interface or some offer a cloud service... Never seen much talk about fan speed control on the Fibaro forum, poor man's solution seems "multiple relays".

kdschlosser commented 5 years ago

can't do anything with multiple relays in my furnaces. they have DC brushless motors with electronic controllers for the "pulsing" to make them move. It's actually kind of cool how they work. the center of the motor is typically the part that spins. on mine it's the opposite. the center is fixed and the outside spins. on a standard furnace the fan is 120 volt if you live in the US otherwise I am sure it is 240 volt. but either way they can make those also change speed. the issue is the more you slow them down the less efficient they get. where as with the motor in mine it gets more efficient as it slows down. and because of the electronic speed control there can be an infinite number of speeds.

Motors like this are not new. they simply were not efficient before. the speed controllers would blow off the excess energy in the form of heat. Now because of the advent of PWM there is really close to 0 loss. you also don't have the loss from the friction caused by the brushes and the heat is also gone. they run much cooler and this improves efficiency as well. The furnaces I have are forced hot air and use gas. the flue pipe for the exhaust is ABS plastic that is how cool the exhaust is by time it leaves the furnace. the connections between furnace, air conditioner and thermostat if all 3 have the ability they connect to each other by use of 4 wires (rs485) and most vendors use the ClimateTalk protocol. I have already started work on making an interface using an Arduino simply because there is not a single thermostat made that allows access to the variable speed features of the system. it only allows for 2-3 pre defined stages. I have not checked yet but I do not think that one exists yet. I know they make a ZigBee module for the Arduino. I am not 100% sure about Z-Wave. I have not really looked into it because the zwave protocol doesn't offer the control I want to have anywho.

What is really batty is when they wrote version 2 they added control for multiple stages for the fan. but not for the operating mode. But they allow the thermostat to report it's operating mode in stages... I am thinking that no one asked the thermostat manufacturers what should be added.

I am not sure what the specific requirements for the Thermostat command classes are. But there has to be something that would be stopping a manufacturer from adding the multi channel command class and the switch multilevel command class to offer variable control.

kdschlosser commented 5 years ago

OK so this is the answer I got..

The header file is wrong the table is right. there is a High for version 1.

I also mentioned the whole high thing and asked how to determine the fan states without the cc having the supported get and report commands. and this is what I was told. which is a bit sideways from how I thought the specification was supposed to be..

In order to determine what fan states can be reported you use the information contained within THERMOSTAT_FAN_MODE_SUPPORTED_REPORT which is from COMMAND_CLASS_THERMOSTAT_FAN_MODE and not COMMAND_CLASS_THERMOSTAT_FAN_STATE..

I am going to assume that this same kind of thing is done between COMMAND_CLASS_THERMOSTAT_MODE and COMMAND_CLASS_THERMOSTAT_OPERATING_STATE as well.

It would appear as tho the command classes do not have the ability to operate independently of one another and are woven into each other in a manner that makes you use information from one CC in another. It would have been nice of them to have in the documentation this information. a simple one liner of "In order to determine the supported states you need to use this command classes supported get and report."

But at least there is now a way of properly determining what the states/modes should be.

kdschlosser commented 4 years ago

This is the response i got from Silicon Labs about the fan state.

Table 137 is correct, it seems the header files did not included the running high fun operating state in v1.

If the device support Thermostat Fan State CC, it will also support Thermostat Fan Mode CC. wouldn't it be easy to get the operating Fan mode using Thermostat Fan Mode CC (it support

Thermostat Fan Mode Supported Get/report command). Then based on the fan mode, the UI can show the fan operating state of the thermostat.

I did not think that the specification was designed to be used in that manner and that each command class was supposed to be able to operate in isolation from one another. I guess I am incorrect in that thought process. So in order to determine what the reported states are going to be you need to use the information from the fan mode command class...

So basically to sum it up any command class that requires another command class as per the specification can also "borrow" data from that class to collect information from the node. in order to determine what may or may not be returned so that an application can correctly display information to the user.

Fishwaldo commented 4 years ago

You need to consult the Device Types and Roles. They dictate what CC’s are mandatory/optional for each device function. This is part of the Specs

kdschlosser commented 4 years ago

is this something that should be fixed in openzwave or let the application deal with it?

Fishwaldo commented 4 years ago

It’s already handled in OZW. Was just pointing it out as a reference.

kdschlosser commented 4 years ago

so in openzwave the value for the Fan State populates the data list from Fan Mode??. I probably didn't see the code for that.

Fishwaldo commented 4 years ago

No. They are all self contained. But if you want to know the relationships you can use the Device Class/roles definitions.

kdschlosser commented 4 years ago

OK so in order to produce the correct label for Fan State I am going to have to get the data list from the manager for the fan mode value. check and see if only a single speed is in the list. and if that is the case then return Running High this is because the thermostat only supports 2 states.. off and full speed (on). otherwise return Running Low. If the underlying value byte is 0x01

I am trying to do energy calculations and this is hard to do because I am going to have to base it on the string value that is returned because thermostat fan state is not a list value it is a string value. Is this OK to do? are the returned strings hard coded into openzwave? or is there some mechanism to change them??

It would be nice if the fan state value was a list value and handled loading the value list based on what is reported by thermostat mode as suggested by Silicon Labs. because of the internationalism support in openzwave it is not a good thing to do equality testing against a string. I do not know of another way to go about getting some other type of data object for a string value one that I can use to do the equality testing and make any replacements that are needed.

In ThermostatFanState.cpp these are the strings that can be returned.

So the only way I can go about replacing that label is I have to use Manager::GetValueListValues passing the value id of the value for the thermostat fan mode. This will return the byte values and I can use that to create a correct set of returned strings. But I am still going to have to do equality testing on the string returned from openzwave for the fan state value to know if i need to replace it..

does that make sense??

There is also a block in the code in ThermostatFanMode.cpp in the ReadXML method.

if (index > 6) /* size of c_modeName excluding Invalid */
                                    {
                                        Log::Write(LogLevel_Warning, GetNodeId(), "index Value in XML was greater than range. Setting to Invalid");
                                        index = 7;
                                    }

this is going to prevent any control of the fan to circulation and lower.

Fishwaldo commented 4 years ago

Summary - Convert ThermostatFanState ValueID to a List.

kdschlosser commented 4 years ago

I wanted to update this with the last message I got from silabs. I am correct and there is a problem with respect to this in the specification. I am also correct in that the command classes should operate independent of one another and the way OZW is set up so the cc's work independent is the correct thing to do.

But....There is no way to determine the state of the fan if it is running on Low speed or running on High speed or Running on High speed if it is a single speed fan unless the THERMOSTAT_FAN_STATE cc uses the information from the THERMOSTAT_FAN_MODE_SUPPORTED_REPORT to determine what fan modes are supported. By the API specification this is not the "correct" way and in most cases doing something like that could have the potential of breaking... In this case it would not break. because the specification states that if a device supports the THERMOSTAT_FAN_MODE cc it must also implement the THERMOSTAT_FAN_STATE cc. and the the THERMOSTAT_FAN_MODE_SUPPORTED_GET and THERMOSTAT_FAN_MODE_SUPPORTED_REPORT have been available since version 1 of the specification.

I left the decision of submitting a request to update the specification to the person I was talking with. Because there is a way to work around the issue and it will always function and cannot break I feel this work around can be used. If it is used I would make sure that there is enough comments outlining what is taking place and why.

They have to make the call on whether or not it is something that should be corrected in the specification. I believe it should be for future devices and to keep the API flow like it should be and keeping the command classes as self contained "modules" that are not dependent on any other command class. This is a decision they will have to make.