[intellicenter2] Initial contribution #13334

valdisrigdon · 2022-08-29T19:15:42Z

Adds a new binding for the IntelliCenter 2 pool automation system from
Pentair.

This adds a new binding for Pentair pool automation systems using IntelliCenter 2. This is different from the Pentair binding as the protocol is completely different and does not require any additional hardware to activate.

I have been running a version of this binding on 3.2 for a few months without any issues.

valdisrigdon · 2022-11-28T21:18:50Z

@jlaur This is ready for a review -- how can I get this assigned for a review?

jlaur

Thanks for your contribution! I have just started a review, but will need to do it in an incremental way to not keep you waiting for too long. So I have provided some initial feedback, which I hope you can use to get started, while I will continue my review as soon as possible.

jlaur · 2022-11-28T21:39:00Z

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

+_Give some details about what this binding is meant for - a protocol, system, specific device._
+
+_If possible, provide some resources like pictures (only PNG is supported currently), a video, etc. to give an impression of what can be done with this binding._
+_You can place such resources into a `doc` folder next to this README.md._
+
+_Put each sentence in a separate line to improve readability of diffs._
+
+## Supported Things
+
+_Please describe the different supported things / devices including their ThingTypeUID within this section._
+_Which different types are supported, which models were tested etc.?_
+_Note that it is planned to generate some part of this based on the XML files within ```src/main/resources/OH-INF/thing``` of your binding._
+
+- `bridge`: Short description of the Bridge, if any
+- `sample`: Short description of the Thing with the ThingTypeUID `sample`
+
+## Discovery
+
+_Describe the available auto-discovery features here._
+_Mention for what it works and what needs to be kept in mind when using it._
+
+## Binding Configuration
+
+_If your binding requires or supports general configuration settings, please create a folder ```cfg``` and place the configuration file ```<bindingId>.cfg``` inside it._
+_In this section, you should link to this file and provide some information about the options._
+_The file could e.g. look like:_
+
+```
+# Configuration for the IntelliCenter2 Binding
+#
+# Default secret key for the pairing of the IntelliCenter2 Thing.
+# It has to be between 10-40 (alphanumeric) characters.
+# This may be changed by the user for security reasons.
+secret=openHABSecret
+```
+
+_Note that it is planned to generate some part of this based on the information that is available within ```src/main/resources/OH-INF/binding``` of your binding._
+
+_If your binding does not offer any generic configurations, you can remove this section completely._
+
+## Thing Configuration
+
+_Describe what is needed to manually configure a thing, either through the UI or via a thing-file._
+_This should be mainly about its mandatory and optional configuration parameters._
+
+_Note that it is planned to generate some part of this based on the XML files within ```src/main/resources/OH-INF/thing``` of your binding._
+
+### `sample` Thing Configuration
+
+| Name            | Type    | Description                           | Default | Required | Advanced |
+|-----------------|---------|---------------------------------------|---------|----------|----------|
+| hostname        | text    | Hostname or IP address of the device  | N/A     | yes      | no       |
+| password        | text    | Password to access the device         | N/A     | yes      | no       |
+| refreshInterval | integer | Interval the device is polled in sec. | 600     | no       | yes      |
+
+## Channels
+
+_Here you should provide information about available channel types, what their meaning is and how they can be used._
+
+_Note that it is planned to generate some part of this based on the XML files within ```src/main/resources/OH-INF/thing``` of your binding._
+
+| Channel | Type   | Read/Write | Description                 |
+|---------|--------|------------|-----------------------------|
+| control | Switch | RW         | This is the control channel |
+
+## Full Example
+
+_Provide a full usage example based on textual configuration files._
+_*.things, *.items examples are mandatory as textual configuration is well used by many users._
+_*.sitemap examples are optional._
+
+## Any custom content here!
+
+_Feel free to add additional sections for whatever you think should also be mentioned about your binding!_


Please document the binding in this file.

jlaur · 2022-11-28T21:41:21Z

...2/src/main/java/org/openhab/binding/intellicenter2/internal/IntelliCenter2Configuration.java

