8000 Current `water_heater` entity missing important attributes, methods · Issue #117 · home-assistant/architecture · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Current water_heater entity missing important attributes, methods #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zxdavb opened this issue Dec 11, 2018 · 12 comments
Closed

Current water_heater entity missing important attributes, methods #117

zxdavb opened this issue Dec 11, 2018 · 12 comments

Comments

@zxdavb
Copy link
zxdavb commented Dec 11, 2018

The current version of water_heater was home-assistant/core#17058, and appears to have been implemented completely independently of the corresponding discussion at #59

This caught many of us by surprise, because the PR was started/accepted after #59, with no mention of it in that discussion (I think it was a Hackoberfest thing) - so the first we knew was when home-assistant/core#17058 was accepted, and #59 was closed.

As it stands, the water_heater has no current_temperature, nor does it have one/off/heating as states!

Ironically, there is home-assistant/core#19093, which would add some of this functionality, but it was been blocked pending an architectural discussion...

So here it is. Discuss.
@zxdavb zxdavb mentioned this issue Dec 11, 2018
Closed
4 tasks
@zxdavb
Copy link
Author
zxdavb commented Dec 11, 2018

A proposed schema

icon = "mdi:thermometer-lines"

supported_features = SUPPORT_OPERATION_MODE | SUPPORT_ON_OFF | support_temperature | support_target_temp
state: [
  STATE_OFF,  # boiler not active (not using energy), current temp is irrelevant
  STATE_ON,  # boiler will act to attain/maintain target temp when required
  STATE_HEATING, # boiler is actively heating the water (assumes STATE_ON) - Optional
]

state_attributes: [
  'operation_mode': [manual, schedule, etc]
  'current_temperature', # reported by a temperature sensor
  'target_temperature', # the temperature to reach...
  'temperature_unit',  # TEMP_CELSIUS, or the other one...
  'min_temperature',  # lowest possible target temp
  'max_temperature',  # maximum possible target temp
  'precision',  #  1, 0.5 or 0.1 (PRECISION_WHOLE, HALVES, TENTHS)
]

Services: [  # some devices allow setting these parameters ahead of time
  'set_operation_mode':  { 'service_data': ['operation_mode'] }
  'set_temperature': { 'service_data': ['target_temperature'] }
  'turn_off',
  'turn_on': {}
]

@zxdavb
Copy link
Author
zxdavb commented Dec 11, 2018 •
edited
Loading

Currently, the water_heater has a piece of code thus:

    @property
    def state(self):
        """Return the current state."""
        return self.current_operation

...where current_operation is one of:

STATE_ECO = 'eco'
STATE_ELECTRIC = 'electric'
STATE_PERFORMANCE = 'performance'
STATE_HIGH_DEMAND = 'high_demand'
STATE_HEAT_PUMP = 'heat_pump'
STATE_GAS = 'gas'

In my opinions, these are not states (they are operating modes). state should really be limited to ON, OFF or HEATING.

See this excellent point made in the climate review: #22 (comment)

@balloob
Copy link
Member
balloob commented Dec 14, 2018

Don't define state as being something independent. State in Home Assistant is just taking the most important property and exposing that as state. State is always defined as just returning a single property.

As discussed in #22, there is operation mode and operating mode. Water heater should have both, even if operation mode is not configurable.

@marchingphoenix marchingphoenix mentioned this issue Dec 15, 2018
Closed
3 tasks
@zxdavb
Copy link
Author
zxdavb commented Dec 17, 2018 •
edited
Loading

Don't define state as being something independent. State in Home Assistant is just taking the most important property and exposing that as state. State is always defined as just returning a single property.

Yes, but it should be the most important property for the platform (e.g. water_heater), rather than for the individual entity (e.g. econet).

My thoughts are this. Sit a bunch of people down as tell them you've got a device that is called a 'water heater' that heats stored water (you can warn them that it may/may not actually boil the water).

Ask them what's the most important things they'd want to know about that water heater?

They will all agree upon:
a) the current temperature of the water (and by implication, the current target temperature)
b) what the device is 'doing' about a)

They will not always agree upon other stuff like, where is the heat coming from, how aggressively it is trying to reach target temp, whether the 'stat has kicked in, how many watts are being consumed, etc. To me, these are all edge cases.

Regarding state, current temp is too variable/volatile, and so state (if it is going to be the same for all water heaters), should be something like:

  • off, it doesn't care what the current temp is
  • on it does care about current temp, and is working to reach/maintain target temp, or
  • heating, it is on, and currently increasing the current temp because it is below target temp)
  • idle, it is on, but not heating the water because current temp is currently at/above target temp

Most water heaters will offer only a subset of the above - this is OK.

For example, my evohome DHW would report off and on only. You could fudge the other two if you had current temp and target temp (however, evohome does not report target temp).

@zxdavb zxdavb closed this as completed Dec 17, 2018
@zxdavb zxdavb reopened this Dec 17, 2018
@zxdavb
Copy link
Author
zxdavb commented Dec 17, 2018

closed in error

@kloknibor
Copy link
kloknibor commented Dec 17, 2018 •
edited
Loading

I would like to add that I also need this added functionality to be able to add in all the functionality in my component.

So there are different vendors that would benefit from this change.

I also agree with @zxdavb 's proposed schema.

@zxdavb zxdavb mentioned this issue Dec 17, 2018
Closed
@zxdavb
Copy link
Author
zxdavb commented Dec 17, 2018 •< 8000 /span>
edited
Loading

Maybe we should wait for the re-write of the climate entity, and take leadership from there?

@kloknibor
Copy link

@zxdavb how long will that take? Because if that'll take a long time I won't mind doing the work twice and prefer to just get my component working soon :)

@zxdavb
Copy link
Author
zxdavb commented Dec 17, 2018

@zxdavb how long will that take? Because if that'll take a long time I won't mind doing the work twice and prefer to just get my component working soon :)

No idea. The answer to that question may be best asked elsewhere... https://discordapp.com/channels/330944238910963714/330990195199442944

@kloknibor
Copy link

Sine the thread is already running for months I would prefer to just implement this already and than hopefully we can use new climate device to make a new water_heater device that will have everything we need :)!

@elupus
Copy link
elupus commented Apr 15, 2019

Just for reference: home-assistant/core#21393

@frenck
Copy link
Member
frenck commented May 11, 2023

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck

@frenck frenck closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@frenck @elupus @balloob @zxdavb @kloknibor
0