[solarmax] Initial contribution #10414

jamietownsend · 2021-03-29T20:10:27Z

See #10413

Add support for SolarMax PowerInverters - Initial contribution

openhab-bot · 2021-03-29T20:11:10Z

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

https://community.openhab.org/t/solarmax-initial-contribution-binding-for-solarmax-power-inverters/119879/1

Skinah · 2021-03-30T04:26:17Z

Two quick comments.

Please remove any commented out code.
Channels should be name with lowerCamelCase java naming convention, see developer guidelines documentation.

Also after you have compiled, look in the target folder for another folder called code-analysis and open the report to fix any things listed in there.

jamietownsend · 2021-04-06T13:47:00Z

I'm unsure what the next step is now. Do I just have to wait until 2 reviewers approve the implementation?

kaikreuzer · 2021-04-10T20:11:10Z

@jamietownsend First thing you'd need to do is to ensure that the "PR-openHAB-Addons" check is green. At the moment it fails with

[ERROR] Failed to execute goal org.openhab.tools.sat:sat-plugin:0.10.0:report (sat-all) on project org.openhab.binding.solarmax:
[ERROR] Code Analysis Tool has found 2 error(s)!
[ERROR] Please fix the errors and rerun the build.

And then: Yes, you'll have to wait for a maintainer to review and approve the code. Unfortunately, there is quite some queue of PRs waiting to be merged and maintainer time is limited. I therefore have to beg for your patience - but rest assured that your contribution is highly appreciated!

jamietownsend · 2021-04-13T07:27:05Z

@kaikreuzer Thanks for your message. The build is now green and ready for review.

bundles/org.openhab.binding.solarmax/src/main/resources/OH-INF/thing/thing-types.xml

bundles/org.openhab.binding.solarmax/README.md

Skinah · 2021-04-24T11:52:30Z

bundles/org.openhab.binding.solarmax/README.md

+
+## Thing Configuration
+
+The IP address and port number (default 12345) of the device needs to be configured.


Can you add the parameters to a table list like other bindings please? Same table that the channels are listed in.... They are: host portNumber refreshInterval, note that they are the camelCase versions that the textual people will need to know.

bundles/org.openhab.binding.solarmax/README.md

Skinah · 2021-04-24T11:57:21Z

bundles/org.openhab.binding.solarmax/README.md

+
+## Full Example
+
+Example Thing Configuration


This is not the intended use of this section, as historically it had the contents for things, items and sitemap files. If you don't use textual config, I would remove this section and just make sure the sections above are all case sensitive correct and documented so that an advanced textual person can work it out from your information.

Skinah · 2021-04-24T12:06:53Z

Thanks for the contribution. See my first round of comments above, which if you commit a suggestion please dont forget to add the signoff in the comment as the very last line so it does not stuff up the DCO signoff now that you have it passing.

Also please consider looking at this binding and how the UOM has been implemented.
#10100

If you change the channels to Number:Power or Number:Energy types instead of Number then you can get openHAB to graph the data automatically for you. This in done in the thing-types.xml file so just compare that file and also how the channel is updated from the handler to specify a unit.

Also see here:
https://www.openhab.org/docs/concepts/units-of-measurement.html

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

fwolter · 2021-12-11T11:56:15Z

@jamietownsend Did you address @Skinah's review findings?

jamietownsend · 2021-12-11T15:44:24Z

@fwolter yes the changes were checked in with ab20835

fwolter · 2021-12-11T17:22:59Z

Can you fix the conflicting files?

jamietownsend · 2022-01-25T10:02:49Z

@fwolter thanks for your message. Indeed I was not aware that something added all those newlines and yet, git diff was suppressing whitespace differences. I've removed them new and I think it all looks good. Thanks for your help!

fwolter

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html after building it with mvn clean install.

bundles/org.openhab.binding.solarmax/README.md

fwolter · 2022-01-26T19:34:43Z

bundles/org.openhab.binding.solarmax/README.md

+| softwareVersion          | Point    | Software Version installed on the SolarMax device      |
+| buildNumber              | Point    | Firmware Build Number installed on the SolarMax device |


