[pentair] Many quality and feature improvements #8844

jsjames · 2020-10-23T17:08:48Z

Implemented controller schedules (both read and write) implementation Various other fixes
Addressed many warning/errors and cleanup
Added support for units on temperatures, power, etc.
Added intelliflo gpm fixed spelling error added intelliflo status
Added direct support for motor (when controller is not present or in service mode)
Removed apache.commons dependency
Removed gnu.io dependency. Reworked some of the state changes in the basebridgehandler.
Added auto discovery

Finished schedule implementation
Various other fixes

Addressed many warning/errors

More cleanup

Updated README with changes.
Added support for UOM

Added intelliflo gpm
fixed spelling error
added intelliflo status

Removed apache.commons import

Removed gnu.io dependency. Reworked some of the state changes in the basebridgehandler.

Added auto discovery

TravisBuddy · 2020-10-23T17:20:00Z

Travis tests were successful

Hey @jsjames,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

fwolter

There is a lot of duplicate code in the ThingHandlers. That is why many review findings are also redundant. You already have the right class structure to abstract this code. You could utilize this.

If I see correctly you renamed a Thing and a Bridge. I commented on the renaming of the bridge below. This also applies to the Thing.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

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

bundles/org.openhab.binding.pentair/src/main/resources/OH-INF/thing/intellichem.xml

bundles/org.openhab.binding.pentair/src/main/resources/OH-INF/thing/serial_bridge.xml

jsjames · 2020-11-30T22:37:25Z

Force pushed to jsjames/main

jsjames · 2020-12-01T02:10:12Z

Hi Fabian - I had lots of issues on my local git library which complicated squashing and checking things in. I ended up just checking out origin/main and then copying over the new files by hand and then recommitting to get a clean single commit (unfortunately, i am a newbie on git). Anyway, I hope things are done correctly. look forward to your review.

...ing.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairControllerCircuit.java

cpmeister · 2020-12-01T04:14:08Z

...ding.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairControllerStatus.java

+ @Override
+ public Object clone() throws CloneNotSupportedException {
+ PentairControllerStatus pcs = (PentairControllerStatus) super.clone();
+
+ pcs.circuits = new boolean[this.circuits.length];
+ System.arraycopy(this.circuits, 0, pcs.circuits, 0, this.circuits.length);
+
+ return pcs;
+ }


Please don't use clone() if possible just write a custom copy method.

Based on restructuring I did from feedback from Fabian, I no longer use clone and so I deleted. I'd be curious on recommendation to not use clone - which from my understanding would be the standard way to do this?

...ab.binding.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairHeatStatus.java

...ng.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairIntelliChlorPacket.java

bundles/org.openhab.binding.pentair/src/main/resources/OH-INF/thing/intellichem.xml

...air/src/main/java/org/openhab/binding/pentair/internal/handler/PentairControllerHandler.java

...r/src/main/java/org/openhab/binding/pentair/internal/handler/PentairIntelliChlorHandler.java

...r/src/main/java/org/openhab/binding/pentair/internal/handler/PentairSerialBridgeHandler.java

bundles/org.openhab.binding.pentair/src/main/resources/OH-INF/binding/binding.xml

fwolter · 2020-12-01T09:32:25Z

Great that you solved your problem on your own, although using steamroller tactics :)

jsjames · 2020-12-02T21:08:15Z

@fwolter @cpmeister - It appears the Jenkins check failed on my latest updates, however in looking at the logs, it looks like everything passed?

...ding.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairControllerStatus.java

...penhab.binding.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairParser.java

...air/src/main/java/org/openhab/binding/pentair/internal/handler/PentairBaseBridgeHandler.java

...r/src/main/java/org/openhab/binding/pentair/internal/handler/PentairIntelliChlorHandler.java

fwolter · 2020-12-06T21:11:57Z

Are you finished making all changes?

jsjames · 2020-12-06T21:25:25Z

Yes, all changes are done.

...penhab.binding.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairParser.java

...air/src/main/java/org/openhab/binding/pentair/internal/handler/PentairBaseBridgeHandler.java

...r/src/main/java/org/openhab/binding/pentair/internal/handler/PentairIntelliChlorHandler.java

...air/src/main/java/org/openhab/binding/pentair/internal/handler/PentairBaseBridgeHandler.java

jsjames · 2020-12-08T04:48:28Z

all requested changes should be made at this point. Thanks.

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

...ng.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairControllerSchedule.java

fwolter · 2020-12-08T17:14:54Z

bundles/org.openhab.binding.pentair/src/main/resources/OH-INF/thing/controller.xml

+
+ <channel-type id="uomtype">
+ <item-type>String</item-type>
+ <label>Unit of measure (Celcius, Fahrenheit)</label>


bundles/org.openhab.binding.pentair/src/main/resources/OH-INF/thing/controller.xml

bundles/org.openhab.binding.pentair/src/test/resources/log4j.xml

jsjames · 2020-12-11T18:22:58Z

This should be ready to go now. Thanks for all the help!

cpmeister

For future code changes to the PR, please make them additional commits. Please don't append to the current commit and then force push. It is getting very difficult to track what changes you are making in response to our reviews. Thank you.

...ng.pentair/src/main/java/org/openhab/binding/pentair/internal/PentairControllerSchedule.java

...ntair/src/main/java/org/openhab/binding/pentair/internal/handler/PentairIPBridgeHandler.java

...r/src/main/java/org/openhab/binding/pentair/internal/handler/PentairIntelliChlorHandler.java

...air/src/main/java/org/openhab/binding/pentair/internal/handler/PentairControllerHandler.java

cpmeister · 2020-12-11T19:43:50Z

...air/src/main/java/org/openhab/binding/pentair/internal/handler/PentairControllerHandler.java

+ if (syncTimeJob != null) {
+ syncTimeJob.cancel(true);
+ }


To make the null checker happy you need to store fields into local variables and perform your operations on those local variables instead.

