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

Add Some field for Air Quality sensor. #6673

Closed
wants to merge 6 commits into from

Conversation

Smanar
Copy link
Collaborator

@Smanar Smanar commented Jan 14, 2023

Already present

  • "state/airqualityppb" as int
  • "state/airquality" as string
  • state/pm2_5 as number

Add :

  • "state/airqualityco2"
  • "state/airqualityformaldehyd"

@manup
Copy link
Member

manup commented Feb 20, 2023

I'm no expert with all these air quality sensor values :) Everybody ok to merge this?

@Smanar
Copy link
Collaborator Author

Smanar commented Mar 5, 2023

And there is a PR already merged that need this PR to work.

@ebaauw
Copy link
Collaborator

ebaauw commented Mar 5, 2023

See also #5351 (comment).

I think this is getting messy real quickly, with all new sort of sensors popping up. I'm kinda ashamed to admit that I've been actively contributing to this mess.

I feel like we should (have) adopt(ed) a naming scheme, like airquality_pollutant_density for pollutants reported as density (in µg/m³) and airquality_pollutantlevel, for pollutants reported as level (in ppb). Note that the conversion factor between density and level depends on (the specific weight of) the pollutant (and the ambient temperature and air pressure).

So I would prefer airquality_co2_level and airquality_formaldehyd_density. And I would like to deprecate pm2_5 and airquality_ppb in favour of airquality_pm2_5_densityand airquality_tvoc_level.

