[haywardomnilogic] Replacement for Hayward Omnilogic Pool Automation Binding #8685

matchews · 2020-10-07T17:24:12Z

This PR is a replacement for PR #8338 , which was submitted against the 2.5.x branch. The 2.5.x version of the binding has been reviewed by @Hilbrand and @fwolter . There are still a couple open issues and changes that are awaiting review by @Hilbrand .

Background
Hayward Omnilogic binding initial contribution
The Hayward Omnilogic binding integrates the Hayward Omnilogic pool controller using the Hayward API.

Forum discussion can be found here.

Signed-off-by: Matt Myers mmyers75@icloud.com

TravisBuddy · 2020-10-07T17:29:41Z

Travis tests were successful

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

matchews · 2020-10-10T12:07:00Z

@Hilbrand, Is there an issue with the status checks? It seems that this PR is stuck. If it is in the review queue, my apologies. I know you guys are busy.

Thanks!

...ilogic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardTypeToRequest.java

...nilogic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardThingHandler.java

bom/openhab-addons/pom.xml

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

bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/pump.xml

...nilogic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardThingHandler.java

...in/java/org/openhab/binding/haywardomnilogic/internal/discovery/HaywardDiscoveryService.java

...gic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardBindingConstants.java

matchews · 2020-11-11T00:36:39Z

I can't seem to correct the commit failing the DCO. I tried rebasing it a couple times with no luck. Any pointers would be appreciated. I'm still new to git. Thx.

fwolter · 2020-11-11T06:24:29Z

When you click on the DCO check "Details", the neccessary commands are listed.

matchews · 2020-11-11T07:42:39Z

When you click on the DCO check "Details", the neccessary commands are listed.

I tried that. From what I understand, the -amend command only applies to the last commit made.

fwolter · 2020-11-11T08:09:43Z

You did a strange merge in your branch. Now the commit in question exists twice in the history. One signed-off and the other don't. This might confuse the DCO check and I didn't manage to sign it off either. We can force the DCO check to pass before merging.

Better use rebase instead of merge in PRs.

cpmeister

Thanks for being so patient.
I've starting to get a bit concerned about what kind of performance impact your use of xpath will cause. Most OH users typically run on slower hardware so performance should be among the higher priorities.
Have you considered parsing the xml into a java class through one of the dom parsers? I feel that would be much faster than repeated use of xpath. WDYT?

...gic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardBindingConstants.java

.../java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardVirtualHeaterHandler.java

...gic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardBindingConstants.java

.../main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardBackyardHandler.java

matchews · 2020-12-16T00:59:31Z

m parsers? I feel that would be much faster than repeated use of xpath. WDYT?

I took a shot at implementing the Java DOM parser, which works fine, but the subsequent operations required to search through the document are turning out to be pretty ugly. If the elements were unique, this approach would work great, but due to most elements being duplicates (i.e. systemID) the code will require numerous nested loops to search through the document to find the Thing first (Heater), then followed by the systemID, then followed by the data. Ultimately I can make this work, but I'm not finding an elegant way to implement.

...in/java/org/openhab/binding/haywardomnilogic/internal/discovery/HaywardDiscoveryService.java

...rc/main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardBridgeHandler.java

...in/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardChlorinatorHandler.java

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

bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/bridge.xml

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

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

fwolter

You code doesn't compile because there are several author tags missing.

...logic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardHandlerFactory.java

...nilogic/src/main/java/org/openhab/binding/haywardomnilogic/internal/HaywardThingHandler.java

...in/java/org/openhab/binding/haywardomnilogic/internal/discovery/HaywardDiscoveryService.java

...rc/main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardBridgeHandler.java

...in/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardChlorinatorHandler.java

matchews · 2020-12-30T13:29:35Z

You code doesn't compile because there are several author tags missing.
Thanks for all of the input and being patient with my learning curve. Regarding the author tags, I was asked to remove these. I assumed the code checks hadn't been updated yet. Let me know how to proceed.
#8685 (comment)

fwolter · 2020-12-30T14:24:54Z

Sorry for the misunderstanding. The author tag in the binding.xml is deprecated, but the author tags in the source files are not.

