[roborock] Address review: thing-type version, dock UNDEF, ring majority

Declare thingTypeVersion 1 on roborock:vacuum so freshly created things
are not treated as migration candidates by update.xml. A docked robot
polls no further maps, so an unknown dock room now goes UNDEF instead of
falling back to possibly stale positions; while cleaning, an
unresolvable position keeps the last room, since the next map cycle
corrects the channel on its own. The ring fallback now requires a real
majority, so a 2/1/1 plurality no longer decides the room.

Signed-off-by: Martin Littkovsky <2018turtle@proton.me>
AI-assisted-by: Claude Code

Author: Martin Littkovsky

bundles/org.openhab.binding.roborock/src/main/java/org/openhab/binding/roborock/internal/RoborockVacuumHandler.java

@@ -1184,51 +1184,59 @@ private void handleGetMap(int requestId, byte[] mapPayload) {
     }
 
     private void updateCurrentRoomState(RRMapData mapData) {
-        updateChannelStateIfExists(RobotCapabilities.CURRENT_ROOM.getChannel(),
-                resolveRoomStateFromMap(mapData, isAtDock(), segmentRoomNames));
+        State roomState = resolveRoomStateFromMap(mapData, isAtDock(), segmentRoomNames);
+        if (roomState != null) {
+            updateChannelStateIfExists(RobotCapabilities.CURRENT_ROOM.getChannel(), roomState);
+        }
     }
 
     /**
      * Republishes {@code status#current-room} from the charging dock's position in the most
-     * recently parsed map, since no map is polled while the robot sits on the dock. When the cached
-     * map holds no dock position the channel is left untouched.
+     * recently parsed map, since no map is polled while the robot sits on the dock. When no dock
+     * room can be resolved the channel goes {@code UNDEF} rather than keeping the room of the
+     * interrupted cleaning run.
      */
     private void updateCurrentRoomStateFromDock() {
-        State dockRoomState = resolveDockRoomState(lastParsedMapData, segmentRoomNames);
-        if (dockRoomState != null) {
-            updateChannelStateIfExists(RobotCapabilities.CURRENT_ROOM.getChannel(), dockRoomState);
-        }
+        updateChannelStateIfExists(RobotCapabilities.CURRENT_ROOM.getChannel(),
+                resolveDockRoomState(lastParsedMapData, segmentRoomNames));
     }
 
     /**
-     * Resolves the {@code status#current-room} state for a freshly parsed map, preferring the
-     * charging dock's position over the possibly lagging robot position while the robot is docked.
+     * Resolves the {@code status#current-room} state for a freshly parsed map: from the charging
+     * dock's position while the robot is docked ({@code UNDEF} when no dock room can be resolved),
+     * from the robot position otherwise, where {@code null} means the channel must be left as it is
+     * because no segment could be resolved at the robot position.
      */
