Pyroon discovery (#44811)

Co-authored-by: Paulus Schoutsen <balloob@gmail.com>

homeassistant/components/roon/config_flow.py

@@ -2,7 +2,7 @@
 import asyncio
 import logging
 
-from roonapi import RoonApi
+from roonapi import RoonApi, RoonDiscovery
 import voluptuous as vol
 
 from homeassistant import config_entries, core, exceptions
@@ -10,6 +10,7 @@ from homeassistant.const import CONF_API_KEY, CONF_HOST
 
 from .const import (  # pylint: disable=unused-import
     AUTHENTICATE_TIMEOUT,
+    CONF_ROON_ID,
     DEFAULT_NAME,
     DOMAIN,
     ROON_APPINFO,
@@ -25,36 +26,79 @@ TIMEOUT = 120
 class RoonHub:
     """Interact with roon during config flow."""
 
-    def __init__(self, host):
-        """Initialize."""
-        self._host = host
+    def __init__(self, hass):
+        """Initialise the RoonHub."""
+        self._hass = hass
+
+    async def discover(self):
+        """Try and discover roon servers."""
+
+        def get_discovered_servers(discovery):
+            servers = discovery.all()
+            discovery.stop()
+            return servers
+
+        discovery = RoonDiscovery(None)
+        servers = await self._hass.async_add_executor_job(
+            get_discovered_servers, discovery
+        )
+        _LOGGER.debug("Servers = %s", servers)
+        return servers
+
+    async def authenticate(self, host, servers):
+        """Authenticate with one or more roon servers."""
+
+        def stop_apis(apis):
+            for api in apis:
+                api.stop()
 
-    async def authenticate(self, hass) -> bool:
-        """Test if we can authenticate with the host."""
         token = None
+        core_id = None
         secs = 0
-        roonapi = RoonApi(ROON_APPINFO, None, self._host, blocking_init=False)
-        while secs < TIMEOUT:
-            token = roonapi.token
+        if host is None:
+            apis = [
+                RoonApi(ROON_APPINFO, None, server[0], server[1], blocking_init=False)
+                for server in servers
+            ]
+        else:
+            apis = [RoonApi(ROON_APPINFO, None, host, blocking_init=False)]
+
+        while secs <= TIMEOUT:
+            # Roon can discover multiple devices - not all of which are proper servers, so try and authenticate with them all.
+            # The user will only enable one - so look for a valid token
+            auth_api = [api for api in apis if api.token is not None]
+
             secs += AUTHENTICATE_TIMEOUT
-            if token:
+            if auth_api:
+                core_id = auth_api[0].core_id
+                token = auth_api[0].token
                 break
+
             await asyncio.sleep(AUTHENTICATE_TIMEOUT)
 
-        token = roonapi.token
-        roonapi.stop()
-        return token
+        await self._hass.async_add_executor_job(stop_apis, apis)
+
+        return (token, core_id)
 
 
-async def authenticate(hass: core.HomeAssistant, host):
+async def discover(hass):
     """Connect and authenticate home assistant."""
 
-    hub = RoonHub(host)
-    token = await hub.authenticate(hass)
+    hub = RoonHub(hass)
+    servers = await hub.discover()
+
+    return servers
+
+
+async def authenticate(hass: core.HomeAssistant, host, servers):
+    """Connect and authenticate home assistant."""
+
+    hub = RoonHub(hass)
+    (token, core_id) = await hub.authenticate(host, servers)
     if token is None:
         raise InvalidAuth
 
-    return {CONF_HOST: host, CONF_API_KEY: token}
+    return {CONF_HOST: host, CONF_ROON_ID: core_id, CONF_API_KEY: token}
 
 
 class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
@@ -66,20 +110,20 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
     def __init__(self):
         """Initialize the Roon flow."""
         self._host = None
+        self._servers = []
 
     async def async_step_user(self, user_input=None):
         """Handle getting host details from the user."""
 
         errors = {}
+        self._servers = await discover(self.hass)
+
+        # We discovered one or more  roon - so skip to authentication
+        if self._servers:
+            return await self.async_step_link()
+
         if user_input is not None:
             self._host = user_input["host"]
-            existing = {
-                entry.data[CONF_HOST] for entry in self._async_current_entries()
-            }
-            if self._host in existing:
-                errors["base"] = "duplicate_entry"
-                return self.async_show_form(step_id="user", errors=errors)
-
             return await self.async_step_link()
 
         return self.async_show_form(
@@ -92,7 +136,7 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
         errors = {}
         if user_input is not None:
             try:
-                info = await authenticate(self.hass, self._host)
+                info = await authenticate(self.hass, self._host, self._servers)
             except InvalidAuth:
                 errors["base"] = "invalid_auth"
             except Exception:  # pylint: disable=broad-except

homeassistant/components/roon/const.py

@@ -4,6 +4,8 @@ AUTHENTICATE_TIMEOUT = 5
 
 DOMAIN = "roon"
 
+CONF_ROON_ID = "roon_server_id"
+
 DATA_CONFIGS = "roon_configs"
 
 DEFAULT_NAME = "Roon Labs Music Player"

homeassistant/components/roon/media_player.py

@@ -175,7 +175,7 @@ class RoonDevice(MediaPlayerEntity):
             "name": self.name,
             "manufacturer": "RoonLabs",
             "model": dev_model,
-            "via_hub": (DOMAIN, self._server.host),
+            "via_hub": (DOMAIN, self._server.roon_id),
         }
 
     def update_data(self, player_data=None):

homeassistant/components/roon/server.py

@@ -8,7 +8,7 @@ from homeassistant.const import CONF_API_KEY, CONF_HOST
 from homeassistant.helpers.dispatcher import async_dispatcher_send
 from homeassistant.util.dt import utcnow
 
-from .const import ROON_APPINFO
+from .const import CONF_ROON_ID, ROON_APPINFO
 
 _LOGGER = logging.getLogger(__name__)
 FULL_SYNC_INTERVAL = 30
@@ -22,28 +22,33 @@ class RoonServer:
         self.config_entry = config_entry
         self.hass = hass
         self.roonapi = None
+        self.roon_id = None
         self.all_player_ids = set()
         self.all_playlists = []
         self.offline_devices = set()
         self._exit = False
         self._roon_name_by_id = {}
 
-    @property
-    def host(self):
-        """Return the host of this server."""
-        return self.config_entry.data[CONF_HOST]
-
     async def async_setup(self, tries=0):
-        """Set up a roon server based on host parameter."""
-        host = self.host
+        """Set up a roon server based on config parameters."""
         hass = self.hass
+        # Host will be None for configs using discovery
+        host = self.config_entry.data[CONF_HOST]
         token = self.config_entry.data[CONF_API_KEY]
-        _LOGGER.debug("async_setup: %s %s", token, host)
-        self.roonapi = RoonApi(ROON_APPINFO, token, host, blocking_init=False)
+        # Default to None for compatibility with older configs
+        core_id = self.config_entry.data.get(CONF_ROON_ID)
+        _LOGGER.debug("async_setup: host=%s core_id=%s token=%s", host, core_id, token)
+
+        self.roonapi = RoonApi(
+            ROON_APPINFO, token, host, blocking_init=False, core_id=core_id
+        )
         self.roonapi.register_state_callback(
             self.roonapi_state_callback, event_filter=["zones_changed"]
         )
 
+        # Default to 'host' for compatibility with older configs without core_id
+        self.roon_id = core_id if core_id is not None else host
+
         # initialize media_player platform
         hass.async_create_task(
             hass.config_entries.async_forward_entry_setup(
@@ -152,11 +157,11 @@ class RoonServer:
         new_dict = zone.copy()
         new_dict.update(output)
         new_dict.pop("outputs")
-        new_dict["host"] = self.host
+        new_dict["roon_id"] = self.roon_id
         new_dict["is_synced"] = len(zone["outputs"]) > 1
         new_dict["zone_name"] = zone["display_name"]
         new_dict["display_name"] = output["display_name"]
         new_dict["last_changed"] = utcnow()
         # we don't use the zone_id or output_id for now as unique id as I've seen cases were it changes for some reason
-        new_dict["dev_id"] = f"roon_{self.host}_{output['display_name']}"
+        new_dict["dev_id"] = f"roon_{self.roon_id}_{output['display_name']}"
         return new_dict

homeassistant/components/roon/strings.json

@@ -2,7 +2,7 @@
   "config": {
     "step": {
       "user": {
-        "description": "Please enter your Roon server Hostname or IP.",
+        "description": "Could not discover Roon server, please enter your the Hostname or IP.",
         "data": {
           "host": "[%key:common::config_flow::data::host%]"
         }
@@ -14,8 +14,7 @@
     },
     "error": {
       "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]",
-      "unknown": "[%key:common::config_flow::error::unknown%]",
-      "duplicate_entry": "That host has already been added."
+      "unknown": "[%key:common::config_flow::error::unknown%]"
     },
     "abort": {
       "already_configured": "[%key:common::config_flow::abort::already_configured_device%]"

tests/components/roon/test_config_flow.py

@@ -3,49 +3,131 @@ from unittest.mock import patch
 
 from homeassistant import config_entries, setup
 from homeassistant.components.roon.const import DOMAIN
-from homeassistant.const import CONF_HOST
-
-from tests.common import MockConfigEntry
 
 
 class RoonApiMock:
-    """Mock to handle returning tokens for testing the RoonApi."""
-
-    def __init__(self, token):
-        """Initialize."""
-        self._token = token
+    """Class to mock the Roon API for testing."""
 
     @property
     def token(self):
-        """Return the auth token from the api."""
-        return self._token
+        """Return a good authentication key."""
+        return "good_token"
 
-    def stop(self):  # pylint: disable=no-self-use
-        """Close down the api."""
+    @property
+    def core_id(self):
+        """Return the roon host."""
+        return "core_id"
+
+    def stop(self):
+        """Stop socket and discovery."""
         return
 
 
-async def test_form_and_auth(hass):
-    """Test we get the form."""
-    await setup.async_setup_component(hass, "persistent_notification", {})
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
-    assert result["type"] == "form"
-    assert result["errors"] == {}
+class RoonApiMockNoToken(RoonApiMock):
+    """Class to mock the Roon API for testing, with failed authorisation."""
 
-    with patch("homeassistant.components.roon.config_flow.TIMEOUT", 0,), patch(
-        "homeassistant.components.roon.const.AUTHENTICATE_TIMEOUT",
-        0,
-    ), patch(
+    @property
+    def token(self):
+        """Return a bad authentication key."""
+        return None
+
+
+class RoonApiMockException(RoonApiMock):
+    """Class to mock the Roon API for testing, throws an unexpected exception."""
+
+    @property
+    def token(self):
+        """Throw exception."""
+        raise Exception
+
+
+class RoonDiscoveryMock:
+    """Class to mock Roon Discovery for testing."""
+
+    def all(self):
+        """Return a discovered roon server."""
+        return ["2.2.2.2"]
+
+    def stop(self):
+        """Stop discovery running."""
+        return
+
+
+class RoonDiscoveryFailedMock(RoonDiscoveryMock):
+    """Class to mock Roon Discovery for testing, with no servers discovered."""
+
+    def all(self):
+        """Return no discovered roon servers."""
+        return []
+
+
+async def test_successful_discovery_and_auth(hass):
+    """Test when discovery and auth both work ok."""
+
+    await setup.async_setup_component(hass, "persistent_notification", {})
+    with patch(
         "homeassistant.components.roon.config_flow.RoonApi",
-        return_value=RoonApiMock("good_token"),
+        return_value=RoonApiMock(),
+    ), patch(
+        "homeassistant.components.roon.config_flow.RoonDiscovery",
+        return_value=RoonDiscoveryMock(),
     ), patch(
         "homeassistant.components.roon.async_setup", return_value=True
-    ) as mock_setup, patch(
+    ), patch(
         "homeassistant.components.roon.async_setup_entry",
         return_value=True,
-    ) as mock_setup_entry:
+    ):
+
+        result = await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_USER}
+        )
+        await hass.async_block_till_done()
+
+        # Should go straight to link if server was discovered
+        assert result["type"] == "form"
+        assert result["step_id"] == "link"
+        assert result["errors"] == {}
+
+        result2 = await hass.config_entries.flow.async_configure(
+            result["flow_id"], user_input={}
+        )
+        await hass.async_block_till_done()
+
+    assert result2["title"] == "Roon Labs Music Player"
+    assert result2["data"] == {
+        "host": None,
+        "api_key": "good_token",
+        "roon_server_id": "core_id",
+    }
+
+
+async def test_unsuccessful_discovery_user_form_and_auth(hass):
+    """Test unsuccessful discover, user adding the host via the form and then successful auth."""
+
+    await setup.async_setup_component(hass, "persistent_notification", {})
+    with patch(
+        "homeassistant.components.roon.config_flow.RoonApi",
+        return_value=RoonApiMock(),
+    ), patch(
+        "homeassistant.components.roon.config_flow.RoonDiscovery",
+        return_value=RoonDiscoveryFailedMock(),
+    ), patch(
+        "homeassistant.components.roon.async_setup", return_value=True
+    ), patch(
+        "homeassistant.components.roon.async_setup_entry",
+        return_value=True,
+    ):
+
+        result = await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_USER}
+        )
+        await hass.async_block_till_done()
+
+        # Should show the form if server was not discovered
+        assert result["type"] == "form"
+        assert result["step_id"] == "user"
+        assert result["errors"] == {}
+
         await hass.config_entries.flow.async_configure(
             result["flow_id"], {"host": "1.1.1.1"}
         )
@@ -54,113 +136,85 @@ async def test_form_and_auth(hass):
         )
         await hass.async_block_till_done()
 
-    assert result2["type"] == "create_entry"
     assert result2["title"] == "Roon Labs Music Player"
-    assert result2["data"] == {"host": "1.1.1.1", "api_key": "good_token"}
-    assert len(mock_setup.mock_calls) == 1
-    assert len(mock_setup_entry.mock_calls) == 1
+    assert result2["data"] == {
+        "host": "1.1.1.1",
+        "api_key": "good_token",
+        "roon_server_id": "core_id",
+    }
 
 
-async def test_form_no_token(hass):
-    """Test we handle no token being returned (timeout or not authorized)."""
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
-    with patch("homeassistant.components.roon.config_flow.TIMEOUT", 0,), patch(
-        "homeassistant.components.roon.const.AUTHENTICATE_TIMEOUT",
+async def test_successful_discovery_no_auth(hass):
+    """Test successful discover, but failed auth."""
+
+    await setup.async_setup_component(hass, "persistent_notification", {})
+    with patch(
+        "homeassistant.components.roon.config_flow.RoonApi",
+        return_value=RoonApiMockNoToken(),
+    ), patch(
+        "homeassistant.components.roon.config_flow.RoonDiscovery",
+        return_value=RoonDiscoveryMock(),
+    ), patch(
+        "homeassistant.components.roon.config_flow.TIMEOUT",
         0,
     ), patch(
-        "homeassistant.components.roon.config_flow.RoonApi",
-        return_value=RoonApiMock(None),
+        "homeassistant.components.roon.config_flow.AUTHENTICATE_TIMEOUT",
+        0.01,
+    ), patch(
+        "homeassistant.components.roon.async_setup", return_value=True
+    ), patch(
+        "homeassistant.components.roon.async_setup_entry",
+        return_value=True,
     ):
-        await hass.config_entries.flow.async_configure(
-            result["flow_id"], {"host": "1.1.1.1"}
+
+        result = await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_USER}
         )
+        await hass.async_block_till_done()
+
+        # Should go straight to link if server was discovered
+        assert result["type"] == "form"
+        assert result["step_id"] == "link"
+        assert result["errors"] == {}
 
         result2 = await hass.config_entries.flow.async_configure(
             result["flow_id"], user_input={}
         )
+        await hass.async_block_till_done()
 
-    assert result2["type"] == "form"
     assert result2["errors"] == {"base": "invalid_auth"}
 
 
-async def test_form_unknown_exception(hass):
-    """Test we handle cannot connect error."""
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
-
-    with patch(
-        "homeassistant.components.roon.config_flow.RoonApi",
-        side_effect=Exception,
-    ):
-        await hass.config_entries.flow.async_configure(
-            result["flow_id"], {"host": "1.1.1.1"}
-        )
-
-        result2 = await hass.config_entries.flow.async_configure(
-            result["flow_id"], user_input={}
-        )
-
-    assert result2["type"] == "form"
-    assert result2["errors"] == {"base": "unknown"}
-
-
-async def test_form_host_already_exists(hass):
-    """Test we add the host if the config exists and it isn't a duplicate."""
-
-    MockConfigEntry(domain=DOMAIN, data={CONF_HOST: "existing_host"}).add_to_hass(hass)
+async def test_unexpected_exception(hass):
+    """Test successful discover, and unexpected exception during auth."""
 
     await setup.async_setup_component(hass, "persistent_notification", {})
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
-    assert result["type"] == "form"
-    assert result["errors"] == {}
-
-    with patch("homeassistant.components.roon.config_flow.TIMEOUT", 0,), patch(
-        "homeassistant.components.roon.const.AUTHENTICATE_TIMEOUT",
-        0,
-    ), patch(
+    with patch(
         "homeassistant.components.roon.config_flow.RoonApi",
-        return_value=RoonApiMock("good_token"),
+        return_value=RoonApiMockException(),
+    ), patch(
+        "homeassistant.components.roon.config_flow.RoonDiscovery",
+        return_value=RoonDiscoveryMock(),
     ), patch(
         "homeassistant.components.roon.async_setup", return_value=True
-    ) as mock_setup, patch(
+    ), patch(
         "homeassistant.components.roon.async_setup_entry",
         return_value=True,
-    ) as mock_setup_entry:
-        await hass.config_entries.flow.async_configure(
-            result["flow_id"], {"host": "1.1.1.1"}
+    ):
+
+        result = await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_USER}
         )
+        await hass.async_block_till_done()
+
+        # Should go straight to link if server was discovered
+        assert result["type"] == "form"
+        assert result["step_id"] == "link"
+        assert result["errors"] == {}
+
         result2 = await hass.config_entries.flow.async_configure(
             result["flow_id"], user_input={}
         )
         await hass.async_block_till_done()
 
-    assert result2["type"] == "create_entry"
-    assert result2["title"] == "Roon Labs Music Player"
-    assert result2["data"] == {"host": "1.1.1.1", "api_key": "good_token"}
-    assert len(mock_setup.mock_calls) == 1
-    assert len(mock_setup_entry.mock_calls) == 2
-
-
-async def test_form_duplicate_host(hass):
-    """Test we don't add the host if it's a duplicate."""
-
-    MockConfigEntry(domain=DOMAIN, data={CONF_HOST: "existing_host"}).add_to_hass(hass)
-
-    await setup.async_setup_component(hass, "persistent_notification", {})
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
-    assert result["type"] == "form"
-    assert result["errors"] == {}
-
-    result2 = await hass.config_entries.flow.async_configure(
-        result["flow_id"], {"host": "existing_host"}
-    )
-
-    assert result2["type"] == "form"
-    assert result2["errors"] == {"base": "duplicate_entry"}
+    assert result2["errors"] == {"base": "unknown"}