Part 3: feature/air pressure measurement cluster #7030

ghost · 2021-05-21T08:25:39Z

Problem

The matter project has only a pressure measurement cluster which is not suitable for air pressure. The value range is too big. To solve this we introduce a cluster which is specific to air pressure measurement.

Change overview

added support for zap tool
added emberAfAirPressureMeasurementClusterServerInitCallback support
added air pressure measurement cluster support to chip-device-clt
added air-pressure-measurement-server.cpp impl.

Testing

Tested on custom accessory with air pressure sensor. Test setup was similar to TE2.

HW Platform

Nordic nRF52840

Notes

Related PRs

bzbarsky-apple · 2021-05-21T21:57:38Z

This sounds like a spec issue, no? That is, the new cluster would need to be added to the spec.

@CamWms in case there is an existing ZCL cluster that is appropriate here.

ghost · 2021-05-25T08:13:51Z

@bzbarsky-apple It's not only the value range of pressure measurement. We have also added altitude to the air pressure measurement cluster to correlate the air pressure according to the height above / below see level.

bzbarsky-apple · 2021-05-25T15:17:40Z

Right, that all sounds like it needs to go into the cluster library spec.

src/app/clusters/air-pressure-measurement-server/air-pressure-measurement-server.cpp

src/app/zap_cluster_list.py

src/app/zap-templates/zcl/air-pressure-measurement.xml

vivien-apple · 2021-05-26T13:02:19Z

src/app/clusters/air-pressure-measurement-server/air-pressure-measurement-server.cpp

+                                      sizeof(*measuredValue));
+}
+
+static EmberAfStatus emberAfAirPressureMeasurementClusterSetAltitudeCallback(chip::EndpointId endpoint, int16_t altitude)


SetAltitude-> SetAltitudeValue ?

to be consisted with AIR_PRESSURE_MEASUREMENT_ALTITUDE I did not name it SetAltitudeValue. But I can change it on both sides if requried?

src/app/clusters/air-pressure-measurement-server/air-pressure-measurement-server.cpp

src/app/clusters/air-pressure-measurement-server/air-pressure-measurement-server.h

src/app/zap-templates/zcl/air-pressure-measurement.xml

Damian-Nordic · 2021-05-27T13:10:55Z

src/app/zap-templates/zcl/air-pressure-measurement.xml

+    <description>Air pressure measurement cluster in hPA with 1 digit fixed point resolution</description>
+    <attribute side="server" code="0x0000" define="AIR_PRESSURE_MEASUREMENT_MEASURED_VALUE" type="INT16U" min="8700" max="11000" writable="false" default="10132" optional="false">measured value</attribute>
+    <attribute side="server" code="0x0001" define="AIR_PRESSURE_MEASUREMENT_ALTITUDE" type="INT16S" min="-32768" max="32767" writable="true" default="0" optional="false">altitude</attribute>
+    <command source="client" code="0x0000" name="altitude" optional="false">


Do we really need a command to just update a single attribute? Can't we use the generic "ZCL write" message? In case we do, please rename it to "SetAltitude" to make it more consistent with other commands.

The problem is that the device cannot measure the altitude by itself. And the altitude measurement available on some devices is indeed using an air pressure sensor which derives the altitude from the air pressure value and data from the internet.
So the controller has to set the altitude to get correct air pressure readings. Typically this value can be auto-generated on the controller by getting it from some internet services based on the geo-location. If this is not possible then the User has to specify the altitude value.

@Damian-Nordic I have removed the command. The fact that the attribute is writable is sufficient.

vivien-apple

Approving for TE3.

It seems like the result of this PR is mostly to expose a set of accessors to facilitate the manipulation of the Binary input (Basic) cluster attributes.
It makes perfect sense to facilitate it imho, but I would rather prefer to autogenerate helpers code such as what is proposed in #7183, instead of having a lot of new clusters introduced into src/app/clusters.

examples/all-clusters-app/all-clusters-common/all-clusters-app.zap

src/app/zap-templates/templates/app/helper.js

bzbarsky-apple · 2021-05-28T21:09:49Z

