[boschshc] Update location properties when initializing things #17893

david-pace · 2024-12-12T19:22:42Z

When a device is initialized, an attempt is made to look up the room name for the room id specified for the device and to store the room name in a thing property. This property is also updated if a room change is detected.

The legacy property Location is removed if present. From now on the property location (with proper lower case spelling) is used.

add constants for location properties
implement location updates in abstract device handler
extend bridge handler to provide a cached list of rooms
add unit tests

closes #16599

When a device is initialized, an attempt is made to look up the room name for the room id specified for the device and to store the room name in a thing property. This property is also updated if a room change is detected. The legacy property `Location` is removed if present. From now on the property `location` (with proper lower case spelling) is used. * add constants for location properties * implement location updates in abstract device handler * extend bridge handler to provide a cached list of rooms * add unit tests Signed-off-by: David Pace <[email protected]>

david-pace · 2024-12-12T19:32:38Z

...chshc/src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCDeviceHandler.java

+            if (currentLocation == null || !currentLocation.equals(roomName)) {
+                logger.debug("Updating property '{}' of thing {} to '{}'.", PROPERTY_LOCATION, getThing().getUID(),
+                        roomName);
+                updateProperty(PROPERTY_LOCATION, roomName);


@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?

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?

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.

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.

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.

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.

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:

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?

GerdZanker · 2024-12-23T09:46:58Z

...schshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BridgeHandler.java

+        return roomCache.getValue();
+    }
+
+    public @Nullable String resolveRoomId(@Nullable String roomId) {


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

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.

GerdZanker

I tested the code and its working with my setup:

I see that old property is removed and a new is added once.
A restart afterwards does not modify the new property.
Only when I change the room name, then the property is changed once.

jlaur

Only some very minor comments (optional), otherwise LGTM.

jlaur · 2024-12-23T10:44:17Z

...chshc/src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCDeviceHandler.java

@@ -83,9 +87,44 @@ public void initialize() {
     *         otherwise
     */
    protected boolean processDeviceInfo(Device deviceInfo) {
+        try {
+            updateLocationPropertiesIfApplicable(deviceInfo);
+        } catch (Exception e) {


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.

jlaur · 2024-12-23T10:44:48Z

...chshc/src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCDeviceHandler.java

+
+    private void updateLocationPropertyIfApplicable(Map<String, String> thingProperties, Device deviceInfo)
+            throws BoschSHCException {
+        @Nullable


Suggested change

@Nullable

jlaur · 2024-12-23T10:44:57Z

...chshc/src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCDeviceHandler.java

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


Suggested change

@Nullable

jlaur · 2024-12-23T10:53:12Z

...chshc/src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCDeviceHandler.java

+        if (roomName != null) {
+            @Nullable
+            String currentLocation = thingProperties.get(PROPERTY_LOCATION);
+            if (currentLocation == null || !currentLocation.equals(roomName)) {


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

Suggested change

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

if (!roomName.equals(currentLocation)) {

jlaur · 2024-12-23T10:56:53Z

...chshc/src/main/java/org/openhab/binding/boschshc/internal/devices/BoschSHCDeviceHandler.java

+            if (currentLocation == null || !currentLocation.equals(roomName)) {
+                logger.debug("Updating property '{}' of thing {} to '{}'.", PROPERTY_LOCATION, getThing().getUID(),
+                        roomName);
+                updateProperty(PROPERTY_LOCATION, roomName);


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:

david-pace requested a review from GerdZanker as a code owner December 12, 2024 19:22

david-pace added the bug An unexpected problem or unintended behavior of an add-on label Dec 12, 2024

david-pace commented Dec 12, 2024

View reviewed changes

david-pace requested a review from jlaur December 12, 2024 19:33

GerdZanker reviewed Dec 23, 2024

View reviewed changes

GerdZanker approved these changes Dec 23, 2024

View reviewed changes

jlaur reviewed Dec 23, 2024

View reviewed changes

david-pace added the work in progress A PR that is not yet ready to be merged label Dec 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[boschshc] Update location properties when initializing things #17893

[boschshc] Update location properties when initializing things #17893

david-pace commented Dec 12, 2024

david-pace Dec 12, 2024

jlaur Dec 20, 2024

david-pace Dec 21, 2024

david-pace Dec 22, 2024

GerdZanker Dec 23, 2024

jlaur Dec 23, 2024

jlaur Dec 23, 2024 •

edited

Loading

david-pace Dec 23, 2024

GerdZanker Dec 23, 2024

david-pace Dec 23, 2024

GerdZanker left a comment

jlaur left a comment

jlaur Dec 23, 2024

jlaur Dec 23, 2024

jlaur Dec 23, 2024

jlaur Dec 23, 2024

jlaur Dec 23, 2024 •

edited

Loading

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

[boschshc] Update location properties when initializing things #17893

Are you sure you want to change the base?

[boschshc] Update location properties when initializing things #17893

Conversation

david-pace commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlaur Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GerdZanker left a comment

Choose a reason for hiding this comment

jlaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlaur Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

jlaur Dec 23, 2024 •

edited

Loading

jlaur Dec 23, 2024 •

edited

Loading