+ */
+package org.openhab.binding.intellicenter2.internal;
+
+import org.apache.commons.lang3.builder.ToStringBuilder;


Please remove this dependency, see #7722.

See bundles/org.openhab.binding.pentair/src/main/java/org/openhab/binding/pentair/internal/config/PentairIPBridgeConfig.java for a possible fix

...va/org/openhab/binding/intellicenter2/internal/discovery/IntelliCenter2DiscoveryService.java

jlaur · 2022-11-28T21:46:01Z

...va/org/openhab/binding/intellicenter2/internal/discovery/IntelliCenter2DiscoveryService.java

+                    }
+                }
+            } catch (Exception e) {
+                logger.error("Unable to discover IntelliCenter2 hardware for {}", discoveryArg, e);


Since this will not prevent the binding from working, log level should be reduced.

jlaur · 2022-11-28T21:58:00Z

...in/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2BridgeHandler.java

+                        "Unable to connect to host " + config.hostname);
+            } catch (Exception e) {
+                logger.error("Error initializing.", e);
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR, e.getMessage());


This thing status detail is not correct, i.e. it doesn't correspond to the status. Please see https://www.openhab.org/docs/concepts/things.html#status-details.

jlaur · 2022-11-28T21:58:58Z

...in/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2BridgeHandler.java

+                String msg = String.format("unknown host name: %s", config.hostname);
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, msg);
+            } catch (IOException e) {
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,


This should probably be COMMUNICATION_ERROR. In this case, how will the binding come online?

jlaur · 2022-11-28T21:59:40Z

...in/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2BridgeHandler.java

+                } catch (Exception ignored) {
+                }
+            }
+            updateStatus(ThingStatus.OFFLINE);


This is not needed.

jlaur · 2022-11-28T22:00:52Z

...in/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2BridgeHandler.java

+                    updateProperty("propertyName", systemInfo.getPropertyName());
+                    updateProperty("version", systemInfo.getVersion());
+                    logger.info("Bridge is ONLINE.");
+                    if (!systemInfo.getIntellicenterVersion().equals("1.064")) {


What is the difference between systemInfo.getVersion() and systemInfo.getIntellicenterVersion()? Would it make sense to keep both as properties?

jlaur · 2022-11-28T22:01:24Z

...n/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2FeatureHandler.java

+ *
+ * @see Circuit
+ */
+@SuppressWarnings("UnstableApiUsage")


What is the reason for this?

jlaur

I have provided some additional comments after second review session. In general the code looks good!

jlaur · 2022-12-02T20:19:46Z

...ain/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2LightHandler.java

+ *
+ * @see IntelliBrite
+ */
+@SuppressWarnings("UnstableApiUsage")


What is the reason for this?

jlaur · 2022-12-02T20:22:17Z

...main/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2PoolHandler.java

+                updateState(channelUID, OnOffType.from(pool.isHeating()));
+                break;
+            default:
+                logger.error("Unable to update state for {}", channelUID);


This should not prevent the binding from working, so could be reduced to log level WARN. See https://next.openhab.org/docs/developer/guidelines.html#f-logging

jlaur · 2022-12-02T20:23:27Z

...main/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2PumpHandler.java

+    @Override
+    public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command command) {
+        super.handleCommand(channelUID, command);
+        switch (channelUID.getId()) {
+            case CHANNEL_PUMP_GPM:
+                if (command instanceof DecimalType) {
+                    // var request = ICRequest.setParamList(
+                    // new RequestObject(getObjectName(), Map.of(Attribute.GPM, command.toString())));
+                    // getProtocol().submit(request);
+                }
+                break;
+        }
+    }


Could this be removed, or did you mean to implement something in this override?

jlaur · 2022-12-02T20:26:36Z

...ain/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2ThingHandler.java

+                // get an updated model from IC
+                final ICProtocol protocol = getProtocol();
+                if (protocol == null) {
+                    updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR);


This is not a valid combination, it should most likely be CONFIGURATION_ERROR, assuming that protocol being null is caused by missing reference to parent bridge handler. See https://www.openhab.org/docs/concepts/things.html#status-details

