home-assistant / architecture

Repo to discuss Home Assistant architecture
319 stars 100 forks source link

Deprecate air_quality entity #362

Closed ahayworth closed 3 years ago

ahayworth commented 4 years ago

Context

The air_quality entity represents what is essentially a collection of 11 (actually, 12) different sensor values, yet it has a separate entity within home assistant core. This leads to subpar "out of the box" support:

I have been working on two things: updating the Awair integration to use the air_quality entity, and also creating a lovelace card that represents the air_quality entity in a reasonable and informative manner. However, the list of problems have made this task much more daunting than it needs to be.

Proposal

I propose that we deprecate the air_quality component and instead have integrations create a collection of sensor values. At its core, the air_quality component is a collection of sensor values - be they remotely measured at a central agency, or locally in your home.

Consequences

Pros:

Cons:

Unknowns

Jc2k commented 4 years ago

I am all for this.

I think this is somewhat related to https://github.com/home-assistant/architecture/issues/359. There instead of adding a attribute onto the battery sensor its proposed to break it out into 2 entities and use the device registry to express the relationship between the 2 entities.

It's also a good example of the HomeKit problem you mentioned - @bdraco is looking into using the device registry to merge 2 entities about the same battery into a single HAP 'service'. It looks like it is do-able from what i have seen, as long as the integrations use the device registry.

Support for the AirQuality entity in homekit stalled though so it's not an immediate worry (https://github.com/home-assistant/core/pull/30383)

Incidentally it is something of a PITA for homekit_controller, right now there is an annoying assumption that theres a 1-1 mapping between entities and services. But I will fix that in homekit_controller if we do break it out (somehow.)

BradleyFord commented 4 years ago

Just an additional note.

This also applies to the Xiaomi air purifiers; at the moment I am breaking out the PM2.5 attribute into a sensor so I display it on the UI alongside the AirVisual data.

bramkragten commented 4 years ago

The Xiaomi air purifier uses a fan entity with all the info in the attributes, that is not specifically related to this, but it should break all the info into separate sensor entities.

I think deprecating air_quality is a good idea.

dgomes commented 4 years ago

If we go this route, we might as well deprecate weather, it's also a collection of sensors (temperature, humidity, pressure, etc)

I'm not sure this is a good idea, it opens a precedent.

Jc2k commented 4 years ago

Maybe there is such a thing as going too far. But what is it and why? A bit of a strawman but if weather is reporting current values for those things as seperate sensors, what would be wrong with that? If the UI didn't change but it made it easier for users to do automations based off weather data or use the other graph panels wouldn't it be a good thing?

To make a decision about this, should we first come up with some guidelines about when things should be an all-in-one entity with lots of attributes and when they should be decomposed into multiple entities? Should that be a seperate ADR?

@frenck said in https://github.com/home-assistant/architecture/issues/359 that it was a good idea to split that sensor in 2 because (sorry if miss-paraphrasing!):

I'd definitely do an automation off CAQI score etc. But maybe not off some of the other values. But i'd like to do graphs of them. Is that enough of a reason?

Some other examples to consider to help better define such guidelines:

What about these 2 attributes mean that they should or shouldn't be decomposed into seperate entities?

Traditionally the auto-UI would have been a concern, i guess, as you would have loads of potentially un-interesting entities pop up and crowd out the interesting stuff. Is that less of an issue now? Should we make these "sub-entities" not part of the auto-UI? In the HomeKit world there is the notion of a primary entity with related sub entities.

dgomes commented 4 years ago

My point is that this decision should not be about deprecating air_quality alone.

It should be about all entities that are bundle of multiple sensors/entities.

bramkragten commented 4 years ago

Ideally, we would break everything up in separate entities and allow to link those together, the same applies to climate, there is a target, current temperature and maybe humidity attribute.

Jc2k commented 4 years ago

@dgomes I read too much into what you meant be precedent then, sorry for that - i thought it implied that it was a bad thing that shouldn't happen, not that we should review everything and do it event more widely!

I think we should deprecate air quality. The air quality entity should really just be a "Device" (in the device registry sense) with a bunch of sensor entities. I think it should have been done this way originally, as it's what the device registry is meant to enable.

I think we should review the other entities too. And for that my point still stands then, we should define some criteria about when an attribute is actually a state that should be a seperate entity. I think the conversation in #359 has a start there.

bachya commented 4 years ago

Has anyone investigated the rationale behind creating air_quality in the first place?

Jc2k commented 4 years ago

https://github.com/home-assistant/architecture/issues/109

Does that help? My take is nothing in there invalidates this discussion. The intention was to group lots of different sensors together. But that’s what the device registry is for. And it does feel like if it were added today it would lean on the device registry and be multiple sensors.

balloob commented 4 years ago

One comment about original proposal:

No built-in lovelace card that can represent this entity does it very well

Frontend issues can never be used as a reason to change the backend.

@bramkragten for climate, I would consider HVAC mode + target range to be together (Google calls it "TemperatureSetting"). We should have pushed current temp/humid out of climate during our big refactor last year :(

I think that the future is that a device is represented by a set of entities with each entity identified by a device class.

Another one to extract is current power usage from switches that support it.

ahayworth commented 4 years ago

Frontend issues can never be used as a reason to change the backend.

Some of the work I initially began was essentially “change the cards hard-coded to only support sensors to also support air_quality” - but I began to worry that the complexity of the air_quality component couldn’t be represented in the sensor card without making that card much more complicated. In essence, I felt that the complexity of the backend would prohibit changes to the front end - I wasn’t sure that the front-end changes would be accepted.

That’s why I then began pursuing a separate card entirely, before finally deciding to propose this deprecation.

Otherwise, though - I understand what you’re saying and mostly agree. And decoupling the front end and backend allows for greater innovation and flexibility in the long run - it’s a smart move.

However: I think this issue should be framed a little differently. The backend in this instance (air_quality) forces additional complexity onto all frontends - not just Lovelace - and it’s unclear what value the additional complexity adds.

While I initially proposed this based on my experience trying to surface the component in Lovelace, after reflecting on it for a few days I think it should be considered in light of reducing complexity and driving uniformity across the entire backend. The fact that the Lovelace benefits is not what I would use to advocate for the deprecation.

With that in mind - and the issue linked that advocated for the air_quality component in the first place - is there any value the component provides on its own now that we have the device registry?

fabaff commented 4 years ago

air_quality, is like weather, a "meta-integration" that focused on gathering data from third-party web services and not local units. They were created to allow users to switch between different integrations back and forth without loosing their history. All platforms are limited to the most common values at the time of creating the integration. If you compare the sensors which are gathering and displaying weather or air quality data and the air_quality and the weather integrations you will see a huge different in the available data.

Nowadays, this concept is kind of obsolete. And I agree that this should be handled by device classes.