Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thermostat setpoint - can only set integer values with Qubino ZMNHID1 #776

Open
stephaneguillard opened this issue Feb 14, 2016 · 24 comments

Comments

@stephaneguillard
Copy link

Hello,

Using OZW control panel:

  • when I enter an integer value (e.g. '22'), it is properly accepted as the new setpoint:

http://s.guillard.free.fr/Domoticz/22.jpg

  • when I enter a value with a decimal place (e.g. '22.5'), it is not properly set, and the thermostat defines a new erroneous integer value:

http://s.guillard.free.fr/Domoticz/22.5.jpg

Here the setpoint was 22; I input '22.5', and the new setpoint is now 49

All of my 4 Qubino ZMNHID1 behave the same.

I have looked into the OpenZWave config file, and "Heating 1" is defined as "decimal":

The ZMNHID1 documentation states (exact text copied below):
"
COMMAND_CLASS_THERMOSTAT_SETPOINT
The [...] thermostat supports temperature setpoint, which is 2 bytes long, scale is °C and precision is 1 (it means 0.1°C)
"

Is the bug in OZW, or in the ZMNHID1 ?

Regards,

Stéphane

@Fishwaldo
Copy link
Member

Can you provide logs please?

@stephaneguillard
Copy link
Author

Sure, I want to get this to work.

Problem is that there is no trace relative to this issue in OZW_log.txt.

Here is the content of options.xml, so that you may tell me what to adjust if needed:

<?xml version="1.0" encoding="utf-8"?>
<!-- To be effective, this file should be placed in the user data folder specified in the Options::Create method -->
<Options xmlns='http://code.google.com/p/open-zwave/'>
  <Option name="logging" value="true" />
  <Option name="Associate" value="true" />
  <Option name="NotifyTransactions" value="false" />
  <Option name="DriverMaxAttempts" value="5" />
  <Option name="SaveConfiguration" value="true" />
  <!-- <Option name="RetryTimeout" value="40000" /> -->
  <!-- If you are using any Security Devices, you MUST set a network Key -->
  <!-- <Option name="NetworkKey" value="0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x10" /> -->

  <!-- Some Devices have a big UserCode Table, that can mean startup times
  when refreshing Session Variables is very long (Especialy on Security
  Devices) This option will make the UserCode CC stop on the first
  "available" usercode slot rather than retrieve every one -->
  <Option name="RefreshAllUserCodes" value="false" />
  <Option name="ThreadTerminateTimeout" value="5000" />
</Options>

Let me add that this is using OpenZWave 1.4-34-gb426e49 static-linked to the current domoticz beta's.

Regards,
Stéphane

@Fishwaldo
Copy link
Member

I believe you have to make a adjustment to Domoticz to enable OZW logging. You would have to ask them, as I don't know how.

@stephaneguillard
Copy link
Author

Here we go, below is the log file showing the issue:

OZW_Log.zip

  • line 2760: the setpoint was 22C, I try to change it to 22.5:

2016-02-18 19:56:24.572 Info, Node007, Value::Set - COMMAND_CLASS_THERMOSTAT_SETPOINT - Heating 1 - 1 - 1 - 22.5

  • line 2949: the setpoint update returned by the module is 49C:

2016-02-18 19:56:34.724 Info, Node007, Received thermostat setpoint report: Setpoint Heating 1 = 49C

This corresponds exactly to the behaviour described in my 1st post above.

There is a strange side effect to this problem: the setpoints for either this thermostat or the others, randomly change unit from C to F, with a decimal place. You may see that at line 3121: the setpoint of node 008 was 22C, it is now... 22.0F !

Let me know how I may help.

@stephaneguillard
Copy link
Author

Justin,

Did you find time to look into the log?

Regards,
Stéphane

@Fishwaldo
Copy link
Member

I will soon. Sorry, been on a rather insane travel schedule these few weeks.

@stephaneguillard
Copy link
Author

Hello Justin,

Worry not, I'm also quite overloaded these days.

@stephaneguillard
Copy link
Author

Hello Justin,

