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 Luxtronik integration for multiple heat pump models #27230

Closed
wants to merge 35 commits into from
Closed

Add Luxtronik integration for multiple heat pump models #27230

wants to merge 35 commits into from

Conversation

Bouni
Copy link
Contributor

@Bouni Bouni commented Oct 5, 2019

Description:

Add support for heat pump units controlled by a Luxtronik controller.

Manufacturers that use Luxtronik heat pump controllers are:

  • Alpha Innotec
  • Siemens Novelan
  • Roth
  • Elco
  • Buderus
  • Nibe
  • Wolf Heiztechnik

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10621

Example entry for configuration.yaml (if applicable):

luxtronik:
  host: 192.168.178.10
  port: 8889

Checklist:

  • [ x ] The code change is tested and works locally.
  • [ x ] Local tests pass with tox. Your PR cannot be merged unless tests pass
  • [ x ] There is no commented out code in this PR.
  • [ x ] I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@Bouni Bouni changed the title Add Luxtronik integration for multiple heat pump models [WIP] Add Luxtronik integration for multiple heat pump models Oct 14, 2019
@Bouni
Copy link
Contributor Author

Bouni commented Oct 14, 2019

I maybe change the naming scheme depending on what the users of my custom component vote for.
Furthermore I might add a climate component as @renevdzee did in his fork of my custom component

@Bouni Bouni changed the title [WIP] Add Luxtronik integration for multiple heat pump models Add Luxtronik integration for multiple heat pump models Nov 15, 2019
@Bouni
Copy link
Contributor Author

Bouni commented Nov 15, 2019

I think I'm done. I have this component running with my heatpump for quite a while and havn't encountered any problems so far.

Copy link
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

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

Just some small stuff to start the reviewing process :)

homeassistant/components/luxtronik/services.yaml Outdated Show resolved Hide resolved
Copy link
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

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

Just some more small stuff to get you started :)

homeassistant/components/luxtronik/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/binary_sensor.py Outdated Show resolved Hide resolved
@Bouni
Copy link
Contributor Author

Bouni commented Dec 10, 2019

I'll take a look at all your points soon!

@remyderuysscher
Copy link

@springstan @Bouni What's the hold up? I would to see this component merged asap. If there is anything I can do to help?

@Bouni
Copy link
Contributor Author

Bouni commented Jan 23, 2020

@remyderuysscher I'm very busy at the moment and whenever @springstan asked me to change certain things, it took several days until I was able to react. Thats probably the main reason why this takes longer than usual.

@Bouni
Copy link
Contributor Author

Bouni commented Jan 27, 2020

@springstan Is there anything else you wan't me to change / improve?

@remyderuysscher
Copy link

@remyderuysscher I'm very busy at the moment and whenever @springstan asked me to change certain things, it took several days until I was able to react. Thats probably the main reason why this takes longer than usual.

I understand, I'm here to help if needed!

homeassistant/components/luxtronik/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Show resolved Hide resolved
homeassistant/components/luxtronik/sensor.py Outdated Show resolved Hide resolved
port = conf[CONF_PORT]
safe = conf[CONF_SAFE]

luxtronik = LuxtronikDevice(host, port, safe)
Copy link
Member

Choose a reason for hiding this comment

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

Can you verify here that the connection works ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could check if the used lib got data from the heatpump?
Is that an option?

self._host = host
self._port = port
self._luxtronik = Lux(host, port, safe)
self.update()
Copy link
Member

@balloob balloob Feb 9, 2020

Choose a reason for hiding this comment

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

No side effects in the constructor, pass True as second arg to add_entities instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give an other integration as an example?


self._host = host
self._port = port
self._luxtronik = Lux(host, port, safe)
Copy link
Member

Choose a reason for hiding this comment

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

Is this data shared between multiple devices ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean in case somebody uses this integration fro two heatpumps to connect to?

@bdraco
Copy link
Member

bdraco commented Mar 23, 2020

Hi @Bouni , it looks like this one got pretty close. Are you still working on this. PR?

@Bouni
Copy link
Contributor Author

Bouni commented Mar 23, 2020

@bdraco Kind of, I wait for one of the maintainers to look at the latest changes I made, on the other hand, there are some open issues I don't know how to fix 😞
For example the requested "Data Update Coordinator" thing.
I fear that I'm not a experienced enough programmer to get this to a point that the maintainers will merge this.

@frenck
Copy link
Member

frenck commented Mar 23, 2020

Ok, @Bouni Let's resolve all comments you've handled. That way you get a better overview of what is open and what is done.
There are some comments open that definitely still can be worked on...

@Bouni
Copy link
Contributor Author

Bouni commented Mar 24, 2020

@frenck Highly appreciate your help!
Maybe a stupid question, but do I have to resolve them and if so by clicking the Resolve conflicts button? I'm not very experianced with the process here on GitHub.

Copy link
Member

frenck commented Mar 24, 2020

Yes, you can close a review comment by clicking resolve (only do that when it is actually resolved ;) ) After that, you will be left with things that need either discussion or still need adjustment. Which makes it easier to overview for you and everybody else.

@Schack17
Copy link

@Bouni
It would be great if the module were soon available in home assistant. Can anyone help?

@Bouni
Copy link
Contributor Author

Bouni commented Apr 17, 2020

@Schack17 I will continue on this next week hopefully!

@VincentVegaOne
Copy link

that integration looks great and is exactly what i need! Excited to test it when its ready.

@Bouni
Copy link
Contributor Author

Bouni commented Apr 30, 2020

I think I'll release a HACS version of this today so that people can start using it.

After thats done, I can focus on fixing all the remaining issues with this PR.
I think that I need to create a config flo for this a swell as it it required no I guess!?
@frenck Is that the case now for all new integrations?

@Bouni
Copy link
Contributor Author

Bouni commented Apr 30, 2020

OK, the HACS repo is done, until it is integrated with HACS officially, you can add it as a custom repo in HACS using the Github URL: https://github.com/Bouni/luxtronik

@remyderuysscher
Copy link

@Bouni the code crashes on parsing a 10.0.0.0/8 IP address.

File "/usr/local/lib/python3.7/ipaddress.py", line 1312, in init
self._check_int_address(address)
File "/usr/local/lib/python3.7/ipaddress.py", line 424, in _check_int_address
self._version))
ipaddress.AddressValueError: 4466016928 (>= 2**32) is not permitted as an IPv4 address

@Bouni
Copy link
Contributor Author

Bouni commented May 4, 2020

@remyderuysscher Please do not report issues with the HACS version here!
The repo you're looking for is https://github.com/Bouni/luxtronik

@stale
Copy link

stale bot commented Jun 3, 2020

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.

@stale stale bot added the stale label Jun 3, 2020
@frenck
Copy link
Member

frenck commented Jun 3, 2020

This PR is running stale and has requested changes. Closing this PR for now. Feel free to re-open when ready to work on it again.

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants