openhab-addons
openhab-addons copied to clipboard
openhab
[pentair] Many enhancements since original commit, including
- 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
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.
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/openhab/openhab-addons/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.
@fwolter was apparently involved in the previous PR, I hope he will continue with this new PR.
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.
@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.
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
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/pentair-binding-not-reading-pump-status/152563/15
@jsjames I created issues for the improved logging (#16301 ) and fix for pool/spa temp updates (#16302). Should should link them to this PR once the 2nd commit is merged.
Closing: #16301 #16302
@lsiepel, is there anything else needed in order to integrate this update? Thx.
@lsiepel, is there anything else needed in order to integrate this update? Thx.
As @fwolter reviewed earlier, it might allready be in a good shape. Nevertheless this is a large PR and those require some longer dedicated time to review. Unfortunately I (and probaly the other mantainers too) don't have that, so other smaller PR's get the available time. Just want to be honest, somebody will look at it, but it might take some time.
Only thing you can do now is keep the PR in shape and maybe do a self-review with this in mind
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 https://github.com/openhab/openhab-addons/pull/13485#issuecomment-2141443506, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHUUTOFBICH4UYWTCBQOUTZFAV35AVCNFSM6AAAAAAQ4C6B46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBRGQ2DGNJQGY . You are receiving this because you were mentioned.Message ID: @.***>
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.
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 https://github.com/openhab/openhab-addons/pull/13485#issuecomment-2159198529, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHUUTKSLBLBFHNL22UN4PLZGYB5ZAVCNFSM6AAAAAAQ4C6B46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZGE4TQNJSHE. You are receiving this because you were mentioned.
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
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.
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.
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.
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