Musiccast grouping fixes (#52339)

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

homeassistant/components/yamaha_musiccast/manifest.json

@@ -4,7 +4,7 @@
   "config_flow": true,
   "documentation": "https://www.home-assistant.io/integrations/yamaha_musiccast",
   "requirements": [
-    "aiomusiccast==0.8.0"
+    "aiomusiccast==0.8.2"
   ],
   "ssdp": [
     {

homeassistant/components/yamaha_musiccast/media_player.py

@@ -34,6 +34,7 @@ from homeassistant.const import (
     STATE_PAUSED,
     STATE_PLAYING,
 )
+from homeassistant.core import callback
 from homeassistant.exceptions import HomeAssistantError
 import homeassistant.helpers.config_validation as cv
 from homeassistant.helpers.entity import Entity
@@ -148,22 +149,28 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
 
         self._cur_track = 0
         self._repeat = REPEAT_MODE_OFF
-        self.coordinator.entities.append(self)
 
     async def async_added_to_hass(self):
         """Run when this Entity has been added to HA."""
         await super().async_added_to_hass()
+        self.coordinator.entities.append(self)
         # Sensors should also register callbacks to HA when their state changes
         self.coordinator.musiccast.register_callback(self.async_write_ha_state)
         self.coordinator.musiccast.register_group_update_callback(
             self.update_all_mc_entities
         )
+        self.coordinator.async_add_listener(self.async_schedule_check_client_list)
 
     async def async_will_remove_from_hass(self):
         """Entity being removed from hass."""
         await super().async_will_remove_from_hass()
+        self.coordinator.entities.remove(self)
         # The opposite of async_added_to_hass. Remove any registered call backs here.
         self.coordinator.musiccast.remove_callback(self.async_write_ha_state)
+        self.coordinator.musiccast.remove_group_update_callback(
+            self.update_all_mc_entities
+        )
+        self.coordinator.async_remove_listener(self.async_schedule_check_client_list)
 
     @property
     def should_poll(self):
@@ -571,12 +578,18 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
 
         return self
 
-    async def update_all_mc_entities(self):
+    async def update_all_mc_entities(self, check_clients=False):
         """Update the whole musiccast system when group data change."""
-        for entity in self.get_all_mc_entities():
+        # First update all servers as they provide the group information for their clients
-            if entity.is_server:
+        for entity in self.get_all_server_entities():
+            if check_clients or self.coordinator.musiccast.group_reduce_by_source:
                 await entity.async_check_client_list()
-            entity.async_write_ha_state()
+            else:
+                entity.async_write_ha_state()
+        # Then update all other entities
+        for entity in self.get_all_mc_entities():
+            if not entity.is_server:
+                entity.async_write_ha_state()
 
     # Services
 
@@ -585,7 +598,7 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
 
         Creates a new group if necessary. Used for join service.
         """
-        _LOGGER.info(
+        _LOGGER.debug(
             "%s wants to add the following entities %s",
             self.entity_id,
             str(group_members),
@@ -597,6 +610,9 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
             if entity.entity_id in group_members
         ]
 
+        if self.state == STATE_OFF:
+            await self.async_turn_on()
+
         if not self.is_server and self.musiccast_zone_entity.is_server:
             # The MusicCast Distribution Module of this device is already in use. To use it as a server, we first
             # have to unjoin and wait until the servers are updated.
@@ -609,38 +625,40 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
             if self.is_server
             else uuid.random_uuid_hex().upper()
         )
+
+        ip_addresses = set()
         # First let the clients join
         for client in entities:
             if client != self:
                 try:
-                    await client.async_client_join(group, self)
+                    network_join = await client.async_client_join(group, self)
                 except MusicCastGroupException:
                     _LOGGER.warning(
                         "%s is struggling to update its group data. Will retry perform the update",
                         client.entity_id,
                     )
-                    await client.async_client_join(group, self)
+                    network_join = await client.async_client_join(group, self)
 
-        await self.coordinator.musiccast.mc_server_group_extend(
+                if network_join:
-            self._zone_id,
+                    ip_addresses.add(client.ip_address)
-            [
+
-                entity.ip_address
+        if ip_addresses:
-                for entity in entities
+            await self.coordinator.musiccast.mc_server_group_extend(
-                if entity.ip_address != self.ip_address
+                self._zone_id,
-            ],
+                list(ip_addresses),
-            group,
+                group,
-            self.get_distribution_num(),
+                self.get_distribution_num(),
-        )
+            )
         _LOGGER.debug(
             "%s added the following entities %s", self.entity_id, str(entities)
         )
-        _LOGGER.info(
+        _LOGGER.debug(
             "%s has now the following musiccast group %s",
             self.entity_id,
             str(self.musiccast_group),
         )
 
-        await self.update_all_mc_entities()
+        await self.update_all_mc_entities(True)
 
     async def async_unjoin_player(self):
         """Leave the group.
@@ -654,15 +672,15 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
         else:
             await self.async_client_leave_group()
 
-        await self.update_all_mc_entities()
+        await self.update_all_mc_entities(True)
 
     # Internal client functions
 
-    async def async_client_join(self, group_id, server):
+    async def async_client_join(self, group_id, server) -> bool:
         """Let the client join a group.
 
         If this client is a server, the server will stop distributing. If the client is part of a different group,
-        it will leave that group first.
+        it will leave that group first. Returns True, if the server has to add the client on his side.
         """
         # If we should join the group, which is served by the main zone, we can simply select main_sync as input.
         _LOGGER.debug("%s called service client join", self.entity_id)
@@ -672,14 +690,16 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
             if server.zone == DEFAULT_ZONE:
                 await self.async_select_source(ATTR_MAIN_SYNC)
                 server.async_write_ha_state()
-                return
+                return False
 
             # It is not possible to join a group hosted by zone2 from main zone.
-            raise Exception("Can not join a zone other than main of the same device.")
+            raise HomeAssistantError(
+                "Can not join a zone other than main of the same device."
+            )
 
         if self.musiccast_zone_entity.is_server:
             # If one of the zones of the device is a server, we need to unjoin first.
-            _LOGGER.info(
+            _LOGGER.debug(
                 "%s is a server of a group and has to stop distribution "
                 "to use MusicCast for %s",
                 self.musiccast_zone_entity.entity_id,
@@ -688,11 +708,11 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
             await self.musiccast_zone_entity.async_server_close_group()
 
         elif self.is_client:
-            if self.coordinator.data.group_id == server.coordinator.data.group_id:
+            if self.is_part_of_group(server):
                 _LOGGER.warning("%s is already part of the group", self.entity_id)
-                return
+                return False
 
-            _LOGGER.info(
+            _LOGGER.debug(
                 "%s is client in a different group, will unjoin first",
                 self.entity_id,
             )
@@ -705,20 +725,14 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
         ):
             # The device is already part of this group (e.g. main zone is also a client of this group).
             # Just select mc_link as source
-            await self.async_select_source(ATTR_MC_LINK)
+            await self.coordinator.musiccast.zone_join(self._zone_id)
-            # As the musiccast group has changed, we need to trigger the servers ha state.
+            return False
-            # In other cases this happens due to the callback after the dist updated message.
-            server.async_write_ha_state()
-            return
 
         _LOGGER.debug("%s will now join as a client", self.entity_id)
         await self.coordinator.musiccast.mc_client_join(
             server.ip_address, group_id, self._zone_id
         )
-
+        return True
-        # Ensure that mc link is selected. If main sync was selected previously, it's possible that this does not
-        # happen automatically
-        await self.async_select_source(ATTR_MC_LINK)
 
     async def async_client_leave_group(self, force=False):
         """Make self leave the group.
@@ -728,18 +742,9 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
         _LOGGER.debug("%s client leave called", self.entity_id)
         if not force and (
             self.source == ATTR_MAIN_SYNC
-            or len(
+            or [entity for entity in self.other_zones if entity.source == ATTR_MC_LINK]
-                [entity for entity in self.other_zones if entity.source == ATTR_MC_LINK]
-            )
-            > 0
         ):
-            # If we are only syncing to main or another zone is also using the musiccast module as client, don't
+            await self.coordinator.musiccast.zone_unjoin(self._zone_id)
-            # kill the client session, just select a dummy source.
-            save_inputs = self.coordinator.musiccast.get_save_inputs(self._zone_id)
-            if len(save_inputs):
-                await self.async_select_source(save_inputs[0])
-            # Then turn off the zone
-            await self.async_turn_off()
         else:
             servers = [
                 server
@@ -747,14 +752,11 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
                 if server.coordinator.data.group_id == self.coordinator.data.group_id
             ]
             await self.coordinator.musiccast.mc_client_unjoin()
-            if len(servers):
+            if servers:
                 await servers[0].coordinator.musiccast.mc_server_group_reduce(
                     servers[0].zone_id, [self.ip_address], self.get_distribution_num()
                 )
 
-        for server in self.get_all_server_entities():
-            await server.async_check_client_list()
-
     # Internal server functions
 
     async def async_server_close_group(self):
@@ -762,7 +764,7 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
 
         Should only be called for servers.
         """
-        _LOGGER.info("%s closes his group", self.entity_id)
+        _LOGGER.debug("%s closes his group", self.entity_id)
         for client in self.musiccast_group:
             if client != self:
                 await client.async_client_leave_group()
@@ -770,6 +772,9 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
 
     async def async_check_client_list(self):
         """Let the server check if all its clients are still part of his group."""
+        if not self.is_server or self.coordinator.data.group_update_lock.locked():
+            return
+
         _LOGGER.debug("%s updates his group members", self.entity_id)
         client_ips_for_removal = []
         for expected_client_ip in self.coordinator.data.group_client_list:
@@ -779,8 +784,8 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
                 # The client is no longer part of the group. Prepare removal.
                 client_ips_for_removal.append(expected_client_ip)
 
-        if len(client_ips_for_removal) > 0:
+        if client_ips_for_removal:
-            _LOGGER.info(
+            _LOGGER.debug(
                 "%s says good bye to the following members %s",
                 self.entity_id,
                 str(client_ips_for_removal),
@@ -793,3 +798,8 @@ class MusicCastMediaPlayer(MusicCastDeviceEntity, MediaPlayerEntity):
             await self.async_server_close_group()
 
         self.async_write_ha_state()
+
+    @callback
+    def async_schedule_check_client_list(self):
+        """Schedule async_check_client_list."""
+        self.hass.create_task(self.async_check_client_list())

requirements_all.txt

@@ -212,7 +212,7 @@ aiolyric==1.0.7
 aiomodernforms==0.1.8
 
 # homeassistant.components.yamaha_musiccast
-aiomusiccast==0.8.0
+aiomusiccast==0.8.2
 
 # homeassistant.components.keyboard_remote
 aionotify==0.2.0

requirements_test_all.txt

@@ -137,7 +137,7 @@ aiolyric==1.0.7
 aiomodernforms==0.1.8
 
 # homeassistant.components.yamaha_musiccast
-aiomusiccast==0.8.0
+aiomusiccast==0.8.2
 
 # homeassistant.components.notion
 aionotion==3.0.2