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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be sufficient?

Suggested change
} catch (Exception e) {
} catch (BoschSHCException e) {

otherwise you could hide bugs in case a RuntimeException is thrown.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Nullable

String roomName = getBridgeHandler().resolveRoomId(deviceInfo.roomId);
if (roomName != null) {
@Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Nullable

String currentLocation = thingProperties.get(PROPERTY_LOCATION);
if (currentLocation == null || !currentLocation.equals(roomName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified since roomName is already checked as being non-null:

Suggested change
if (currentLocation == null || !currentLocation.equals(roomName)) {
if (!roomName.equals(currentLocation)) {

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed I did not answer your second question. The use-case of having it is purely informational, there is no technical requirement to have location properties / room names and the location is not required in any thing/channel logics.

In this case It might also be an option to just remove the property completely since it is technically not required at the moment.

A possible future enhancement could be to expose rooms as openHAB things. If it becomes a requirement to model device <-> room associations, this would be done using device/room IDs, not the user-specified room names.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed I did not answer your second question. The use-case of having it is purely informational, there is no technical requirement to have location properties / room names and the location is not required in any thing/channel logics.

I agree. During the development of the thing discovery I added this room information to help the users. With the additional information it is easier to organize all the new discovered things.

Copy link
Contributor

Choose a reason for hiding this comment

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

The use-case of having it is purely informational, there is no technical requirement to have location properties / room names and the location is not required in any thing/channel logics.

In that case I think a property is the right approach. For sure it wouldn't make sense to expose as a channel.

I can think of Hue and Netatmo also having room assignments. It would be cool if this could be somehow mapped to the semantic model of openHAB, but I don't think that's currently possible, and I don't have a concrete idea what could even be gained from that. 🙂 @lolodomo, tagging you in case you have any thoughts on this topic, otherwise feel free to ignore. In any case, this is a side-topic.

Copy link
Contributor

@jlaur jlaur Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, there is also a concept of a location of a Thing: https://www.openhab.org/docs/configuration/things.html#defining-things-using-files

It can be set dynamically by setLocation in ThingHandler, but I guess this will only work for managed Things. I'm not sure if location is considered deprecated after the introduction of the semantic model. My configuration is file-based and I'm using location to keep track of where my devices are physically placed:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good point @jlaur, this would indeed be a good alternative to store the location.

However, as you mentioned, I am uncertain whether

  • What should be done in case of file-based thing configurations, in which case the location probably cannot be set
  • Things are allowed to set/overwrite the location automatically, or if the location should strictly be set manually by the user
  • If the thing location is still relevant with the semantic model

It would be great if we could get some general conceptual input. Maybe @lolodomo has some ideas, or knows someone we could ask?

}
}
}

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@david-pace, do you think a unit tests for the new public function could make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, it would make sense to add more unit tests for these new methods. I will add them as soon as I find some time.

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