From a76187fa3ad00983fb7b23163de820d884ed8834 Mon Sep 17 00:00:00 2001 From: Jacob Laursen Date: Mon, 4 Dec 2023 22:40:31 +0100 Subject: [PATCH] Fix multiple state updates (#15905) Fixes #15700 Signed-off-by: Jacob Laursen --- .../hue/internal/api/dto/clip2/Resource.java | 38 +- .../api/dto/clip2/helper/Setters.java | 59 ++- .../internal/handler/Clip2BridgeHandler.java | 15 +- .../hue/internal/clip2/SettersTest.java | 438 ++++++++++++++++++ 4 files changed, 539 insertions(+), 11 deletions(-) create mode 100644 bundles/org.openhab.binding.hue/src/test/java/org/openhab/binding/hue/internal/clip2/SettersTest.java diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/Resource.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/Resource.java index 04e92905ba6cc..c41ce35f94e01 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/Resource.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/Resource.java @@ -105,7 +105,7 @@ public class Resource { private @Nullable @SerializedName("relative_rotary") RelativeRotary relativeRotary; private @Nullable List children; private @Nullable JsonElement status; - private @Nullable @SuppressWarnings("unused") Dynamics dynamics; + private @Nullable Dynamics dynamics; private @Nullable @SerializedName("contact_report") ContactReport contactReport; private @Nullable @SerializedName("tamper_reports") List tamperReports; private @Nullable String state; @@ -121,6 +121,36 @@ public Resource(@Nullable ResourceType resourceType) { } } + /** + * Check if light or grouped_light resource contains any + * relevant fields to process according to its type. + * + * As an example, {@link #colorTemperature} is relevant for a light + * resource because it's needed for updating the color-temperature channels. + * + * @return true is resource contains any relevant field + */ + public boolean hasAnyRelevantField() { + return switch (getType()) { + // https://developers.meethue.com/develop/hue-api-v2/api-reference/#resource_light_get + case LIGHT -> hasHSBField() || colorTemperature != null || dynamics != null || effects != null + || timedEffects != null; + // https://developers.meethue.com/develop/hue-api-v2/api-reference/#resource_grouped_light_get + case GROUPED_LIGHT -> on != null || dimming != null || alert != null; + default -> throw new IllegalStateException(type + " is not supported by hasAnyRelevantField()"); + }; + } + + /** + * Check if resource contains any field which is needed to represent an HSB value + * (on, dimming or color). + * + * @return true if resource has any HSB field + */ + public boolean hasHSBField() { + return on != null || dimming != null || color != null; + } + public @Nullable List getActions() { return actions; } @@ -777,7 +807,7 @@ public Resource setColorTemperature(ColorTemperature colorTemperature) { return this; } - public Resource setColorXy(ColorXy color) { + public Resource setColorXy(@Nullable ColorXy color) { this.color = color; return this; } @@ -787,7 +817,7 @@ public Resource setContactReport(ContactReport contactReport) { return this; } - public Resource setDimming(Dimming dimming) { + public Resource setDimming(@Nullable Dimming dimming) { this.dimming = dimming; return this; } @@ -844,7 +874,7 @@ public Resource setOnOff(Command command) { return this; } - public void setOnState(OnState on) { + public void setOnState(@Nullable OnState on) { this.on = on; } diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/helper/Setters.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/helper/Setters.java index 641a8025718c6..35083b09e8f43 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/helper/Setters.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/helper/Setters.java @@ -14,8 +14,13 @@ import java.math.BigDecimal; import java.time.Duration; +import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Objects; +import java.util.Set; import javax.measure.Unit; @@ -33,6 +38,7 @@ import org.openhab.binding.hue.internal.api.dto.clip2.TimedEffects; import org.openhab.binding.hue.internal.api.dto.clip2.enums.ActionType; import org.openhab.binding.hue.internal.api.dto.clip2.enums.EffectType; +import org.openhab.binding.hue.internal.api.dto.clip2.enums.ResourceType; import org.openhab.core.library.types.DecimalType; import org.openhab.core.library.types.HSBType; import org.openhab.core.library.types.PercentType; @@ -52,6 +58,8 @@ @NonNullByDefault public class Setters { + private static final Set LIGHT_TYPES = Set.of(ResourceType.LIGHT, ResourceType.GROUPED_LIGHT); + /** * Setter for Alert field: * Use the given command value to set the target resource DTO value based on the attributes of the source resource @@ -341,7 +349,56 @@ public static Resource setResource(Resource target, Resource source) { targetTimedEffects.setDuration(duration); } } - return target; } + + /** + * Merge on/dimming/color fields from light and grouped light resources. + * Subsequent resources will be merged into the first one. + * Full state resources are not supported by this method. + */ + public static Collection mergeLightResources(Collection resources) { + Map resourceIndex = new HashMap<>(); + Iterator iterator = resources.iterator(); + while (iterator.hasNext()) { + Resource resource = iterator.next(); + String id = resource.getId(); + + if (resource.hasFullState()) { + throw new IllegalStateException("Resource " + id + " has full state, this is not expected"); + } + + Resource indexedResource = resourceIndex.get(id); + if (indexedResource == null) { + resourceIndex.put(id, resource); + continue; + } + + if (!LIGHT_TYPES.contains(resource.getType()) || !resource.hasHSBField()) { + continue; + } + + OnState onState = resource.getOnState(); + if (onState != null) { + indexedResource.setOnState(onState); + resource.setOnState(null); + } + Dimming dimming = resource.getDimming(); + if (dimming != null) { + indexedResource.setDimming(dimming); + resource.setDimming(null); + } + ColorXy colorXy = resource.getColorXy(); + if (colorXy != null) { + indexedResource.setColorXy(colorXy); + resource.setColorXy(null); + } + + if (!resource.hasAnyRelevantField()) { + iterator.remove(); + } + } + + return resources; + } } diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java index c94d39ef85199..e6dd7181ea26a 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java @@ -37,6 +37,7 @@ import org.openhab.binding.hue.internal.api.dto.clip2.Resources; import org.openhab.binding.hue.internal.api.dto.clip2.enums.Archetype; import org.openhab.binding.hue.internal.api.dto.clip2.enums.ResourceType; +import org.openhab.binding.hue.internal.api.dto.clip2.helper.Setters; import org.openhab.binding.hue.internal.config.Clip2BridgeConfig; import org.openhab.binding.hue.internal.connection.Clip2Bridge; import org.openhab.binding.hue.internal.connection.HueTlsTrustManagerProvider; @@ -528,13 +529,15 @@ public void onResourcesEvent(List resources) { } private void onResourcesEventTask(List resources) { - logger.debug("onResourcesEventTask() resource count {}", resources.size()); + int numberOfResources = resources.size(); + logger.debug("onResourcesEventTask() resource count {}", numberOfResources); + Setters.mergeLightResources(resources); + if (numberOfResources != resources.size()) { + logger.debug("onResourcesEventTask() merged to {} resources", resources.size()); + } getThing().getThings().forEach(thing -> { - ThingHandler handler = thing.getHandler(); - if (handler instanceof Clip2ThingHandler) { - resources.forEach(resource -> { - ((Clip2ThingHandler) handler).onResource(resource); - }); + if (thing.getHandler() instanceof Clip2ThingHandler clip2ThingHandler) { + resources.forEach(resource -> clip2ThingHandler.onResource(resource)); } }); } diff --git a/bundles/org.openhab.binding.hue/src/test/java/org/openhab/binding/hue/internal/clip2/SettersTest.java b/bundles/org.openhab.binding.hue/src/test/java/org/openhab/binding/hue/internal/clip2/SettersTest.java new file mode 100644 index 0000000000000..87697fda07814 --- /dev/null +++ b/bundles/org.openhab.binding.hue/src/test/java/org/openhab/binding/hue/internal/clip2/SettersTest.java @@ -0,0 +1,438 @@ +/** + * Copyright (c) 2010-2023 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.hue.internal.clip2; + +import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Stream; + +import org.eclipse.jdt.annotation.NonNullByDefault; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.openhab.binding.hue.internal.api.dto.clip2.ColorTemperature; +import org.openhab.binding.hue.internal.api.dto.clip2.Dimming; +import org.openhab.binding.hue.internal.api.dto.clip2.Effects; +import org.openhab.binding.hue.internal.api.dto.clip2.OnState; +import org.openhab.binding.hue.internal.api.dto.clip2.Resource; +import org.openhab.binding.hue.internal.api.dto.clip2.enums.ResourceType; +import org.openhab.binding.hue.internal.api.dto.clip2.helper.Setters; +import org.openhab.binding.hue.internal.exceptions.DTOPresentButEmptyException; + +/** + * Tests for {@link Setters}. + * + * @author Jacob Laursen - Initial contribution + */ +@NonNullByDefault +public class SettersTest { + + /** + * Tests merging of on state and dimming for same resource. + * + * Input: + * - Resource 1: type=light/grouped_light, sparse, id=1, on=on + * - Resource 2: type=light/grouped_light, sparse, id=1, dimming=50 + * + * Expected output: + * - Resource 1: type=light/grouped_light, sparse, id=1, on=on, dimming=50 + * + * @throws DTOPresentButEmptyException + */ + @ParameterizedTest + @MethodSource("provideLightResourceTypes") + void mergeLightResourcesMergeOnStateAndDimmingWhenSparseAndSameId(ResourceType resourceType) + throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource1 = createResource(resourceType, "1"); + resource1.setOnState(createOnState(true)); + resources.add(resource1); + + Resource resource2 = createResource(resourceType, "1"); + resource2.setDimming(createDimming(50)); + resources.add(resource2); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(1))); + Resource mergedResource = resources.get(0); + assertThat(mergedResource.getId(), is(equalTo(resource1.getId()))); + assertThat(mergedResource.getType(), is(equalTo(resourceType))); + assertThat(mergedResource.hasFullState(), is(false)); + OnState actualOnState = mergedResource.getOnState(); + assertThat(actualOnState, is(notNullValue())); + if (actualOnState != null) { + assertThat(actualOnState.isOn(), is(true)); + } + Dimming actualDimming = mergedResource.getDimming(); + assertThat(actualDimming, is(notNullValue())); + if (actualDimming != null) { + assertThat(actualDimming.getBrightness(), is(equalTo(50.0))); + } + } + + private static Stream provideLightResourceTypes() { + return Stream.of(Arguments.of(ResourceType.LIGHT), Arguments.of(ResourceType.GROUPED_LIGHT)); + } + + /** + * Tests merging of dimming for same resource where last value wins. + * + * Input: + * - Resource 1: type=light, sparse, id=1, dimming=49 + * - Resource 2: type=light, sparse, id=1, dimming=50 + * + * Expected output: + * - Resource 1: type=light, sparse, id=1, dimming=50 + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesMergeDimmingToLatestValueWhenSparseAndSameId() throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource1 = createResource(ResourceType.LIGHT, "1"); + resource1.setDimming(createDimming(49)); + resources.add(resource1); + + Resource resource2 = createResource(ResourceType.LIGHT, "1"); + resource2.setDimming(createDimming(50)); + resources.add(resource2); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(1))); + Resource mergedResource = resources.get(0); + assertThat(mergedResource.hasFullState(), is(false)); + Dimming actualDimming = mergedResource.getDimming(); + assertThat(actualDimming, is(notNullValue())); + if (actualDimming != null) { + assertThat(actualDimming.getBrightness(), is(equalTo(50.0))); + } + } + + /** + * Tests merging of HSB type fields while keeping resource with effects. + * + * Input: + * - Resource 1: type=light, sparse, id=1, dimming=49 + * - Resource 2: type=light, sparse, id=1, on=on, dimming=50, effect=xxx + * + * Expected output: + * - Resource 1: type=light, sparse, id=1, on=on, dimming=50 + * - Resource 2: type=light, sparse, id=1, effect=xxx + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesMergeHSBFieldsDoNotRemoveResourceWithEffect() throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource1 = createResource(ResourceType.LIGHT, "1"); + resource1.setDimming(createDimming(49)); + resources.add(resource1); + + Resource resource2 = createResource(ResourceType.LIGHT, "1"); + resource2.setDimming(createDimming(50)); + resource2.setOnState(createOnState(true)); + resource2.setFixedEffects(new Effects()); + resources.add(resource2); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(2))); + Resource mergedResource = resources.get(0); + assertThat(mergedResource.hasFullState(), is(false)); + OnState actualOnState = mergedResource.getOnState(); + assertThat(actualOnState, is(notNullValue())); + if (actualOnState != null) { + assertThat(actualOnState.isOn(), is(true)); + } + Dimming actualDimming = mergedResource.getDimming(); + assertThat(actualDimming, is(notNullValue())); + if (actualDimming != null) { + assertThat(actualDimming.getBrightness(), is(equalTo(50.0))); + } + + Resource effectsResource = resources.get(1); + assertThat(effectsResource.hasFullState(), is(false)); + assertThat(effectsResource.getFixedEffects(), is(notNullValue())); + } + + /** + * Tests leaving different resources separated. + * + * Input: + * - Resource 1: type=light, sparse, id=1, on=on + * - Resource 2: type=light, sparse, id=2, dimming=50 + * + * Expected output: + * - Resource 1: type=light, sparse, id=1, on=on + * - Resource 2: type=light, sparse, id=2, dimming=50 + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesDoNotMergeOnStateAndDimmingWhenSparseAndDifferentId() throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource1 = createResource(ResourceType.LIGHT, "1"); + resource1.setOnState(createOnState(true)); + resources.add(resource1); + + Resource resource2 = createResource(ResourceType.LIGHT, "2"); + resource2.setDimming(createDimming(50)); + resources.add(resource2); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(2))); + Resource firstResource = resources.get(0); + OnState actualOnState = firstResource.getOnState(); + assertThat(actualOnState, is(notNullValue())); + if (actualOnState != null) { + assertThat(actualOnState.isOn(), is(true)); + } + assertThat(firstResource.getDimming(), is(nullValue())); + + Resource secondResource = resources.get(1); + assertThat(secondResource.getOnState(), is(nullValue())); + Dimming actualDimming = secondResource.getDimming(); + assertThat(actualDimming, is(notNullValue())); + if (actualDimming != null) { + assertThat(actualDimming.getBrightness(), is(equalTo(50.0))); + } + } + + /** + * Tests merging of on state and dimming for same resource when full is first. + * + * Input: + * - Resource 1: type=light, full, id=1, on=on + * + * Expected output: + * - Exception thrown, full state is not supported/expected. + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesMergeOnStateAndDimmingWhenFullStateFirstAndSameId() throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource = new Resource(ResourceType.LIGHT); + resource.setId("1"); + + resources.add(resource); + + assertThrows(IllegalStateException.class, () -> Setters.mergeLightResources(resources)); + } + + /** + * Tests leaving resources with on state and color temperature separated. + * In this case they could be merged, but it's not needed. + * + * Input: + * - Resource 1: type=light, sparse, id=1, on=on + * - Resource 2: type=light, sparse, id=1, color temperature=370 mirek + * + * Expected output: + * - Resource 1: type=light, sparse, id=1, on=on + * - Resource 2: type=light, sparse, id=1, color temperature=370 mirek + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesDoNotMergeOnStateAndColorTemperatureWhenSparseAndSameId() + throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource1 = createResource(ResourceType.LIGHT, "1"); + resource1.setOnState(createOnState(true)); + resources.add(resource1); + + Resource resource2 = createResource(ResourceType.LIGHT, "1"); + resource2.setColorTemperature(createColorTemperature(370)); + resources.add(resource2); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(2))); + Resource firstResource = resources.get(0); + OnState actualOnState = firstResource.getOnState(); + assertThat(actualOnState, is(notNullValue())); + if (actualOnState != null) { + assertThat(actualOnState.isOn(), is(true)); + } + assertThat(firstResource.getColorTemperature(), is(nullValue())); + + Resource secondResource = resources.get(1); + assertThat(secondResource.getOnState(), is(nullValue())); + ColorTemperature actualColorTemperature = secondResource.getColorTemperature(); + assertThat(actualColorTemperature, is(notNullValue())); + if (actualColorTemperature != null) { + assertThat(actualColorTemperature.getMirek(), is(equalTo(370L))); + } + } + + /** + * Tests merging resources with on state/color and leaving color temperature separated. + * In this case they could be merged, but it's not needed. + * + * Input: + * - Resource 1: type=light, sparse, id=1, on=on + * - Resource 2: type=light, sparse, id=1, dimming=50, color temperature=370 mirek + * + * Expected output: + * - Resource 1: type=light, sparse, id=1, on=on, dimming=50 + * - Resource 2: type=light, sparse, id=1, color temperature=370 mirek + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesMergeOnStateAndDimmingButNotColorTemperatureWhenSparseAndSameId() + throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource1 = createResource(ResourceType.LIGHT, "1"); + resource1.setOnState(createOnState(true)); + resources.add(resource1); + + Resource resource2 = createResource(ResourceType.LIGHT, "1"); + resource2.setDimming(createDimming(50)); + resource2.setColorTemperature(createColorTemperature(370)); + resources.add(resource2); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(2))); + Resource firstResource = resources.get(0); + OnState actualOnState = firstResource.getOnState(); + assertThat(actualOnState, is(notNullValue())); + if (actualOnState != null) { + assertThat(actualOnState.isOn(), is(true)); + } + Dimming actualDimming = firstResource.getDimming(); + assertThat(actualDimming, is(notNullValue())); + if (actualDimming != null) { + assertThat(actualDimming.getBrightness(), is(equalTo(50.0))); + } + assertThat(firstResource.getColorTemperature(), is(nullValue())); + + Resource secondResource = resources.get(1); + assertThat(secondResource.getOnState(), is(nullValue())); + assertThat(secondResource.getDimming(), is(nullValue())); + ColorTemperature actualColorTemperature = secondResource.getColorTemperature(); + assertThat(actualColorTemperature, is(notNullValue())); + if (actualColorTemperature != null) { + assertThat(actualColorTemperature.getMirek(), is(equalTo(370L))); + } + } + + /** + * Tests preserving resource with on state and color temperature. + * + * Input: + * - Resource 1: type=light, sparse, id=1, on=on, color temperature=370 + * + * Expected output: + * - Resource 1: type=light, sparse, id=1, on=on, color temperature=370 + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesSeparateOnStateAndColorTemperatureWhenSparseAndSameId() throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource = createResource(ResourceType.LIGHT, "1"); + resource.setOnState(createOnState(true)); + resource.setColorTemperature(createColorTemperature(370)); + resources.add(resource); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(1))); + + Resource firstResource = resources.get(0); + OnState actualOnState = firstResource.getOnState(); + assertThat(actualOnState, is(notNullValue())); + if (actualOnState != null) { + assertThat(actualOnState.isOn(), is(true)); + } + ColorTemperature actualColorTemperature = firstResource.getColorTemperature(); + assertThat(actualColorTemperature, is(notNullValue())); + if (actualColorTemperature != null) { + assertThat(actualColorTemperature.getMirek(), is(equalTo(370L))); + } + } + + /** + * Tests that resources that are not light or grouped_light will not throw. + * + * Input: + * - Resource 1: type=motion, sparse, id=1 + * + * Expected output: + * - Resource 1: type=motion, sparse, id=1 + * + * @throws DTOPresentButEmptyException + */ + @Test + void mergeLightResourcesNonLightResourceShouldNotThrow() throws DTOPresentButEmptyException { + List resources = new ArrayList<>(); + + Resource resource = createResource(ResourceType.MOTION, "1"); + resources.add(resource); + + Setters.mergeLightResources(resources); + + assertThat(resources.size(), is(equalTo(1))); + + Resource firstResource = resources.get(0); + assertThat(firstResource.getType(), is(equalTo(ResourceType.MOTION))); + } + + private OnState createOnState(boolean on) { + OnState onState = new OnState(); + onState.setOn(on); + + return onState; + } + + private Dimming createDimming(double brightness) { + Dimming dimming = new Dimming(); + dimming.setBrightness(brightness); + + return dimming; + } + + private ColorTemperature createColorTemperature(double mirek) { + ColorTemperature colorTemperature = new ColorTemperature(); + colorTemperature.setMirek(mirek); + + return colorTemperature; + } + + private Resource createResource(ResourceType resourceType, String id) { + Resource resource = new Resource(resourceType); + resource.setId(id); + resource.markAsSparse(); + + return resource; + } +}