[sonnen] Initial contribution of new binding for solar battery

[chingon007]

Initial contribution for a new binding which communicates with a solar battery from sonnen. It is a simple read only binding which just reads the values of the solar battery.

I mentioned my initiative in the forum here: https://community.openhab.org/t/new-binding-sonnen-battery/129178

Regards
Chingon

[lolodomo]

You broke the BOM POM (you forgot a tag). That's the reason of the failing build.

[chingon007]

Thanks, just a small copy & paste error. Now it should work.

[chingon007]

@lolodomo Ready to review. Clean and building :-)

[lolodomo]

@lolodomo Ready to review. Clean and building :-)

Bravo ;)

I will review your proposal certainly next year now ;)

[openhab-bot]

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

https://community.openhab.org/t/new-binding-sonnen-battery/129178/2

[lolodomo]

Review part 1 of 2

[lolodomo]

When I try to compile your branch, I have errors with wrong copyrights. It looks like the compiler is using the check with 2021 and not 2022. How do you solve that to get a full compilation (mvn clean install) ?

Edit: works of course with mvn clean install -DskipChecks

[lolodomo]

I just open a PR to your repo to provide the default translations properties file.
It is the result of the command mvn i18n:generate-default-translations
Of course, it will be required to update it if you change your XML thing file.

[chingon007]

I had the same problem. I was wondering how to update the local mvn install. However I switched back the copyright 2021 in order to compile and then changed it back to 2022

[lolodomo]

I created another PR that resolves the @Nullable config parameter. I see no compilation or SAT errors.

Note that you should move all your classes in internal packages. I discovered that some of them are not. I let you change that.

[lolodomo]

I created a last PR with an improved class SonnenJSONCommunication resolving our discussions about nullable batteryData and some of my other review comments for this class. It is ok to keep batteryData nullable but it has to be set to null when appropriate.

[lolodomo]

I submitted to your repo a new PR to finalize the thing definition and the documentation.

[chingon007]

I submitted to your repo a new PR to finalize the thing definition and the documentation.

Thats great. Very much appreciated.

[chingon007]

And to have something clean, your class Helper should be replaced by an exception class. I will create another PR when you have made progress on the other points.

I will work on that in the next days, as soon as I have time again.

[lolodomo]

I submitted a PR with my last enhancements regarding java code.

[lolodomo]

@chingon007 : you implemented your thing handler in such a way the refresh job is started only when at least one channel is linked. This is correct in theory but adds a lot of complexity to your thing handler, while in practice you have no user that will create a thing without linking at least one channel. In fact, this will only happen during a short time when setting up the binding.
In my opinion, it would be ok to start your refresh job without checking if one channel is linked. Like that, your handler will be very simple, no need to override channelLinked, channelUnlinked and to have your map linkedChannels. You just have to handle the REFRESH command.

I let you decide, I think the code is OK but you have to test it seriously in case you want to keep your optimization..

[chingon007]

I submitted a PR with my last enhancements regarding java code.

Thanks a lot. I appreciate it. With this change I will make an enhancement to my other older binding in order to improve the code there as well.

[chingon007]

@chingon007 : you implemented your thing handler in such a way the refresh job is started only when at least one channel is linked. This is correct in theory but adds a lot of complexity to your thing handler, while in practice you have no user that will create a thing without linking at least one channel. In fact, this will only happen during a short time when setting up the binding. In my opinion, it would be ok to start your refresh job without checking if one channel is linked. Like that, your handler will be very simple, no need to override channelLinked, channelUnlinked and to have your map linkedChannels. You just have to handle the REFRESH command.

I let you decide, I think the code is OK but you have to test it seriously in case you want to keep your optimization..

Thanks for the hint. I took that implementation from a other binding when I developed my first binding over a year ago. I understand what you mean. For the moment I will keep it and consider it to change it in the future.

[lolodomo]

Thanks a lot. I appreciate it. With this change I will make an enhancement to my other older binding in order to improve the code there as well.

Note that the way I changed with your method now returning the error message or an empty string is probably not the best implementation. Using an exception would have been probably a better implementation.

Thanks for the hint. I took that implementation from a other binding when I developed my first binding over a year ago. I understand what you mean. For the moment I will keep it and consider it to change it in the future.

Ok

I will have a final look today but everything is certainly ok now. Do you need time to test again that everything is still working or can I merge if I have no other comments ?

[chingon007]

I made just now the final testing. Found some typos. After this recent commit is passed it can be merged. Thanks a lot for your contribution.

[lolodomo]

I just submitted a last PR adding 2 missing channels in README and with the last run of the i18n tool.

[chingon007]

I just submitted a last PR adding 2 missing channels in README and with the last run of the i18n tool.

Thanks. You are right. I forgot to add them.

[lolodomo]

Code LGTM

[lolodomo]

@chingon007 : thank your for contributing this new binding.

Now, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website