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 Water Heater platform to MQTT integration #93644

Merged
merged 18 commits into from Jun 8, 2023

Conversation

hookedonunix
Copy link
Contributor

@hookedonunix hookedonunix commented May 27, 2023

Proposed change

An addition to the existing MQTT component that allows support for the water heater entity. The code lends heavily from the MQTT climate integration, as the functionality is mostly a simplified version of it.

I'm building this to communicate with a desktop vaporizer I own, which I've been able to test it successfully with. It's built to the spec of the base water heater component but I don't own an actual smart water heater to test it with, so some functionality I've just tested with mocks.
This PR is paired with this one, which will allow for the temperature of water heater entities to be controllable through the google assistant component.

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)
  • 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

I've never contributed to the climate entity (or hass) before, so all my knowledge is assumptions based on the code. With that in mind, I have a couple questions that have come up as I've been working on it:

  • The ATTR_TEMP_HIGH and ATTR_TEMP_LOW attributes are in both the climate and water heater entities, which seem to control the target temperature range, if applicable. They seem to be implemented in the climate entity, but I can't see any way of setting them in the water heater entity, are these actually used or were they never implemented for the water heater? A test I saw for the water heater only accepted a single temperature instead of a range.
  • The power and away topics and commands have been deprecated from the mqtt climate integration. I've not included power, but did include the away topic, as the base water heater entity has them and from what I could find the reason for the removal wasn't relevant for the water heater. If someone who knows more could advise I would appreciate it though!

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
  • I have followed the perfect PR recommendations
  • 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hi @hookedonunix

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

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

Code owner commands

Code owners of mqtt 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 mqtt Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@hookedonunix hookedonunix marked this pull request as ready for review May 27, 2023 07:19
@hookedonunix hookedonunix changed the title Mqtt water heater Add Water Heater platform to MQTT integration May 27, 2023
tests/components/mqtt/test_water_heater.py Outdated Show resolved Hide resolved
tests/components/mqtt/test_water_heater.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution, that is very appreciated. Do you have a specific device in mind that needs MQTT support. The problem with the current climate/HAVC component is that it has a lot of topics and templates, which makes tot all very complex.
I hope we can avoid that for new integration. Further we should try to keep a PR for a new platform minimal.

So I suggest we only introduce a command_topic and state_topic and add a command plus value template for them. Further it would be a good idea to allow parsing messages on state_topic as json. For the command topic we can use the attributes as template variables. An inplementation like this the the siren platform. May be you can have a look at that?

May be that will not work. I suggest we start with operational mode and set temperature services. They should have their own topics, but we should avoid code duplication. There is some code that can be cleaned up from climate first (deprecated stuff). I'll see if that can be removed now. Reused constants of climate need to move to be moved const.py.

Last question is:
Are there any water heaters known that support MQTT yet?

@home-assistant
Copy link

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

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft May 28, 2023 18:28
@hookedonunix
Copy link
Contributor Author

hookedonunix commented May 29, 2023

@jbouwh
I'm using this to communicate with the Volcano Hybrid which is a desktop vaporizer that exposes bluetooth. I've adapted a Bluetooth to MQTT package to keep most of the logic out of Home Assistant, but have been trying to find the best way to integrate it. I started with MQTT Climate but was having trouble getting Google Assistant working as they place a limit on the temperature setting (the device operates from 40C to 230C), so the water heater entity seemed like the best fit. I really only need current temp (topic) and target temp (command and topic) as I'm controlling the heat and fan with separate switches, but I imagine mode might be useful for other people using the water heater component.

I wrote it with "batteries included" but agree it's definitely over-complicated, I think the minimum you could get it down to would be something like the TemperatureControl trait, which exposes:

  • Min temp
  • Max temp
  • Current temp
  • Target temp
  • Temperature unit
  • Precision (maybe?)

To remove some of the clutter I'll take out the temp high/low and away mode, as I don't think either are needed. Let me know if you have any other recommendations, thank you!

To answer your last question I can see a few people expressing interest a couple years ago, as well as a Tasmota OpenTherm module. I'll have a look through that now.

@jbouwh
Copy link
Contributor

jbouwh commented May 29, 2023

Thanx that helps.

@jbouwh
Copy link
Contributor

jbouwh commented May 29, 2023

May be we can somehow integrate the two classes in one. If we keep the the platform setup we could easily implement an common class that implements the shared methods. We should only take out the not shared elements.

@hookedonunix
Copy link
Contributor Author

I like the sound of that, like class MqttTemperatureEntity(MqttEntity): for the parent and class MqttClimateEntity(MqttTemperatureEntity, ClimateEntity): for the child as an example?

@jbouwh
Copy link
Contributor

jbouwh commented May 29, 2023

Yeah,

I like the sound of that, like class MqttTemperatureEntity(MqttEntity): for the parent and class MqttClimateEntity(MqttTemperatureEntity, ClimateEntity): for the child as an example?

That is the idea. So we can combine all common functionality into MqttTemperatureEntity

@hookedonunix hookedonunix marked this pull request as ready for review June 8, 2023 11:23
Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

Two small comments on doc strings.

homeassistant/components/mqtt/climate.py Outdated Show resolved Hide resolved
homeassistant/components/mqtt/water_heater.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft June 8, 2023 14:10
Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

LGTM,
Thnx @hookedonunix !

@jbouwh jbouwh marked this pull request as ready for review June 8, 2023 14:30
Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@hookedonunix
Copy link
Contributor Author

Cheers! 🎉

@jbouwh jbouwh merged commit 18cbc9b into home-assistant:dev Jun 8, 2023
52 checks passed
@frenck frenck added the noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Jun 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed core has-tests integration: mqtt new-feature new-platform noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) Quality Scale: gold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants