core icon indicating copy to clipboard operation
core copied to clipboard

Door entity as enum sensor at Home Connect

Open Diegorro98 opened this issue 1 year ago • 12 comments

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

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

This PR will be keept in draft until https://github.com/home-assistant/core/pull/126143 gets merged, because it will allow to refactor code.

  • [X] The code change is tested and works locally.
  • [X] Local tests pass. Your PR cannot be merged unless tests pass
  • [X] There is no commented out code in this PR.
  • [X] I have followed the development checklist
  • [X] I have followed the perfect PR recommendations
  • [X] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [X] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [X] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [X] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [X] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Diegorro98 avatar Sep 17 '24 22:09 Diegorro98

Hey there @davidmstraub, mind taking a look at this pull request as it has been labeled with an integration (home_connect) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of home_connect can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign home_connect Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Sep 17 '24 22:09 home-assistant[bot]

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 avatar Sep 17 '24 22:09 Diegorro98

Downside about dynamically fetching is the lack of translations, and thus a suboptimal user experience for a lot of people

joostlek avatar Sep 18 '24 18:09 joostlek

That's a good point, Another downside, but about hardcoding options, is that if a value that it is not in the hardcoded in the options is sent from the API, it will raise an exception

Diegorro98 avatar Sep 18 '24 18:09 Diegorro98

Btw, I just discovered that refrigeration doors possible states are Refrigeration.Common.EnumType.Door.States.Closed and Refrigeration.Common.EnumType.Door.States.Open, but I can't find such values at API docs. @beastie29a, is this something that you found by fetching your own appliances' states?

Diegorro98 avatar Sep 18 '24 18:09 Diegorro98

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

joostlek avatar Sep 18 '24 18:09 joostlek

Btw, I just discovered that refrigeration doors possible states are Refrigeration.Common.EnumType.Door.States.Closed and Refrigeration.Common.EnumType.Door.States.Open, but I can't find such values at API docs. @beastie29a, is this something that you found by fetching your own appliances' states?

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.

beastie29a avatar Sep 19 '24 02:09 beastie29a

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.

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 entity_registry and then query that data to see if an API call needs to be made.

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 DataUpdateCoordinator is to take advantage if/when new features become available such as caching data from the API and a way to force an update say for example if the units change from Celsius to Fahrenheit in the Set Point Temperature entity.

beastie29a avatar Sep 19 '24 03:09 beastie29a

By the way, are you both able to send me a message on discord and maybe add yourselves as codeowner for the integrations?

joostlek avatar Sep 24 '24 11:09 joostlek

Any suggestion about how to mark the old way to get the Door entity as deprecated? I tried this, but it raises a TypeError (metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases) when classes inherit from DeviceWithDoor:

@deprecated_class(
    "homeassistant.components.home_connect.sensor.HomeConnectSensor",
    breaks_in_ha_version="2025.5",
)

https://github.com/home-assistant/core/blob/639033ecc094200c57950c1c2899f2e1b9dab595/homeassistant/components/home_connect/api.py#L247-L248

Maybe there is another way to remember in 6 months that the binary entity and this old way to get the door entity should be deleted

EDIT: now this is outdated because it doesn't use this class anymore, but I don't know yet how something should be marked as deprecated

Diegorro98 avatar Oct 07 '24 17:10 Diegorro98

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Oct 13 '24 11:10 home-assistant[bot]

As this PR is not going to be a deprecation of the common door binary sensor, should we update de labels?

Diegorro98 avatar Oct 13 '24 20:10 Diegorro98