src/app/zap-templates/zcl/air-pressure-measurement.xml

+  <cluster>
+    <domain>Measurement &amp; Sensing</domain>
+    <name>Air Pressure Measurement</name>
+    <code>0x0407</code>


This is the ZCL cluster id for the "Leaf Wetness" cluster. This needs a different ID. Please speak to @CamWms about one.

I'm in talks with @CamWms regarding this issue. Will update it when I've got the correct cluster id.

bzbarsky-apple · 2021-05-28T21:10:57Z

src/app/zap-templates/zcl/air-pressure-measurement.xml

+    <client init="false" tick="false">true</client>
+    <server init="false" tick="false">true</server>
+    <description>Air pressure measurement cluster in hPA with 1 digit fixed point resolution</description>
+    <attribute side="server" code="0x0000" define="AIR_PRESSURE_MEASUREMENT_MEASURED_VALUE" type="INT16U" min="8700" max="11000" writable="false" default="10132" optional="false">measured value</attribute>


The 8700 is suspiciously close to "the lowest we've measured in a hurricane recently". Is that really what the min should be in the spec? Do we ever expect one of these sensors to be in a tornado?

11000 seems to assume this will never be used in any sort of submersible?

Are you suggesting to remove the min / max values and just use the value range of the INT16U as min / max values? The min / max values where intended to indicate the possible value range of the atmospheric pressure. I would agree that maybe changing them a bit lower / higher would be beneficial to compensate some sensor measurement tolerances. So I would e.g. suggest 8000 for min and 12000 for max.

The intention of this cluster was to measure air pressure / atmospheric pressure. I don't know how this could be related to submersible applications? Could you provide an example.

src/app/zap-templates/zcl/air-pressure-measurement.xml

src/app/zap_cluster_list.py

- replace emberAfReadAttribute with emberAfReadServerAttribute - replace emberAfWriteAttribute with emberAfWriteServerAttribute - add emberAfAirPressureMeasurementClusterPrintln - let the caller do the logging

The default values are populated by the attribute storage.

The altitude attribute is writable so it is possible to write to the attribute without a command.

…allback

CamWms · 2021-06-01T14:37:41Z

I think that we need to keep the Pressure Measurement quite simple and generic. The client that gets pressure and altitude can do the calculation for the application solution, not the sensor. For pressure measurement we should use the pattern of the Concentration Measurement cluster (ZCL8) where we use the same cluster, but use Alias cluster IDs that add semantics to measurement: Air Pressure, Water Pressure, etc.

bzbarsky-apple · 2021-06-01T14:41:52Z

The min / max values where intended to indicate the possible value range of the atmospheric pressure.

Right, I understand the intent. I am asking:

What are the deployment situations expected for this cluster?
Do these values make sense given those deployment situations?

So I would e.g. suggest 8000 for min and 12000 for max.

Would you expect these sensors to be used in airplane cabins? If so, would you expect their altitude to be changed as the plane's pressurization setpoint changes, or would you expect them to read the "actual" pressure (which I understand can be below 800 millibars)?

I don't know how this could be related to submersible applications? Could you provide an example.

Air pressure sensor in a submarine, say. Which are generally kept at 1 atmosphere or so, but I'm not sure how much variation is within spec there.

ghost · 2021-06-01T15:37:16Z

We will withdraw the current air pressure measurement cluster for TE3, due to time constrains and some concerns state here. We will consider the pressure measurement cluster as suggested by @CamWms and see if this will fit our needs.

boring-cyborg bot added the examples label May 21, 2021

restyled-io bot mentioned this pull request May 21, 2021

Restyle Part 3: feature/air pressure measurement cluster #7031

Closed

This was referenced May 21, 2021

Part 4: feature/binary input basic cluster #7032

Merged

Part 2: feature/relative humidity measurement cluster #7028

Merged

Part 1: feature/temperature measurement cluster #7026

Merged

vivien-apple reviewed May 26, 2021

View reviewed changes

ghost marked this pull request as ready for review May 26, 2021 15:55

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst and woody-apple May 26, 2021 16:00

