MallocArray / airgradient_esphome

ESPHome definition for an AirGradient DIY device to send data to HomeAssistant and AirGradient servers
GNU General Public License v3.0
205 stars 29 forks source link

Could we add device_class: aqi for the aqi sensors and device_class: volatile_organic_compounds for voc sensors? #68

Closed ncd7 closed 3 days ago

ncd7 commented 3 weeks ago

For the AQI This will help with some compatibility with other sensors from other integrations (right now my workaround is to add a customize: section to my configuration.yaml file). Also, if possible pls make sure it is lower case aqi and not AQI because some parts of HA care about a perfect string match which is case sensitive. I believe the correct constant is lower case as I've seen that from other integrations.

For the VOC This might potentially be trickier as the VOC uses an Index and the HA doc states that if you use the device_class: volatile_organic_compounds class you should set the units to µg/m³ but I don't know if that truly matters... I find as a whole the HA meta data about sensors kind of messy. Perhaps only do the AQI above and skip the VOC not sure...

MallocArray commented 3 weeks ago

Are you talking about the device_class for the sensor named "PM 2.5 AQI"? If so, that looks like a reasonable request. Regarding the capitalization, are you still taking about device_class: aqi being in lower case, or the unit_of_measurement: "AQI" part, or talking the name in general? AQI is an acronym so it should be all caps when talking about it in general and with the name https://www.airnow.gov/aqi/aqi-basics/ But if you are talking about the unit of measure being lower case, I could be talked into that. I'm not sure if there is a fixed list of values that unit_of_measure should be, or if it is arbitrary. Can you include some specific examples of it being lower case in other integrations for comparison?

For VOC, those are all coming from the sgp4x sensor integration and it actually specifies device_class: aqi for both VOC and NOx Indexes. If that should be changed, it would be best to open a request in the ESPHome Issues section to see if that maintainer would want to change it, but probably would just leave it alone.

ncd7 commented 2 weeks ago

Apologies I missed your prompt response!

I am comparing it to the IQAir (the swiss company) AirVisual Pro integration. There, it is using both device_class: aqi and also unit_of_measurement: aqi.

The way I faced the issue is, I created a HA "helper" sensor that combines the AQI values from AirGradient and IQAir Air Visual Pro sensors by taking the "max". The helper sensor failed because one integration had AQI and the other aqi and it requires identical units of measurement (reasonably so).

To your point, I believe unit_of_measurement is actually quite ad-hoc and I agree with your general statement that AQI is an acronym and should be capitalized - that makes the most sense. My only slight counter-argument would be that it seems that it would be inconsistent with the device_class: aqi constant which I believe should be lower case as all device_class constants seems to be lower case in HA. But those are not meant to be visible in Dashboards and are more internal. So mine is not a strong enough argument.

So in summary I'd say: I would not push for changing unit_of_measurement to be lower case, let's keep it upper case as it is now. And to solve the discrepancy with AirVisual Pro I'll just use a customize section in my configuration.yaml, not a big deal.

But for device_class of the PM AQI sensor, I would still request that we add device_class: aqi (lower case) to match the AirVisual Pro integration but also the general constants in HA for device classes being lower case.

For the VOC -- understood, I didn't realize that. I will do that because I definitely see that in HA there is a constant for VOC for device_class so I think volatile_organic_compounds is the more accurate constant rather than aqi but as you say this isn't to be handled by your repo.

Let me know if this made sense, I can re-summarize more simply if needed. Thanks.

MallocArray commented 2 weeks ago

I have device_class: aqi added to the AQI sensor and running it through tests now. Expect to be included in the next release

ncd7 commented 2 weeks ago

Thank you very much @MallocArray you are the best! I'll close the issue once it's released.

MallocArray commented 3 days ago

Added in the latest 4.0.0 release