RobertD502 / hass-flair-helper

Flair API helper used by home-assistant-flair component
MIT License
2 stars 2 forks source link

FlairRoom ClimateEntity implementation results in confusing/non-functional UI elements #5

Closed smcpeck closed 2 years ago

smcpeck commented 2 years ago

Much of the newly implemented HvacUnit functionality doesn't implement the climate component properly.

Notice set_hvac_mode throws a NotImplementedError. https://github.com/home-assistant/core/blob/dev/homeassistant/components/climate/__init__.py#L466

There is an async version of the method that would behave properly. https://github.com/home-assistant/core/blob/dev/homeassistant/components/climate/__init__.py#L468

I know async all the way through is part of what you have in the works, but I just wanted to capture this.

RobertD502 commented 2 years ago

Based on HA dev documentation only one needs to be implemented and it doesn't have to be the async version. This error won't be raised with set_hvac_mode implemented.

smcpeck commented 2 years ago

Hmmm.

Sorry, should've included the stacktrace. I saw the error when I clicked on some buttons of the climate widget.

Traceback (most recent call last):
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/components/websocket_api/commands.py", line 186, in handle_call_service
    await hass.services.async_call(
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/core.py", line 1625, in async_call
    task.result()
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/core.py", line 1662, in _execute_service
    await cast(Callable[[ServiceCall], Awaitable[None]], handler.job.target)(
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/helpers/entity_component.py", line 203, in handle_service
    await self.hass.helpers.service.entity_service_call(
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/helpers/service.py", line 668, in entity_service_call
    future.result()  # pop exception if have
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/helpers/entity.py", line 898, in async_request_call
    await coro
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/helpers/service.py", line 705, in _handle_entity_call
    await result
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/components/climate/__init__.py", line 470, in async_set_hvac_mode
    await self.hass.async_add_executor_job(self.set_hvac_mode, hvac_mode)
  File "/usr/lib/python3.9/concurrent/futures/thread.py", line 52, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/mnt/c/Users/Shaun/source/home-assistant/core/homeassistant/components/climate/__init__.py", line 466, in set_hvac_mode
    raise NotImplementedError()
NotImplementedError
RobertD502 commented 2 years ago

That is odd, but the way the climate entity is written adheres to their documentation. Can't say I've ever encountered that error. It would be great if you could reproduce it to pinpoint what exactly triggered it.

smcpeck commented 2 years ago

Ahhh! Got it.

In climate.py, you are adding a ClimateEntity for each hvac-unit and for each room.

FlairRoom doesn't fully implement what the climate component needs. (like set_hvac_mode, for example). That's why I am getting the default component implementation with the NotImplementedError.

RobertD502 commented 2 years ago

Correct! The room's climate entity returns what mode it is in, but I did not implement setting hvac mode for room climate entities (this is noted in the Flair Integration documentation 😉). The reason for this choice is because, within Flair, the hvac mode is changed for the structure and not within individual rooms. I didn't want to implement changing the room hvac mode and having it in turn change the structure mode because it could potentially lead users to think that they can change the mode on a room by room basis. If they want to change the HVAC mode for the rooms, I implemented this ability in the Structure Mode select entity found within the Home Assistant Structure device.

smcpeck commented 2 years ago

Gotcha. Makes total sense -- especially cross-referencing the Flair Rooms API documentation.

It makes me wonder, though, if a ClimateEntity is the right implementation for a Flair Room.

It gives a lot of false expectations to the user: image 👆 That makes me think the entity is actively cooling from 78 to 73. But that's false; the mini-split is turned off.

Can we extend the ClimateEntity and then override some of the UI visibility in our implementation?

RobertD502 commented 2 years ago

Flair Rooms report temperature, humidity, and have a set point. The climate entity is the only one that fits the bill and is the closest 1 to 1 replication between the Flair app and Home Assistant. The flair app itself doesn't show if a room is actively heating or cooling (a room without a mini split) and the API doesn't return this info (not even at the Thermostat endpoint). Those that don't use mini splits will know if a room is actively heating or cool by having their thermostats integrated separately since the Flair API or app wouldn't tell them that info.

Those that do have mini-splits in rooms can tell if a room is actively heating, cooling, fan only through the Mini Split climate entity. I have implemented Home Assistant's HVAC Actions for the mini split climate entities. If your minisplit is actively heating and cooling that is displayed- Click on the three dots in the upper right hand corner of your card and you will see the current hvac action (not mode).

For example my house is actively being heated right now and I can tell by seeing the climate entity reporting "Heating(Heat)":

image

You can also see this by selecting your climate entity within the Home Assistant developer tools and checking out what the hvac_action attribute is showing.

Regarding UI in general:

I would recommend looking into custom lovelace thermostat cards. The generic thermostat card built into HA is pretty bland. A Home Assistant integration can't make specific UI changes to the built in cards - custom lovelace cards are reserved for this.

RobertD502 commented 2 years ago

Home Assistant integration now logs an error instead of raising an exception when users try to set HVAC mode from a room and this is also noted in the documentation.