Trying to get this moving, I have investigated the observed behaviour vs. the code, and done some googling on the thermostat setpoint data frame (which doesn't return much anyway).

What is the reasoning behind this line in ThermostatSetpoint.cpp/bool hermostatSetpoint::HandleMsg():

        value->SetUnits( scale ? "F" : "C" );

Is this assuming that if scale is not null, then the setpoint is in F, a ZW-standard conformant behaviour?

Regards,
Stéphane

@Fishwaldo
Copy link
Member

Unlikely to be a bug… scale is decoded from the packet sent by the device…..

From: Stéphane Guillard [email protected]
Reply-To: OpenZWave/open-zwave [email protected]
Date: Tuesday, 29 March 2016 at 12:30 AM
To: OpenZWave/open-zwave [email protected]
Cc: Justin Hammond [email protected]
Subject: Re: [open-zwave] Thermostat setpoint - can only set integer values with Qubino ZMNHID1 (#776)

Hello Justin,

Trying to get this moving, I have investigated the observed behaviour vs. the code, and done some googling on the thermostat setpoint data frame (which doesn't return much anyway).

What is the reasoning behind this line in ThermostatSetpoint.cpp/bool hermostatSetpoint::HandleMsg()
value->SetUnits( scale ? "F" : "C" );

Is this assuming that if we get a scale, then the setpoint is in F? I tend to think this is wrong from my experiments with the Qubino.
Regards,
Stéphane


You are receiving this because you commented.
Reply to this email directly or view it on GitHub

@stephaneguillard
Copy link
Author

Hello Justin,

There is definately something wrong. When I change any of my 4 Qubino's setpoints using the OZW control panel, even to an integer °C value, they randomly (but frequently) return an aberrant decimal °F value. Example like in my top post: I switch from 19 to 20C; the module returns -6.8F.

It is very difficult to diagnose on my house system, as when it occurs, it takes a lot of delete zwcfg/reboot cycles before they all revert to °C setpoints, meanwhile the house heating behaves randomly.

So I have bought a spare Zstick and a spare Qubino thermostat, which I will set up as a troubleshooting platform.

I will let you know when I have found something.

Regards,
Stéphane

@stephaneguillard
Copy link
Author

Hello Justin,

Another try:

  • the thermostat setpoint report frame received from the Qubino by Driver.cpp/ReadMsg() is:

0x01 0x0E 0x00 0x04 0x00 0x08 0x08 0xx43 0x03 0x01 0x34 0x00 0x00 0x00 0xDC 0x5C

I have manually traced it through ReadMsg() -> ProcessMsg() -> HandleApplicationCommandHandlerRequest() -> ApplicationCommandHandler() -> ThermostatSetpoint.cpp/HandleMsg() -> CommandClass.cpp/ExtractValue()

And this is where there seems to be something wrong:

  • CommandClass.cpp/ExtractValue()'s _data[0] input buffer starts at the original frame's offset 10, which is 0x34
  • ExtractValue() computes 'size' = _data[0] & c_sizeMask = 0x34 & 0x07 = 4
  • ExtractValue() computes 'precision' = (_data[0] & c_precisionMask) >> c_precisionShift = (0x34 & 0xe0) >> 0x05 = 1
  • ExtractValue() computes 'scale' = (_data[0] & c_scaleMask) >> c_scaleShift = (0x34 & 0x18) >> 0x03 = 2
  • then ExtractValue() computes 'value' using '_valueOffset' which from ExtractValue() prototype, is said to be

uint8 _valueOffset // = 1

Note the // = 1 comment. As far as I can see, _valueOffset is used without being initialized, since ThermostatSetpoint.cpp/HandleMsg() calls ExtractValue() with only 3 parameters :

        string temperature = ExtractValue( &_data[2], &scale, &precision );

and _valueOffset is the 4th parameter of ExtractValue() prototype :

string CommandClass::ExtractValue
(
uint8 const* _data,
uint8* _scale,
uint8* _precision,
uint8 _valueOffset // = 1
)

Thus these lines from ExtractValue() can't work if they assume _valueOffset to be 1 while this variable hasn't been set:

uint32 value = 0;
uint8 i;
for( i=0; i<size; ++i )
{
    value <<= 8;
    value |= (uint32)_data[i+(uint32)_valueOffset];
}

Which would pretty much explain why I get random thermostat setpoint report values?

Regards,
Stéphane

@Fishwaldo
Copy link
Member

Check the prototype in the header file. That's where the default value is set if it's not passed.

@stephaneguillard
Copy link
Author

Hello Justin,

All right, as a primarily C coder, I'm not so much used to not feeding all parameters to functions and relying on const presets for others. I checked the prototype and indeed the default is 1.

But I went on manually decoding my frame. ExtractValue() receives 0x34 0x00 0x00 0x00 0xDC, which turns out to be:

  • size=4
  • precision=1
  • scale=2
  • value=0x000000DC=0xDC=220; given that precision is 1, then value is 22.0 (which is what it should be: 22.0°C)

Problem is that as scale is 2 (and not 0), ThermostatSetpoint.cpp/HandleMsg() assumes that this value is in F and not in C, which is obviously wrong in our case.

This is the issue I was suspecting in an earlier post above.

I've googled and found similar issues with other thermostats and other zwave stacks (zwave.me seems to have similar issues).

So I guess that the assumption value->SetUnits( scale ? "F" : "C" ) in ThermostatSetpoint.cpp/HandleMsg() doesn't [always] work; at least for the Qubino it doesn't.

Is there a reference documentation somewhere as to how to interpret the scale value? in which case, what does '2' mean?

For the record, here is what I found at https://community.smartthings.com/t/integration-of-z-wave-danfoss-thermostat/23310/12 :

def cmdScale = cmd.scale == 1 ? "F" : "C"

This one would give me C rather than F with my scale == 2 :)

Regards and thank you for your continued/appreciated support,
Stéphane

@stephaneguillard
Copy link
Author

Hooray!

I build an OZW+domoticz with just the change below in ThermostatSetpoint.cpp:

value->SetUnits(scale ? "F" : "C");

changed to:

value->SetUnits(scale == 1 ? "F" : "C");

And this not only removes all those nasty C to F + aberrant setpoint value issues,

But also, allows me to input and set decimal setpoints like 20.1! and it just works.

There is still one (minor) issue, which I'll be running after now: in OZWCP, in order to set 20.1, I have to enter 20.10 otherwise the input value is rounded to 20. Justin, could you hint me where to look at? (I suspect wrong decimal or precision encoding or precision in ozw).

Regards,
Stéphane

@stephaneguillard
Copy link
Author

Okay I've also found the cause for this last one.

When entering a setpoint with one decimal place, like 22.5, OZW rightly turns that into an integer (225) and a 'precision' field of 1, telling the receiving thermostat that the last 1 digit in the integer is decimal part.

Then OZW rightly checks that 225 as an integer can be transmitted with a single byte, so size == 1.

Problem, the Qubino explicitly requires 2 bytes for setpoints : http://doc.eedomus.com/files/Qubino_Flush_on_off_thermostat-PLUS_user-manual_V1.1_eng-3.pdf says:

COMMAND_CLASS_THERMOSTAT_SETPOINT
The Flush on/off thermostat supports temperature set point, which is 2 bytes long, scale is °C and its precision is 1 (it means 0,1°C).

So we have a case where:

  • OZW righty determines that the setpoint can be sent using a single byte,
  • but the thermostat wants 2 bytes and ignores the 1 byte setpoint message

Which I temporarily hack-fixed in CommandClass.cpp/ValueToInteger() by changing:

if( ( val & 0xffffff00 ) == 0 )
 {
    *o_size = 1;
}

to:

if((( val & 0xffffff00 ) == 0 ) and (precision != 1))
{
    *o_size = 1;
}

This works fine, from domoticz and from OZWCP I can now successfully set any 1-decimal place setpoint like 22.2, 19.3 etc.

Of course it can't stay that way.

I will ask Qubino why their module ignores 1-byte value setpoints.

Justin, what is your take on this?

Regards,
Stéphane

@Fishwaldo
Copy link
Member

For the Scale Issue - There is no Value Defined for 2. Its only 0 - F and 1 - C. I suspect a Encoding Error here when Qubino send the message (they send 0b10 rather than 0b01 for C) - A few other vendors have got this messed up. I'll add a workaround for that soon. (#819)

For the Precision Problem - This is a bug from the device. If they want 2 decimal places, they should set precision to 2. Again, another work around is required. (#820)

@stephaneguillard
Copy link
Author

Hello Justin,

Glad you finally found time to catch up:)

For the C/F issue, may I suggest to keep it simple and just apply the following change to ThermostatSetpoint.cpp:

value->SetUnits(scale ? "F" : "C");

changed to:

value->SetUnits(scale == 1 ? "F" : "C");

This fixes the issue with ZMNHID1, while leaving the spec-conformant modules unaffected; this is the solution applied in other ZWave stacks.

For the precision/encoding issue, I dare disagree :) the problem is not an issue with precision/decimal places (ZMNHID1 accepts 0, 1 or 2 decimal places), but rather in the fact that ZMNHID1 requires at least 2 bytes when there is at least 1 decimal place for the equivalent integer value, even when the value can be encoded with 1 byte.

Examples:

  • 25 has precision 0, and can be encoded with a single byte, which is how OZW sends it. ZMNHID1 is OK with that.
  • 25.5 has precision 1, and can be encoded with a single byte, which is how OZW sends it. For whatever reason, ZMNHID1 doesn't accept decimals encoded with a single byte, it wants at least 2 byte so it fails.
  • 25.6 has precision 1, and can_not_ be encoded with a single byte but requires 2 (0x01 0x00), which is how OZW sends it. ZMNHID1 is OK with that.
  • 2.55 has precision 2, and can be encoded with a single byte, which is how OZW sends it. For whatever reason, ZMNHID1 doesn't accept decimals encoded with a single byte, it wants at least 2 byte so it fails.
  • 2.56 has precision 2, and can_not_ be encoded with a single byte but requires 2 (0x01 0x00), which is how OZW sends it. ZMNHID1 is OK with that.

So what we have is OZW encoding with a single byte all values with an integer equivalent below or equal 255, regardless of precision. But ZMNHID1 rejects single byte setpoints except when precision == 0.

Why is beyond me but thats how it works.

So the workaround I implemented is to force OZW to encode with 2 or 4 bytes (but not 1) when precision >= 1. This works, but cannot stay since it doesn't make much sense to generalize such a limitation just for the ZMNHID1...

Bottom line, I don't know a smart way to fix this, except maybe to create an exception list in OZW, which would for instance forbid 1 byte value encoding when talking to a ZMNHID1 thermostat...

Regards,
Stéphane

@Fishwaldo
Copy link
Member

Try putting:

in the device config file and lets see what happens....
On Fri, 2016-04-08 at 08:37 -0700, Stéphane Guillard wrote:

Hello Justin,
Glad you finally found time to catch up:)
For the C/F issue, may I suggest to keep it simple and just apply the
following change to ThermostatSetpoint.cpp:
value->SetUnits(scale ? "F" : "C");
changed to:
value->SetUnits(scale == 1 ? "F" : "C");
This fixes the issue with Qubino's, while leaving the spec-conformant
modules unaffected; this is the solution applied in other ZWave
stacks.
For the precision/encoding issue, I dare disagree :) the problem is
not an issue with the decimal places (they want one, but also accept
2), but rather in the fact that they require 2 bytes, even when the
value can be encoded with 1 byte, regardless of decimal places.
Examples:
22 has precision 0, and can be encoded with a single byte, which is
how OZW sends it. Qubino is OK with that.
22.5 has precision 1, and can be encoded with a single byte, which is
how OZW sends it. For whatever reason, Qubino doesn't accept decimals
encoded with a single byte, they want 2 byte so it fails.
25.5 has precision 1, and can be encoded with a single byte, which is
how OZW sends it. For whatever reason, Qubino doesn't accept decimals
encoded with a single byte, they want 2 byte so it fails.
25.6 has precision 1, and cannot be encoded with a single byte but
requires 2 (0x01 0x00), which is how OZW sends it. Qubino is OK with
that.
2.55 has precision 2, and can be encoded with a single byte, which is
how OZW sends it. For whatever reason, Qubino doesn't accept decimals
encoded with a single byte, they want 2 byte so it fails.
2.56 has precision 2, and cannot be encoded with a single byte but
requires 2 (0x01 0x00), which is how OZW sends it. Qubino is OK with
that.
So what we have is OZW encoding with a single byte all values with an
integer equivalent below or equal 255, regardless of precision.
Qubino rejects single byte setpoints except when precision == 0.
Why is beyond me but thats how it works.
So the workaround I implemented is to force OZW to encode with 2 or 4
bytes (but not 1) when precision == 1. This works, but cannot stay
since it doesn't make much sense to generalize such a limitation just
for the Qubino's...
Bottom line, I don't know a smart way to fix this, except maybe to
create an exception list in OZW, which would for instance forbid 1
byte value encoding when talking to a Qubino ZMNHID1 thermostat...
Regards,
Stéphane

