[insteon] Fix legacy backward compatibility (openhab#17981)

Signed-off-by: jsetton <[email protected]>

Author: jsetton

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

@@ -26,6 +26,7 @@ You can follow the [migration guide](#migration-guide).
 
 However, the new version is fully backward compatible by supporting the legacy things.
 On the first start, existing `device` things connected to a `network` bridge will be migrated to the `legacy-device` thing type while still keeping the same ids to prevent any breakage.
+For textual configuration with defined thing channels, the channel types must be manually updated to the new ones by adding the `legacy` prefix and capitalizing the first letter, as shown in [these examples](#full-example).
 It is important to note that once the migration has occurred, downgrading to an older version will not be possible.
 
 ## Supported Things
@@ -474,27 +475,27 @@ Bridge insteon:plm:home [serialPort="/dev/ttyUSB0"] {
   Bridge insteon:network:home [port="/dev/ttyUSB0"] {
     Thing device 22F8A8 [address="22.F8.A8", productKey="F00.00.15"] {
       Channels:
-        Type keypadButtonA : keypadButtonA [ group=3 ]
-        Type keypadButtonB : keypadButtonB [ group=4 ]
-        Type keypadButtonC : keypadButtonC [ group=5 ]
-        Type keypadButtonD : keypadButtonD [ group=6 ]
+        Type legacyKeypadButtonA : keypadButtonA [ group=3 ]
+        Type legacyKeypadButtonB : keypadButtonB [ group=4 ]
+        Type legacyKeypadButtonC : keypadButtonC [ group=5 ]
+        Type legacyKeypadButtonD : keypadButtonD [ group=6 ]
     }
     Thing device 238D93 [address="23.8D.93", productKey="F00.00.12"]
     Thing device 238F55 [address="23.8F.55", productKey="F00.00.11"] {
       Channels:
-        Type dimmer        : dimmer [related="23.B0.D9+23.8F.C9"]
+        Type legacyDimmer        : dimmer [related="23.B0.D9+23.8F.C9"]
     }
     Thing device 238FC9 [address="23.8F.C9", productKey="F00.00.11"] {
       Channels:
-        Type dimmer        : dimmer [related="23.8F.55+23.B0.D9"]
+        Type legacyDimmer        : dimmer [related="23.8F.55+23.B0.D9"]
     }
     Thing device 23B0D9 [address="23.B0.D9", productKey="F00.00.11"] {
       Channels:
-        Type dimmer        : dimmer [related="23.8F.55+23.8F.C9"]
+        Type legacyDimmer        : dimmer [related="23.8F.55+23.8F.C9"]
     }
     Thing device 243141 [address="24.31.41", productKey="F00.00.11"]  {
       Channels:
-        Type dimmer        : dimmer [dimmermax=60]
+        Type legacyDimmer        : dimmer [dimmermax=60]
     }
   }
   ```
@@ -644,11 +645,11 @@ Thing device 23b0d9 [address="23.B0.D9"] {
   Bridge insteon:network:home [port="/dev/ttyUSB0"] {
     Thing device AABBCC [address="AA.BB.CC", productKey="F00.00.11"]  {
       Channels:
-        Type dimmer     : dimmer [dimmermax=70]
+        Type legacyDimmer     : dimmer [dimmermax=70]
     }
     Thing device AABBCD [address="AA.BB.CD", productKey="F00.00.15"]  {
       Channels:
-        Type loadDimmer : loadDimmer [dimmermax=60]
+        Type legacyLoadDimmer : loadDimmer [dimmermax=60]
     }
   }
   ```
@@ -782,10 +783,10 @@ end
   Bridge insteon:network:home [port="/dev/ttyUSB0"] {
     Thing device AABBCC [address="AA.BB.CC", productKey="F00.00.15"] {
       Channels:
-        Type keypadButtonA : keypadButtonA [ group="0xf3" ]
-        Type keypadButtonB : keypadButtonB [ group="0xf4" ]
-        Type keypadButtonC : keypadButtonC [ group="0xf5" ]
-        Type keypadButtonD : keypadButtonD [ group="0xf6" ]
+        Type legacyKeypadButtonA : keypadButtonA [ group="0xf3" ]
+        Type legacyKeypadButtonB : keypadButtonB [ group="0xf4" ]
+        Type legacyKeypadButtonC : keypadButtonC [ group="0xf5" ]
+        Type legacyKeypadButtonD : keypadButtonD [ group="0xf6" ]
     }
   }
   ```
@@ -1324,7 +1325,7 @@ An `ON` state indicates that all the device states associated to a scene are mat
   Bridge insteon:network:home [port="/dev/ttyUSB0"] {
     Thing device AABBCC             [address="AA.BB.CC", productKey="0x000045"] {
       Channels:
-        Type broadcastOnOff : broadcastOnOff#2
+        Type legacyBroadcastOnOff : broadcastOnOff#2
     }
   }
   ```
@@ -1426,11 +1427,11 @@ For scenes, these will be polled based on the modem database, after sending a gr
   Bridge insteon:network:home [port="/dev/ttyUSB0"] {
     Thing device AABBCC [address="AA.BB.CC", productKey="F00.00.11"] {
       Channels:
-        Type dimmer : dimmer [related="AA.BB.DD"]
+        Type legacyDimmer : dimmer [related="AA.BB.DD"]
     }
     Thing device AABBDD [address="AA.BB.DD", productKey="F00.00.11"] {
       Channels:
-        Type dimmer : dimmer [related="AA.BB.CC"]
+        Type legacyDimmer : dimmer [related="AA.BB.CC"]
     }
   }
   ```
@@ -1444,7 +1445,7 @@ For scenes, these will be polled based on the modem database, after sending a gr
   Bridge insteon:network:home [port="/dev/ttyUSB0"] {
     Thing device AABBCC [address="AA.BB.CC", productKey="0x000045"] {
       Channels:
-        Type broadcastOnOff : broadcastOnOff#3 [related="AA.BB.DD+AA.BB.EE"]
+        Type legacyBroadcastOnOff : broadcastOnOff#3 [related="AA.BB.DD+AA.BB.EE"]
     }
     Thing device AABBDD [address="AA.BB.DD", productKey="F00.00.11"]
     Thing device AABBEE [address="AA.BB.EE", productKey="F00.00.11"]

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/discovery/InsteonLegacyDiscoveryService.java

@@ -59,13 +59,16 @@ public void addInsteonDevices(List<InsteonAddress> addresses) {
             ThingUID bridgeUID = handler.getThing().getUID();
             String id = address.toString().replace(".", "");
             ThingUID thingUID = new ThingUID(THING_TYPE_LEGACY_DEVICE, bridgeUID, id);
+            ThingUID oldThingUID = new ThingUID(THING_TYPE_DEVICE, bridgeUID, id);
             String label = "Insteon Device (Legacy) " + address;
             Map<String, Object> properties = new HashMap<>();
             properties.put(PROPERTY_DEVICE_ADDRESS, address.toString());
 
             thingDiscovered(DiscoveryResultBuilder.create(thingUID).withBridge(bridgeUID).withLabel(label)
                     .withProperties(properties).withRepresentationProperty(PROPERTY_DEVICE_ADDRESS).build());
 
+            thingRemoved(oldThingUID);
+
             logger.debug("added Insteon device {} to inbox", address);
         }
     }

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/handler/InsteonDeviceHandler.java

@@ -15,10 +15,8 @@
 import static org.openhab.binding.insteon.internal.InsteonBindingConstants.*;
 
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -32,7 +30,6 @@
 import org.openhab.binding.insteon.internal.device.InsteonDevice;
 import org.openhab.binding.insteon.internal.device.InsteonEngine;
 import org.openhab.binding.insteon.internal.device.InsteonModem;
-import org.openhab.core.config.core.Configuration;
 import org.openhab.core.thing.Bridge;
 import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
@@ -114,10 +111,7 @@ public void initialize() {
 
     private void changeThingType(ThingTypeUID thingTypeUID, @Nullable BridgeHandler bridgeHandler) {
         if (bridgeHandler instanceof InsteonLegacyNetworkHandler legacyNetworkHandler) {
-            Map<ChannelUID, Configuration> channelConfigs = getThing().getChannels().stream()
-                    .collect(Collectors.toMap(Channel::getUID, Channel::getConfiguration));
-
-            legacyNetworkHandler.addChannelConfigs(channelConfigs);
+            getThing().getChannels().forEach(legacyNetworkHandler::cacheChannel);
         }
 
         changeThingType(thingTypeUID, getConfig());

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/handler/InsteonLegacyDeviceHandler.java

@@ -19,11 +19,9 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -37,7 +35,6 @@
 import org.openhab.binding.insteon.internal.device.LegacyDeviceFeature;
 import org.openhab.binding.insteon.internal.device.LegacyDeviceTypeLoader;
 import org.openhab.binding.insteon.internal.device.X10Address;
-import org.openhab.core.config.core.Configuration;
 import org.openhab.core.thing.Bridge;
 import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
@@ -46,6 +43,7 @@
 import org.openhab.core.thing.ThingStatusDetail;
 import org.openhab.core.thing.binding.BaseThingHandler;
 import org.openhab.core.thing.binding.ThingHandlerCallback;
+import org.openhab.core.thing.binding.builder.ChannelBuilder;
 import org.openhab.core.thing.type.ChannelTypeUID;
 import org.openhab.core.types.Command;
 import org.openhab.core.util.StringUtils;
@@ -203,12 +201,17 @@ public void initialize() {
                 if (feature != null) {
                     if (!feature.isFeatureGroup()) {
                         if (channelId.equalsIgnoreCase(BROADCAST_ON_OFF)) {
-                            Set<String> broadcastChannels = new HashSet<>();
                             for (Channel channel : thing.getChannels()) {
                                 String id = channel.getUID().getId();
                                 if (id.startsWith(BROADCAST_ON_OFF)) {
                                     channelMap.put(id, channel);
-                                    broadcastChannels.add(id);
+                                }
+                            }
+
+                            for (Channel channel : getInsteonNetworkHandler().getCachedChannels(thing.getUID())) {
+                                String id = channel.getUID().getId();
+                                if (id.startsWith(BROADCAST_ON_OFF)) {
+                                    channelMap.putIfAbsent(id, createChannel(id, BROADCAST_ON_OFF, callback));
                                 }
                             }
 
@@ -220,10 +223,7 @@ public void initialize() {
                                     for (Object value : list) {
                                         if (value instanceof Double doubleValue && doubleValue % 1 == 0) {
                                             String id = BROADCAST_ON_OFF + "#" + doubleValue.intValue();
-                                            if (!broadcastChannels.contains(id)) {
-                                                channelMap.put(id, createChannel(id, BROADCAST_ON_OFF, callback));
-                                                broadcastChannels.add(id);
-                                            }
+                                            channelMap.putIfAbsent(id, createChannel(id, BROADCAST_ON_OFF, callback));
                                         } else {
                                             valid = false;
                                             break;
@@ -315,6 +315,9 @@ public void dispose() {
             if (getBridge() != null && address != null) {
                 getInsteonBinding().removeDevice(address);
 
+                getThing().getChannels().stream().map(Channel::getUID).filter(this::isLinked)
+                        .forEach(this::channelUnlinked);
+
                 logger.debug("removed {} address = {}", getThing().getUID().getAsString(), address);
             }
 
@@ -481,23 +484,18 @@ private Channel createChannel(String channelId, String channelTypeId, ThingHandl
         ChannelUID channelUID = new ChannelUID(getThing().getUID(), channelId);
         ChannelTypeUID channelTypeUID = new ChannelTypeUID(InsteonBindingConstants.BINDING_ID,
                 CHANNEL_TYPE_ID_PREFIX + StringUtils.capitalize(channelTypeId));
-        Configuration channelConfig = getChannelConfig(channelUID);
+        Channel oldChannel = getInsteonNetworkHandler().pollCachedChannel(channelUID);
         Channel channel = getThing().getChannel(channelUID);
         if (channel == null) {
-            channel = callback.createChannelBuilder(channelUID, channelTypeUID).withConfiguration(channelConfig)
-                    .build();
+            if (oldChannel == null) {
+                channel = callback.createChannelBuilder(channelUID, channelTypeUID).build();
+            } else {
+                channel = ChannelBuilder.create(oldChannel).withType(channelTypeUID).build();
+            }
         }
         return channel;
     }
 
-    private Configuration getChannelConfig(ChannelUID channelUID) {
-        try {
-            return getInsteonNetworkHandler().getChannelConfig(channelUID);
-        } catch (IllegalArgumentException e) {
-            return new Configuration();
-        }
-    }
-
     private InsteonLegacyNetworkHandler getInsteonNetworkHandler() {
         Bridge bridge = getBridge();
         if (bridge == null) {

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/handler/InsteonLegacyNetworkHandler.java

@@ -28,10 +28,10 @@
 import org.openhab.binding.insteon.internal.device.DeviceAddress;
 import org.openhab.binding.insteon.internal.device.InsteonAddress;
 import org.openhab.binding.insteon.internal.discovery.InsteonLegacyDiscoveryService;
-import org.openhab.core.config.core.Configuration;
 import org.openhab.core.io.console.Console;
 import org.openhab.core.io.transport.serial.SerialPortManager;
 import org.openhab.core.thing.Bridge;
+import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
 import org.openhab.core.thing.Thing;
 import org.openhab.core.thing.ThingManager;
@@ -74,7 +74,7 @@ public class InsteonLegacyNetworkHandler extends BaseBridgeHandler {
     private ThingRegistry thingRegistry;
     private Map<String, String> deviceInfo = new ConcurrentHashMap<>();
     private Map<String, String> channelInfo = new ConcurrentHashMap<>();
-    private Map<ChannelUID, Configuration> channelConfigs = new ConcurrentHashMap<>();
+    private Map<ChannelUID, Channel> channelCache = new ConcurrentHashMap<>();
 
     public InsteonLegacyNetworkHandler(Bridge bridge, HttpClient httpClient, SerialPortManager serialPortManager,
             ThingManager thingManager, ThingRegistry thingRegistry) {
@@ -318,12 +318,17 @@ public void unlinked(ChannelUID uid) {
         channelInfo.remove(uid.getAsString());
     }
 
-    public Configuration getChannelConfig(ChannelUID channelUID) {
-        return channelConfigs.getOrDefault(channelUID, new Configuration());
+    public List<Channel> getCachedChannels(ThingUID thingUID) {
+        return channelCache.values().stream().filter(channel -> channel.getUID().getThingUID().equals(thingUID))
+                .toList();
     }
 
-    public void addChannelConfigs(Map<ChannelUID, Configuration> channelConfigs) {
-        this.channelConfigs.putAll(channelConfigs);
+    public @Nullable Channel pollCachedChannel(ChannelUID channelUID) {
+        return channelCache.remove(channelUID);
+    }
+
+    public void cacheChannel(Channel channel) {
+        channelCache.put(channel.getUID(), channel);
     }
 
     private void display(Console console, Map<String, String> info) {