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 Support for Luxtronik2 heatpumps #86636

Closed
wants to merge 16 commits into from
Closed

Conversation

BenPru
Copy link

@BenPru BenPru commented Jan 25, 2023

Breaking change

The hacs integration Bouni/luxtronik has the same domain. Perhaps it conflict. Please upgrade Bouni/luxtronik to the latest version to ensure the changed and current domain.
This integration replaces the custom_component integration BenPru/luxtronik. The config and sensor keys are compatible and are converted. But the yaml configuratin is ignored. Please use Bouni/luxtronik if you wan't use yaml configuration.

Proposed change

Add new integration to read out and control a luxtronik2 heatpump.
Manufacturers that use Luxtronik heat pump controllers are: Alpha Innotec, Siemens Novelan, Roth, Elco, Buderus, Nibe and Wolf Heiztechnik.
The full integration is currently available as custom_component BenPru/luxtronik.

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

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: Only things to complete not full realized ha core module water_heater.
  • 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:

Open

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:

@Bouni
Copy link
Contributor

Bouni commented Jan 27, 2023

@frenck You've said (here) that a integration for Luxtronik controllers should support all versions and you don't want to have seperate integartions vor different versions of that controller.
Do you still insist on that? We discussed if it would make sense to have both Luxtronik controller geneartions in the same integration and came to the conclusion that it doesn't make sense as the use a completely different protocoll and barely share the same functionality.
It would be awesome if you could give us some advice how to proceed!

@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 Apr 29, 2023
@BenPru
Copy link
Author

BenPru commented Apr 30, 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.

Still open!

@github-actions github-actions bot removed the stale label Apr 30, 2023
@scaronni
Copy link

scaronni commented May 5, 2023

How can we make sure someone takes this up for review? Thanks.

Copy link
Member

@gjohansson-ST gjohansson-ST left a comment

Choose a reason for hiding this comment

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

Gave some comments on the initial files in the beginning.
There seems to be a bit off cleaning needed.
Also needs to remove translations and other legacy things as it's been here for a while I think

from .const import CONF_COORDINATOR, DOMAIN, PLATFORMS
from .coordinator import LuxtronikCoordinator

# endregion Imports
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# endregion Imports

async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Set up Luxtronik from a config entry."""

hass.data.setdefault(DOMAIN, {})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hass.data.setdefault(DOMAIN, {})

Comment on lines +25 to +27
data = hass.data.setdefault(DOMAIN, {})
data[entry.entry_id] = {}
data[entry.entry_id][CONF_COORDINATOR] = coordinator
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = hass.data.setdefault(DOMAIN, {})
data[entry.entry_id] = {}
data[entry.entry_id][CONF_COORDINATOR] = coordinator
hass.data.setdefault(DOMAIN, {})[entry.entry_id] = coordinator

Can set the coordinator directly there as you probably aren't going to store anything else

Comment on lines +31 to +32
# Trigger a refresh again now that all platforms have registered
hass.async_create_task(coordinator.async_refresh())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Trigger a refresh again now that all platforms have registered
hass.async_create_task(coordinator.async_refresh())

No need

# Trigger a refresh again now that all platforms have registered
hass.async_create_task(coordinator.async_refresh())

# hass.config_entries.async_setup_platforms(entry, PLATFORMS)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# hass.config_entries.async_setup_platforms(entry, PLATFORMS)

# endregion Imports


PORT_SELECTOR = vol.All(
Copy link
Member

Choose a reason for hiding this comment

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

Set the NumberSelector directly in STEP_USER_DATA_SCHEMA and then convert to int in validation or when reading the value

Comment on lines +41 to +43
def _get_options_schema() -> vol.Schema:
"""Build and return the options schema."""
return vol.Schema({})
Copy link
Member

Choose a reason for hiding this comment

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

What's this?

"""Return config entry title."""
return self._title

async def async_step_options(
Copy link
Member

Choose a reason for hiding this comment

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

What's this meant to do and why not part of the normal user step?

Comment on lines +183 to +200
async def async_step_init(
self, user_input: dict[str, Any] | None = None
) -> FlowResult:
"""Manage the options."""
return await self.async_step_user(user_input)

async def async_step_user(self, user_input=None) -> FlowResult:
"""Handle a flow initialized by the user."""
if user_input is not None:
return self.async_create_entry(title="", data=user_input)
coordinator = LuxtronikCoordinator.connect(self.hass, self.config_entry)
title = f"{coordinator.manufacturer} {coordinator.model} {coordinator.serial_number}"
name = f"{title} ({self.config_entry.data[CONF_HOST]}:{self.config_entry.data[CONF_PORT]})"
return self.async_show_form(
step_id="user",
data_schema=_get_options_schema(),
description_placeholders={"name": name},
)
Copy link
Member

Choose a reason for hiding this comment

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

This does not appear to give the user any chance to change anything?

)


# config_entry_flow.register_discovery_flow(DOMAIN, "Luxtronik", _async_has_devices)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# config_entry_flow.register_discovery_flow(DOMAIN, "Luxtronik", _async_has_devices)

@home-assistant home-assistant bot marked this pull request as draft June 20, 2023 21:43
@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.

BenPru referenced this pull request in BenPru/luxtronik Sep 10, 2023
@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 Sep 18, 2023
@scaronni
Copy link

Bumping to keep the review alive. The extension works perfectly in my setup, have been using it for more than a year.

Thanks!

@frenck
Copy link
Member

frenck commented Nov 29, 2023

Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale.

Feel free to re-open this PR when you are ready to pick up work on it again 👍

@frenck frenck closed this Nov 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants