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

jsjames · 2022-10-04T00:56:11Z

- changed to more generic "controller" name vs. specific to "easytouch" - not backwards compatible
- Integrated the "feature" support in to new controller groups.
- 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
- 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

Closes 13485

lolodomo · 2022-10-04T05:49:04Z

If you have created one PR for each new feature/enhancement rather than one enormous PR of almost 8000 new lines, you will have encouraged a faster review.
I will be honnest with you, I will not start a review myself of such a big PR.

jsjames · 2022-10-04T16:21:32Z

If you have created one PR for each new feature/enhancement rather than one enormous PR of almost 8000 new lines, you will have encouraged a faster review. I will be honnest with you, I will not start a review myself of such a big PR.

The majority was reviewed in this PR: [https://github.com//pull/8844]. Many of the changes were as a result of that review and many hours were spent to address the comments (even when they were just stylistic comments). Unfortunately, github got out of sync with my branch and the suggestion was to submit a new review - where I ran out of time. As a recent comment was made on removing gnu.io and the Pentair binding was listed as not having made the change, I decided to resubmit now.

lolodomo · 2022-10-04T17:29:12Z

@fwolter was apparently involved in the previous PR, I hope he will continue with this new PR.

fwolter · 2022-11-20T11:21:05Z

Is this ready for review again? You added the work in progress label?

jsjames · 2022-11-20T13:34:10Z

After the review process for a different binding (juicenet), I decided to take a once over the code and address some of the same types of edits suggested in that review. Its taking a little longer than anticipated and in the process cleaning up some of the code. It will probably be several days before i'm done, but it should "hopefully" make your review much simpler.

jsjames · 2022-11-29T17:28:23Z

@fwolter, I've submitted a new version. There are quite a few changes to clean up and restructure the code. I think it reads much better and fixed a lot of the style issues. Unfortunately, the changes were fairly large in number.

fwolter

I reviewed the readme and the first code file for now. Here is my feedback.

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

There are some compiler warnings about null access left. You could take a look at mvn clean install. To fix these, you need to store the field to a local variable and do the null check on that.

README.md

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

fwolter · 2022-12-26T11:24:22Z

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

+| tank1                | Number     | R   | Tank 1 level. |
+| tank2                | Number     | R   | Tank 2 level. |


Is this in percent?

No, these are values between 1-7. I added this in the description.

fwolter · 2022-12-26T11:25:00Z

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

+| mode1                | Number     | R   | Mode 1 status. |
+| mode2                | Number     | R   | Mode 2 status. |


Do you know what this is about?

fwolter · 2022-12-26T11:38:19Z

...air/src/main/java/org/openhab/binding/pentair/internal/actions/PentairControllerActions.java

+                (byte) ControllerCommand.SET_CIRCUIT_SWITCH.send, (byte) 0x02, (byte) circuit,
+                (byte) ((state) ? 1 : 0) };
+
+        logger.debug("setCircuitSwitch: {}, {}", circuit, state);


Can this message be replaced by using the debugger? See point 4 https://www.openhab.org/docs/developer/guidelines.html#f-logging

fwolter · 2022-12-26T11:38:57Z

...air/src/main/java/org/openhab/binding/pentair/internal/actions/PentairControllerActions.java