-    static State resolveRoomStateFromMap(RRMapData mapData, boolean atDock, Map<Integer, String> segmentRoomNames) {
-        MapPoint position = atDock ? dockPosition(mapData) : null;
-        if (position == null) {
-            position = robotPosition(mapData);
-        }
+    static @Nullable State resolveRoomStateFromMap(RRMapData mapData, boolean atDock,
+            Map<Integer, String> segmentRoomNames) {
+        if (atDock) {
+            // No map is polled while the robot sits docked, so an unknown dock room must go UNDEF
+            // instead of borrowing the possibly stale robot position.
+            MapPoint dockPosition = dockPosition(mapData);
+            State dockRoomState = dockPosition == null ? null : roomStateAt(mapData, dockPosition, segmentRoomNames);
+            return dockRoomState == null ? UnDefType.UNDEF : dockRoomState;
+        }
+        MapPoint position = robotPosition(mapData);
         if (position == null) {
             return UnDefType.UNDEF;
         }
+        // While cleaning, an unresolvable position keeps the previous room because the next map
+        // cycle corrects the channel on its own.
         return roomStateAt(mapData, position, segmentRoomNames);
     }
 
     /**
      * Resolves the {@code status#current-room} state from the charging dock's position in the given
-     * cached map, or {@code null} when that map holds no dock position and the channel must be left
-     * as it is.
+     * cached map, going {@code UNDEF} whenever no dock room can be resolved, since nothing
+     * self-corrects the channel while the robot sits on the dock.
      */
-    static @Nullable State resolveDockRoomState(@Nullable RRMapData mapData, Map<Integer, String> segmentRoomNames) {
+    static State resolveDockRoomState(@Nullable RRMapData mapData, Map<Integer, String> segmentRoomNames) {
         if (mapData == null) {
-            return null;
+            return UnDefType.UNDEF;
         }
         MapPoint dockPosition = dockPosition(mapData);
-        if (dockPosition == null) {
-            return null;
-        }
-        return roomStateAt(mapData, dockPosition, segmentRoomNames);
+        State dockRoomState = dockPosition == null ? null : roomStateAt(mapData, dockPosition, segmentRoomNames);
+        return dockRoomState == null ? UnDefType.UNDEF : dockRoomState;
     }
 
     /**
@@ -1242,9 +1250,18 @@ static boolean hasJustDocked(@Nullable Integer previousStateId, int newStateId)
         return previousStateId == null || !StatusType.getType(previousStateId.intValue()).isAtDock();
     }
 
-    private static State roomStateAt(RRMapData mapData, MapPoint position, Map<Integer, String> segmentRoomNames) {
+    /**
+     * Resolves the room state at the given map position, returning {@code null} when no segment
+     * can be resolved there at all and {@code UNDEF} when the resolved segment has no name in the
+     * table.
+     */
+    private static @Nullable State roomStateAt(RRMapData mapData, MapPoint position,
+            Map<Integer, String> segmentRoomNames) {
         Optional<Integer> segmentId = RoomAtRobotResolver.resolveSegmentId(mapData, position.x(), position.y());
-        String roomName = segmentId.map(segmentRoomNames::get).orElse(null);
+        if (segmentId.isEmpty()) {
+            return null;
+        }
+        String roomName = segmentRoomNames.get(segmentId.get());
         return roomName != null ? new StringType(roomName) : UnDefType.UNDEF;
     }
 

bundles/org.openhab.binding.roborock/src/main/java/org/openhab/binding/roborock/internal/map/RoomAtRobotResolver.java

@@ -42,9 +42,9 @@ private RoomAtRobotResolver() {
 
     /**
      * Resolves the segment id of the map pixel at the given position, falling back to the nearest
-     * ring within {@link #FALLBACK_SEARCH_RADIUS} that holds any segmented-floor pixels; the
-     * segment with the strict majority on that ring wins, and a tie resolves to empty rather than
-     * depending on scan order.
+     * ring within {@link #FALLBACK_SEARCH_RADIUS} that holds any segmented-floor pixels; a segment
+     * wins only by holding more than half of that ring's segmented pixels, so ties and mere
+     * pluralities resolve to empty rather than depending on scan order.
      */
     public static Optional<Integer> resolveSegmentId(RRMapData mapData, int positionX, int positionY) {
         int width = mapData.imageWidth();
@@ -90,18 +90,18 @@ private static Map<Integer, Integer> collectRing(byte[] imageData, int width, in
     private static Optional<Integer> dominantSegment(Map<Integer, Integer> segmentCounts) {
         int dominantId = -1;
         int highestCount = 0;
-        boolean tied = false;
+        int totalCount = 0;
         for (Map.Entry<Integer, Integer> candidate : segmentCounts.entrySet()) {
             int count = candidate.getValue();
+            totalCount += count;
             if (count > highestCount) {
                 highestCount = count;
                 dominantId = candidate.getKey();
-                tied = false;
-            } else if (count == highestCount) {
-                tied = true;
             }
         }
-        return tied || dominantId < 0 ? Optional.empty() : Optional.of(dominantId);
+        // The winner must hold more than half of all counted ring pixels, so a mere plurality like
+        // 2/1/1 stays unresolved instead of deciding the room with only half the evidence.
+        return highestCount * 2 > totalCount ? Optional.of(dominantId) : Optional.empty();
     }
 
     private static Optional<Integer> segmentAt(byte[] imageData, int width, int height, int x, int y) {

bundles/org.openhab.binding.roborock/src/main/resources/OH-INF/thing/thing-types.xml

@@ -41,6 +41,10 @@
 			<channel-group id="info" typeId="info"/>
 		</channel-groups>
 
+		<properties>
+			<property name="thingTypeVersion">1</property>
+		</properties>
+
 		<representation-property>duid</representation-property>
 
 		<config-description>

bundles/org.openhab.binding.roborock/src/test/java/org/openhab/binding/roborock/internal/RoborockVacuumHandlerCurrentRoomWiringTest.java

@@ -122,7 +122,7 @@ void updateCurrentRoomStateUsesRoomAtRobotResolverAndSegmentRoomNames() throws I
         String updateCurrentRoomStateBody = extractMethodBody(source,
                 "private void updateCurrentRoomState(RRMapData mapData)");
         String roomStateAtBody = extractMethodBody(source,
-                "private static State roomStateAt(RRMapData mapData, MapPoint position,");
+                "private static @Nullable State roomStateAt(RRMapData mapData, MapPoint position,");
 
         assertTrue(updateCurrentRoomStateBody.contains("segmentRoomNames"),
                 "updateCurrentRoomState should resolve the room name via the segmentRoomNames table");

bundles/org.openhab.binding.roborock/src/test/java/org/openhab/binding/roborock/internal/RoborockVacuumHandlerDockRoomTest.java

@@ -83,24 +83,41 @@ void mapCycleWhileNotDockedResolvesFromTheRobotPosition() {
     }
 
     @Test
-    void dockingWithoutACachedMapLeavesTheChannelUntouched() {
-        assertNull(RoborockVacuumHandler.resolveDockRoomState(null, ROOM_NAMES));
+    void dockingWithoutACachedMapReportsUndef() {
+        assertEquals(UnDefType.UNDEF, RoborockVacuumHandler.resolveDockRoomState(null, ROOM_NAMES));
     }
 
     @Test
-    void dockingWithoutAChargerPositionInTheCachedMapLeavesTheChannelUntouched() {
+    void dockingWithoutAChargerPositionInTheCachedMapReportsUndef() {
         RRMapData mapData = twoRoomMap(true, false);
 
-        assertNull(RoborockVacuumHandler.resolveDockRoomState(mapData, ROOM_NAMES));
+        assertEquals(UnDefType.UNDEF, RoborockVacuumHandler.resolveDockRoomState(mapData, ROOM_NAMES));
     }
 
     @Test
-    void mapCycleWhileDockedFallsBackToTheRobotPositionWithoutAChargerPosition() {
+    void mapCycleWhileDockedWithoutAChargerPositionReportsUndefInsteadOfTheRobotPosition() {
         RRMapData mapData = twoRoomMap(true, false);
 
         State roomState = RoborockVacuumHandler.resolveRoomStateFromMap(mapData, true, ROOM_NAMES);
 
-        assertEquals(new StringType("Flur"), roomState);
+        assertEquals(UnDefType.UNDEF, roomState);
+    }
+
+    @Test
+    void anUnresolvablePositionWhileCleaningLeavesTheChannelUntouched() {
+        RRMapData mapData = unsegmentedFloorMap(true, false);
+
+        assertNull(RoborockVacuumHandler.resolveRoomStateFromMap(mapData, false, ROOM_NAMES),
+                "the next map cycle corrects the room on its own, so a transient resolver miss must not "
+                        + "drag the channel to UNDEF");
+    }
+
+    @Test
+    void anUnresolvableDockPositionReportsUndefBecauseNothingSelfCorrectsOnTheDock() {
+        RRMapData mapData = unsegmentedFloorMap(false, true);
+
+        assertEquals(UnDefType.UNDEF, RoborockVacuumHandler.resolveRoomStateFromMap(mapData, true, ROOM_NAMES));
+        assertEquals(UnDefType.UNDEF, RoborockVacuumHandler.resolveDockRoomState(mapData, ROOM_NAMES));
     }
 
     @Test
@@ -188,6 +205,20 @@ private static RRMapData twoRoomMap(boolean withRobotPosition, boolean withCharg
                 List.of(), new byte[WIDTH * HEIGHT]);
     }
 
+    /** A map without any segmented-floor pixels, so no position on it resolves to a room. */
+    private static RRMapData unsegmentedFloorMap(boolean withRobotPosition, boolean withChargerPosition) {
+        byte[] imageData = new byte[WIDTH * HEIGHT];
+
+        Integer robotX = withRobotPosition ? Integer.valueOf(toMapCoordinate(ROBOT_PIXEL_X)) : null;
+        Integer robotY = withRobotPosition ? Integer.valueOf(toMapCoordinate(ROBOT_PIXEL_Y)) : null;
+        Integer chargerX = withChargerPosition ? Integer.valueOf(toMapCoordinate(DOCK_PIXEL_X)) : null;
+        Integer chargerY = withChargerPosition ? Integer.valueOf(toMapCoordinate(DOCK_PIXEL_Y)) : null;
+
+        return new RRMapData(WIDTH, HEIGHT, 0, 0, imageData, robotX, robotY, null, chargerX, chargerY, null, null,
+                List.of(), List.of(), List.of(), List.of(), List.of(), List.of(), List.of(), List.of(), List.of(),
+                List.of(), new byte[WIDTH * HEIGHT]);
+    }
+
     private static void fillSegment(byte[] imageData, int fromX, int fromY, int toX, int toY, int segmentId) {
         // Classifier 7 in the low 3 bits, segment id in the high 5 bits.
         byte pixel = (byte) ((segmentId << 3) | 0x07);

bundles/org.openhab.binding.roborock/src/test/java/org/openhab/binding/roborock/internal/map/RoomAtRobotResolverTest.java

@@ -170,6 +170,27 @@ void picksTheDominantSegmentWhenTheFallbackRingHoldsTwoRooms() {
         assertEquals(3, segmentId.get());
     }
 
+    @Test
+    void returnsEmptyWhenTheLargestSegmentOnTheRingHoldsOnlyHalfOfItsPixels() {
+        int width = 10;
+        int height = 10;
+        byte[] imageData = new byte[width * height];
+        int px = 5;
+        int py = 5;
+        imageData[py * width + px] = 0x01; // MAP_WALL
+        // A 2/1/1 split: segment 3 leads but owns exactly half of the ring, not a majority.
+        imageData[(py - 1) * width + px] = (byte) ((3 << 3) | 0x07);
+        imageData[(py + 1) * width + px] = (byte) ((3 << 3) | 0x07);
+        imageData[py * width + (px - 1)] = (byte) ((4 << 3) | 0x07);
+        imageData[py * width + (px + 1)] = (byte) ((5 << 3) | 0x07);
+
+        RRMapData mapData = mapDataWithImage(width, height, 0, 0, imageData);
+
+        Optional<Integer> segmentId = RoomAtRobotResolver.resolveSegmentId(mapData, (px + 1) * MM, (py + 1) * MM);
+
+        assertFalse(segmentId.isPresent());
+    }
+
     @Test
     void returnsEmptyWhenTheNearestFallbackRingIsTiedBetweenTwoRooms() {
         int width = 10;