Conversation
Split each room into Devices beneath the Hub, instead of entities.
|
Thanks for the PR @pink88, glad to see you figured out the preset problem (although still strange that I wasn't seeing the same issue). Could you split this into two PRs? It would be easier for me to review and approve since the two changes seem unrelated. Thanks! |
|
Thanks for splitting @pink88. If you rebase and re-push I can review the remaining changes. [EDIT] Forget that, I was able to do it - no need to do anything |
tonyroberts
left a comment
tonyroberts
left a comment
There was a problem hiding this comment.
Many thanks for the PR @pink88, appreciate it!
I hope you don't mind but I've added some comments for your consideration.
| "separate_room_devices": "Separate Room Devices" | ||
| }, | ||
| "data_description": { | ||
| "separate_room_devices": "When enabled, each room becomes a separate device. When disabled, all entities are grouped under the hub device (legacy mode). WARNING: Switching from enabled to disabled may leave orphaned room devices that need to be manually removed from Settings > Devices & Services > Devices." |
There was a problem hiding this comment.
| "separate_room_devices": "When enabled, each room becomes a separate device. When disabled, all entities are grouped under the hub device (legacy mode). WARNING: Switching from enabled to disabled may leave orphaned room devices that need to be manually removed from Settings > Devices & Services > Devices." | |
| "separate_room_devices": "When enabled, separate devices are created for each room. By default, all entities are grouped under a single device." |
Reworded and removed warning since it seems that the cleanup should now be done automatically.
| "separate_room_devices": "Separate Room Devices" | ||
| }, | ||
| "data_description": { | ||
| "separate_room_devices": "When enabled, each room becomes a separate device. When disabled, all entities are grouped under the hub device (legacy mode). WARNING: Switching from enabled to disabled may leave orphaned room devices that need to be manually removed from Settings > Devices & Services > Devices." |
There was a problem hiding this comment.
| "separate_room_devices": "When enabled, each room becomes a separate device. When disabled, all entities are grouped under the hub device (legacy mode). WARNING: Switching from enabled to disabled may leave orphaned room devices that need to be manually removed from Settings > Devices & Services > Devices." | |
| "separate_room_devices": "When enabled, separate devices are created for each room. By default, all entities are grouped under a single device." |
Reworded and removed warning since it seems that the cleanup should now be done automatically.
|
|
||
| async def _cleanup_room_devices(hass: HomeAssistant, config_entry: ConfigEntry) -> None: | ||
| """Remove room devices when switching to legacy mode.""" | ||
| from homeassistant.helpers import device_registry as dr |
There was a problem hiding this comment.
Can this import be moved to the top of the module?
| # Check if this is a room device (not the main hub) | ||
| for identifier in device.identifiers: | ||
| if identifier[0] == DOMAIN and "_room_" in str(identifier[1]): | ||
| _LOGGER.info(f"Removing orphaned room device: {device.name} ({identifier[1]})") |
There was a problem hiding this comment.
I think this should be using the debug log level
| break | ||
|
|
||
| if removed_count > 0: | ||
| _LOGGER.info(f"Removed {removed_count} orphaned room device(s) when switching to legacy mode") |
| name: str, | ||
| coordinator: WundasmartDataUpdateCoordinator, | ||
| description: WundaSensorDescription, | ||
| room_id: str = None, |
There was a problem hiding this comment.
This should be required since the room_id is now always supplied
| self._attr_device_info = coordinator.device_info | ||
|
|
||
| # Determine device info based on whether this sensor belongs to a room | ||
| if room_id is not None: |
There was a problem hiding this comment.
This can be simplified since we will always have a room id and the logic for which device info to get is already in coordinator.get_room_device_info. Duplicating the logic here makes the code marginally harder to maintain.
| coordinator, | ||
| desc) | ||
| desc, | ||
| room_id=wunda_id) |
There was a problem hiding this comment.
I understand the wunda_id is the same as the room id for the type ROOM, but for consistency/clarity, it might be worth using get_room_id_from_device here too. The marginal overhead of that function call is worth it for clarity, IMHO
| self._wunda_id = wunda_id | ||
| self._attr_name = device["name"] | ||
| self._attr_device_info = coordinator.get_room_device_info(wunda_id, device) | ||
| # Use simple name for separate devices, full name for legacy mode |
There was a problem hiding this comment.
full name is used in the default mode, so doesn't follow to call that legacy mode here. Maybe "Use simple name for separate devices, or full name otherwise"?
| self._attr_name = device["name"] | ||
| self._attr_device_info = coordinator.get_room_device_info(wunda_id, device) | ||
| # Use simple name for separate devices, full name for legacy mode | ||
| if coordinator._separate_room_devices: |
There was a problem hiding this comment.
Minor point but this could be a single line, if it's not too long. Happy with leaving it this way if it looks better to you.
2 participants
I'm not sure if this is something you'd want, but I made a change such that instead of a single hub device with all entities beneath it there are now individual devices for each room, with the relevant entities beneath them. It reuses all the entity ids, so its a pretty seamless switch. The hub device remains in place for things like Hot Water and the heartbeat. I also added an options flow in the config to switch between the two modes on-the-fly called "Separate Rooms". If you switch from separate rooms to no rooms, it should auto clean up any orphaned devices.
I also added a preset_mode of NONE which has resolved the issue I was getting when manually changing the temperatures.
I've not encountered any issues with it in my testing.