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

[sungrow] Initial contribution #15130

Merged
merged 30 commits into from
Feb 19, 2024
Merged

[sungrow] Initial contribution #15130

merged 30 commits into from
Feb 19, 2024

Conversation

soenkekueper
Copy link
Contributor

@soenkekueper soenkekueper commented Jun 22, 2023

PR Content

This PR implements a new modbus binding for openHAB, that integrates Sungrow inverters.
With this binding its very easy to monitor the current values for PV production within the smart home system - without the need to worry about modbus registers, types and transformations. It's implemented along the Sungrow specification "Communication Protocol of Residential Hybrid Inverter V1.0.23",
which can be found here: bohdan-s/SunGather#36.

The binding adds a new thing type "sungrow inverter" that uses the default modbus bridges and needs no further configuration.

ToDos

- Check provided values for battery information
- complete documentation

  • Test compatibility with other sungrow inverters.

This binding was already mentioned within thread https://community.openhab.org/t/sungrow-inverter-binding-or-modbus-config/139360/17.

@soenkekueper soenkekueper requested a review from a team as a code owner June 22, 2023 11:47
@soenkekueper soenkekueper added new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged labels Jun 22, 2023
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/sungrow-binding/147442/1

CODEOWNERS Outdated Show resolved Hide resolved
CODEOWNERS Show resolved Hide resolved
@soenkekueper soenkekueper force-pushed the sungrow branch 3 times, most recently from efb341b to cbee75a Compare December 18, 2023 20:15
@soenkekueper soenkekueper removed the work in progress A PR that is not yet ready to be merged label Dec 18, 2023
@soenkekueper soenkekueper changed the title [sungrow][WIP] Initial contribution [sungrow] Initial contribution Dec 18, 2023
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Looks very solid. Just some small comments you might want to look at.

Edit: Also some javadoc are incomplete like the public methods in SungrowInverterRegisters.java lack the parameter and return.

Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
…king. Added categories and tags for channels.

Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
… (instead of floats issues).

Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
soenkekueper and others added 7 commits January 1, 2024 19:07
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
@soenkekueper
Copy link
Contributor Author

Thanks for your review @lsiepel.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Some more comments, i also noticed that there are some todo's in the start post. Are they done?

bundles/org.openhab.binding.modbus.sungrow/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.modbus.sungrow/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.modbus.sungrow/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.modbus.sungrow/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.modbus.sungrow/README.md Outdated Show resolved Hide resolved
ericbodden and others added 7 commits January 13, 2024 20:51
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
@soenkekueper
Copy link
Contributor Author

Some more comments, i also noticed that there are some todo's in the start post. Are they done?

Yes, battery information is working and documentation is updated.
As i've only one inverter, i'm not able to test more. But on forum there are hints on others, that are working.

@soenkekueper
Copy link
Contributor Author

@lsiepel Is everything ok now, or do you have any further comments / questions?

@lsiepel
Copy link
Contributor

lsiepel commented Feb 10, 2024

@lsiepel Is everything ok now, or do you have any further comments / questions?

Some comments are marked as resolved, but i don't see any change or comment. Did you push all changes? Can you check all of them and add a comment like 'fxied' or the reason why it is not canged?

Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
@soenkekueper
Copy link
Contributor Author

Sorry i don't know what happend - maybe some rebase/merging error on my side. sorry for that.
Now all remarks should be resolved - exept the sitemap - i don't have one as i'm only using the main-ui.
Is it required?

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks for providing this new binding. LGTM

@lsiepel lsiepel merged commit 31e8869 into openhab:main Feb 19, 2024
3 checks passed
@lsiepel lsiepel added this to the 4.2 milestone Feb 19, 2024
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* 0000: Implementation

---------

Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Co-authored-by: Wouter Born <github@maindrain.net>
Co-authored-by: Eric Bodden <eric.bodden@upb.de>
Co-authored-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* 0000: Implementation

---------

Signed-off-by: Sönke Küper <soenkekueper@gmx.de>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Co-authored-by: Wouter Born <github@maindrain.net>
Co-authored-by: Eric Bodden <eric.bodden@upb.de>
Co-authored-by: Leo Siepel <leosiepel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants