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

BillBaird commented 2 weeks ago

Issue

The pentair binding, when using the Serial Bridge ignores the "id" thing configuration. The makes the binding when using the serial bridge configuration essentially read-only. It should be noted that the IP Bridge binding properly sets the "id" and behaves properly.

Expected Behavior

The thing's "id" is included in commands sent using the Pentair binding, enabling the Pentair EasyTouch system to accept and act on the command.

Current Behavior

Currently, commands written to the RS485 bus use a value of 00 for the source id, as seen in this log entry: [DEBUG] [rnal.handler.PentairBaseBridgeHandler] - Writing packet: FF FF FF 00 FF A5 01 10 00 86 02 09 01 01 48 The correct behavior should have a configured value of 34 (hex 22) for the source id, as seen in this log entry generated using the IP Bridge: [DEBUG] [rnal.handler.PentairBaseBridgeHandler] - Writing packet: FF FF FF 00 FF A5 01 10 22 86 02 09 01 01 6A

The difference can be seen in the incorrect 10 00 86 sequence and the correct 10 22 86 sequence.

Possible Solution

Add this line in PentairSerialBridgeHandler.java at line 53:

id = configuration.id;

This would make the connect() method begin with

    @Override
    protected synchronized void connect() {
        PentairSerialBridgeConfig configuration = getConfigAs(PentairSerialBridgeConfig.class);

        id = configuration.id;

        try {
            CommPortIdentifier ci = CommPortIdentifier.getPortIdentifier(configuration.serialPort);
            CommPort cp = ci.open("openhabpentairbridge", 10000);

which is exactly what the PentairIPBridgeHandler.java connect() method does.

It should also be noted that PentairBaseBridgeHandler.java indicates that subclasses need to assign, but the PentairSerialBridgeHandler.java does not.

    /** ID to use when sending commands on Pentair bus - subclass needs to assign based on configuration parameter */
    protected int id;

Interestingly, the README.md documentation indicates that it defaults to 34 for both the ip_bridge and serial_bridge

| Thing         | Configuration Parameters                                     |
| ------------- | ------------------------------------------------------------ |
| ip_bridge     | address - IP address for the RS-485 adapter - Required.      |
|               | port - TCP port for the RS-485 adapter - Not Required - default = 10000. |
|               | id - ID to use when communicating on Pentair control bus - default = 34. |
| serial_bridge | serialPort - Serial port for the IT-100s bridge - Required.  |
|               | baud - Baud rate of the IT-100 bridge - Not Required - default = 9600. |
|               | pollPeriod - Period of time in minutes between the poll command being sent to the IT-100 bridge - Not Required - default=1. |
|               | id - ID to use when communciating on Pentair control bus - default = 34. |

The code not only does not set the default of 34 (at least as I read it), nor does it set the id at all for the serial_bridge. Either the code should actually implement a default of 34, or the docs should be changed to indicate that id should typically be set to 34.

Steps to Reproduce (for Bugs)

Prerequisite - you would need a Pentair EasyTouch setup on your swimming pool.

Configure your easy touch as per the documentation, using the Serial_Bridge configuration.
Verify that you are able to see the status of one of your configured features (on-off circuits)
Send a command to change its state. It shouldn't work. Observe the address of '00' in the pentair debug log.

Context

I had a hard time believing this was the issue as this binding has been around for a number of years. In an effort to standardize my installation, I recently converted to it from an early version which I had modified several years ago. I could not get the current binding to work. Observing the log I saw the my ancient working version was sending the hex 22 source address, but the new version was not. I dug into the new code and noticed that the address was only set when using the IP_bridge. I changed my configuration from using a serial bridge to an IP bridge (with the id set to 34), the log entries looked correct, and the things started working.

The good news is that I have a workaround (use the IP bridge). My EasyTouch (I suspect all EasyTouches) require the source address. I don't see how anyone could get the serial binding to work without this fix.

Your Environment

Version used: OpenHabian 4.1.2
Raspberry PI 4, 4GB.
Operating System and version (The latests Openhabian (4.1.2)):

lsiepel commented 5 days ago

@jsjames as you have a PR pending, is this something you can confirm (and or fix?)

lsiepel commented 5 days ago

@BillBaird there is a significant update coming, but that will not make it into 4.2 that will be out in a couple of weeks. I expect it to be in one of the first snapshots of 4.3

jsjames commented 5 days ago

@BillBaird - As @lsiepel mentioned, there is a significant update to the binding coming which adds quite a few additional features and is much more stable. I verified that this issue should not exist in the new code since I restructured the code better so there is more common code between the 2 bridge types. If you want to try the new version out, you can download this file and put in your addons directory. Note, the configuration of the binding is different. The updated code/documentation is here: https://github.com/jsjames/openhab-addons/tree/pentair2

https://drive.google.com/file/d/13QnBZr2yHtC9pNmJQkOW-T33YmFvKg2y/view?usp=sharing

BillBaird commented 5 days ago

Thank you @jsjames. I have reviewed your changes to the initialization of both the IPBridge and SerialBridge handlers and your refactoring does appear to address the issue I raised. Hopefully I can try it this weekend and safely go back to the serial interface.

Looking quickly at the docs for the new version, I see you added support for Pentair programs (1 through 9). I thought I had previously read that Intellitouch systems supported up to 12 programs. I just searched for this and see in this doc https://www.pentair.com/content/dam/extranet/nam/pentair-pool/residential/automation/intellitouch/manual/intellitouch-pool-control-system-users-guide-english.pdf on page 28 that "Up to 99 total programs may be created for all circuits combined.". I've only created/removed schedules using the Screenlogic interface, so I look forward to trying what you have added.

I have also noticed that when using your current Pentair binding that pump rpms sometimes report differently than expected. For instance, I may have the pump set at 2600 rpm (using a Pentair schedule) and while the Pentair pump status (in ScreenLogic) reports it as running at 2600, the Pentair binding it reports it at 2744. I haven't worried about it, but will certainly check once I've tried your updated binding.

Thanks for writing / supporting it.

Bill

On Fri, Jun 28, 2024 at 3:35 PM jsjames @.***> wrote:

@BillBaird https://github.com/BillBaird - As @lsiepel https://github.com/lsiepel mentioned, there is a significant update to the binding coming which adds quite a few additional features and is much more stable. I verified that this issue should not exist in the new code since I restructured the code better so there is more common code between the 2 bridge types. If you want to try the new version out, you can download this file and put in your addons directory. Note, the configuration of the binding is different. The updated code/documentation is here: https://github.com/jsjames/openhab-addons/tree/pentair2

https://drive.google.com/file/d/13QnBZr2yHtC9pNmJQkOW-T33YmFvKg2y/view?usp=sharing

— Reply to this email directly, view it on GitHub https://github.com/openhab/openhab-addons/issues/16874#issuecomment-2197737032, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGVCIOBF4SM2KLTKBXC6PDZJXQK5AVCNFSM6AAAAABJMAXQCCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXG4ZTOMBTGI . You are receiving this because you were mentioned.Message ID: @.***>

jsjames commented 5 days ago

I do recall the RPM being an issue that I corrected. My pump does not output the RPM, so I couldn't test this originally.

Regarding the number of programs, if you have an intellitouch, we could work on a future enhancement to the binding to support more programs. Right now the binding does not determine the type of pentair controller, so it assumes they are all an easytouch.

openhab / openhab-addons