[homekit] Allow configuring secondary services as members of a group (#13879)

* [homekit] allow configuring secondary services as members of a group

Required introduction of AccessoryGroup to represent the base
AccessoryInformationService for ease of configuring multiple of the
same service.

This is also "breaking" in that someone who previously had HomeKit
accessories nested directly inside of a group that was itself a
HomeKit accessory will now have those items grouped within the Home
app.

* [homekit] combine multiple readme sections on complex accessories

Signed-off-by: Cody Cutrer <cody@cutrer.us>

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitAccessoryType.java

@@ -55,6 +55,7 @@ public enum HomekitAccessoryType {
     FAUCET("Faucet"),
     MICROPHONE("Microphone"),
     SLAT("Slat"),
+    ACCESSORY_GROUP("AccessoryGroup"),
     DUMMY("Dummy");
 
     private static final Map<String, HomekitAccessoryType> TAG_MAP = new HashMap<>();

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitChangeListener.java

@@ -12,6 +12,7 @@
  */
 package org.openhab.io.homekit.internal;
 
+import java.lang.reflect.InvocationTargetException;
 import java.time.Clock;
 import java.time.Duration;
 import java.util.ArrayList;
@@ -47,6 +48,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import io.github.hapjava.accessories.HomekitAccessory;
+import io.github.hapjava.characteristics.impl.common.NameCharacteristic;
 import io.github.hapjava.server.impl.HomekitRoot;
 
 /**
@@ -438,11 +440,19 @@ public class HomekitChangeListener implements ItemRegistryChangeListener {
     private void createRootAccessories(Item item) {
         final List<Entry<HomekitAccessoryType, HomekitCharacteristicType>> accessoryTypes = HomekitAccessoryFactory
                 .getAccessoryTypes(item, metadataRegistry);
+        if (accessoryTypes.isEmpty()) {
+            return;
+        }
+
         final List<GroupItem> groups = HomekitAccessoryFactory.getAccessoryGroups(item, itemRegistry, metadataRegistry);
+        // Don't create accessories that are sub-accessories of other accessories
+        if (groups.stream().anyMatch(g -> !HomekitAccessoryFactory.getAccessoryTypes(g, metadataRegistry).isEmpty())) {
+            return;
+        }
+
         final @Nullable Map<String, Object> itemConfiguration = HomekitAccessoryFactory.getItemConfiguration(item,
                 metadataRegistry);
-        if (accessoryTypes.isEmpty() || !(groups.isEmpty() || groups.stream().noneMatch(g -> g.getBaseItem() == null))
-                || !itemIsForThisBridge(item, itemConfiguration)) {
+        if (!itemIsForThisBridge(item, itemConfiguration)) {
             return;
         }
 
@@ -451,19 +461,37 @@ public class HomekitChangeListener implements ItemRegistryChangeListener {
         logger.trace("Item {} is a HomeKit accessory of types {}. Primary type is {}", item.getName(), accessoryTypes,
                 primaryAccessoryType);
         final HomekitOHItemProxy itemProxy = new HomekitOHItemProxy(item);
-        final HomekitTaggedItem taggedItem = new HomekitTaggedItem(new HomekitOHItemProxy(item), primaryAccessoryType,
-                itemConfiguration);
+        final HomekitTaggedItem taggedItem = new HomekitTaggedItem(itemProxy, primaryAccessoryType, itemConfiguration);
         try {
             final AbstractHomekitAccessoryImpl accessory = HomekitAccessoryFactory.create(taggedItem, metadataRegistry,
                     updater, settings);
+            if (accessory.isLinkedServiceOnly()) {
+                logger.warn("Item '{}' is a '{}' which must be nested another another accessory.", taggedItem.getName(),
+                        primaryAccessoryType);
+                return;
+            }
 
             accessoryTypes.stream().filter(aType -> !primaryAccessoryType.equals(aType.getKey()))
                     .forEach(additionalAccessoryType -> {
                         final HomekitTaggedItem additionalTaggedItem = new HomekitTaggedItem(itemProxy,
                                 additionalAccessoryType.getKey(), itemConfiguration);
                         try {
-                            final HomekitAccessory additionalAccessory = HomekitAccessoryFactory
+                            final AbstractHomekitAccessoryImpl additionalAccessory = HomekitAccessoryFactory
                                     .create(additionalTaggedItem, metadataRegistry, updater, settings);
+                            // Secondary accessories that don't explicitly specify a name will implicitly
+                            // get a name characteristic based on the item's name
+                            if (!additionalAccessory.getCharacteristic(HomekitCharacteristicType.NAME).isPresent()) {
+                                try {
+                                    additionalAccessory.addCharacteristic(
+                                            new NameCharacteristic(() -> additionalAccessory.getName()));
+                                } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+                                    // This should never happen; all services should support NameCharacteristic as an
+                                    // optional Characteristic.
+                                    // If HAP-Java defined a service that doesn't support
+                                    // addOptionalCharacteristic(NameCharacteristic), then it's a bug there, and we're
+                                    // just going to ignore the exception here.
+                                }
+                            }
                             accessory.getServices().add(additionalAccessory.getPrimaryService());
                         } catch (HomekitException e) {
                             logger.warn("Cannot create additional accessory {}", additionalTaggedItem);

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/accessories/AbstractHomekitAccessoryImpl.java

@@ -12,9 +12,11 @@
  */
 package org.openhab.io.homekit.internal.accessories;
 
+import java.lang.reflect.InvocationTargetException;
 import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -41,6 +43,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import io.github.hapjava.accessories.HomekitAccessory;
+import io.github.hapjava.characteristics.Characteristic;
 import io.github.hapjava.characteristics.HomekitCharacteristicChangeCallback;
 import io.github.hapjava.characteristics.impl.base.BaseCharacteristic;
 import io.github.hapjava.services.Service;
@@ -58,6 +61,7 @@ public abstract class AbstractHomekitAccessoryImpl implements HomekitAccessory {
     private final HomekitAccessoryUpdater updater;
     private final HomekitSettings settings;
     private final List<Service> services;
+    private final Map<Class<? extends Characteristic>, Characteristic> rawCharacteristics;
 
     public AbstractHomekitAccessoryImpl(HomekitTaggedItem accessory, List<HomekitTaggedItem> characteristics,
             HomekitAccessoryUpdater updater, HomekitSettings settings) {
@@ -66,10 +70,27 @@ public abstract class AbstractHomekitAccessoryImpl implements HomekitAccessory {
         this.updater = updater;
         this.services = new ArrayList<>();
         this.settings = settings;
+        this.rawCharacteristics = new HashMap<>();
+    }
+
+    /**
+     * @param parentAccessory The primary service to link to.
+     * @return If this accessory should be nested as a linked service below a primary service,
+     *         rather than as a sibling.
+     */
+    public boolean isLinkable(HomekitAccessory parentAccessory) {
+        return false;
+    }
+
+    /**
+     * @return If this accessory is only valid as a linked service, not as a standalone accessory.
+     */
+    public boolean isLinkedServiceOnly() {
+        return false;
     }
 
     @NonNullByDefault
-    protected Optional<HomekitTaggedItem> getCharacteristic(HomekitCharacteristicType type) {
+    public Optional<HomekitTaggedItem> getCharacteristic(HomekitCharacteristicType type) {
         return characteristics.stream().filter(c -> c.getCharacteristicType() == type).findAny();
     }
 
@@ -298,8 +319,34 @@ public abstract class AbstractHomekitAccessoryImpl implements HomekitAccessory {
     }
 
     @NonNullByDefault
-    protected void addCharacteristic(HomekitTaggedItem characteristic) {
-        characteristics.add(characteristic);
+    protected void addCharacteristic(HomekitTaggedItem item, Characteristic characteristic)
+            throws NoSuchMethodException, IllegalAccessException, InvocationTargetException {
+        characteristics.add(item);
+        addCharacteristic(characteristic);
+    }
+
+    /**
+     * @param type
+     * @param characteristic
+     */
+    @NonNullByDefault
+    public void addCharacteristic(Characteristic characteristic)
+            throws NoSuchMethodException, IllegalAccessException, InvocationTargetException {
+        if (rawCharacteristics.containsKey(characteristic.getClass())) {
+            logger.warn("Accessory {} already has a characteristic of type {}; ignoring additional definition.",
+                    accessory.getName(), characteristic.getClass().getSimpleName());
+            return;
+        }
+        rawCharacteristics.put(characteristic.getClass(), characteristic);
+        var service = getPrimaryService();
+        // find the corresponding add method at service and call it.
+        service.getClass().getMethod("addOptionalCharacteristic", characteristic.getClass()).invoke(service,
+                characteristic);
+    }
+
+    @NonNullByDefault
+    public <T> Optional<T> getCharacteristic(Class<? extends T> klazz) {
+        return Optional.ofNullable((T) rawCharacteristics.get(klazz));
     }
 
     /**

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/accessories/HomekitAccessoryFactory.java

@@ -21,11 +21,13 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 import java.util.TreeMap;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -50,7 +52,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import io.github.hapjava.characteristics.Characteristic;
-import io.github.hapjava.services.Service;
+import io.github.hapjava.characteristics.impl.common.NameCharacteristic;
 
 /**
  * Creates a HomekitAccessory for a given HomekitTaggedItem.
@@ -66,6 +68,7 @@ public class HomekitAccessoryFactory {
     /** List of mandatory attributes for each accessory type. **/
     private final static Map<HomekitAccessoryType, HomekitCharacteristicType[]> MANDATORY_CHARACTERISTICS = new HashMap<HomekitAccessoryType, HomekitCharacteristicType[]>() {
         {
+            put(ACCESSORY_GROUP, new HomekitCharacteristicType[] {});
             put(LEAK_SENSOR, new HomekitCharacteristicType[] { LEAK_DETECTED_STATE });
             put(MOTION_SENSOR, new HomekitCharacteristicType[] { MOTION_DETECTED_STATE });
             put(OCCUPANCY_SENSOR, new HomekitCharacteristicType[] { OCCUPANCY_DETECTED_STATE });
@@ -108,6 +111,7 @@ public class HomekitAccessoryFactory {
     /** List of service implementation for each accessory type. **/
     private final static Map<HomekitAccessoryType, Class<? extends AbstractHomekitAccessoryImpl>> SERVICE_IMPL_MAP = new HashMap<HomekitAccessoryType, Class<? extends AbstractHomekitAccessoryImpl>>() {
         {
+            put(ACCESSORY_GROUP, HomekitAccessoryGroupImpl.class);
             put(LEAK_SENSOR, HomekitLeakSensorImpl.class);
             put(MOTION_SENSOR, HomekitMotionSensorImpl.class);
             put(OCCUPANCY_SENSOR, HomekitOccupancySensorImpl.class);
@@ -169,9 +173,16 @@ public class HomekitAccessoryFactory {
      * @throws HomekitException exception in case HomeKit accessory could not be created, e.g. due missing mandatory
      *             characteristic
      */
-    @SuppressWarnings("null")
     public static AbstractHomekitAccessoryImpl create(HomekitTaggedItem taggedItem, MetadataRegistry metadataRegistry,
             HomekitAccessoryUpdater updater, HomekitSettings settings) throws HomekitException {
+        Set<HomekitTaggedItem> ancestorServices = new HashSet<>();
+        return create(taggedItem, metadataRegistry, updater, settings, ancestorServices);
+    }
+
+    @SuppressWarnings("null")
+    private static AbstractHomekitAccessoryImpl create(HomekitTaggedItem taggedItem, MetadataRegistry metadataRegistry,
+            HomekitAccessoryUpdater updater, HomekitSettings settings, Set<HomekitTaggedItem> ancestorServices)
+            throws HomekitException {
         final HomekitAccessoryType accessoryType = taggedItem.getAccessoryType();
         logger.trace("Constructing {} of accessory type {}", taggedItem.getName(), accessoryType.getTag());
         final List<HomekitTaggedItem> foundCharacteristics = getMandatoryCharacteristicsFromItem(taggedItem,
@@ -187,10 +198,17 @@ public class HomekitAccessoryFactory {
             final @Nullable Class<? extends AbstractHomekitAccessoryImpl> accessoryImplClass = SERVICE_IMPL_MAP
                     .get(accessoryType);
             if (accessoryImplClass != null) {
+                if (ancestorServices.contains(taggedItem)) {
+                    logger.warn("Item {} has already been created. Perhaps you have circular Homekit accessory groups?",
+                            taggedItem.getName());
+                    throw new HomekitException("Circular accessory references");
+                }
+                ancestorServices.add(taggedItem);
                 accessoryImpl = accessoryImplClass.getConstructor(HomekitTaggedItem.class, List.class,
                         HomekitAccessoryUpdater.class, HomekitSettings.class)
                         .newInstance(taggedItem, foundCharacteristics, updater, settings);
                 addOptionalCharacteristics(taggedItem, accessoryImpl, metadataRegistry);
+                addLinkedServices(taggedItem, accessoryImpl, metadataRegistry, updater, settings, ancestorServices);
                 return accessoryImpl;
             } else {
                 logger.warn("Unsupported HomeKit type: {}", accessoryType.getTag());
@@ -252,9 +270,9 @@ public class HomekitAccessoryFactory {
      */
     public static List<GroupItem> getAccessoryGroups(Item item, ItemRegistry itemRegistry,
             MetadataRegistry metadataRegistry) {
-        return (item instanceof GroupItem) ? Collections.emptyList() : item.getGroupNames().stream().flatMap(name -> {
+        return item.getGroupNames().stream().flatMap(name -> {
             final @Nullable Item groupItem = itemRegistry.get(name);
-            if ((groupItem instanceof GroupItem) && ((GroupItem) groupItem).getBaseItem() == null) {
+            if (groupItem instanceof GroupItem) {
                 return Stream.of((GroupItem) groupItem);
             } else {
                 return Stream.empty();
@@ -347,7 +365,6 @@ public class HomekitAccessoryFactory {
             MetadataRegistry metadataRegistry) {
         Map<HomekitCharacteristicType, GenericItem> characteristics = getOptionalCharacteristics(
                 accessory.getRootAccessory(), metadataRegistry);
-        Service service = accessory.getPrimaryService();
         HashMap<String, HomekitOHItemProxy> proxyItems = new HashMap<>();
         proxyItems.put(taggedItem.getItem().getUID(), taggedItem.getProxyItem());
         // an accessory can have multiple optional characteristics. iterate over them.
@@ -362,19 +379,78 @@ public class HomekitAccessoryFactory {
                         getItemConfiguration(item, metadataRegistry));
                 final Characteristic characteristic = HomekitCharacteristicFactory.createCharacteristic(optionalItem,
                         accessory.getUpdater());
-                // find the corresponding add method at service and call it.
-                service.getClass().getMethod("addOptionalCharacteristic", characteristic.getClass()).invoke(service,
-                        characteristic);
-                accessory.addCharacteristic(optionalItem);
+                accessory.addCharacteristic(optionalItem, characteristic);
             } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException | HomekitException e) {
-                logger.warn("Unsupported optional HomeKit characteristic: service type {}, characteristic type {}",
-                        service.getType(), type.getTag());
+                logger.warn("Unsupported optional HomeKit characteristic: type {}, characteristic type {}",
+                        accessory.getPrimaryService(), type.getTag());
             }
         });
     }
 
     /**
-     * collect optional HomeKit characteristics for an OH item.
+     * creates HomeKit services for an openhab item that are members of this group item.
+     *
+     * @param taggedItem openhab item tagged as HomeKit item
+     * @param AbstractHomekitAccessoryImpl the accessory to add services to
+     * @param metadataRegistry openhab metadata registry required to get item meta information
+     * @param updater OH HomeKit update class that ensure the status sync between OH item and corresponding HomeKit
+     *            characteristic.
+     * @param settings OH settings
+     * @param ancestorServices set of all accessories/services under the same root accessory, for
+     *            for preventing circular references
+     * @throws HomekitException exception in case HomeKit accessory could not be created, e.g. due missing mandatory
+     *             characteristic
+     */
+    private static void addLinkedServices(HomekitTaggedItem taggedItem, AbstractHomekitAccessoryImpl accessory,
+            MetadataRegistry metadataRegistry, HomekitAccessoryUpdater updater, HomekitSettings settings,
+            Set<HomekitTaggedItem> ancestorServices) throws HomekitException {
+        final var item = taggedItem.getItem();
+        if (!(item instanceof GroupItem))
+            return;
+
+        for (var groupMember : ((GroupItem) item).getMembers().stream()
+                .sorted((lhs, rhs) -> lhs.getName().compareTo(rhs.getName())).collect(Collectors.toList())) {
+            final var characteristicTypes = getAccessoryTypes(groupMember, metadataRegistry);
+            var accessoryTypes = characteristicTypes.stream().filter(c -> c.getValue() == EMPTY)
+                    .collect(Collectors.toList());
+
+            logger.trace("accessory types for {} are {}", groupMember.getName(), accessoryTypes);
+            if (accessoryTypes.isEmpty())
+                continue;
+
+            if (accessoryTypes.size() > 1) {
+                logger.warn("Item {} is a HomeKit sub-accessory, but multiple accessory types are not allowed.",
+                        groupMember.getName());
+                continue;
+            }
+
+            final @Nullable Map<String, Object> itemConfiguration = getItemConfiguration(groupMember, metadataRegistry);
+
+            final var accessoryType = accessoryTypes.iterator().next().getKey();
+            logger.trace("Item {} is a HomeKit sub-accessory of type {}.", groupMember.getName(), accessoryType);
+            final var itemProxy = new HomekitOHItemProxy(groupMember);
+            final var subTaggedItem = new HomekitTaggedItem(itemProxy, accessoryType, itemConfiguration);
+            final var subAccessory = create(subTaggedItem, metadataRegistry, updater, settings, ancestorServices);
+
+            try {
+                subAccessory.addCharacteristic(new NameCharacteristic(() -> subAccessory.getName()));
+            } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+                // This should never happen; all services should support NameCharacteristic as an optional
+                // Characteristic.
+                // If HAP-Java defined a service that doesn't support addOptionalCharacteristic(NameCharacteristic),
+                // Then it's a bug there, and we're just going to ignore the exception here.
+            }
+
+            if (subAccessory.isLinkable(accessory)) {
+                accessory.getPrimaryService().addLinkedService(subAccessory.getPrimaryService());
+            } else {
+                accessory.getServices().add(subAccessory.getPrimaryService());
+            }
+        }
+    }
+
+    /**
+     * collect optional HomeKit characteristics for a OH item.
      *
      * @param taggedItem main OH item
      * @param metadataRegistry OH metadata registry

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/accessories/HomekitAccessoryGroupImpl.java

@@ -0,0 +1,33 @@
+/**
+ * Copyright (c) 2010-2022 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.io.homekit.internal.accessories;
+
+import java.util.List;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.openhab.io.homekit.internal.HomekitAccessoryUpdater;
+import org.openhab.io.homekit.internal.HomekitSettings;
+import org.openhab.io.homekit.internal.HomekitTaggedItem;
+
+/**
+ * Bare accessory (for being the root of a multi-service accessory).
+ *
+ * @author Cody Cutrer - Initial contribution
+ */
+@NonNullByDefault
+public class HomekitAccessoryGroupImpl extends AbstractHomekitAccessoryImpl {
+    public HomekitAccessoryGroupImpl(HomekitTaggedItem taggedItem, List<HomekitTaggedItem> mandatoryCharacteristics,
+            HomekitAccessoryUpdater updater, HomekitSettings settings) throws IncompleteAccessoryException {
+        super(taggedItem, mandatoryCharacteristics, updater, settings);
+    }
+}