[insteon] Refactor msg definition/factory and product data classes

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/command/DebugCommand.java

@@ -33,10 +33,10 @@
 import org.openhab.binding.insteon.internal.device.X10Address;
 import org.openhab.binding.insteon.internal.device.X10Device;
 import org.openhab.binding.insteon.internal.transport.PortListener;
+import org.openhab.binding.insteon.internal.transport.message.Direction;
 import org.openhab.binding.insteon.internal.transport.message.FieldException;
 import org.openhab.binding.insteon.internal.transport.message.InvalidMessageTypeException;
 import org.openhab.binding.insteon.internal.transport.message.Msg;
-import org.openhab.binding.insteon.internal.transport.message.Msg.Direction;
 import org.openhab.binding.insteon.internal.transport.message.MsgDefinitionRegistry;
 import org.openhab.binding.insteon.internal.utils.HexUtils;
 import org.openhab.core.io.console.Console;

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/InsteonModem.java

@@ -245,7 +245,7 @@ private void handleModemInfo(Msg msg) throws FieldException {
         int deviceCategory = msg.getInt("DeviceCategory");
         int subCategory = msg.getInt("DeviceSubCategory");
 
-        ProductData productData = ProductDataRegistry.getInstance().getProductData(deviceCategory, subCategory);
+        ProductData productData = ProductData.makeInsteonProduct(deviceCategory, subCategory);
         productData.setFirmwareVersion(msg.getInt("FirmwareVersion"));
 
         DeviceType deviceType = productData.getDeviceType();

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/InsteonScene.java

@@ -87,9 +87,9 @@ public List<DeviceFeature> getFeatures(InsteonAddress address) {
     }
 
     public State getState() {
-        return getEntries().stream().allMatch(entry -> entry.getState() == UnDefType.NULL) ? UnDefType.NULL
-                : OnOffType.from(getEntries().stream().filter(entry -> entry.getState() != UnDefType.NULL)
-                        .allMatch(entry -> entry.getState().equals(entry.getOnState())));
+        return getEntries().stream().noneMatch(SceneEntry::isStateDefined) ? UnDefType.NULL
+                : OnOffType
+                        .from(getEntries().stream().filter(SceneEntry::isStateDefined).allMatch(SceneEntry::isStateOn));
     }
 
     public boolean hasEntry(InsteonAddress address) {
@@ -354,6 +354,14 @@ public State getState() {
             return state;
         }
 
+        public boolean isStateDefined() {
+            return !UnDefType.NULL.equals(state);
+        }
+
+        public boolean isStateOn() {
+            return getOnState().equals(state);
+        }
+
         public void setState(State state) {
             this.state = state;
         }

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/ProductData.java

@@ -231,21 +231,9 @@ public static ProductData makeInsteonProduct(int deviceCategory, int subCategory
         ProductData productData = new ProductData();
         productData.setDeviceCategory(deviceCategory);
         productData.setSubCategory(subCategory);
-        return productData;
-    }
-
-    /**
-     * Factory method for creating a ProductData for an Insteon product
-     *
-     * @param deviceCategory the Insteon device category
-     * @param subCategory the Insteon device subcategory
-     * @param srcData the source product data to use
-     * @return the product data
-     */
-    public static ProductData makeInsteonProduct(int deviceCategory, int subCategory, @Nullable ProductData srcData) {
-        ProductData productData = makeInsteonProduct(deviceCategory, subCategory);
-        if (srcData != null) {
-            productData.update(srcData);
+        ProductData resourceData = ProductDataRegistry.getInstance().getProductData(deviceCategory, subCategory);
+        if (resourceData != null) {
+            productData.update(resourceData);
         }
         return productData;
     }

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/ProductDataRegistry.java

@@ -45,29 +45,17 @@ private ProductDataRegistry() {
      *
      * @param deviceCategory device category to match
      * @param subCategory device subcategory to match
-     * @return product data matching provided parameters
+     * @return product data if matching provided parameters, otherwise null
      */
-    public ProductData getProductData(int deviceCategory, int subCategory) {
+    public @Nullable ProductData getProductData(int deviceCategory, int subCategory) {
         int productId = getProductId(deviceCategory, subCategory);
         if (!products.containsKey(productId)) {
             logger.warn("unknown product for devCat:{} subCat:{} in device products xml file",
                     HexUtils.getHexString(deviceCategory), HexUtils.getHexString(subCategory));
             // fallback to matching product id using device category only
             productId = getProductId(deviceCategory, ProductData.SUB_CATEGORY_UNKNOWN);
         }
-
-        return ProductData.makeInsteonProduct(deviceCategory, subCategory, products.get(productId));
-    }
-
-    /**
-     * Returns the device type for a given dev/sub category
-     *
-     * @param deviceCategory device category to match
-     * @param subCategory device subcategory to match
-     * @return device type matching provided parameters
-     */
-    public @Nullable DeviceType getDeviceType(int deviceCategory, int subCategory) {
-        return getProductData(deviceCategory, subCategory).getDeviceType();
+        return products.get(productId);
     }
 
     /**
@@ -143,7 +131,9 @@ private void parseProduct(Element element) throws SAXException {
             logger.warn("overwriting previous definition of product {}", products.get(productId));
         }
 
-        ProductData productData = ProductData.makeInsteonProduct(deviceCategory, subCategory);
+        ProductData productData = new ProductData();
+        productData.setDeviceCategory(deviceCategory);
+        productData.setSubCategory(subCategory);
         productData.setProductKey(productKey);
         productData.setFirstRecordLocation(firstRecord);
 

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/RampRate.java

@@ -17,6 +17,7 @@
 import java.text.DecimalFormat;
 import java.util.Arrays;
 import java.util.Comparator;
+import java.util.List;
 import java.util.Map;
 import java.util.function.Function;
 import java.util.stream.Collectors;
@@ -64,6 +65,8 @@ public enum RampRate {
     SEC_0_2(0x1E, 0.2),
     INSTANT(0x1F, 0.1);
 
+    private static final List<String> SUPPORTED_FEATURE_TYPES = List.of(FEATURE_TYPE_GENERIC_DIMMER);
+
     private static final Map<Integer, RampRate> VALUE_MAP = Arrays.stream(values())
             .collect(Collectors.toUnmodifiableMap(rate -> rate.value, Function.identity()));
 
@@ -105,7 +108,7 @@ public String toString() {
      * @return true if supported
      */
     public static boolean supportsFeatureType(String featureType) {
-        return FEATURE_TYPE_GENERIC_DIMMER.equals(featureType);
+        return SUPPORTED_FEATURE_TYPES.contains(featureType);
     }
 
     /**

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/database/ModemDBReader.java

@@ -24,7 +24,6 @@
 import org.openhab.binding.insteon.internal.device.InsteonAddress;
 import org.openhab.binding.insteon.internal.device.InsteonModem;
 import org.openhab.binding.insteon.internal.device.ProductData;
-import org.openhab.binding.insteon.internal.device.ProductDataRegistry;
 import org.openhab.binding.insteon.internal.transport.PortListener;
 import org.openhab.binding.insteon.internal.transport.message.FieldException;
 import org.openhab.binding.insteon.internal.transport.message.InvalidMessageTypeException;
@@ -290,7 +289,7 @@ private void handleProductData(Msg msg) throws FieldException {
         int subCategory = Byte.toUnsignedInt(toAddr.getMiddleByte());
         int firmware = Byte.toUnsignedInt(toAddr.getLowByte());
         int hardware = msg.getInt("command2");
-        ProductData productData = ProductDataRegistry.getInstance().getProductData(deviceCategory, subCategory);
+        ProductData productData = ProductData.makeInsteonProduct(deviceCategory, subCategory);
         productData.setFirmwareVersion(firmware);
         productData.setHardwareVersion(hardware);
         // set product data if in modem db

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

@@ -62,8 +62,7 @@ protected void startScan() {
 
     public void discoverInsteonDevice(InsteonAddress address, @Nullable ProductData productData) {
         InsteonModem modem = handler.getModem();
-        if (handler.isDeviceDiscoveryEnabled() && modem != null && modem.getDB().hasEntry(address)
-                && !modem.hasDevice(address)) {
+        if (modem != null && modem.getDB().hasEntry(address) && !modem.hasDevice(address)) {
             addInsteonDevice(address, productData);
         } else {
             removeInsteonDevice(address);
@@ -72,8 +71,7 @@ public void discoverInsteonDevice(InsteonAddress address, @Nullable ProductData
 
     public void discoverInsteonScene(int group) {
         InsteonModem modem = handler.getModem();
-        if (handler.isSceneDiscoveryEnabled() && modem != null && modem.getDB().hasBroadcastGroup(group)
-                && !modem.hasScene(group)) {
+        if (modem != null && modem.getDB().hasBroadcastGroup(group) && !modem.hasScene(group)) {
             addInsteonScene(group);
         } else {
             removeInsteonScene(group);

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

@@ -284,14 +284,14 @@ private void cleanUpStorage(InsteonAddress address) {
 
     protected void discoverInsteonDevice(InsteonAddress address, @Nullable ProductData productData) {
         InsteonDiscoveryService discoveryService = getDiscoveryService();
-        if (discoveryService != null) {
+        if (discoveryService != null && isDeviceDiscoveryEnabled()) {
             scheduler.execute(() -> discoveryService.discoverInsteonDevice(address, productData));
         }
     }
 
     protected void discoverInsteonScene(int group) {
         InsteonDiscoveryService discoveryService = getDiscoveryService();
-        if (discoveryService != null) {
+        if (discoveryService != null && isSceneDiscoveryEnabled()) {
             scheduler.execute(() -> discoveryService.discoverInsteonScene(group));
         }
     }

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/transport/message/DataType.java

@@ -18,6 +18,7 @@
 import java.util.stream.Collectors;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
 
 /**
  * Defines the data types that can be used in the fields of a message.
@@ -29,8 +30,7 @@
 @NonNullByDefault
 public enum DataType {
     BYTE("byte", 1),
-    ADDRESS("address", 3),
-    INVALID("invalid", -1);
+    ADDRESS("address", 3);
 
     private static final Map<String, DataType> NAME_MAP = Arrays.stream(values())
             .collect(Collectors.toUnmodifiableMap(type -> type.name, Function.identity()));
@@ -55,9 +55,9 @@ public int getSize() {
      * Factory method for getting a DataType from the data type name
      *
      * @param name the data type name
-     * @return the data type
+     * @return the data type if defined, otherwise null
      */
-    public static DataType get(String name) {
-        return NAME_MAP.getOrDefault(name, DataType.INVALID);
+    public static @Nullable DataType get(String name) {
+        return NAME_MAP.get(name);
     }
 }

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/transport/message/Direction.java

@@ -0,0 +1,46 @@
+/**
+ * Copyright (c) 2010-2024 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.binding.insteon.internal.transport.message;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
+
+/**
+ * The {@link Direction} represents a message direction
+ *
+ * @author Jeremy Setton - Initial contribution
+ */
+@NonNullByDefault
+public enum Direction {
+    FROM_MODEM("IN"),
+    TO_MODEM("OUT");
+
+    private final String label;
+
+    private Direction(String label) {
+        this.label = label;
+    }
+
+    @Override
+    public String toString() {
+        return label;
+    }
+
+    public static @Nullable Direction get(String value) {
+        try {
+            return Direction.valueOf(value);
+        } catch (IllegalArgumentException e) {
+            return null;
+        }
+    }
+}

bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/transport/message/Field.java

@@ -34,6 +34,12 @@ public final class Field {
     private final int offset;
     private final DataType type;
 
+    public Field(String name, int offset, DataType type) {
+        this.name = name;
+        this.offset = offset;
+        this.type = type;
+    }
+
     public String getName() {
         return name;
     }
@@ -42,16 +48,6 @@ public int getOffset() {
         return offset;
     }
 
-    public DataType getType() {
-        return type;
-    }
-
-    public Field(String name, DataType type, int offset) {
-        this.name = name;
-        this.type = type;
-        this.offset = offset;
-    }
-
     private void check(int len, DataType t) throws FieldException {
         if (offset + type.getSize() > len) {
             throw new FieldException("field write beyond end of msg");
@@ -61,16 +57,16 @@ private void check(int len, DataType t) throws FieldException {
         }
     }
 
-    public void set(byte[] data, Object o) throws FieldException {
+    public void set(byte[] data, String value) throws FieldException, IllegalArgumentException {
         switch (type) {
             case BYTE:
-                setByte(data, (Byte) o);
+                byte b = value.isEmpty() ? 0x00 : (byte) HexUtils.toInteger(value);
+                setByte(data, b);
                 break;
             case ADDRESS:
-                setAddress(data, (InsteonAddress) o);
+                InsteonAddress address = value.isEmpty() ? InsteonAddress.UNKNOWN : new InsteonAddress(value);
+                setAddress(data, address);
                 break;
-            default:
-                throw new FieldException("field data type unknown");
         }
     }
 
@@ -141,8 +137,6 @@ public String toString(byte[] data) {
                 case ADDRESS:
                     s += getAddress(data).toString();
                     break;
-                default:
-                    throw new FieldException("field data type unknown");
             }
         } catch (FieldException e) {
             s += "NULL";