Skip to content
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.

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

Add Generic Water Heater #44605

Closed
wants to merge 21 commits into from
Closed

Conversation

dgomes
Copy link
Contributor

@dgomes dgomes commented Dec 29, 2020

Proposed change

Adds a generic water heater. This is mostly a dumb water reservoir with a resistance in it which can be controlled with a switch entity. Inside the reservoir there is a temperature sensor. You can control the target temperature and turn off the heater in advance, avoiding overshoot.

Type of change

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

Example entry for configuration.yaml:

# Example configuration.yaml
generic_water_heater:
   bath water:
     heater_switch: switch.dhw_switch
     temperature_sensor: sensor.dhw_temperature
     target_temperature: 50
     delta_temperature: 5

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • 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:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@davet2001
Copy link
Contributor

I don't agree with this.

The 'generic' integration currently provides a camera only.

While it is possible to add other unrelated entities to this, I don't think it's a good idea, mainly because it would make the config flow really confusing to the user.

I have a config flow PR open for the existing generic component, and making it also deal with water heaters would make no sense.

Probably 'generic' should be renamed to 'generic_ip_camera' - that is how it appears in the documentation.

My suggestion is that your new code goes into a new 'generic_water_heater' integration, along with its own config flow.

@dgomes
Copy link
Contributor Author

dgomes commented Dec 29, 2020

I don't necessarily disagree with you :)

But config flows are reserved for integrations that communicate with devices or services (see #44562 (comment))

I realised there was generic (with the camera) and generic_thermostat. But it is also true that you have template (with almost every possible platform). I therefore decided to follow the template organisation pattern.

Let me be clear, I have no problem refactoring the code into a generic_water_heater.

@dgomes
Copy link
Contributor Author

dgomes commented Dec 30, 2020

Since the initial PR did not address ADDR-0007 I decided to take the opportunity to move the integration into it's own directory, therefore clearing any concerns from @davet2001 😉

Still think generic and template are parallel and some refactoring is necessary (at least rename generic -> generic_camera)

@davet2001
Copy link
Contributor

Since the initial PR did not address ADDR-0007 I decided to take the opportunity to move the integration into it's own directory, therefore clearing any concerns from @davet2001 😉

Still think generic and template are parallel and some refactoring is necessary (at least rename generic -> generic_camera)

Thank you! :)
Yes, agree about the refactor. But renaming would cause a breaking change to existing configuration.yaml files. Possible route:

  1. config flow for generic
  2. deprecate yaml for generic & migrate to storage
  3. remove yaml for generic
  4. rename generic -> generic_ip_camera

@dgomes
Copy link
Contributor Author

dgomes commented Dec 30, 2020

@davet2001 I think you should open up the discussion in it's proper issue (possibility in the architecture repository ?) and leave the discussion here for the Generic Water Heater 😉

@Vizards
Copy link

Vizards commented Dec 31, 2020

I'm looking for a way to build a generic water heater for homekit. Thank you for your hard work.
By the way, may be a water heater should have some status like heating/cooling/keep_warm/idle?

@dgomes
Copy link
Contributor Author

dgomes commented Dec 31, 2020

By the way, may be a water heater should have some status like heating/cooling/keep_warm/idle?

I'm going for a 1st "as simple as it can get" PR (easier to be reviewed). Afterwards we can go beyond on/off :)

@afaucogney
Copy link
Contributor

I m curious to know the difference with a generic_thermostat ?

@dgomes
Copy link
Contributor Author

dgomes commented Feb 10, 2021

I m curious to know the difference with a generic_thermostat ?

This is a different platform. It's like the difference between switch and light, you can accomplish the same effect but are different things.

@afaucogney
Copy link
Contributor

Sorry if I miss understood @dgomes, but switch and light does'nt have the same effect ! Because a switch can be used for something else !
In addition, every heater water is composed from a thermostat !
From my POV, it doesn't make sens to build new platform for every high level composant that is composed by a thermostat ...

  • Generic Electrical Heater,
  • Generic Gaz Heater,
  • Generic Heat Pump Water Water,
  • Generic Heat Pump Water Air....

This is like with you case the light. Today, in HA, if the light is not connected, we just need a switch that is wire to the bulb, and we can call the switch switch_for_my_light, but I understant your aim to rebuild a switch and recall it Generic Light.