jlaur · 2022-12-02T20:27:44Z

...ain/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2ThingHandler.java

+                getProtocol().subscribe(this, model.asRequestObject());
+                updateStatus(ThingStatus.ONLINE);
+                updateState(model);
+            } catch (Exception e) {


Do you need to catch Exception here, or could you handle only specializations?

jlaur · 2022-12-02T20:55:45Z

...licenter2/src/main/java/org/openhab/binding/intellicenter2/internal/protocol/ICProtocol.java

+                logger.trace("Sending request {}", jsonRequest);
+                write(jsonRequest);
+            } catch (Exception e) {
+                logger.error("Error writing request {}", jsonRequest);


Is this log needed - where will the exception thrown below be caught?

jlaur · 2022-12-02T20:56:25Z

...licenter2/src/main/java/org/openhab/binding/intellicenter2/internal/protocol/ICProtocol.java

+        subscriptions.get(request.getObjectName()).add(listener);
+        String response = getUnchecked(submit(ICRequest.requestParamList(request))).getResponse();
+        if (!"200".equals(response)) {
+            logger.error("Error subscribing listener {} for request {}", listener, request);


I suppose this should not prevent the binding from working, so probably log level can be reduced?

jlaur · 2022-12-02T20:58:41Z

...main/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2PoolHandler.java

+    protected void updateState(ChannelUID channelUID, Body pool) {
+        switch (channelUID.getId()) {
+            case CHANNEL_CURRENT_TEMPERATURE:
+                updateState(channelUID, new DecimalType(pool.getCurrentTemperature()));


I believe channel type is defined as "Number:Temperature", so you need QuantityType here for correctly supporting UoM:

Suggested change

updateState(channelUID, new DecimalType(pool.getCurrentTemperature()));

updateState(channelUID, new QuantityType<>(pool.getCurrentTemperature()), SIUnits.CELSIUS);

jlaur · 2022-12-02T20:58:56Z

...main/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2PoolHandler.java

+                updateState(channelUID, new DecimalType(pool.getCurrentTemperature()));
+                break;
+            case CHANNEL_TARGET_TEMPERATURE:
+                updateState(channelUID, new DecimalType(pool.getTargetTemperature()));


And the same here.

...main/java/org/openhab/binding/intellicenter2/internal/handler/IntelliCenter2PoolHandler.java

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

lsiepel · 2023-02-24T14:18:51Z

@valdisrigdon do you need anything to proceed with this PR?

valdisrigdon · 2023-02-24T14:25:41Z

@isiepel nope, I just need time to respond and update based on all the feedback.

gutermhub · 2023-09-26T21:55:34Z

Hey guys, do you need help merging suggested changes / getting this over the line?
I'd love to see intellicenter2 support in openhab, will try to help.

valdisrigdon · 2023-09-27T15:52:47Z

@gutermhub I would love some help merging the suggested changes and getting this over the line. I've been running with this build and it's been super stable.

lsiepel · 2024-04-29T11:21:29Z

ve some help merging the suggested changes and getting this over the line. I've been running with this build and it's been super stable.

Anything specific you need help with? There are some review comments to address, you also need to get the PR updated from the main branch.

lsiepel · 2024-12-05T19:57:10Z

Unless someone want to continue on this PR, i'm about to close this due to inactivty. There are some comments open without repsonses for over two years.

valdisrigdon · 2024-12-05T20:08:25Z

Could you keep it open? I just picked this up the other day after upgrading to 4.2.0. I'll push up the changes and address the comments soon.

lsiepel · 2024-12-05T21:33:52Z

just picked this up the other day after upg

Ah great, will leave this open.

…Pentair

wborn · 2024-12-15T20:53:08Z

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

+  <parent>
+    <groupId>org.openhab.addons.bundles</groupId>
+    <artifactId>org.openhab.addons.reactor.bundles</artifactId>
+    <version>4.3.0-SNAPSHOT</version>


Suggested change

<version>4.3.0-SNAPSHOT</version>

<version>5.0.0-SNAPSHOT</version>

valdisrigdon requested a review from a team as a code owner August 29, 2022 19:15

jlaur changed the title ~~- [intellicenter2] - new binding~~ [intellicenter2] Initial contribution Aug 29, 2022

jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Aug 29, 2022

valdisrigdon force-pushed the intellicenter2 branch 3 times, most recently from 39798d3 to 4643a69 Compare August 29, 2022 19:44

jlaur requested changes Nov 28, 2022

View reviewed changes

jlaur requested changes Dec 2, 2022

View reviewed changes

wborn reviewed Dec 29, 2022

View reviewed changes

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

lsiepel added the awaiting feedback Awaiting feedback from the pull request author label Feb 24, 2023

lsiepel removed the awaiting feedback Awaiting feedback from the pull request author label Feb 24, 2023

jlaur added the awaiting feedback Awaiting feedback from the pull request author label Apr 26, 2023

lsiepel added the stale As soon as a PR is marked stale, it can be removed 6 months later. label Sep 6, 2024

lsiepel removed the stale As soon as a PR is marked stale, it can be removed 6 months later. label Dec 5, 2024

valdisrigdon force-pushed the intellicenter2 branch from 4643a69 to ea1aefc Compare December 5, 2024 21:40

valdisrigdon requested review from hmerk, cweitkamp, kaikreuzer, mherwege, marcelrv and ssalonen as code owners December 5, 2024 21:40

valdisrigdon added 2 commits December 6, 2024 11:01

fixup! [intellicenter2] feat: adds IntelliCenter2 binding for Pentair

419b476

fixup! fixup! [intellicenter2] feat: adds IntelliCenter2 binding for …

c66de7c

…Pentair

wborn reviewed Dec 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[intellicenter2] Initial contribution #13334

[intellicenter2] Initial contribution #13334

valdisrigdon commented Aug 29, 2022

valdisrigdon commented Nov 28, 2022

jlaur left a comment

jlaur Nov 28, 2022

jlaur Nov 28, 2022

lsiepel Feb 24, 2023 •

edited

Loading

jlaur Nov 28, 2022

jlaur Nov 28, 2022

valdisrigdon Dec 6, 2024

jlaur Nov 28, 2022

jlaur Nov 28, 2022

jlaur Nov 28, 2022

jlaur Nov 28, 2022

jlaur left a comment

jlaur Dec 2, 2022

jlaur Dec 2, 2022

jlaur Dec 2, 2022

jlaur Dec 2, 2022

jlaur Dec 2, 2022

jlaur Dec 2, 2022

jlaur Dec 2, 2022

jlaur Dec 2, 2022

jlaur Dec 2, 2022

lsiepel commented Feb 24, 2023

valdisrigdon commented Feb 24, 2023

gutermhub commented Sep 26, 2023

valdisrigdon commented Sep 27, 2023

lsiepel commented Apr 29, 2024

lsiepel commented Dec 5, 2024

valdisrigdon commented Dec 5, 2024

lsiepel commented Dec 5, 2024

wborn Dec 15, 2024

	updateState(channelUID, new DecimalType(pool.getCurrentTemperature()));
	updateState(channelUID, new QuantityType<>(pool.getCurrentTemperature()), SIUnits.CELSIUS);

	<version>4.3.0-SNAPSHOT</version>
	<version>5.0.0-SNAPSHOT</version>

[intellicenter2] Initial contribution #13334

Are you sure you want to change the base?

[intellicenter2] Initial contribution #13334

Conversation

valdisrigdon commented Aug 29, 2022

valdisrigdon commented Nov 28, 2022

jlaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsiepel Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsiepel commented Feb 24, 2023

valdisrigdon commented Feb 24, 2023

gutermhub commented Sep 26, 2023

valdisrigdon commented Sep 27, 2023

lsiepel commented Apr 29, 2024

lsiepel commented Dec 5, 2024

valdisrigdon commented Dec 5, 2024

lsiepel commented Dec 5, 2024

Choose a reason for hiding this comment

lsiepel Feb 24, 2023 •

edited

Loading