Add default values back to the temperature measurement cluster #7810
Conversation
pullapprove
bot
requested review from
chrisdecenzo,
jelderton,
jmartinez-silabs,
LuDuda,
mspang,
saurabhst,
selissia and
woody-apple
ghost
mentioned this pull request
andy31415
changed the title
andy31415
changed the title
| @@ -25,8 +25,8 @@ limitations under the License. | |||
| <client tick="false" init="false">true</client> | |||
| <server tick="false" init="false">true</server> | |||
| <attribute side="server" code="0x0000" define="TEMP_MEASURED_VALUE" type="INT16S" writable="false" optional="false">measured value</attribute> | |||
| <attribute side="server" code="0x0001" define="TEMP_MIN_MEASURED_VALUE" type="INT16S" min="-38221" max="65535" writable="false" optional="false">min measured value</attribute> | |||
| <attribute side="server" code="0x0002" define="TEMP_MAX_MEASURED_VALUE" type="INT16S" min="-38221" max="65535" writable="false" optional="false">max measured value</attribute> | |||
| <attribute side="server" code="0x0001" define="TEMP_MIN_MEASURED_VALUE" type="INT16S" min="-38221" max="65535" writable="false" default="0x8000" optional="false">min measured value</attribute> | |||
There was a problem hiding this comment.
What section of the spedc are these min/max/default values defined? I tried to find them but somehow failed.
When looking at this, I would expect that min to be (INT16_max) and max to be (int16_min) so that they are obvious defaults and change. Having the same defaults for both min an max seems odd to me (extra odd having them specified in hex when the the min/max values are in integer).
Generally what confuses me is:
- 65535 does not look like a valid value for a signed int16 - looks like a off-by-one unsigned int16
- "-38221" looks very odd as well (-32768 seems the right value for signed int16_t)
- having both min/max equal and hex seems odd (the hex value, if treated as positive, seems just out of range of signed int16_t)
Would really like to understand the spec here.
There was a problem hiding this comment.
-27315 is the correct min value. Please update that as well.
There was a problem hiding this comment.
Max also needs to be updated 32766 and 32767. Or better yet, if supported, use hex for those.
There was a problem hiding this comment.
Yeah, this got pretty broken in #7224. The right min is -27313 (i.e. 0x954E - 65535 as a signed number) and the right max is 32766 for MinMeasuredValue, 32767 for MaxMeasuredValue.
There was a problem hiding this comment.
You are right. I have fixed that now.
|
This may also need a regen of files since it changes XML. |
| @@ -25,8 +25,8 @@ limitations under the License. | |||
| <client tick="false" init="false">true</client> | |||
| <server tick="false" init="false">true</server> | |||
| <attribute side="server" code="0x0000" define="TEMP_MEASURED_VALUE" type="INT16S" writable="false" optional="false">measured value</attribute> | |||
There was a problem hiding this comment.
This also had a default="0x8000" before, no?
There was a problem hiding this comment.
According to the spec there is no default value defined. The 0x8000 comes from the fact that:
A MaxMeasuredValue of 0x8000 indicates that the maximum value is unknown.
Thats why the defaults should be 0x8000 unless the "user space application" sets them to the correct values.
There was a problem hiding this comment.
I also looked in the old version of src/app/zap-templates/zcl/data-model/silabs/ha.xml and there the default value for measured value was 0x8000 but I cannot find any text which mentions this in the matter spec. Could this be a spec issue?
There was a problem hiding this comment.
Please make sure you are reading the most updated spec. That means after https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/3224/files (which is not merged in yet).
That get rids of all mention of 0x8000, because null is represented as TLV null in Matter, not as 0x8000.
But more to the point, it's not clear to me whether this is trying to restore the state things were in (which had 0x8000 as default value), or trying to align with the spec, which should happen after the spec is updated to make sense (minimally after the above-linked PR is merged).
There was a problem hiding this comment.
The idea of 0x8000 was that when the accessory boots up all the clusters have a well known state. If the accessory vendor for whatever reason forgets to populate this value. At least this was my understand of 0x8000 for now.
| @@ -25,8 +25,8 @@ limitations under the License. | |||
| <client tick="false" init="false">true</client> | |||
| <server tick="false" init="false">true</server> | |||
| <attribute side="server" code="0x0000" define="TEMP_MEASURED_VALUE" type="INT16S" writable="false" optional="false">measured value</attribute> | |||
| <attribute side="server" code="0x0001" define="TEMP_MIN_MEASURED_VALUE" type="INT16S" min="-38221" max="65535" writable="false" optional="false">min measured value</attribute> | |||
| <attribute side="server" code="0x0002" define="TEMP_MAX_MEASURED_VALUE" type="INT16S" min="-38221" max="65535" writable="false" optional="false">max measured value</attribute> | |||
| <attribute side="server" code="0x0001" define="TEMP_MIN_MEASURED_VALUE" type="INT16S" min="-38221" max="65535" writable="false" default="0x8000" optional="false">min measured value</attribute> | |||
There was a problem hiding this comment.
Yeah, this got pretty broken in #7224. The right min is -27313 (i.e. 0x954E - 65535 as a signed number) and the right max is 32766 for MinMeasuredValue, 32767 for MaxMeasuredValue.
Looks like we don't use these min/max values for anything yet, or at least the ZAP CI passes.... |
Min / Max Measured values should have spec compliend defaults.
…ct-chip#7810) * fix temperature measurement cluster Min / Max Measured values should have spec compliend defaults. * add proper min / max value ranges for temerature measurement cluster
Problem
#7224 moved the temperature measurement cluster from the silabs xml code to its own file. Somehow the default values went missing. This PR fixes this.
Change overview
Testing
Generated code locally with ZAP to see if fix applies defaults to entpoints_config.h
I would also appreciate if someone else would also test this change. Because it seems that ZAP does create a second SQL version of temperature measurement in the database and when changing to the version with defaults it does not remove the entry form the zap file and marks it only as disabled. Which can lead to confusions.