fwolter · 2021-01-04T21:36:58Z

Are you finished making all changes?

matchews · 2021-01-04T21:39:17Z

Are you finished making all changes?

Yes. I just re-requested a review. Thx.

...rc/main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardBridgeHandler.java

fwolter

Please stop adding any new features to this PR. When this is merged, you can file a new PR.

...in/java/org/openhab/binding/haywardomnilogic/internal/discovery/HaywardDiscoveryService.java

...rc/main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardBridgeHandler.java

matchews · 2021-01-06T22:49:35Z

Please stop adding any new features to this PR. When this is merged, you can file a new PR.

Understood. I found a glaring temperature conversion error.

fwolter

LGTM

Since this is a new binding, another maintainer needs to review your code before it can be merged.

Signed-off-by: matchews <mmyers75@icloud.com>

…initions Signed-off-by: matchews <mmyers75@icloud.com>

…and HaywardException from mspConfigDiscovery Signed-off-by: matchews <mmyers75@icloud.com>

Signed-off-by: matchews <mmyers75@icloud.com>

…parameter Signed-off-by: matchews <mmyers75@icloud.com>

Signed-off-by: matchews <mmyers75@icloud.com>

matchews · 2021-01-22T02:48:08Z

Final cleanup.

Thank you for all of your help!

