Door entities as enum sensors at Home Connect #126158
Conversation
home-assistant
bot
added
cla-signed
deprecation
|
Hey there @DavidMStraub, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Also I'd like to explore whether it's better to retrieve the available options for an enum device class sensor entity from the Home Connect API, rather than hardcoding them in the entity description. |
Diegorro98
force-pushed
the
home_connect_door_enum
branch
from
8a04e23
to
e8fd52d
Compare
Diegorro98
force-pushed
the
home_connect_door_enum
branch
from
e8fd52d
to
759d380
Compare
|
Downside about dynamically fetching is the lack of translations, and thus a suboptimal user experience for a lot of people |
Diegorro98
force-pushed
the
home_connect_door_enum
branch
from
759d380
to
86a5111
Compare
|
That's a good point, |
|
Btw, I just discovered that refrigeration doors possible states are |
|
Yes, but I think it's a nice way to gather all possible values. It might bite in the start for a group of users, but in the end you're left with a lot more knowledge about the API |
Correct, and there's one other undocumented API common door state I found BSH.Common.EnumType.DoorState.Ajar. I don't know how or what can trigger it, but it was interesting seeing it returned from the API. |
The integration is already plagued with API rate limit issues and adding more would probably make the UX worse in my opinion. Though, I do think it would be ideal to fetch the values from the API. There's an experiment I'm working on inside the integration to get cached values from the entity registry and have it managed by the DataUpdateCoordinator, which I would recommend using here for coordinating the API data retrieval. This is sort of a POC for what I had in mind where we load data from the I wasn't able to find an example in other integrations so I'm certain it doesn't follow the rules 😅 , though on discord I found a thread that pointed to good information from someone with the same requirements and also linked an architecture discussion along the same lines. Now the reason I'd say use the |
| "Closed", | ||
| "Open", |
There was a problem hiding this comment.
Since it was only 2 states I ended up switching these to the binary_sensor platform in PR #125490 .
There was a problem hiding this comment.
It makes sense.
Given that the API docs specifies that door states use an enum data type, I believe enum sensors are a better choice.
Here’s why: if any of these entities, which we currently assume that they have only 2 states, suddenly support an additional state (like a locked state for doors, or any new possible state), we need to ensure compatibility. This would mean deprecating the binary door sensor and introducing a new enum sensor, along with handling any collateral effects.
| @@ -182,12 +331,12 @@ async def async_update(self) -> None: | |||
| class HomeConnectAlarmSensor(HomeConnectEntity, SensorEntity): | |||
| """Sensor entity setup using SensorEntityDescription.""" | |||
|
|
|||
| entity_description: HomeConnectSensorEntityDescription | |||
| entity_description: HomeConnectAlarmSensorEntityDescription | |||
There was a problem hiding this comment.
To me the main reason to use EntityDescription data classes was to have a unified class for the Platform to make it easier for entities to be added in the future since a developer or user would only need to add an item to the appropriate tuple instead of duplicating a class and changing its logic.
Separate classes also make it similar to the current method of adding entities which, as you've probably seen in the switch entity, can make adding additional entities complicated and add more duplicate code than necessary.
There was a problem hiding this comment.
I understand your idea, however, I think that simplifying so much the sensor entity class results in more complicated entity descriptions that contains duplicated code.
Also, this PR was about door enum entities, code quailty improvements and simplifications comes in another PR I'm preparing these days.
Breaking change
Proposed change
Because door states are specified to be enumeration data types at Home Connect API Documentation (link), door entity sensors might should be sensors with Enum device class.
This change would deprecate door binary sensor (maybe no needed for recently added binary door sensors as they have not been released yet).
Also added more door sensors.
Type of change
Additional information
This PR will be keept in draft until #126143 gets merged, because it will allow to refactor code.
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: