[tacmi] Fix unintended toggleing and add update policy (#10095)

* [tacmi] TA-CMI: Improvements on Schema API Connection

* Removed deprecation warning
* Ensure channels get re-added when removed manually
* Debounce unintended toggeling when status-polling overlaps with new commands.
  Status fetching sometimes takes longer and then it occurs the status page reports still the old/previous value.
  With this change values for things will be ignored when the thing was updated after the start of the page fetch.

Signed-off-by: Christian Niessner <github-marvkis@christian-niessner.de>

* [tacmi] TA-CMI: Schema API Connection: Bugfix: On initialization check if custom channel types still exists and re-create them when needed...

Signed-off-by: Christian Niessner <github-marvkis@christian-niessner.de>

* [tacmi] TA-CMI: Schema API Connection: Improvement: channels now have a 'updatePolicy' configuration parameter to specify the update behaviour

Signed-off-by: Christian Niessner <github-marvkis@christian-niessner.de>

* [tacmi] TA-CMI: Improvements & some initial SI-Units for the COE connection handler

Signed-off-by: Christian Niessner <github-marvkis@christian-niessner.de>

* [tacmi] TA-CMI: only catch URISyntaxException instead of generic Exception

Signed-off-by: Christian Niessner <github-marvkis@christian-niessner.de>

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

@@ -127,10 +127,22 @@ The thing has no channels by default - they have to be added manually matching t
 
 ### TA C.M.I. schema API connection
 
-The channels provided by this thing depends on the configuration of the "schema API page".
+The channels provided by this thing depend on the configuration of the "schema API page".
 All the channels are dynamically created to match it.
 Also when the API Page is updated, the channels are also updated during the next refresh.
 
+The channels have a parameter allowing to configure their update behavior:
+
+| Parameter Label         | Parameter ID | Description                                                                                                   | Accepted values        |
+|-------------------------|--------------|-----------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------|
+| Update Policy           | updatePolicy | Update policy for this channel. Default means "On-Change" for channels that can be modified and "On-Change" for read-only channels.  | 0 (Default), 1 (On-Fetch), 2 (On-Change)   |
+
+The behavior in detail:
+
+* `Default` (`0`): When the channel is 'read-only' the update-policy defaults to _On-Fetch_ . When the channel is linked to something that can be modified it defaults to _On-Change_ .
+* `On-Fetch` (`1`): This is the default for read-only values. This means the channel is updated every time the schema page is polled. Ideally for values you want to monitor and log into charts.
+* `On-Change` (`2`): When channel values can be changed via OH it is better to only update the channel when the value changes. The binding will cache the previous value and only send an update when it changes to the previous known value. This is especially useful if you intend to link other things (like i.e. ZigBee or Shelly switches) to the TA via OH that can be controlled by different sources. This prevents unintended toggles or even toggle-loops.
+
 ### TA C.M.I. CoE Connection
 
 Some comments on the CoE Connection and channel configuration:

bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/TACmiBindingConstants.java

@@ -32,6 +32,8 @@ public class TACmiBindingConstants {
     public static final ThingTypeUID THING_TYPE_COE_BRIDGE = new ThingTypeUID(BINDING_ID, "coe-bridge");
     public static final ThingTypeUID THING_TYPE_CMI_SCHEMA = new ThingTypeUID(BINDING_ID, "cmiSchema");
 
+    public static final String CONFIG_DESCRIPTION_API_SCHEMA_DEFAULTS = "channel-type:tacmi:schemaApiDefaults";
+
     public static final ChannelTypeUID CHANNEL_TYPE_COE_DIGITAL_IN_UID = new ChannelTypeUID(BINDING_ID,
             "coe-digital-in");
     public static final ChannelTypeUID CHANNEL_TYPE_COE_ANALOG_IN_UID = new ChannelTypeUID(BINDING_ID, "coe-analog-in");

bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/coe/TACmiHandler.java

@@ -31,6 +31,9 @@ import org.openhab.binding.tacmi.internal.message.Message;
 import org.openhab.binding.tacmi.internal.message.MessageType;
 import org.openhab.core.library.types.DecimalType;
 import org.openhab.core.library.types.OnOffType;
+import org.openhab.core.library.types.QuantityType;
+import org.openhab.core.library.unit.SIUnits;
+import org.openhab.core.library.unit.Units;
 import org.openhab.core.thing.Bridge;
 import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
@@ -286,7 +289,7 @@ public class TACmiHandler extends BaseThingHandler {
         if (analog) {
             final TACmiMeasureType measureType = TACmiMeasureType
                     .values()[((TACmiChannelConfigurationAnalog) channelConfig).type];
-            final DecimalType dt = (DecimalType) command;
+            final Number dt = (Number) command;
             final double val = dt.doubleValue() * measureType.getOffset();
             modified = message.setValue(outputIdx, (short) val, measureType.ordinal());
         } else {
@@ -352,7 +355,29 @@ public class TACmiHandler extends BaseThingHandler {
 
             if (message.getType() == MessageType.ANALOG) {
                 final AnalogValue value = ((AnalogMessage) message).getAnalogValue(output);
-                updateState(channel.getUID(), new DecimalType(value.value));
+                State newState;
+                switch (value.measureType) {
+                    case TEMPERATURE:
+                        newState = new QuantityType<>(value.value, SIUnits.CELSIUS);
+                        break;
+                    case KILOWATT:
+                        // TA uses kW, in OH we use W
+                        newState = new QuantityType<>(value.value * 1000, Units.WATT);
+                        break;
+                    case KILOWATTHOURS:
+                        newState = new QuantityType<>(value.value, Units.KILOWATT_HOUR);
+                        break;
+                    case MEGAWATTHOURS:
+                        newState = new QuantityType<>(value.value, Units.MEGAWATT_HOUR);
+                        break;
+                    case SECONDS:
+                        newState = new QuantityType<>(value.value, Units.SECOND);
+                        break;
+                    default:
+                        newState = new DecimalType(value.value);
+                        break;
+                }
+                updateState(channel.getUID(), newState);
             } else {
                 final boolean state = ((DigitalMessage) message).getPortState(output);
                 updateState(channel.getUID(), state ? OnOffType.ON : OnOffType.OFF);

bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/ApiPageEntry.java

@@ -67,6 +67,12 @@ public class ApiPageEntry {
      */
     private State lastState;
 
+    /**
+     * Timestamp (epoch ms) when last 'outgoing' command was sent.
+     * Required for de-bounce overlapping effects when status-poll's and updates overlap.
+     */
+    private long lastCommandTS;
+
     protected ApiPageEntry(final Type type, final Channel channel, @Nullable final String address,
             @Nullable ChangerX2Entry changerX2Entry, State lastState) {
         this.type = type;
@@ -83,4 +89,12 @@ public class ApiPageEntry {
     public State getLastState() {
         return lastState;
     }
+
+    public long getLastCommandTS() {
+        return lastCommandTS;
+    }
+
+    public void setLastCommandTS(long lastCommandTS) {
+        this.lastCommandTS = lastCommandTS;
+    }
 }

bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/ApiPageParser.java

@@ -14,11 +14,13 @@ package org.openhab.binding.tacmi.internal.schema;
 
 import java.math.BigDecimal;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
@@ -91,6 +93,9 @@ public class ApiPageParser extends AbstractSimpleMarkupHandler {
     private Map<String, ApiPageEntry> entries;
     private Set<String> seenNames = new HashSet<>();
     private List<Channel> channels = new ArrayList<>();
+    // Time stamp when status request was started.
+    private final long statusRequestStartTS;
+    private static @Nullable URI configDescriptionUriAPISchemaDefaults;
 
     public ApiPageParser(TACmiSchemaHandler taCmiSchemaHandler, Map<String, ApiPageEntry> entries,
             TACmiChannelTypeProvider channelTypeProvider) {
@@ -98,6 +103,16 @@ public class ApiPageParser extends AbstractSimpleMarkupHandler {
         this.taCmiSchemaHandler = taCmiSchemaHandler;
         this.entries = entries;
         this.channelTypeProvider = channelTypeProvider;
+        this.statusRequestStartTS = System.currentTimeMillis();
+        if (configDescriptionUriAPISchemaDefaults == null) {
+            try {
+                configDescriptionUriAPISchemaDefaults = new URI(
+                        TACmiBindingConstants.CONFIG_DESCRIPTION_API_SCHEMA_DEFAULTS);
+            } catch (URISyntaxException ex) {
+                logger.warn("Can't create ConfigDescription URI '{}', ConfigDescription for channels not avilable!",
+                        TACmiBindingConstants.CONFIG_DESCRIPTION_API_SCHEMA_DEFAULTS);
+            }
+        }
     }
 
     @Override
@@ -291,8 +306,8 @@ public class ApiPageParser extends AbstractSimpleMarkupHandler {
 
     private void getApiPageEntry(@Nullable String id2, int line, int col, String shortName, String description,
             Object value) {
-        if (logger.isDebugEnabled()) {
+        if (logger.isTraceEnabled()) {
-            logger.debug("Found parameter {}:{}:{} [{}] : {} \"{}\" = {}", id, line, col, this.fieldType, shortName,
+            logger.trace("Found parameter {}:{}:{} [{}] : {} \"{}\" = {}", id, line, col, this.fieldType, shortName,
                     description, value);
         }
         if (!this.seenNames.add(shortName)) {
@@ -411,6 +426,7 @@ public class ApiPageParser extends AbstractSimpleMarkupHandler {
                 return;
         }
         ApiPageEntry e = this.entries.get(shortName);
+        boolean isNewEntry;
         if (e == null || e.type != type || !channelType.equals(e.channel.getAcceptedItemType())) {
             @Nullable
             Channel channel = this.taCmiSchemaHandler.getThing().getChannel(shortName);
@@ -427,7 +443,7 @@ public class ApiPageParser extends AbstractSimpleMarkupHandler {
                     logger.warn("Error loading API Scheme: {} ", ex.getMessage());
                 }
             }
-            if (channel == null) {
+            if (channel == null || !Objects.equals(ctuid, channel.getChannelTypeUID())) {
                 logger.debug("Creating / updating channel {} of type {} for '{}'", shortName, channelType, description);
                 this.configChanged = true;
                 ChannelUID channelUID = new ChannelUID(this.taCmiSchemaHandler.getThing().getUID(), shortName);
@@ -436,8 +452,74 @@ public class ApiPageParser extends AbstractSimpleMarkupHandler {
                 if (ctuid != null) {
                     channelBuilder.withType(ctuid);
                 } else if (cx2e != null) {
-                    StateDescriptionFragmentBuilder sdb = StateDescriptionFragmentBuilder.create()
+                    ChannelType ct = buildAndRegisterChannelType(shortName, type, cx2e);
-                            .withReadOnly(type.readOnly);
+
+                    channelBuilder.withType(ct.getUID());
+                } else {
+                    logger.warn("Error configurating channel for {}: channeltype cannot be determined!", shortName);
+                }
+                channel = channelBuilder.build(); // add configuration property...
+            } else if (ctuid == null && cx2e != null) {
+                // custom channel type - check if it already exists and recreate when needed...
+                ChannelTypeUID curCtuid = channel.getChannelTypeUID();
+                if (curCtuid != null) {
+                    ChannelType ct = channelTypeProvider.getChannelType(curCtuid, null);
+                    if (ct == null) {
+                        buildAndRegisterChannelType(shortName, type, cx2e);
+                    }
+                }
+            }
+            this.configChanged = true;
+            e = new ApiPageEntry(type, channel, address, cx2e, state);
+            this.entries.put(shortName, e);
+            isNewEntry = true;
+        } else {
+            isNewEntry = false;
+        }
+        this.channels.add(e.channel);
+        // only update the state when there was no state change sent to C.M.I. after we started
+        // polling the state. It might deliver the previous / old state.
+        if (e.getLastCommandTS() < this.statusRequestStartTS) {
+            Number updatePolicyI = (Number) e.channel.getConfiguration().get("updatePolicy");
+            int updatePolicy = updatePolicyI == null ? 0 : updatePolicyI.intValue();
+            switch (updatePolicy) {
+                case 0: // 'default'
+                default:
+                    // we do 'On-Fetch' update when channel is changeable, otherwise 'On-Change'
+                    switch (e.type) {
+                        case NUMERIC_FORM:
+                        case STATE_FORM:
+                        case SWITCH_BUTTON:
+                        case SWITCH_FORM:
+                            if (isNewEntry || !state.equals(e.getLastState())) {
+                                e.setLastState(state);
+                                this.taCmiSchemaHandler.updateState(e.channel.getUID(), state);
+                            }
+                            break;
+                        case READ_ONLY_NUMERIC:
+                        case READ_ONLY_STATE:
+                        case READ_ONLY_SWITCH:
+                            e.setLastState(state);
+                            this.taCmiSchemaHandler.updateState(e.channel.getUID(), state);
+                            break;
+                    }
+                    break;
+                case 1: // On-Fetch
+                    e.setLastState(state);
+                    this.taCmiSchemaHandler.updateState(e.channel.getUID(), state);
+                    break;
+                case 2: // On-Change
+                    if (isNewEntry || !state.equals(e.getLastState())) {
+                        e.setLastState(state);
+                        this.taCmiSchemaHandler.updateState(e.channel.getUID(), state);
+                    }
+                    break;
+            }
+        }
+    }
+
+    private ChannelType buildAndRegisterChannelType(String shortName, Type type, ChangerX2Entry cx2e) {
+        StateDescriptionFragmentBuilder sdb = StateDescriptionFragmentBuilder.create().withReadOnly(type.readOnly);
         String itemType;
         switch (cx2e.optionType) {
             case NUMBER:
@@ -467,24 +549,19 @@ public class ApiPageParser extends AbstractSimpleMarkupHandler {
             default:
                 throw new IllegalStateException();
         }
-                    ChannelType ct = ChannelTypeBuilder
+        ChannelTypeBuilder<?> ctb = ChannelTypeBuilder
                 .state(new ChannelTypeUID(TACmiBindingConstants.BINDING_ID, shortName), shortName, itemType)
-                            .withDescription("Auto-created for " + shortName).withStateDescriptionFragment(sdb.build())
+                .withDescription("Auto-created for " + shortName).withStateDescriptionFragment(sdb.build());
-                            .build();
+
+        // add config description URI
+        URI cdu = configDescriptionUriAPISchemaDefaults;
+        if (cdu != null) {
+            ctb = ctb.withConfigDescriptionURI(cdu);
+        }
+
+        ChannelType ct = ctb.build();
         channelTypeProvider.addChannelType(ct);
-                    channelBuilder.withType(ct.getUID());
+        return ct;
-                } else {
-                    logger.warn("Error configurating channel for {}: channeltype cannot be determined!", shortName);
-                }
-                channel = channelBuilder.build(); // add configuration property...
-            }
-            this.configChanged = true;
-            e = new ApiPageEntry(type, channel, address, cx2e, state);
-            this.entries.put(shortName, e);
-        }
-        this.channels.add(e.channel);
-        e.setLastState(state);
-        this.taCmiSchemaHandler.updateState(e.channel.getUID(), state);
     }
 
     protected boolean isConfigChanged() {

bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/TACmiSchemaHandler.java

@@ -152,8 +152,8 @@ public class TACmiSchemaHandler extends BaseThingHandler {
             responseString = response.getContentAsString();
         }
 
-        if (logger.isDebugEnabled()) {
+        if (logger.isTraceEnabled()) {
-            logger.debug("Response body was: {} ", responseString);
+            logger.trace("Response body was: {} ", responseString);
         }
 
         final ISimpleMarkupParser parser = new SimpleMarkupParser(this.noRestrictions);
@@ -170,9 +170,9 @@ public class TACmiSchemaHandler extends BaseThingHandler {
             final ApiPageParser pp = parsePage(schemaApiPage,
                     new ApiPageParser(this, entries, this.channelTypeProvider));
 
-            if (pp.isConfigChanged()) {
-                // we have to update our channels...
             final List<Channel> channels = pp.getChannels();
+            if (pp.isConfigChanged() || channels.size() != this.getThing().getChannels().size()) {
+                // we have to update our channels...
                 final ThingBuilder thingBuilder = editThing();
                 thingBuilder.withChannels(channels);
                 updateThing(thingBuilder.build());
@@ -271,6 +271,7 @@ public class TACmiSchemaHandler extends BaseThingHandler {
                 return;
         }
         try {
+            e.setLastCommandTS(System.currentTimeMillis());
             ContentResponse res = reqUpdate.send();
             if (res.getStatus() == 200) {
                 // update ok, we update the state

bundles/org.openhab.binding.tacmi/src/main/resources/OH-INF/config/config.xml

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<config-description:config-descriptions
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xmlns:config-description="https://openhab.org/schemas/config-description/v1.0.0"
+	xsi:schemaLocation="https://openhab.org/schemas/config-description/v1.0.0 https://openhab.org/schemas/config-description-1.0.0.xsd">
+
+	<config-description uri="channel-type:tacmi:schemaApiDefaults">
+		<parameter name="updatePolicy" type="integer" min="0" max="2">
+			<label>Update Policy</label>
+			<description>Update policy for this channel. Default means "On-Change" for channels that can be modified and
+				"On-Fetch" for read-only channels.</description>
+			<default>0</default>
+			<options>
+				<option value="0">Default</option>
+				<option value="1">On-Fetch</option>
+				<option value="2">On-Update</option>
+			</options>
+		</parameter>
+	</config-description>
+</config-description:config-descriptions>

bundles/org.openhab.binding.tacmi/src/main/resources/OH-INF/thing/thing-types.xml

@@ -151,22 +151,26 @@
 		<label>Switch State (Read-Only)</label>
 		<description>An On/Off state read from C.M.I.</description>
 		<state readOnly="true"/>
+		<config-description-ref uri="channel-type:tacmi:schemaApiDefaults"/>
 	</channel-type>
 	<channel-type id="schema-switch-rw">
 		<item-type>Switch</item-type>
 		<label>Switch State (Changeable)</label>
 		<description>A modifiable On/Off state read from C.M.I.</description>
+		<config-description-ref uri="channel-type:tacmi:schemaApiDefaults"/>
 	</channel-type>
 	<channel-type id="schema-numeric-ro">
 		<item-type>Number</item-type>
 		<label>Value</label>
 		<description>A numeric value read from C.M.I.</description>
 		<state readOnly="true"/>
+		<config-description-ref uri="channel-type:tacmi:schemaApiDefaults"/>
 	</channel-type>
 	<channel-type id="schema-state-ro">
 		<item-type>String</item-type>
 		<label>Value</label>
 		<description>A state value read from C.M.I.</description>
 		<state readOnly="true"/>
+		<config-description-ref uri="channel-type:tacmi:schemaApiDefaults"/>
 	</channel-type>
 </thing:thing-descriptions>