pullapprove bot added the review - pending label May 26, 2021

vivien-apple mentioned this pull request May 27, 2021

Zap add accessors helpers for attributes #7183

Merged

Damian-Nordic reviewed May 27, 2021

View reviewed changes

boring-cyborg bot added app controller darwin labels May 27, 2021

pullapprove bot requested a review from selissia May 27, 2021 14:19

vivien-apple approved these changes May 27, 2021

View reviewed changes

jmartinez-silabs approved these changes May 28, 2021

View reviewed changes

Damian-Nordic approved these changes May 28, 2021

View reviewed changes

bzbarsky-apple requested changes May 28, 2021

View reviewed changes

Dimitri Mizenko added 2 commits May 31, 2021 11:43

format endpointClusterWithInit for easier pull requests

c0be967

add air pressure measurement clsuter definition to zap tool

81bf8ab

Dimitri Mizenko and others added 21 commits May 31, 2021 11:43

add support for emberAfAirPressureMeasurementClusterServerInitCallback

425f476

add air pressure measurement cluster to controller-clusters.zap

97a66f4

add impl. for air pressure measurement cluster on the server side

442c6c1

Add new clusters to zap_cluster_list.py

d2ff9e1

Restyled by clang-format

27a13ea

apply fixes

310b630

- replace emberAfReadAttribute with emberAfReadServerAttribute - replace emberAfWriteAttribute with emberAfWriteServerAttribute - add emberAfAirPressureMeasurementClusterPrintln - let the caller do the logging

remove emberAfAirPressureMeasurementClusterServerInitCallback impl.

5e1e86b

The default values are populated by the attribute storage.

fix year in copyright

c01614e

remove unrelated clusters in zap_cluster_list.py

b6daa01

remove altitude command

d7082b4

The altitude attribute is writable so it is possible to write to the attribute without a command.

add air pressure measurement to all-cluster-app.zap

14cb669

fix includes in air-pressure-measurement-cluster.cpp

2b6f7b9

expose emberAfAirPressureMeasurementClusterSetAltitudeValue

7b326ee

try to fix all-cluster-app/esp32

4660f5b

another try to fix all-cluster-app/esp32

268ca65

move air pressure measurement cluster to endpoint 1

5418e13

disable generation of emberAfAirPressureMeasurementClusterServerInitC…

eefa01b

…allback

adapt description in air-pressure-measurment.xml

c587937

extend value range of min / max of air pressure measurement

70f5a63

regen all

22af391

Restyled by clang-format

fdbb482

restyled-io bot mentioned this pull request May 31, 2021

Restyle Part 3: feature/air pressure measurement cluster #7255

Closed

ghost closed this Jun 1, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part 3: feature/air pressure measurement cluster #7030

Part 3: feature/air pressure measurement cluster #7030

ghost commented May 21, 2021 •

edited by ghost

Loading

bzbarsky-apple commented May 21, 2021

ghost commented May 25, 2021

bzbarsky-apple commented May 25, 2021

vivien-apple May 26, 2021

ghost May 26, 2021

Damian-Nordic May 27, 2021

bauerschwan May 27, 2021

ghost May 27, 2021

vivien-apple left a comment

bzbarsky-apple May 28, 2021

ghost Jun 1, 2021

bzbarsky-apple May 28, 2021

ghost May 31, 2021

CamWms commented Jun 1, 2021

bzbarsky-apple commented Jun 1, 2021

ghost commented Jun 1, 2021

Part 3: feature/air pressure measurement cluster #7030

Part 3: feature/air pressure measurement cluster #7030

Conversation

ghost commented May 21, 2021 • edited by ghost Loading

Problem

Change overview

Testing

HW Platform

Notes

bzbarsky-apple commented May 21, 2021

ghost commented May 25, 2021

bzbarsky-apple commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivien-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CamWms commented Jun 1, 2021

bzbarsky-apple commented Jun 1, 2021

ghost commented Jun 1, 2021

ghost commented May 21, 2021 •

edited by ghost

Loading