…omation Binding (openhab#8685)" This reverts commit ab290c5.

…Binding (openhab#8685) * Initial Contribution Signed-off-by: Matt Myers <mmyers75@icloud.com> Signed-off-by: John Marshall <john.marshall.au@gmail.com>

…Binding (openhab#8685) * Initial Contribution Signed-off-by: Matt Myers <mmyers75@icloud.com>

matchews requested a review from a team as a code owner October 7, 2020 17:24

fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Oct 8, 2020

cpmeister requested changes Oct 30, 2020

View reviewed changes

matchews requested a review from cpmeister November 11, 2020 00:35

cpmeister requested changes Dec 4, 2020

View reviewed changes

matchews requested a review from cpmeister December 16, 2020 01:00

fwolter requested changes Dec 19, 2020

View reviewed changes

matchews requested review from fwolter and Confectrician December 21, 2020 01:40

fwolter requested changes Dec 26, 2020

View reviewed changes

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

fwolter requested changes Dec 29, 2020

View reviewed changes

matchews requested a review from fwolter January 4, 2021 21:38

fwolter requested changes Jan 4, 2021

View reviewed changes

...rc/main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardBridgeHandler.java Outdated Show resolved Hide resolved

matchews requested a review from fwolter January 6, 2021 01:34

fwolter requested changes Jan 6, 2021

View reviewed changes

...in/java/org/openhab/binding/haywardomnilogic/internal/discovery/HaywardDiscoveryService.java Outdated Show resolved Hide resolved

...rc/main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardBridgeHandler.java Outdated Show resolved Hide resolved

matchews requested a review from fwolter January 6, 2021 22:52

fwolter approved these changes Jan 8, 2021

View reviewed changes

fwolter added the cre Coordinated Review Effort label Jan 9, 2021

matchews added 19 commits January 21, 2021 21:43

Add backyard properties (units, VSP SPeed Format)

5d0d6d3

Signed-off-by: matchews <mmyers75@icloud.com>

Removed unused constants

ede044e

Signed-off-by: matchews <mmyers75@icloud.com>

Move fields to top of classes

2140885

Signed-off-by: matchews <mmyers75@icloud.com>

Added Author tags back in

fb99b87

Signed-off-by: matchews <mmyers75@icloud.com>

ThingHandler & getTelemetry are now abstract

94e2d43

Signed-off-by: matchews <mmyers75@icloud.com>

Adding Null Checks

cf62cb2

Signed-off-by: matchews <mmyers75@icloud.com>

Clean up exception handling

58518ff

Signed-off-by: matchews <mmyers75@icloud.com>

Updated InterruptedException handling

f5e3268

Signed-off-by: matchews <mmyers75@icloud.com>

Removed compiler warning

e94b556

Signed-off-by: matchews <mmyers75@icloud.com>

Added mspConfigUnits to pull units from Hayward, updated xml unit def…

1aeffd7

…initions Signed-off-by: matchews <mmyers75@icloud.com>

Removed unused haywardDiscoveryService assignment from bridgehandler …

43a7622

…and HaywardException from mspConfigDiscovery Signed-off-by: matchews <mmyers75@icloud.com>

updated readme

2fe9147

Signed-off-by: matchews <mmyers75@icloud.com>

update headers 2021

7af0539

Signed-off-by: matchews <mmyers75@icloud.com>

cpmeister requested changes 1/17/21

8f66754

Signed-off-by: matchews <mmyers75@icloud.com>

Refactor DiscoveryHandler

792a1fd

Signed-off-by: matchews <mmyers75@icloud.com>

Refactor DiscoveryService

38204b1

Signed-off-by: matchews <mmyers75@icloud.com>

Eliminated HawardDiscoveryService.discoverDevices useZeroForBowIndex …

01a107b

…parameter Signed-off-by: matchews <mmyers75@icloud.com>

Final cleanup

d48f64b

Signed-off-by: matchews <mmyers75@icloud.com>

Code check fix

6b99461

Signed-off-by: matchews <mmyers75@icloud.com>

matchews requested a review from cpmeister January 22, 2021 02:47

wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 22, 2021

cpmeister approved these changes Jan 22, 2021

View reviewed changes

cpmeister merged commit ab290c5 into openhab:main Jan 22, 2021

cpmeister added this to the 3.1 milestone Jan 22, 2021

lsiepel added a commit to lsiepel/openhab-addons that referenced this pull request Feb 9, 2021

Revert "[haywardomnilogic] Replacement for Hayward Omnilogic Pool Aut…

5c8e64f

…omation Binding (openhab#8685)" This reverts commit ab290c5.

thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021

[haywardomnilogic] Replacement for Hayward Omnilogic Pool Automation …

09a6995

…Binding (openhab#8685) * Initial Contribution Signed-off-by: Matt Myers <mmyers75@icloud.com>

marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022

[haywardomnilogic] Replacement for Hayward Omnilogic Pool Automation …

0c71386

…Binding (openhab#8685) * Initial Contribution Signed-off-by: Matt Myers <mmyers75@icloud.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[haywardomnilogic] Replacement for Hayward Omnilogic Pool Automation Binding #8685

[haywardomnilogic] Replacement for Hayward Omnilogic Pool Automation Binding #8685

matchews commented Oct 7, 2020

TravisBuddy commented Oct 7, 2020

matchews commented Oct 10, 2020

matchews commented Nov 11, 2020

fwolter commented Nov 11, 2020

matchews commented Nov 11, 2020

fwolter commented Nov 11, 2020

cpmeister left a comment

matchews commented Dec 16, 2020

fwolter left a comment •

edited

Loading

matchews commented Dec 30, 2020

fwolter commented Dec 30, 2020

fwolter commented Jan 4, 2021

matchews commented Jan 4, 2021

fwolter left a comment

matchews commented Jan 6, 2021

fwolter left a comment

matchews commented Jan 22, 2021

[haywardomnilogic] Replacement for Hayward Omnilogic Pool Automation Binding #8685

[haywardomnilogic] Replacement for Hayward Omnilogic Pool Automation Binding #8685

Conversation

matchews commented Oct 7, 2020

TravisBuddy commented Oct 7, 2020

Travis tests were successful

matchews commented Oct 10, 2020

matchews commented Nov 11, 2020

fwolter commented Nov 11, 2020

matchews commented Nov 11, 2020

fwolter commented Nov 11, 2020

cpmeister left a comment

Choose a reason for hiding this comment

matchews commented Dec 16, 2020

fwolter left a comment • edited Loading

Choose a reason for hiding this comment

matchews commented Dec 30, 2020

fwolter commented Dec 30, 2020

fwolter commented Jan 4, 2021

matchews commented Jan 4, 2021

fwolter left a comment

Choose a reason for hiding this comment

matchews commented Jan 6, 2021

fwolter left a comment

Choose a reason for hiding this comment

matchews commented Jan 22, 2021

fwolter left a comment •

edited

Loading