I also have a number of concerns for airquality itself, but I'm not sure how best to address these:

  • We might want to indicate on which pollutant the qualification is based.
  • Different pollutants have different thresholds; if we have different qualifications per pollitant (airquality_tvoc vs airquality_pm2_5, we could maintain the thresholds in the airquality_pollutantitem.json files. Otherwise, it would be in the device.json files.
  • It's a jungle out there: different national authorities use different numbers of qualifications, different labels, and different thresholds for the same pollutant;
  • Most devices I've seen report either six (0-5 stars or leaves) or three (green, amber, red) labels.

I think, instead of using a label, I would prefer to standardise on a numeric value from 0 (worst) to 5 (best), or use something like capabilities/airquality/max to indicate the max (best) value.

We also need to figure out how to deal with devices measuring multiple pollutants:

  • Use a separate ZHAAirQuality resource per pollutant, each with their own airquality (where appropriate). This seems natural for the VINDSTRYKA, as it reports PM2.5 and tVOC on different Zigbee clusters (in line with ZCL spec v8);
  • Use a single ZHAAirQuality resource, combining all pollutants. This leaves ambiguous on which pollutant airquality is based. Unless, of course, we would use airquality_pollutant.

I now have four Air Quality sensors: the Eve Room, the Aqara tVOC sensor, the IKEA STARKVIND Air Purifier, and the IKEA VINDSTYRKA. They're worse than themperature sensors: even where they (allegedly) measure the same pollutant, they report different values when placed next to each other.

@ebaauw
Copy link
Collaborator

ebaauw commented Mar 5, 2023

Oh, "formaldehyde" ends with an e. I think it is a volatile organic compound, see https://en.wikipedia.org/wiki/Formaldehyde.

Maybe use the slightly inaccurate, but more generic airquality_tvoc_density instead?

@SwoopX
Copy link
Collaborator

SwoopX commented Mar 5, 2023

Gents, that is really getting a bit fugly 🙂

We should keep all those readings in a very generic sensor type, something like ZHAMeasurement, in my view. It should expose an item unitofmeasurement, the raw value and a scaled value maybe?

It should be applicable at least for cluster 0x0042, but could be applied for temperature, humidity, airquality, etc.
It would also allow to introduce some transition time to iron out sensor mistakes made previously, by exposing the old and "new" one in parallel.

@ebaauw
Copy link
Collaborator

ebaauw commented Mar 5, 2023

I like the idea of exposing the unit of measurement, e.g. as capabilities/unit.

However, we would still need a separate attribute to indicate what quantity is being measured. In my view using type for that is preferable over using an additional attribute. In other words: I prefer a type: "ZHATemperature" over a combination of type: "ZHAMeasurement" and a capabilities/measurement_type: "temperature".

I like the idea of a generic state/measured_value attribute from an API client perspective. However, this would enforce having a separate (subdevice) resource for each quantity, as you can no longer expose multiple measurement types on the same resource. Cf. separate 0x04xx cluster per measurement type, each with a 0x0000 Measured Value attribute.

From an API development perspective, it would be more work, since you cannot use (device) generic parse and read functions in measured_value_item.json. It would have to parse and read a different cluster and/or attribute for each different measurement type.

@manup
Copy link
Member

manup commented Mar 13, 2023

I'm also in the boat to abstract the the unit for numeric items. That would allow more expressive item descriptions and moreover allows us to do unit conversions when needed.

{
  "name": "state/airquality",
  "unit": "co2",
  "scale": 12345.6
}

{
  "name": "state/temperature",
  "unit": "celsius",
  "unit_scale": 100
}

The REST client could optionally configure unit conversions (on a per client/session base) for the REST API, like:

[
  {
    "name": "state/temperature",
    "src_unit": "celsius",
    "dst_unit": "farenheit",
    "dst_unit_scale": 1
  },
  ...
]

Main point here would be to bring knowledge about the unit/scale in the DDF to allow more generic clients.

@SwoopX
Copy link
Collaborator

SwoopX commented Mar 13, 2023

@manup pretty close to what I had in mind 🙂 I'd expose the raw value as well as the scaled value (or whatever you want to call it when some calculations are applied). Something like:

{
"name": "state/oxygen",
"unit": "o2",
"rawvalue": 12345.6
"value": 123,
"type": "ZHAMeasurement"
}

@ebaauw
Copy link
Collaborator

ebaauw commented Mar 13, 2023

Neither oxygen (O2) nor carbon dioxide (CO2) are units; they're gasses for which you measure the level or density, like you would measure temperature. The unit tells you how to express the measured value (like temperature in °C or °F). The unit would be ppb (for oxygen or carbon dioxide level) or µg/m³ (for oxygen or carbon dioxide density).

If you want to expose multiple values, you would need to supply the proper unit (and scale) for each value. Like temperature in °C (with two decimals) vs temperature in °F vs temperature in 0.01 °C (as integer, cf. the Zigbee standard).

BTW do you have a Zibbee oxygen sensor? I've never seen one of these. Does it measure oxygen level or oxygen density?

@manup
Copy link
Member

manup commented Mar 16, 2023

Indeed these are good points. Guess we need to figure out how to represent this to support the wild Zigbee zoo of different devices and measurements/values. Overall as long as the system has enough knowledge/meta data about a value so this details aren't hidden in custom JS expressions, it should be able to do the right thing 🤔

@m4k2k
Copy link

m4k2k commented Mar 18, 2023

Hello all,
the discussion seems very can necessary, but .. I'm waiting for this change to happen to support a sensor (using this new types) in the rest plugin. This will then communicate the data to HA. There are several other pull-requests (on other projects) that demands this to have the chain working.
Why not move the discussion and architectural design discission into a separate thread?
So this PR (which is not different than the others contributing to the "unit mess") can continue and merge.
best regards :)

@Smanar
Copy link
Collaborator Author

Smanar commented Mar 19, 2023

Have corrected to

  • state/airquality_co2_density
  • state/airquality_formaldehyde_density

@Kane610
Copy link

Kane610 commented Mar 19, 2023

Great that this is underway, it replaces #6810 as a fix. We are awaiting this be merged prior to merging Kane610/deconz#408 and a related PR on home assistant core.

@Smanar
Copy link
Collaborator Author

Smanar commented Mar 20, 2023

@Kane610 I need too correct the DDF already merged, if this PR is valided (it use old field)

@m4k2k
Copy link

m4k2k commented Apr 24, 2023

Guys, what is missing here?
Is there a roadmap to get this PR done?
Please have clear instructions what should be done to get this PR merged.
@Smanar @ebaauw @SwoopX