+
+        logger.debug("setCircuitSwitch: {}, {}", circuit, state);
+        if (!getWriter().writePacket(packet, ControllerCommand.SET_CIRCUIT_SWITCH.response, 1)) {
+            logger.trace("setCircuitSwitch: Timeout");


Logging. Same as above. Please check all.

wborn · 2023-07-24T16:36:18Z

Hi @jsjames! Can you fix the review comments and merge conflicts?

openhab-bot · 2023-09-26T18:37:49Z

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

https://community.openhab.org/t/pentair-serial-connection-not-working/149860/5

lsiepel · 2024-01-15T21:14:03Z

@jsjames can you resolve the conflicts?

lsiepel

Might come back to some parts later on. Finished all files. Can you also update your branch to main as that makes it easier to checkout and build locally agains current main.

...inding.pentair/src/test/java/org/openhab/binding/pentair/internal/PentairHeatStatusTest.java

...ab.binding.pentair/src/test/java/org/openhab/binding/pentair/internal/PentairParserTest.java

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

...ir/src/main/java/org/openhab/binding/pentair/internal/discovery/PentairDiscoveryService.java

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

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

lsiepel · 2024-05-31T08:09:13Z

@jsjames are you able to proceed with this PR?

jsjames · 2024-05-31T19:21:54Z

Yes, I will update the PR within the coming week.

…

On Fri, May 31, 2024 at 1:09 AM lsiepel ***@***.***> wrote: @jsjames <https://github.com/jsjames> are you able to proceed with this PR? — Reply to this email directly, view it on GitHub <#13485 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHUUTOFBICH4UYWTCBQOUTZFAV35AVCNFSM6AAAAAAQ4C6B46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBRGQ2DGNJQGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Signed-off-by: Jeff James <jeff@james-online.com>

jsjames · 2024-06-10T00:57:12Z

@isiepel, should be ready to go.

lsiepel · 2024-06-10T20:10:47Z

nu.io dependency. Reworked some of the state changes in the basebridgehandler.

As there is only one milestone left with just a few days before stable release, i hope you can confirm this PR is well tested and confirmed by atleast some users to work well. If so, i can merge this. Otherwise i would prefer to postpone a merge to after the stable release.

jsjames · 2024-06-10T21:14:59Z

I have been using this for awhile and have shared built version for others to try over time. I can’t say it was exhaustively tested by others, but in my home system it works 1000% better than original. Having said that, if you want to do right after the the milestone, that is fine with me as well.

…

On Jun 10, 2024, at 1:11 PM, lsiepel ***@***.***> wrote: nu.io dependency. Reworked some of the state changes in the basebridgehandler. As there is only one milestone left with just a few days before stable release, i hope you can confirm this PR is well tested and confirmed by atleast some users to work well. If so, i can merge this. Otherwise i would prefer to postpone a merge to after the stable release. — Reply to this email directly, view it on GitHub <#13485 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHUUTKSLBLBFHNL22UN4PLZGYB5ZAVCNFSM6AAAAAAQ4C6B46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZGE4TQNJSHE>. You are receiving this because you were mentioned.

lsiepel · 2024-06-10T21:18:26Z

I have been using this for awhile and have shared built version for others to try over time. I can’t say it was exhaustively tested by others, but in my home system it works 1000% better than original. Having said that, if you want to do right after the the milestone, that is fine with me as well.
…
On Jun 10, 2024, at 1:11 PM, lsiepel @.***> wrote: nu.io dependency. Reworked some of the state changes in the basebridgehandler. As there is only one milestone left with just a few days before stable release, i hope you can confirm this PR is well tested and confirmed by atleast some users to work well. If so, i can merge this. Otherwise i would prefer to postpone a merge to after the stable release. — Reply to this email directly, view it on GitHub <#13485 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHUUTKSLBLBFHNL22UN4PLZGYB5ZAVCNFSM6AAAAAAQ4C6B46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZGE4TQNJSHE. You are receiving this because you were mentioned.

That means it won’t be in 4.2 stable. It will be included in first 4.3 milestone. If that is ok, I’ll merge it later, but would be nice if we can get some feedback in the next two weeks and get it in 4.2

lsiepel · 2024-07-08T16:18:29Z

Can you resolve the conflict? I will then merge this

jsjames · 2024-07-08T17:28:13Z

Can you resolve the conflict? I will then merge this

This conflict was introduced when the PR to update the spelling in a log message. That log message no longer exists which caused the conflict. You can just overwrite the change in the master when you checkin this PR.

lsiepel · 2024-07-08T19:31:00Z

is conflict was introduced when the PR to update the spelling in a log message. That log message no longer exists which caused the conflict. You can just overwrite the change in the master when you checkin this PR.

I'm not comitting changes to this PR, as by then i can't merge this.

jsjames · 2024-07-08T20:38:41Z

is conflict was introduced when the PR to update the spelling in a log message. That log message no longer exists which caused the conflict. You can just overwrite the change in the master when you checkin this PR.

I'm not comitting changes to this PR, as by then i can't merge this.

Okay - i was hoping you could just overwrite the previous accepted change. I'm not a git expert, so I'll try to figure out how to resolve the conflict.

Signed-off-by: jsjames <jeff@james-online.com>

lsiepel · 2024-07-08T22:01:25Z

Now this is merged, can you create an upgrade notice for the breaking change?
Should go here: https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst#L1491

jsjames · 2024-07-08T22:24:54Z

an also delete th

I added a [4.3.0] tag, hopefully that was correct.

…#13485) * Updated per design review comments * Added unitHint to Dimensionless items Signed-off-by: Jeff James <jeff@james-online.com>

…#13485) * Updated per design review comments * Added unitHint to Dimensionless items Signed-off-by: Jeff James <jeff@james-online.com> Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>

…#13485) * Updated per design review comments * Added unitHint to Dimensionless items Signed-off-by: Jeff James <jeff@james-online.com>

jsjames force-pushed the pentair2 branch from a9be783 to 66a4f29 Compare October 4, 2022 00:57

jsjames added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible labels Oct 4, 2022

jsjames linked an issue Oct 4, 2022 that may be closed by this pull request

Replace gnu.io dependency of add-ons with serial transport #7573

Open

22 tasks

jsjames added the work in progress A PR that is not yet ready to be merged label Nov 14, 2022

jsjames force-pushed the pentair2 branch from 66a4f29 to e50b956 Compare November 29, 2022 17:22

jsjames requested a review from a team as a code owner November 29, 2022 17:22

lsiepel removed a link to an issue Dec 23, 2022

Replace gnu.io dependency of add-ons with serial transport #7573

Open

22 tasks

lsiepel mentioned this pull request Dec 23, 2022

Replace gnu.io dependency of add-ons with serial transport #7573

Open

22 tasks

lsiepel added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 23, 2022

fwolter requested changes Dec 26, 2022

View reviewed changes

wborn added the awaiting feedback label Sep 2, 2023

wborn assigned jsjames Sep 2, 2023

jsjames force-pushed the pentair2 branch 3 times, most recently from 790cbf0 to 2ba923f Compare September 26, 2023 21:20

jsjames removed the awaiting feedback label Sep 26, 2023

jsjames force-pushed the pentair2 branch 2 times, most recently from 2f4d091 to ff99c67 Compare January 16, 2024 02:29

jsjames removed the work in progress A PR that is not yet ready to be merged label Jan 16, 2024

lsiepel requested changes May 10, 2024

View reviewed changes

jsjames force-pushed the pentair2 branch 2 times, most recently from 167810d to 3660df8 Compare June 2, 2024 16:51

jsjames added 3 commits June 9, 2024 17:44

"rebase" to main

f04cde5

Signed-off-by: Jeff James <jeff@james-online.com>

Updated per design review comments

aa0cded

Signed-off-by: Jeff James <jeff@james-online.com>

Added unitHint to Dimensionless items

c4091d8

Signed-off-by: Jeff James <jeff@james-online.com>

jsjames force-pushed the pentair2 branch from 9778cb8 to c4091d8 Compare June 10, 2024 00:45

lsiepel added this to the 4.3 milestone Jun 28, 2024

Merge branch 'main' into pentair2

8635964

Signed-off-by: jsjames <jeff@james-online.com>

lsiepel approved these changes Jul 8, 2024

View reviewed changes

lsiepel merged commit c0d2638 into openhab:main Jul 8, 2024
5 checks passed

jsjames deleted the pentair2 branch July 8, 2024 22:27

lsiepel mentioned this pull request Jul 8, 2024

Update update.lst for Pentair binding changes openhab/openhab-distro#1671

Merged

lsiepel mentioned this pull request Aug 10, 2024

[pentair] SerialBridge does not set configured id and documented default is not applied #16874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jsjames commented Oct 4, 2022 •

edited by lsiepel

Loading

lolodomo commented Oct 4, 2022

jsjames commented Oct 4, 2022

lolodomo commented Oct 4, 2022

fwolter commented Nov 20, 2022

jsjames commented Nov 20, 2022

jsjames commented Nov 29, 2022

fwolter left a comment

fwolter Dec 26, 2022

jsjames Dec 31, 2022

fwolter Dec 26, 2022

fwolter Dec 26, 2022

fwolter Dec 26, 2022

wborn commented Jul 24, 2023

openhab-bot commented Sep 26, 2023

lsiepel commented Jan 15, 2024

lsiepel left a comment

lsiepel commented May 31, 2024

jsjames commented May 31, 2024 via email

jsjames commented Jun 10, 2024

lsiepel commented Jun 10, 2024

jsjames commented Jun 10, 2024 via email

lsiepel commented Jun 10, 2024

lsiepel commented Jul 8, 2024

jsjames commented Jul 8, 2024

lsiepel commented Jul 8, 2024

jsjames commented Jul 8, 2024

lsiepel commented Jul 8, 2024

jsjames commented Jul 8, 2024

		\| tank1 \| Number \| R \| Tank 1 level. \|
		\| tank2 \| Number \| R \| Tank 2 level. \|

		\| mode1 \| Number \| R \| Mode 1 status. \|
		\| mode2 \| Number \| R \| Mode 2 status. \|

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

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

Conversation

jsjames commented Oct 4, 2022 • edited by lsiepel Loading

lolodomo commented Oct 4, 2022

jsjames commented Oct 4, 2022

lolodomo commented Oct 4, 2022

fwolter commented Nov 20, 2022

jsjames commented Nov 20, 2022

jsjames commented Nov 29, 2022

fwolter left a comment

Choose a reason for hiding this comment

fwolter Dec 26, 2022

Choose a reason for hiding this comment

jsjames Dec 31, 2022

Choose a reason for hiding this comment

fwolter Dec 26, 2022

Choose a reason for hiding this comment

fwolter Dec 26, 2022

Choose a reason for hiding this comment

fwolter Dec 26, 2022

Choose a reason for hiding this comment

wborn commented Jul 24, 2023

openhab-bot commented Sep 26, 2023

lsiepel commented Jan 15, 2024

lsiepel left a comment

Choose a reason for hiding this comment

lsiepel commented May 31, 2024

jsjames commented May 31, 2024 via email

jsjames commented Jun 10, 2024

lsiepel commented Jun 10, 2024

jsjames commented Jun 10, 2024 via email

lsiepel commented Jun 10, 2024

lsiepel commented Jul 8, 2024

jsjames commented Jul 8, 2024

lsiepel commented Jul 8, 2024

jsjames commented Jul 8, 2024

lsiepel commented Jul 8, 2024

jsjames commented Jul 8, 2024

jsjames commented Oct 4, 2022 •

edited by lsiepel

Loading