Please tell me where I'm wrong here ?

@dgomes
Copy link
Contributor Author

dgomes commented Feb 10, 2021

@afaucogney I understand you have issues with the concept of water_heater more then with the concept of generic_water_heater, please understand I'm not proposing a new component, I'm proposing a new instantiation of such component.

This has been discussed previously in:
#17058
home-assistant/architecture#59

Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@dgomes
Copy link
Contributor Author

dgomes commented Aug 9, 2021

I can understand that for a generic integration that is no more then a template packaged as an integration. But if you add logic (which would otherwise would be a better fit for python script) I think there is room for the generic platform.

Is the general consensus described in any architectural issue / discussion ?

@frenck
Copy link
Member

frenck commented Aug 9, 2021

It needs to be formalized, I'm just hitting the breaks here, before we merge stuff (like generic humidifier was) that we actually don't want anymore. We have considered reverting the generic humifier.

@github-actions
Copy link

github-actions bot commented Nov 7, 2021

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 7, 2021
@dgomes dgomes removed the stale label Nov 7, 2021
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 17, 2022
@dgomes dgomes removed the stale label Feb 17, 2022
@BeardedTinker
Copy link

I'm a bit concerned about this integration. While I do think it's awesome idea, this also rings a lot of bells for thermal runaway - something that cheap/malfunctioning 3d printers are known for.
Detecting unknown temperature sensor is great, but it doesn't resolve all potential cases where bad thermometer/thermostat could keep heating water when not needed.
3D printers have PID tuning to learn heating curve for heater and if it doesn't match it or intended temperature, it shuts down to protect from potential fire hazard.

@dgomes
Copy link
Contributor Author

dgomes commented Jun 21, 2022

I'm a bit concerned about this integration. While I do think it's awesome idea, this also rings a lot of bells for thermal runaway - something that cheap/malfunctioning 3d printers are known for.
Detecting unknown temperature sensor is great, but it doesn't resolve all potential cases where bad thermometer/thermostat could keep heating water when not needed.
3D printers have PID tuning to learn heating curve for heater and if it doesn't match it or intended temperature, it shuts down to protect from potential fire hazard.

Thank you for commenting, this is an initial PR which should be as basic as possible to ease the review process. Of course we can add many other features in future PR's.

Since this PR is blocked I'm maintaining a HACS fork https://github.com/dgomes/ha_generic_water_heater

@davet2001
Copy link
Contributor

@BeardedTinker The problems you describe are not eliminated by a PID or any software. Most water heaters include a thermal cutout for this reason, i.e. they have a built in mechanical device such as a bi-metalic strip which shuts off power above a certain temperature.

I don't think we should claim that the integration has any dependable safety features - in my view the user should ensure that their system will be safe even if the output is stuck in the on state.

Copy link

@Rudd-O Rudd-O left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a useful thing, in general.

)


async def async_setup(hass, hass_config):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrate to async_setup_entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, but this integration is YAML only, so no config_entries

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 22, 2023
@dgomes dgomes removed the stale label Jan 22, 2023
@marcelveldt
Copy link
Member

I've marked this PR as draft, as changes are requested that need to be processed or, in this case, more discussion is needed.
Please un-draft it once it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

Marcel

Learn more about our pull request process.

@marcelveldt marcelveldt marked this pull request as draft June 7, 2023 10:38
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 7, 2023
@Rudd-O
Copy link

Rudd-O commented Sep 18, 2023

@dgomes can you restart activity on this PR?

@dgomes
Copy link
Contributor Author

dgomes commented Sep 18, 2023

@dgomes can you restart activity on this PR?

Given the current stance, better discuss this at https://github.com/dgomes/ha_generic_water_heater

@github-actions github-actions bot removed the stale label Sep 21, 2023
@frenck
Copy link
Member

frenck commented Oct 6, 2023

I'm going to close and decline this PR for now, until we have a proper way to handle with "generic" type of integrations. As shown above, there is now a custom integration in case one really wants this (although, of course, is not supported by the Home Assistant project).

../Frenck

@frenck frenck closed this Oct 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Cancelled
Status: Cancelled
Development

Successfully merging this pull request may close these issues.

10 participants