These seem rather properties than Channels.

I currently have no time to read into what would need to be changed, make the change and test it. Since it currently works as required, I hope it's okay if we put it in the backlog for next time.

Changing these to properties would be a breaking change, once released. If you don't have the time to take a look into this, you could remove them for now and add them in a future release.

So I had a crack at this and it seems to be working now returning these values as properties.

bundles/org.openhab.binding.solarmax/cfg/solarmax.cfg

...g.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxBindingConstants.java

...olarmax/src/main/java/org/openhab/binding/solarmax/internal/connector/SolarMaxConnector.java

fwolter · 2022-01-26T19:46:30Z

...olarmax/src/main/java/org/openhab/binding/solarmax/internal/connector/SolarMaxConnector.java

+
+            // SolarMax can't deal with requests too close to one another, so just wait a moment
+            try {
+                Thread.sleep(10);


Small values can be skipped by the scheduler entirely. To make this work it should be at least 100ms. However, this will block a shared thread in the framework. You could schedule a task to do the next request.

This code is proven to work. Waiting 100ms instead of 10ms will significantly increase the delay between the values being read, meaning they're less likely to actually make sense, because they were read at significantly different times. No action taken.

I understand your point, that these values should be read at the same time at best. But a delay of less than 100ms can be skipped (i.e. results in no sleep), if the machine has high CPU, IO or interrupt load. However, I won't insist on changing this, because this is not a functional review, but you're warned.

Thanks for the warning. A quick search only showed me a bunch of old bugs in this space, but no indication that this was expected behaviour, nor that the bugs are still present. If you have a further reference for me (particularly one with a nice solution that doesn't just waste CPU power), I could look at implementing it.

There's no chance to implement a reliable delay shorter than 100ms but longer than 0ms, because the JVM has no real-time capabilities. You can either use 100ms, which slows things down or use a smaller value, which might get skipped, sometimes.

...olarmax/src/main/java/org/openhab/binding/solarmax/internal/connector/SolarMaxConnector.java

bundles/org.openhab.binding.solarmax/src/main/resources/OH-INF/thing/thing-types.xml

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

...ing.solarmax/src/main/java/org/openhab/binding/solarmax/internal/connector/SolarMaxData.java

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

fwolter

Your UoM implementation looks good.

bundles/org.openhab.binding.solarmax/README.md

...ab.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandler.java

...b.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxProperty.java

fwolter · 2022-02-05T17:51:55Z

...olarmax/src/main/java/org/openhab/binding/solarmax/internal/connector/SolarMaxConnector.java

+
+            // SolarMax can't deal with requests too close to one another, so just wait a moment
+            try {
+                Thread.sleep(10);


There's no chance to implement a reliable delay shorter than 100ms but longer than 0ms, because the JVM has no real-time capabilities. You can either use 100ms, which slows things down or use a smaller value, which might get skipped, sometimes.

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

fwolter · 2022-06-19T15:21:03Z

@jamietownsend did you address all review findings?

bundles/org.openhab.binding.solarmax/pom.xml

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

jamietownsend · 2022-09-19T13:38:52Z

@fwolter if the requested changes are okay for you, could you please approve the merge? Thanks.

fwolter

LGTM

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

fwolter · 2022-09-27T04:59:36Z

...ing.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java

+@NonNullByDefault
+@Component(configurationPid = "binding.solarmax", service = ThingHandlerFactory.class)
+public class SolarMaxHandlerFactory extends BaseThingHandlerFactory {
+    private final Logger logger = LoggerFactory.getLogger(SolarMaxHandlerFactory.class);


This is not used and can therefore be removed.

...ing.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java

…/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de>

...ing.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java

…/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de>

* SolarMax Binding Initial implementation Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 camelCaserizeTheChannelNames Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 Delete commented code and Refactor Brute Force Command Discovery into something commitable Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Delete commented code and Refactor Brute Force Command Discovery into something commitable Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 Codestyle Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Codestyle Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 corrected sat-plugin errors Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 updates from code reviews in PR openhab#10414 Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 mvn spotless:apply Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Updated to 3.2.0-SNAPSHOT Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Fixed conflicts introduced by foreign commit. Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Updated copyright years Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Ran mvn spotless:apply to resolve formatting issues Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Updates from review Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Switch to using Units & move softwareVersion & buildNumber to properties Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * A couple of review related updates Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * A couple more review related changes. Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Added Full Example to README.md Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Update parent pom.xml version Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de> * Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de> Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> Signed-off-by: Fabian Wolter <github@fabian-wolter.de> Co-authored-by: Fabian Wolter <github@fabian-wolter.de>

* SolarMax Binding Initial implementation Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 camelCaserizeTheChannelNames Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 Delete commented code and Refactor Brute Force Command Discovery into something commitable Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Delete commented code and Refactor Brute Force Command Discovery into something commitable Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 Codestyle Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Codestyle Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 corrected sat-plugin errors Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 updates from code reviews in PR openhab#10414 Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 mvn spotless:apply Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Updated to 3.2.0-SNAPSHOT Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Fixed conflicts introduced by foreign commit. Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Updated copyright years Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Ran mvn spotless:apply to resolve formatting issues Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Updates from review Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Switch to using Units & move softwareVersion & buildNumber to properties Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * A couple of review related updates Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * A couple more review related changes. Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Added Full Example to README.md Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Update parent pom.xml version Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de> * Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de> Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> Signed-off-by: Fabian Wolter <github@fabian-wolter.de> Co-authored-by: Fabian Wolter <github@fabian-wolter.de> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>

* SolarMax Binding Initial implementation Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 camelCaserizeTheChannelNames Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 Delete commented code and Refactor Brute Force Command Discovery into something commitable Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Delete commented code and Refactor Brute Force Command Discovery into something commitable Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 Codestyle Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Codestyle Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 corrected sat-plugin errors Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * openhab#10413 updates from code reviews in PR openhab#10414 Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 mvn spotless:apply Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * 10413 Updated to 3.2.0-SNAPSHOT Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Fixed conflicts introduced by foreign commit. Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Updated copyright years Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Ran mvn spotless:apply to resolve formatting issues Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Updates from review Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Switch to using Units & move softwareVersion & buildNumber to properties Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * A couple of review related updates Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * A couple more review related changes. Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Added Full Example to README.md Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Update parent pom.xml version Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> * Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de> * Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de> Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com> Signed-off-by: Fabian Wolter <github@fabian-wolter.de> Co-authored-by: Fabian Wolter <github@fabian-wolter.de>

jamietownsend requested a review from a team as a code owner March 29, 2021 20:10

hmerk added the new binding If someone has started to work on a binding. For a new binding PR. label Mar 29, 2021

jamietownsend force-pushed the solarmax branch from 0699447 to a802227 Compare March 30, 2021 11:41

Skinah reviewed Apr 24, 2021

View reviewed changes

bundles/org.openhab.binding.solarmax/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved

Skinah reviewed Apr 24, 2021

View reviewed changes

bundles/org.openhab.binding.solarmax/README.md Outdated Show resolved Hide resolved

Skinah reviewed Apr 24, 2021

View reviewed changes

bundles/org.openhab.binding.solarmax/README.md Outdated Show resolved Hide resolved

Skinah reviewed Apr 24, 2021

View reviewed changes

bundles/org.openhab.binding.solarmax/README.md Outdated Show resolved Hide resolved

Skinah reviewed Apr 24, 2021

View reviewed changes

jamietownsend added a commit to jamietownsend/openhab-addons that referenced this pull request Apr 27, 2021

openhab#10413 updates from code reviews in PR openhab#10414

11e901c

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

jamietownsend added a commit to jamietownsend/openhab-addons that referenced this pull request Apr 27, 2021

openhab#10413 updates from code reviews in PR openhab#10414

ab20835

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

jamietownsend force-pushed the solarmax branch from 11e901c to ab20835 Compare April 27, 2021 20:45

fwolter added the awaiting feedback Awaiting feedback from the pull request author label Jul 25, 2021

fwolter removed the awaiting feedback Awaiting feedback from the pull request author label Dec 11, 2021

jamietownsend requested review from 3cky, 9037568, alexf2015, andrewfg, aogorek, bern77 and bobadair as code owners December 15, 2021 21:53

fwolter requested changes Jan 26, 2022

View reviewed changes

Updates from review

46a375d

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

jamietownsend requested a review from fwolter January 28, 2022 11:55

fwolter reviewed Jan 29, 2022

View reviewed changes

...ing.solarmax/src/main/java/org/openhab/binding/solarmax/internal/connector/SolarMaxData.java Outdated Show resolved Hide resolved

jamietownsend added 2 commits January 31, 2022 15:36

Switch to using Units & move softwareVersion & buildNumber to properties

5a68ba9

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

A couple of review related updates

795803c

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

fwolter requested changes Feb 5, 2022

View reviewed changes

A couple more review related changes.

0bf67e3

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

wborn reviewed Jul 10, 2022

View reviewed changes

bundles/org.openhab.binding.solarmax/pom.xml Outdated Show resolved Hide resolved

Added Full Example to README.md

7398cb1

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

jamietownsend requested review from fwolter and removed request for Skinah September 11, 2022 20:57

Update parent pom.xml version

860e077

Signed-off-by: Jamie Townsend <jamie_townsend@hotmail.com>

fwolter approved these changes Sep 27, 2022

View reviewed changes

Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab…

41e68c3

…/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de>

fwolter reviewed Sep 27, 2022

View reviewed changes

...ing.solarmax/src/main/java/org/openhab/binding/solarmax/internal/SolarMaxHandlerFactory.java Outdated Show resolved Hide resolved

Update bundles/org.openhab.binding.solarmax/src/main/java/org/openhab…

24c657d

…/binding/solarmax/internal/SolarMaxHandlerFactory.java Signed-off-by: Fabian Wolter <github@fabian-wolter.de>

fwolter merged commit 015a370 into openhab:main Sep 27, 2022

fwolter added this to the 3.4 milestone Sep 27, 2022

jamietownsend deleted the solarmax branch September 27, 2022 07:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[solarmax] Initial contribution #10414

[solarmax] Initial contribution #10414

jamietownsend commented Mar 29, 2021

openhab-bot commented Mar 29, 2021

Skinah commented Mar 30, 2021

jamietownsend commented Apr 6, 2021

kaikreuzer commented Apr 10, 2021

jamietownsend commented Apr 13, 2021

Skinah Apr 24, 2021

Skinah Apr 24, 2021

Skinah commented Apr 24, 2021

fwolter commented Dec 11, 2021

jamietownsend commented Dec 11, 2021

fwolter commented Dec 11, 2021

jamietownsend commented Jan 25, 2022

fwolter left a comment

fwolter Jan 26, 2022

jamietownsend Jan 27, 2022

fwolter Jan 29, 2022

jamietownsend Jan 31, 2022

fwolter Jan 26, 2022

jamietownsend Jan 27, 2022

fwolter Jan 29, 2022

jamietownsend Jan 31, 2022

fwolter Feb 5, 2022

fwolter left a comment

fwolter Feb 5, 2022

fwolter commented Jun 19, 2022

jamietownsend commented Sep 19, 2022

fwolter left a comment

fwolter Sep 27, 2022


		## Thing Configuration

		The IP address and port number (default 12345) of the device needs to be configured.

		\| softwareVersion \| Point \| Software Version installed on the SolarMax device \|
		\| buildNumber \| Point \| Firmware Build Number installed on the SolarMax device \|

[solarmax] Initial contribution #10414

[solarmax] Initial contribution #10414

Conversation

jamietownsend commented Mar 29, 2021

openhab-bot commented Mar 29, 2021

Skinah commented Mar 30, 2021

jamietownsend commented Apr 6, 2021

kaikreuzer commented Apr 10, 2021

jamietownsend commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skinah commented Apr 24, 2021

fwolter commented Dec 11, 2021

jamietownsend commented Dec 11, 2021

fwolter commented Dec 11, 2021

jamietownsend commented Jan 25, 2022

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwolter commented Jun 19, 2022

jamietownsend commented Sep 19, 2022

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment