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

[solarman] Initial contribution #16835

Merged
merged 5 commits into from
Jul 30, 2024
Merged

Conversation

catalinsanda
Copy link
Contributor

As asked by @mstormi on the forum (https://community.openhab.org/t/new-solarman-v5-binding-for-deye-sofar-and-other-type-of-solar-inverters/146112/49), I'm opening a PR for the Solarman Binding.

This is a binding used to communicate with Solarman (IGEN-Tech) v5 based solar inverter data loggers in direct-mode over the local network.

@catalinsanda catalinsanda requested a review from a team as a code owner June 1, 2024 16:22
@lsiepel lsiepel changed the title Initial contribution for the Solarman Binding. [solarman] Initial contribution Jun 1, 2024
@lsiepel lsiepel added the new binding If someone has started to work on a binding. For a new binding PR. label Jun 1, 2024
@catalinsanda
Copy link
Contributor Author

The build is failing and I don't know why. mvn clean install -pl :org.openhab.binding.solarman works locally. Can somebody point me in the right direction?

@lsiepel
Copy link
Contributor

lsiepel commented Jun 2, 2024

The build is failing and I don't know why. mvn clean install -pl :org.openhab.binding.solarman works locally. Can somebody point me in the right direction?

add the binding to bom/openhab-addons/pom.xml and also add yourself to the codeowner file

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 your contribution. This is a partial review.
Besides these comments, you can also check that all SAT errors are fixed and no compilation warnings occur.

Edit: And fix DCO as that is mandatory.

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.

While checking if the commit fixed the issues, i concluded not all review comments are fixed yet. I also found some new issues.

@catalinsanda
Copy link
Contributor Author

My bad @lsiepel. I'm adding commits to the PR as I get time to implement some of the changes.
When done I'll rebase and squash them and let you know that it's ready for another round of review.

@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/new-solarman-v5-binding-for-deye-sofar-and-other-type-of-solar-inverters/146112/56

@catalinsanda catalinsanda force-pushed the initial_commit branch 2 times, most recently from 0e87c38 to 6ae22f5 Compare June 8, 2024 19:47
@catalinsanda
Copy link
Contributor Author

@lsiepel - can you please give it another go?

There are still a few SAT warnings, let me know if any are blockers.

@catalinsanda catalinsanda requested a review from lsiepel June 8, 2024 20:08
@catalinsanda
Copy link
Contributor Author

ping @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.

Unfortunately my time is limited today, but i will come back to this in the coming days. In the mean time it would be nice if you can fix these comments and also fix all SAT warnings. It is nice to see that there are no compile warnings. Merging is not far away.

@catalinsanda catalinsanda force-pushed the initial_commit branch 3 times, most recently from 35eb715 to 7723015 Compare June 23, 2024 08:21
@catalinsanda
Copy link
Contributor Author

@lsiepel - fixed all SAT warnings and notices. Please give it another go.

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.

Covered all files. Did not look a build warnings and SAT, please double check there are no build warnings and no SAT errors.

@catalinsanda
Copy link
Contributor Author

catalinsanda commented Jun 30, 2024

@lsiepel - implemented CR suggestions.

For some reason I can't comment inline on #16835 (comment), so I'm putting my answer here.

I can't put the UoM in the channel definition/Item type because those channels are used for user defined requests, and the type of the value returned depends on the registers being read. Here is an example of such a usage:

Thing solarman:logger:local [ hostname="x.x.x.x", inverterType="deye_sg04lp3", serialNumber="1234567890", additionalRequests="0x03:0x27D-0x27E" ] {
Channels:
	Type number : inverter-frequency [scale="0.01", uom="Hz", rule="3", registers="0x27E"]
}

@catalinsanda
Copy link
Contributor Author

ping @lsiepel - can you please have another look?

@catalinsanda catalinsanda requested a review from wborn July 9, 2024 20:35
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 addressing allmost all comments. There are 2 or 3 open ones from previous review rounds. This looks like the last review round before we can proceed to merge.

Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
@catalinsanda catalinsanda force-pushed the initial_commit branch 2 times, most recently from e17ccbf to fad80ad Compare July 20, 2024 10:03
Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
@catalinsanda
Copy link
Contributor Author

@lsiepel - let's try again.

@lsiepel
Copy link
Contributor

lsiepel commented Jul 26, 2024

I'll try to look at this next week.

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.

LGTM.

Now, you could add your binding's logo to the openHAB website. See https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui

@lsiepel lsiepel merged commit 1cd7a4f into openhab:main Jul 30, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Jul 30, 2024
digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Aug 29, 2024
* Initial commit for the Solarman Binding.

Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
* Initial commit for the Solarman Binding.

Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* Initial commit for the Solarman Binding.

Signed-off-by: Catalin Sanda <catalin.sanda@gmail.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
* Initial commit for the Solarman Binding.

Signed-off-by: Catalin Sanda <catalin.sanda@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.

4 participants