There are several of these benign null check warnings where I didn't think its worth addressing since they aren't real issues (I'm hoping the null check tools will improve). I did address this one.

...air/src/main/java/org/openhab/binding/pentair/internal/handler/PentairControllerHandler.java

jsjames · 2020-12-15T04:33:40Z

Just pushed up a most recent set of requested modifications. Thanks.

fwolter · 2020-12-16T17:21:30Z

Can you fix the missing sign-off?

jsjames · 2020-12-17T03:05:03Z

Reopened after signing

Can you fix the missing sign-off?

Hi Fabian - I followed the instructions to follow the sign-off commit with the following:
git push --force-with-lease jsjames main

Unfortunately that seemed to mess up this PR and I can't seem to reopen. suggestions on what to do?

fwolter · 2020-12-17T09:45:18Z

I can't say what happened here. Seems like there are missing some commits in between. Can you make a test commit?

jsjames · 2020-12-17T18:15:33Z

Test commit to reopen

I can't say what happened here. Seems like there are missing some commits in between. Can you make a test commit?

I submitted a test commit and tried to "Reopen and comment" on this PR. I still get a "no new commits on jsjames:main". When i look at jsjames:main, it is clear there are commits - not sure what could be going on? Should I open a brand new PR?

fwolter · 2020-12-17T18:20:41Z

Seems like GitHub lost track between your branch and this PR. When you create a new PR it's a good idea not to create it from your main branch, but from a new one, only for the PR. Before creating a new PR, maybe @cpmeister has an idea.

fwolter · 2020-12-25T15:03:51Z

I think you can open a new PR, if you want.

jsjames force-pushed the main branch from 4d811df to d628f93 Compare October 23, 2020 17:11

jsjames added the enhancement An enhancement or new feature for an existing add-on label Oct 23, 2020

wborn changed the title ~~[pentair] Many quality and feature improvements to the binding for OH3.~~ [pentair] Many quality and feature improvements Oct 24, 2020

jsjames requested review from cpmeister and cweitkamp October 27, 2020 22:37

fwolter self-assigned this Nov 1, 2020

fwolter requested changes Nov 1, 2020

View reviewed changes

fwolter added the (potentially) not backward compatible label Nov 3, 2020

jsjames closed this Nov 30, 2020

jsjames force-pushed the main branch from 693a2d4 to 1e89b79 Compare November 30, 2020 00:04

jsjames reopened this Nov 30, 2020

jsjames force-pushed the main branch 2 times, most recently from 850c322 to 13cc346 Compare December 1, 2020 02:05

cpmeister requested changes Dec 1, 2020

View reviewed changes

jsjames force-pushed the main branch from 13cc346 to 5bd3627 Compare December 1, 2020 23:23

cpmeister requested changes Dec 3, 2020

View reviewed changes

jsjames force-pushed the main branch 2 times, most recently from f513104 to b19553e Compare December 4, 2020 03:00

kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 4, 2020

cpmeister requested changes Dec 7, 2020

View reviewed changes

jsjames force-pushed the main branch from b19553e to 3fd4682 Compare December 8, 2020 04:47

fwolter requested changes Dec 8, 2020

View reviewed changes

jsjames force-pushed the main branch from 3fd4682 to 7cd2935 Compare December 8, 2020 20:30

cpmeister requested changes Dec 11, 2020

View reviewed changes

jsjames closed this Dec 17, 2020

jsjames force-pushed the main branch from 9a079ce to 072113f Compare December 17, 2020 02:58

jsjames mentioned this pull request Oct 4, 2022

[pentair] Many enhancements since original commit, including #13485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pentair] Many quality and feature improvements #8844

[pentair] Many quality and feature improvements #8844

jsjames commented Oct 23, 2020

TravisBuddy commented Oct 23, 2020

fwolter left a comment

jsjames commented Nov 30, 2020

jsjames commented Dec 1, 2020

cpmeister Dec 1, 2020

jsjames Dec 1, 2020

fwolter commented Dec 1, 2020

jsjames commented Dec 2, 2020

fwolter commented Dec 6, 2020

jsjames commented Dec 6, 2020

jsjames commented Dec 8, 2020

fwolter Dec 8, 2020

jsjames commented Dec 11, 2020

cpmeister left a comment

cpmeister Dec 11, 2020

jsjames Dec 15, 2020

jsjames commented Dec 15, 2020

fwolter commented Dec 16, 2020

jsjames commented Dec 17, 2020

fwolter commented Dec 17, 2020

jsjames commented Dec 17, 2020

fwolter commented Dec 17, 2020

fwolter commented Dec 25, 2020

[pentair] Many quality and feature improvements #8844

[pentair] Many quality and feature improvements #8844

Conversation

jsjames commented Oct 23, 2020

TravisBuddy commented Oct 23, 2020

Travis tests were successful

fwolter left a comment

Choose a reason for hiding this comment

jsjames commented Nov 30, 2020

jsjames commented Dec 1, 2020

cpmeister Dec 1, 2020

Choose a reason for hiding this comment

jsjames Dec 1, 2020

Choose a reason for hiding this comment

fwolter commented Dec 1, 2020

jsjames commented Dec 2, 2020

fwolter commented Dec 6, 2020

jsjames commented Dec 6, 2020

jsjames commented Dec 8, 2020

fwolter Dec 8, 2020

Choose a reason for hiding this comment

jsjames commented Dec 11, 2020

cpmeister left a comment

Choose a reason for hiding this comment

cpmeister Dec 11, 2020

Choose a reason for hiding this comment

jsjames Dec 15, 2020

Choose a reason for hiding this comment

jsjames commented Dec 15, 2020

fwolter commented Dec 16, 2020

jsjames commented Dec 17, 2020

fwolter commented Dec 17, 2020

jsjames commented Dec 17, 2020

fwolter commented Dec 17, 2020

fwolter commented Dec 25, 2020