@Mimiix
Copy link
Collaborator

Mimiix commented Apr 24, 2023

That's more a question for @manup

@BabaIsYou
Copy link
Contributor

BabaIsYou commented Apr 25, 2023

My last 2cents ... ;-)

We will still have to compromise at some point, this kind of request to take these different types of measurements into consideration is not new (#4997 ) and will progress as the Zigbee specifications go accept them and DeCONZ-GUI will display them (#6906 ).
Even if it is clear that the measurement unit will remain dependent on the different devices (even when the Zigbee specification specifies it, just look at the mess with the Tuya derivatives currently) there is still a pattern that is emerging and which perhaps allows to have a compromise facing of the different ideas suggested above.

I think we could create one sensor per type of ambient substance like ZHAAirMeasurement and ZHAWaterMeasurement with for these sensors the items that we find for each of them in the Zigbee specifications, namely MeasuredValue, MinMeasuredValue and MaxMeasuredValue. We could add a static type that would contain the unit of measurement (which we could also standardize iusing constants).

Thus we can adjust in each DDF the measurement reported through its uuid (which would end with the value of the cluster of the specification, for example "uuid": ["$address.ext", "0x01", "0x042B"] for formaldehyde, 0x040c for CO, 0x042E for VOC, etc ...), and its description and provide the unit of measurement.

Proposed Items

{

"schema": "resourceitem1.schema.json",
"id": "state/measuredvalue",
"datatype": "SingleFloat",
"access": "R",
"public": true,
"default":0,
"description": "Represents the concentration as a fraction of 1"
}
{
"schema": "resourceitem1.schema.json",
"id": "state/minmeasuredvalue",
"datatype": "SingleFloat",
"access": "R",
"public": true,
"description": "Minimum value of MeasuredValue that is capable of being measured"
}
{
"schema": "resourceitem1.schema.json",
"id": "state/maxmeasuredvalue",
"datatype": "SingleFloat",
"access": "R",
"public": true,
"description": "Maximum value of MeasuredValue that is capable of being measured."
}
{
	"schema": "resourceitem1.schema.json",
	"id": "state/measuredunit",
	"datatype": "String",
	"access": "R",
	"public": true,
	"description": "The unit of the measured value. To be defined by static in DDF",
	"default": "µg/m³",
	"values": [
		["µg/m³", "microgram per cubic meter"],
		["PPM", "Parts Per Million"],
		["PPB", "Parts Per Billion"]
	]
}

proposed subdevices

{
     "schema": "subdevice1.schema.json",
     "type": "$TYPE_MEASURED_SENSOR",
     "name": "ZHAAIRMEASURED",
     "restored": "/sensors",
     "order": 23,
     "items": [
         "state/measuredvalue",
         "state/minmeasuredvalue",
         "state/maxmeasuredvalue",
         "state/measuredunit"
     ]
}
{
     "schema": "subdevice1.schema.json",
     "type": "$TYPE_MEASURED_SENSOR",
     "name": "ZHAWATERMEASURED",
     "restored": "/sensors",
     "order": 23,
     "items": [
         "state/measuredvalue",
         "state/minmeasuredvalue",
         "state/maxmeasuredvalue",
         "state/measuredunit"
     ]
}

We can also, as suggested by Swoopx (#6673 (comment)) replace the generic items by specific ones (state/oxygen, state/VOC , state/carbondioxide, ...) to be more "specification friendly" and have more latitude in combining the values measured in the same cluster. Because the generic situation that I am proposing requires having only one measurement reported per cluster/sensor.

edit : i forgot a point, these mesured values need to be data type float (#6904 ) to be compliant to specification.

second edit: Looking forward in Zigbee specification, I realized that we already have this kind of issue because in the 0x702 Metring Cluster we're already using in ZHAConsumption subdevice and state/consumption item we consider that this measure is always kW (see devices/generic/items/state_consumption_item.json) but specifications speaks about this cluster to "retrieve usage information from Electric, Gas, Water, and potentially Thermal metering devices". The attribute 0x0x300 is supposed to give the unit used for the measured value of the other attribute-set. Then we should have already added a measuredunit item in our ZHAConsumption subdevice.

@m4k2k
Copy link

m4k2k commented May 17, 2023

Guys, what's up with this?

Is any dev planning to change it according to the recommendation?
Is there a timeline / plan? This is open since 5 month and the whole progress started already in November last year, so it's even longer!
Could we first merge then do the enhancement?
This is blocking the whole sensor's function! All other repos have already successfully merged.

@Mimiix
Copy link
Collaborator

Mimiix commented May 17, 2023

Guys, what's up with this?

Is any dev planning to change it according to the recommendation? Is there a timeline / plan? This is open since 5 month and the whole progress started already in November last year, so it's even longer! Could we first merge then do the enhancement? This is blocking the whole sensor's function! All other repos have already successfully merged.

I pinged them on discord.

@BabaIsYou BabaIsYou mentioned this pull request May 23, 2023
@BabaIsYou
Copy link
Contributor

For the VINDSTRYKA, I simply created two ZHAAirQuality sensors, one for PM2.5 Measurement and one for (IKEA-specific) TVOC Measurement. The first one has state/pm2_5; the second state/airqualityppb. Both have state/airquality, based on the corresponding pollutant.

Just to reactivate an old post ... For VINDSTRYKA, like any other device based on https://sensirion.com/products/catalog/SEN54/ sensr for VOC measure, it's not a concentration that is output but a calculated index between 1 and 500 based on a variation https://sensirion.com/media/documents/02232963/6294E043/Info_Note_VOC_Index.pdf

@manup
Copy link
Member

manup commented Jul 10, 2023

In order to proceed I think @BabaIsYou proposal in #6673 (comment) represents the most generic way which is also validator and automatic tests friendly without having to add infinite rules to check. While it means having more sensors for new devices there is no hidden magic and implementation detail hidden behind a state/value_of_unit string, and values can be converted by minimal code.

@BabaIsYou @ebaauw @SwoopX @Smanar @Kane610 @Thomas-Vos

Please use thumbs up/down for on this comment to decide if we can go with this.

I'd make the implementation for our our upcoming beta release with two changes from the proposal:

  • Use double instead 32-bit floats as floating point format. This is also used in transit via JSON as well as the Javascript engine.

  • Inside DDF use ASCII strings instead non ASCII Unicode characters (decimal values > 127) for state/measuredunit enumeration values. While µg/m³ looks nice it can be encoded in multiple ways in UTF-8 and needs normalization in order to compare strings. if (val == "µg/m³") { } may or may not work, we already had problems with Unicode characters causing problems for clients therefore I'd prefer to keep UTF-8 limited to very few items like attr/name.

    In the DDF µg/m³ is written as 1e-6 g/m^3 or ug/m^3

    Externally for the REST-API / Websockets we can expose units as written in the DDF or transformed into normalized UTF-8 strings like µg/m³.

manup added a commit to manup/deconz-rest-plugin that referenced this pull request Jul 10, 2023
The PR allows creation of ResourceItemsDescriptors from `generic/items` JSON files.
Works the same as the items defined in C++ code.

Dynamic items are only added if they aren't already defined. Later on we can remove items defined in C++ code, but need to reroute const char* suffix pointers like `RStateOn`.
The PR also defines the items described in dresden-elektronik#6673 (comment)
@BabaIsYou
Copy link
Contributor

And about creation of one sensor per type of ambient substance like ZHAAirMeasurement and ZHAWaterMeasurement to "support" a default use of these new items ?

@manup
Copy link
Member

manup commented Jul 12, 2023

And about creation of one sensor per type of ambient substance like ZHAAirMeasurement and ZHAWaterMeasurement to "support" a default use of these new items ?

Good question, I'm not sure about this. We already have sensors like ZHATemperature, ZHAWater, ZHAAirQuality, etc. So we have two possible ways to tackle this:

  1. Extend the existing sensors with with our new measured items.
  2. Create the extra ZHA*Measurement sensors.

I'm more in favor of 1) since it's more lightweight and doesn't introduce lots of extra resources for sub-devices. It also allows to one day perhaps deprecating some items in favor for more generic measurement items.

Opinions?

@BabaIsYou
Copy link
Contributor

BabaIsYou commented Jul 12, 2023

I thought that the aim of this was also to have a more generic approach regarding to measurement sensors to reduce the needs for specific sensor in the future. Zhawater was not created for measurement but for leak alert. Air quality is not the same as standard measurement of some gas concentrations in the air (see above comment about specific Sen54 sensors based device).

Having a generic zhawatermeasurement for instance would probably avoid creating a specific zhavalve sensor for instance.

Or may be just a ZHAMeasurement ?

@ebaauw
Copy link
Collaborator

ebaauw commented Jul 14, 2023

I thought that the aim of this was [...] to reduce the needs for specific sensor in the future.

Honestly, I don't understand why we would want to do this.

In HomeKit, there's no such thing as a "generic measurement sensor". I need to map each sensors resources to a particular HomeKit service type. That means, I need the API to tell me the measurement type, what quantity is being measured, preferably through a type-like attribute, rather than the API client trying to reverse-engineer this from the exposed state attributes.

Creating a generic type value ZHAMeasurement with an additional attribute to indicate what quantity is actually being measured makes no sense to me (as an API client).

for these sensors the items that we find for each of them in the Zigbee specifications, namely MeasuredValue, MinMeasuredValue and MaxMeasuredValue

I'm in favour of standardising the state attribute into a generic measured_value (I think for consistency we'd prefer that over measuredvalue or measuredValue). If we go this way, we definitely need the quantity being measured, the type, exposed explicitly.

This might actually be a non-breaking change to start exposing temperature, humidity, atmospheric pressure, electric consumption, voltage, power, frequency, ... as floating point numbers, e.g.: expose state/measured_value in °C on a ZHATemperature resource, next to state/temperature (in 0.01°C), which can then be deprecated.

Min and max measured value and unit should be exposed as capabilities attributes, not as state attributes.

I'm more in favor of [Extend the existing sensors with with our new measured items] since it's more lightweight

In the C++ code, introducing a new value for the type attribute of a sensors resource is a bit of a hassle, but I don't see why that would still be the case, now we're switching to DDFs. Just like we now can define new items in DDF, without having to define them in C++, we should be able to define a new sensors type solely in DDF.

We would need a corresponding generic function to handle the creation of CLIP sensors, preferably leveraging the logic of the DDF validator. Again, using a generic state/measured_value will greatly simplify this.

I'm more in favor of [Extend the existing sensors with with our new measured items] since [...] it doesn't introduce lots of extra resources for sub-devices

We expose a resource / sub-device per Zigbee cluster. That's how the uniqueid is build up. That makes sense functionally as well, since state/lastupdated reflects the last time communication from that cluster was received.

We could add a static type that would contain the unit of measurement

I categorically disagree. The API should standardise/normalise the units of measurement for each quantity (sensor type). I don't want ZHATemperature to include a unit (like °C, °F, or K, or worse: 0.01°C). I want the API to standardise these. Same as for e.g. config/sensitivity.

I'm happy for the unit to be exposed through the API (as capabilities attribute), so the API becomes more self-documenting, but please keep the unit the same for all instances of the same measurement type (quantity).

@manup
Copy link
Member

manup commented Jul 14, 2023

So we keep explicit sensor types and above that also provide a generic view via capabilities and state/measured_value. With the uniqueid it's possible to have also multiple sub-devices of the same time. Sounds good to me.

This might actually be a non-breaking change to start exposing temperature, humidity, atmospheric pressure, electric consumption, voltage, power, frequency, ... as floating point numbers, e.g.: expose state/measured_value in °C on a ZHATemperature resource, next to state/temperature (in 0.01°C), which can then be deprecated.

Yes, that would be great and can be done with only a few new items in the DDF.

@BabaIsYou sorry my bad, yes ZHAWater doesn't fall into of floating point range category and hosts a boolean value, so here ZHAWaterMeasured makes sense to not break existing meaning of ZHAWater.

Min and max measured value and unit should be exposed as capabilities attributes, not as state attributes.

Agree, to confirm this would be cap/measured_value/min, cap/measured_value/max and cap/measured_value/unit?

In the C++ code, introducing a new value for the type attribute of a sensors resource is a bit of a hassle, but I don't see why that would still be the case, now we're switching to DDFs. Just like we now can define new items in DDF, without having to define them in C++, we should be able to define a new sensors type solely in DDF.

That's already possible as these are described in the generic/sub-devices, the DDF, DDF Editor and Device code doesn't work with hard coded descriptions here.

I'm not sure if it already works for CLIP sensors, this might need some C++ additions to get this working.

I'm happy for the unit to be exposed through the API (as capabilities attribute), so the API becomes more self-documenting, but please keep the unit the same for all instances of the same measurement type (quantity).

Agree.

manup added a commit to manup/deconz-rest-plugin that referenced this pull request Jul 20, 2023
The PR allows creation of ResourceItemsDescriptors from `generic/items` JSON files.
Works the same as the items defined in C++ code.

Dynamic items are only added if they aren't already defined. Later on we can remove items defined in C++ code, but need to reroute const char* suffix pointers like `RStateOn`.
The PR also defines the items described in dresden-elektronik#6673 (comment)

Update measured_value items according to discussion


Remove measure_value items from this PR
manup added a commit to manup/deconz-rest-plugin that referenced this pull request Jul 20, 2023
Related to dresden-elektronik#6673 (comment)

state/measured_value
cap/measured_value/min
cap/measured_value/max
cap/measured_value/unit
@manup
Copy link
Member

manup commented Jul 20, 2023

I have adapted the discussed changes in #7119 can merged soon if there are no other change requests.

@Smanar I think you can adapt the state/measured_value and capability items in this PR now.

After that the DDF validator should be happy I think.

@Kane610
Copy link

Kane610 commented Jul 20, 2023

Sorry for being late to the game here. Can you share a couple of practical examples of how it will look with the REST API? DDF examples are irrelevant to me

@Smanar
Copy link
Collaborator Author

Smanar commented Jul 20, 2023

And If I want to create a TYPE_AIR_QUALITY_SENSOR with 3 custom fields ?
It can work for 1 value by Sensor only.

BTW I will close this PR, I will make a new one to update the devices\tuya_TZE200_dwcarsat_air_sensor.json DDF.

@Smanar Smanar closed this Jul 20, 2023
@BabaIsYou
Copy link
Contributor

You can adjust in each sensor the measurement reported through its uuid (which would end with the value of the cluster of the zigbee specification, for example "uuid": ["$address.ext", "0x01", "0x042B"] for formaldehyde, 0x040c for CO, 0x042E for VOC, etc ...), and its description and provide the unit of measurement.

@Smanar
Copy link
Collaborator Author

Smanar commented Jul 20, 2023

Yep but on DDF side, you can have only one state/measured_value by sensor, on the DDF there is 4 values on the same TYPE_AIR_QUALITY_SENSOR
https://github.com/dresden-elektronik/deconz-rest-plugin/blob/master/devices/tuya/_TZE200_dwcarsat_air_sensor.json

  • state/airqualityformaldehyd
  • state/airqualityppb
  • state/pm2_5
  • state/airqualityco2

Need to create 1 ZHASensor for each measured_value and in deconz a simple sensor mean so much more fields just for 1 value

@manup
Copy link
Member

manup commented Jul 20, 2023

At least for now, later on we can simplify all DDFs by removing the always existing items like attr/id, attr/modelid etc. These currently take up a large portion for each sub-device in a DDF and are mainly there for the auto documentation generator, but this is only temporary until the remaining tooling caught up.

@Smanar
Copy link
Collaborator Author

Smanar commented Jul 21, 2023

Prototype here #7121

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

Successfully merging this pull request may close these issues.

8 participants