-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[haywardomnilogic] Add support for ColorLogic V2 Lights, Update Chlor Enable, Alert, Error, Status #15478
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/hayward-omnilogic-pool-automation-binding/104105/119 |
.../src/main/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardPumpHandler.java
Outdated
Show resolved
Hide resolved
d1f7450
to
26711bb
Compare
Please note there is a file conflict to resolve. |
Resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
...in/java/org/openhab/binding/haywardomnilogic/internal/discovery/HaywardDiscoveryService.java
Show resolved
Hide resolved
bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/pump.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/pump.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/pump.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/pump.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/pump.xml
Outdated
Show resolved
Hide resolved
My apologies for having all changes in a single commit. My local repo blew up. |
Anyone available to review this? |
a6e8f12
to
4a5a9f3
Compare
4a5a9f3
to
aa71ef6
Compare
@lolodomo would you be able to proceed with the review? If you are too busy, i can spend some time on this if needed. |
aa71ef6
to
130d95b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to fix. Mosty looks good, did you test the upgrade instructions? That is the last requirement i think before it can be merged.
...ain/java/org/openhab/binding/haywardomnilogic/internal/handler/HaywardColorLogicHandler.java
Outdated
Show resolved
Hide resolved
...les/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/update/instructionsV1.xml
Outdated
Show resolved
Hide resolved
Signed-off-by: Matt Myers <[email protected]>
…dditional Shows) Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
…rays based on updated Hayward API Signed-off-by: Matt Myers <[email protected]>
… values Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
9418966
to
911c777
Compare
Signed-off-by: Matt Myers <[email protected]>
Good catch! I tested (and fixed) the upgrade instructions. Is the breaking change alert no longer needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments and re-opened an earlier one to look into.
bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/colorlogic.xml
Show resolved
Hide resolved
bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/chlorinator.xml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
#16733/
From: lsiepel ***@***.***>
Sent: Thursday, May 9, 2024 7:09 AM
To: openhab/openhab-addons ***@***.***>
Cc: Matt ***@***.***>; State change ***@***.***>
Subject: Re: [openhab/openhab-addons] [haywardomnilogic] Add support for ColorLogic V2 Lights, Update Chlor Enable, Alert, Error, Status (PR #15478)
@lsiepel commented on this pull request.
_____
In bundles/org.openhab.binding.haywardomnilogic/src/main/resources/OH-INF/thing/chlorinator.xml <#15478 (comment)> :
@@ -51,7 +50,7 @@
<item-type>Number:Dimensionless</item-type>
<label>Salt Output (%)</label>
<description>Current salt output setting for the chlorinator (%).</description>
- <state min="0" max="100" step="1.0" pattern="%d %unit%" readOnly="false"/>
+ <state min="0" max="100" step="1.0" pattern="%d %" readOnly="false"/>
Ah, sorry i missed this open comment and allready merged it. If you can create a follow up PR with this fix i can merge it really soon.
—
Reply to this email directly, view it on GitHub <#15478 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACQXZPSZBSOE2FZWFCQGPULZBNKK5AVCNFSM6AAAAAA3ZZGYSSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBXG4YTMNBSGM> .
You are receiving this because you modified the open/close state. <https://github.com/notifications/beacon/ACQXZPWPTJ4TWKBUBBJZS7LZBNKK5A5CNFSM6AAAAAA3ZZGYSSWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT2BWWEO.gif> Message ID: ***@***.*** ***@***.***> >
…--
This email has been checked for viruses by AVG antivirus software.
www.avg.com
|
I don't see any breaking changes that should be mentioned. The thing upgrade instructions should do it's work. |
…Enable, Alert, Error, Status (openhab#15478) * Update polling times based on Hayward API recommendations --------- Signed-off-by: Matt Myers <[email protected]>
…Enable, Alert, Error, Status (openhab#15478) * Update polling times based on Hayward API recommendations --------- Signed-off-by: Matt Myers <[email protected]>
…Enable, Alert, Error, Status (openhab#15478) * Update polling times based on Hayward API recommendations --------- Signed-off-by: Matt Myers <[email protected]> Signed-off-by: Paul Smedley <[email protected]>
…Enable, Alert, Error, Status (openhab#15478) * Update polling times based on Hayward API recommendations --------- Signed-off-by: Matt Myers <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…Enable, Alert, Error, Status (openhab#15478) * Update polling times based on Hayward API recommendations --------- Signed-off-by: Matt Myers <[email protected]>
…Enable, Alert, Error, Status (openhab#15478) * Update polling times based on Hayward API recommendations --------- Signed-off-by: Matt Myers <[email protected]>
Added support for ColorLogic V2 Lights (Speed & Brightness)
Updated Chlor Enable based on updated Hayward API
Updated Chlor Alert, Error & Status to be string bit arrays based on new API intel
Updated miscellaneous units and patterns
Updated getTelemetry poll time based on Hayward API recommendations
https://community.openhab.org/t/hayward-omnilogic-pool-automation-binding/104105/119?u=matchews