You are receiving this because you commented.
Reply to this email directly or view it on GitHub

@Fishwaldo
Copy link
Member

looking at your decode above, its a truely messed up device.

I thought maybe we could store the size param we receive from the device, and use that that when sending a value, but they send as 4 bytes with precision of 1, and then expect a 2 byte set message....

Rant Mode on: everything these guys do is inconsistent. They have other multiinstance devices that accept encapsulated messages, but send raw reports back (so we can't determine what instance sent the report) - and they then go and say the controller should implement MultiInstanceAssoications and you associate with different instances on the controller, when all they have to do is encapsulate the report back to the control in a MultiInstance class, just like they receive a SET message, and everything is hunky dory. Just like every other vendor on the market does!

@Fishwaldo
Copy link
Member

btw, the override_precision is going to mess up your REPORT messages, but it should fix the SET messages. if that works out, then I can put in a config option for these fubar devices and go from there

@nechry
Copy link
Member

nechry commented Apr 28, 2016

@stephaneguillard do you wrote a device xml file for the module?
I don't have this device but I'm also fight with this device for a jeedom user.

@Fishwaldo Fishwaldo mentioned this issue May 16, 2016
@stephaneguillard
Copy link
Author

Hello Justin,

The problem does not lie in OZW, but in the fact that the module doesn't accept 1-byte encoding, regardless of precision.

As said already, any setpoint, regardless of precision, which integer equivalent value is <=256, will be encoded as a single byte value by OZW. This is perfectly valid, but the Qubino will ignore the setpoint message.

What I did for my workaround patch is not to tweak precision, but to force OZW to send 2 bytes even when the integer value is <=256.

I have no idea how to handle this adequately, other than ask qubino to fix their firmware, which wont happen...

At least you could patch the °C/°K issue in the way I suggested, as this works and is harmless for other appliances.

nchery: I have made a working XML file for this device. I don't remember if I commited it to OZW github; as a matter of fact, it doesn't seem to be in the config/qubino directory...

@jotakar
Copy link

jotakar commented May 31, 2016

when will this change be incorporated to a OZwave update?
I talk about:
value->SetUnits(scale ? "F" : "C");
changed to:
value->SetUnits(scale == 1 ? "F" : "C");

in ThermostatSetpoint.cpp file.

@Fishwaldo
Copy link
Member

@stephaneguillard Did you do any testing with my suggestion above? if you can try that, and post the log file, I know how to add this fix to OZW via a Config option.... until then...

Regarding precision - As I already pointed out, I will take a different approach. (I have been told that Z-Wave supports reporting in kelvin's - although I've never seen a device do/need kelvins...)

The correct fix is to put in a config option that says these devices send the scale as a bitmap rather than a byte (or visa versa)

As for when:
http://www.michaelbromley.co.uk/blog/529/why-i-havent-fixed-your-issue-yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants