Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[boschshc] Update location properties when initializing things #17893

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[boschshc] Update location properties when initializing things #17893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCBindingConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,8 @@ private BoschSHCBindingConstants() {

// static device/service names
public static final String SERVICE_INTRUSION_DETECTION = "intrusionDetectionSystem";

// thing properties
public static final String PROPERTY_LOCATION_LEGACY = "Location";
public static final String PROPERTY_LOCATION = "location";
}
39 changes: 39 additions & 0 deletions ...hc/src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCDeviceHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
*/
package org.openhab.binding.boschshc.internal.devices;

import static org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants.PROPERTY_LOCATION;
import static org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY;

import java.util.Map;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeoutException;

Expand Down Expand Up @@ -83,9 +87,44 @@ public void initialize() {
* otherwise
*/
protected boolean processDeviceInfo(Device deviceInfo) {
try {
updateLocationPropertiesIfApplicable(deviceInfo);
} catch (Exception e) {
logger.warn("Error while updating location properties for thing {}.", getThing().getUID(), e);
}
// do not cancel thing initialization if location properties cannot be updated
return true;
}

private void updateLocationPropertiesIfApplicable(Device deviceInfo) throws BoschSHCException {
Map<String, String> thingProperties = getThing().getProperties();
removeLegacyLocationPropertyIfApplicable(thingProperties);
updateLocationPropertyIfApplicable(thingProperties, deviceInfo);
}

private void updateLocationPropertyIfApplicable(Map<String, String> thingProperties, Device deviceInfo)
throws BoschSHCException {
@Nullable
String roomName = getBridgeHandler().resolveRoomId(deviceInfo.roomId);
if (roomName != null) {
@Nullable
String currentLocation = thingProperties.get(PROPERTY_LOCATION);
if (currentLocation == null || !currentLocation.equals(roomName)) {
logger.debug("Updating property '{}' of thing {} to '{}'.", PROPERTY_LOCATION, getThing().getUID(),
roomName);
updateProperty(PROPERTY_LOCATION, roomName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlaur I have a general conceptual question on this change: we store the room names in which devices are located in a thing property, which used to have the non-compliant spelling Location, and was now changed to location starting with a lower case letter.

My question is whether a thing property is the correct place to store this in general, because I looked at properties of other things and found that they are usually quite technical / hardware-specific. It this the right place to store location metadata, or do you have better ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot about this again after receiving an e-mail notification amongst many. Can you provide an example of a location, and perhaps more importantly, what is the use-case of having it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jlaur,

no problem, this PR is not time-critical, as it does not affect the actual thing/channel logics. It is kind of in between a bug and an enhancement 😉 I'm grateful for your support in any case.

The Bosch Smart Home system allows users to manage their rooms and to assign devices to rooms. For example, users can specify that window sensor A ist located in the living room and that smoke detector B is located in the dining room.

Internally, each room has a somewhat cryptic ID (like hz_1), and a user-provided name. Users will usually assign a name in their native language. So the actual name of hz_1 might be Living Room or Wohnzimmer.

In rare cases, for example if devices are repurposed or moved, the room assignment might change. This is not reflected in openHAB so far.

Behavior before this PR:

  • Things that are found during discovery have a thing property called Location. The value is the "resolved" room name (e.g. Living Room)
  • If the location/room changes, this is not reflected in a thing property change.

Behavior with this PR:

  • If a thing has a property called Location (with upper case L), the property is removed.
  • Things that are found during discovery have a thing property called location (note the lower case l to comply with openHAB naming guideline). The value is still the "resolved" room name (e.g. Living Room)
  • If the location changes or if the location property is not present yet, the thing property location is updated with the new "resolved" room name (e.g. Kitchen)

The question is whether a thing property is the right place to store room names at all.

}
}
}

private void removeLegacyLocationPropertyIfApplicable(Map<String, String> thingProperties) {
if (thingProperties.containsKey(PROPERTY_LOCATION_LEGACY)) {
logger.debug("Removing legacy property '{}' from thing {}.", PROPERTY_LOCATION_LEGACY, getThing().getUID());
// null value indicates that the property should be removed
updateProperty(PROPERTY_LOCATION_LEGACY, null);
}
}

/**
* Attempts to obtain information about the device with the specified ID via a REST call.
* <p>
Expand Down
33 changes: 32 additions & 1 deletion ...shc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BridgeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.eclipse.jetty.http.HttpMethod.PUT;

import java.lang.reflect.Type;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -54,6 +55,7 @@
import org.openhab.binding.boschshc.internal.serialization.GsonUtils;
import org.openhab.binding.boschshc.internal.services.dto.BoschSHCServiceState;
import org.openhab.binding.boschshc.internal.services.dto.JsonRestExceptionResponse;
import org.openhab.core.cache.ExpiringCache;
import org.openhab.core.library.types.StringType;
import org.openhab.core.thing.Bridge;
import org.openhab.core.thing.Channel;
Expand Down Expand Up @@ -88,6 +90,8 @@ public class BridgeHandler extends BaseBridgeHandler {

private static final String HTTP_CLIENT_NOT_INITIALIZED = "HttpClient not initialized";

private static final Duration ROOM_CACHE_DURATION = Duration.ofMinutes(2);

private final Logger logger = LoggerFactory.getLogger(BridgeHandler.class);

/**
Expand All @@ -107,13 +111,22 @@ public class BridgeHandler extends BaseBridgeHandler {

/**
* SHC thing/device discovery service instance.
* Registered and unregistered if service is actived/deactived.
* Registered and unregistered if service is activated/deactivated.
* Used to scan for things after bridge is paired with SHC.
*/
private @Nullable ThingDiscoveryService thingDiscoveryService;

private final ScenarioHandler scenarioHandler;

private ExpiringCache<List<Room>> roomCache = new ExpiringCache<>(ROOM_CACHE_DURATION, () -> {
try {
return getRooms();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
return null;
}
});

public BridgeHandler(Bridge bridge) {
super(bridge);
scenarioHandler = new ScenarioHandler();
Expand Down Expand Up @@ -437,6 +450,24 @@ public List<Room> getRooms() throws InterruptedException {
}
}

public @Nullable List<Room> getRoomsWithCache() {
return roomCache.getValue();
}

public @Nullable String resolveRoomId(@Nullable String roomId) {
if (roomId == null) {
return null;
}

@Nullable
List<Room> rooms = getRoomsWithCache();
if (rooms != null) {
return rooms.stream().filter(r -> r.id.equals(roomId)).map(r -> r.name).findAny().orElse(null);
}

return null;
}

/**
* Get public information from Bosch SHC.
*/
Expand Down
2 changes: 1 addition & 1 deletion .../src/main/java/org/openhab/binding/boschshc/internal/discovery/ThingDiscoveryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ protected void addDevice(Device device, String roomName) {
discoveryResult.withBridge(thingHandler.getThing().getUID());

if (!roomName.isEmpty()) {
discoveryResult.withProperty("Location", roomName);
discoveryResult.withProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, roomName);
}
thingDiscovered(discoveryResult.build());

Expand Down
69 changes: 69 additions & 0 deletions ...java/org/openhab/binding/boschshc/internal/devices/AbstractBoschSHCDeviceHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,20 @@

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeoutException;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;
import org.openhab.binding.boschshc.internal.devices.bridge.dto.Device;
Expand All @@ -42,11 +48,34 @@
public abstract class AbstractBoschSHCDeviceHandlerTest<T extends BoschSHCDeviceHandler>
extends AbstractBoschSHCHandlerTest<T> {

protected static final String TAG_LEGACY_LOCATION_PROPERTY = "LegacyLocationProperty";
protected static final String TAG_LOCATION_PROPERTY = "LocationProperty";
protected static final String DEFAULT_ROOM_ID = "hz_1";

@Override
protected void configureDevice(Device device) {
super.configureDevice(device);

device.id = getDeviceID();
device.roomId = DEFAULT_ROOM_ID;
}

@Override
protected void beforeHandlerInitialization(TestInfo testInfo) {
super.beforeHandlerInitialization(testInfo);
Set<String> tags = testInfo.getTags();
if (tags.contains(TAG_LEGACY_LOCATION_PROPERTY) || tags.contains(TAG_LOCATION_PROPERTY)) {
Map<String, String> properties = new HashMap<>();
when(getThing().getProperties()).thenReturn(properties);

if (tags.contains(TAG_LEGACY_LOCATION_PROPERTY)) {
properties.put(BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY, "Living Room");
}

if (tags.contains(TAG_LOCATION_PROPERTY)) {
when(getBridgeHandler().resolveRoomId(DEFAULT_ROOM_ID)).thenReturn("Kitchen");
}
}
}

@Override
Expand Down Expand Up @@ -80,4 +109,44 @@ void initializeHandleExceptionDuringDeviceInfoRestCall(Exception exception)
argThat(status -> status.getStatus().equals(ThingStatus.OFFLINE)
&& status.getStatusDetail().equals(ThingStatusDetail.CONFIGURATION_ERROR)));
}

@Tag(TAG_LEGACY_LOCATION_PROPERTY)
@Test
protected void deleteLegacyLocationProperty() {
verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY, null);
verify(getCallback()).thingUpdated(getThing());
}

@Tag(TAG_LOCATION_PROPERTY)
@Test
protected void locationPropertyDidNotChange() {
verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
verify(getCallback()).thingUpdated(getThing());

getThing().getProperties().put(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");

// re-initialize
getFixture().initialize();

verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
verify(getCallback()).thingUpdated(getThing());
}

@Tag(TAG_LOCATION_PROPERTY)
@Test
protected void locationPropertyDidChange() {
verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
verify(getCallback()).thingUpdated(getThing());

getThing().getProperties().put(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");

getDevice().roomId = "hz_2";
when(getBridgeHandler().resolveRoomId("hz_2")).thenReturn("Dining Room");

// re-initialize
getFixture().initialize();

verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Dining Room");
verify(getCallback(), times(2)).thingUpdated(getThing());
}
}
54 changes: 54 additions & 0 deletions ...c/src/test/java/org/openhab/binding/boschshc/internal/devices/relay/RelayHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@
*/
package org.openhab.binding.boschshc.internal.devices.relay;

import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.collection.IsCollectionWithSize.hasSize;
import static org.hamcrest.collection.IsMapContaining.hasKey;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
Expand Down Expand Up @@ -376,4 +379,55 @@ void testUpdateModePropertyIfApplicableImpulseSwitchMode() {
verify(getCallback(), times(2)).thingUpdated(argThat(t -> ImpulseSwitchService.IMPULSE_SWITCH_SERVICE_NAME
.equals(t.getProperties().get(RelayHandler.PROPERTY_MODE))));
}

/**
* This has to be tested differently for the RelayHandler because the thing mock
* will be replaced by a real thing during the first initialization, which
* modifies the channels.
*/
@Test
@Tag(TAG_LEGACY_LOCATION_PROPERTY)
@Override
protected void deleteLegacyLocationProperty() {
ArgumentCaptor<Thing> thingCaptor = ArgumentCaptor.forClass(Thing.class);
verify(getCallback(), times(3)).thingUpdated(thingCaptor.capture());
List<Thing> allValues = thingCaptor.getAllValues();
assertThat(allValues, hasSize(3));
assertThat(allValues.get(2).getProperties(), not(hasKey(BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY)));
}

/**
* This has to be tested differently for the RelayHandler because the thing mock
* will be replaced by a real thing during the first initialization, which
* modifies the channels.
*/
@Test
@Tag(TAG_LOCATION_PROPERTY)
@Override
protected void locationPropertyDidNotChange() {
// re-initialize
getFixture().initialize();

verify(getCallback(), times(3)).thingUpdated(
argThat(t -> t.getProperties().get(BoschSHCBindingConstants.PROPERTY_LOCATION).equals("Kitchen")));
}

/**
* This has to be tested differently for the RelayHandler because the thing mock
* will be replaced by a real thing during the first initialization, which
* modifies the channels.
*/
@Test
@Tag(TAG_LOCATION_PROPERTY)
@Override
protected void locationPropertyDidChange() {
getDevice().roomId = "hz_2";
when(getBridgeHandler().resolveRoomId("hz_2")).thenReturn("Dining Room");

// re-initialize
getFixture().initialize();

verify(getCallback(), times(4)).thingUpdated(
argThat(t -> t.getProperties().get(BoschSHCBindingConstants.PROPERTY_LOCATION).equals("Dining Room")));
}
}
3 changes: 2 additions & 1 deletion .../test/java/org/openhab/binding/boschshc/internal/discovery/ThingDiscoveryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ void testAddDevice() {
assertThat(result.getThingUID().getId(), is("testDevice_ID"));
assertThat(result.getBridgeUID().getId(), is("testSHC"));
assertThat(result.getLabel(), is("Test Name"));
assertThat(String.valueOf(result.getProperties().get("Location")), is("TestRoom"));
assertThat(String.valueOf(result.getProperties().get(BoschSHCBindingConstants.PROPERTY_LOCATION)),
is("TestRoom"));
}

@Test
Expand Down