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

New integration for Kostal Plenticore solar inverters #43404

Merged
merged 23 commits into from
Apr 7, 2021
Merged

New integration for Kostal Plenticore solar inverters #43404

merged 23 commits into from
Apr 7, 2021

Conversation

stegm
Copy link
Contributor

@stegm stegm commented Nov 19, 2020

Proposed change

Adds an integration for Kostal Plenticore solar inverters (https://www.kostal-solar-electric.com/de-de/produkte/solar-wechselrichter/plenticore-plus). This component reads process data and setting values via REST-API and is also capable to change some of the inverter settings (battery minimal SoC for example). It uses a library which was inspired by an app https://github.com/kilianknoll, but the one for this integration uses async requests and has a dynamic data model which allows it to use it with different firmware versions.

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:

This component uses Config Flow for configuration. Two information are needed: Host or IP and password.

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:

@homeassistant
Copy link
Contributor

Hi @stegm,

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!

@MartinHjelmare MartinHjelmare changed the title New integration for Kostal Plenticore solar inverters. New integration for Kostal Plenticore solar inverters Nov 19, 2020
@stegm stegm marked this pull request as ready for review November 20, 2020 23:05
@pergolafabio
Copy link

this will not work for kostal piko covertors, right?

@stegm
Copy link
Contributor Author

stegm commented Nov 22, 2020

this will not work for kostal piko covertors, right?

@pergolafabio I'm not sure, but as far as I know from the https://www.photovoltaikforum.com the piko inverters uses a different API. Perhaps you can try https://www.msxfaq.de/sonst/iot/kostal15.htm.

@pergolafabio
Copy link

i am using this custom right now, not sure if its using the same api, or just webscraping
https://github.com/gieljnssns/kostalpiko-sensor-homeassistant

@stegm
Copy link
Contributor Author

stegm commented Nov 22, 2020

i am using this custom right now, not sure if its using the same api, or just webscraping
https://github.com/gieljnssns/kostalpiko-sensor-homeassistant

The KostalPyko library which is used in their integration uses a different method as the one for the plenticore.

@pergolafabio
Copy link

Make you can integrate it also? That's makes a new integration possible for older invertor too :-)

No need for custom then

@stegm
Copy link
Contributor Author

stegm commented Nov 22, 2020

Make you can integrate it also? That's makes a new integration possible for older invertor too :-)

I think devices with separate communication libraries should be implemented in separate integrations. I have no possiblity to test other devices. Besides there will be only little common code between the two integrations.

@pergolafabio
Copy link

Ok, np :-)

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

First impression is that the integration looks complicated and not in line with our design standards. I would advice to try and simplify this, at least for the first PR. Remove the writing of data and just focus on getting the reading (data fetching) done good.

homeassistant/components/kostal_plenticore/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/sensor.py Outdated Show resolved Hide resolved
@stegm stegm requested a review from MartinHjelmare January 2, 2021 22:24
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Nice redesign!

homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/const.py Outdated Show resolved Hide resolved
tests/components/kostal_plenticore/test_config_flow.py Outdated Show resolved Hide resolved
@stegm
Copy link
Contributor Author

stegm commented Jan 6, 2021

@MartinHjelmare Thank you for the review! I learned a lot.

@stegm stegm requested a review from MartinHjelmare January 6, 2021 19:34
@menkej
Copy link

menkej commented Feb 18, 2021

What is the status of this PR, @MartinHjelmare? It seems that all requested changes have been done. I'd really love to integrate my Plenticore into HA...

@MartinHjelmare
Copy link
Member

Sorry, I'll get back to this asap.

homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/helper.py Outdated Show resolved Hide resolved
tests/components/kostal_plenticore/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/kostal_plenticore/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/kostal_plenticore/test_config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/const.py Outdated Show resolved Hide resolved
homeassistant/components/kostal_plenticore/sensor.py Outdated Show resolved Hide resolved
@MartinHjelmare
Copy link
Member

The branch is tainted with unrelated commits now. Please rebase and remove those commits.

@stegm stegm requested a review from MartinHjelmare April 6, 2021 18:15
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare MartinHjelmare merged commit 589f224 into home-assistant:dev Apr 7, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2021
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