Improve roon integraton (#66000)

* Update to new library, revise discovery to work with new library, specify port to work with new library.

* Move user gui to fallback.

* Revise tests.

* Handle old config.

* Improve debugging, refresh faster on load.

* Remove duplicate.

* Bump library version.

* Fix docstring per review.

* Review suggestion

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

* Review suggestion

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

* Add check for duplicate host.

* Add error message to strings.

* Tidy.

* Review changes.

* Remove default.

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

homeassistant/components/roon/__init__.py

@@ -4,14 +4,17 @@ from homeassistant.const import CONF_HOST
 from homeassistant.core import HomeAssistant
 from homeassistant.helpers import device_registry as dr
 
-from .const import DOMAIN
+from .const import CONF_ROON_NAME, DOMAIN
 from .server import RoonServer
 
 
 async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Set up a roonserver from a config entry."""
     hass.data.setdefault(DOMAIN, {})
-    host = entry.data[CONF_HOST]
+
+    # fallback to using host for compatibility with older configs
+    name = entry.data.get(CONF_ROON_NAME, entry.data[CONF_HOST])
+
     roonserver = RoonServer(hass, entry)
 
     if not await roonserver.async_setup():
@@ -23,7 +26,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
         config_entry_id=entry.entry_id,
         identifiers={(DOMAIN, entry.entry_id)},
         manufacturer="Roonlabs",
-        name=host,
+        name=f"Roon Core ({name})",
     )
     return True
 

homeassistant/components/roon/config_flow.py

@@ -6,11 +6,13 @@ from roonapi import RoonApi, RoonDiscovery
 import voluptuous as vol
 
 from homeassistant import config_entries, core, exceptions
-from homeassistant.const import CONF_API_KEY, CONF_HOST
+from homeassistant.const import CONF_API_KEY, CONF_HOST, CONF_PORT
+import homeassistant.helpers.config_validation as cv
 
 from .const import (
     AUTHENTICATE_TIMEOUT,
     CONF_ROON_ID,
+    CONF_ROON_NAME,
     DEFAULT_NAME,
     DOMAIN,
     ROON_APPINFO,
@@ -18,7 +20,12 @@ from .const import (
 
 _LOGGER = logging.getLogger(__name__)
 
-DATA_SCHEMA = vol.Schema({vol.Required("host"): str})
+DATA_SCHEMA = vol.Schema(
+    {
+        vol.Required("host"): cv.string,
+        vol.Required("port", default=9330): cv.port,
+    }
+)
 
 TIMEOUT = 120
 
@@ -45,7 +52,7 @@ class RoonHub:
         _LOGGER.debug("Servers = %s", servers)
         return servers
 
-    async def authenticate(self, host, servers):
+    async def authenticate(self, host, port, servers):
         """Authenticate with one or more roon servers."""
 
         def stop_apis(apis):
@@ -54,6 +61,7 @@ class RoonHub:
 
         token = None
         core_id = None
+        core_name = None
         secs = 0
         if host is None:
             apis = [
@@ -61,7 +69,7 @@ class RoonHub:
                 for server in servers
             ]
         else:
-            apis = [RoonApi(ROON_APPINFO, None, host, blocking_init=False)]
+            apis = [RoonApi(ROON_APPINFO, None, host, port, 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.
@@ -71,6 +79,7 @@ class RoonHub:
             secs += AUTHENTICATE_TIMEOUT
             if auth_api:
                 core_id = auth_api[0].core_id
+                core_name = auth_api[0].core_name
                 token = auth_api[0].token
                 break
 
@@ -78,7 +87,7 @@ class RoonHub:
 
         await self._hass.async_add_executor_job(stop_apis, apis)
 
-        return (token, core_id)
+        return (token, core_id, core_name)
 
 
 async def discover(hass):
@@ -90,15 +99,21 @@ async def discover(hass):
     return servers
 
 
-async def authenticate(hass: core.HomeAssistant, host, servers):
+async def authenticate(hass: core.HomeAssistant, host, port, servers):
     """Connect and authenticate home assistant."""
 
     hub = RoonHub(hass)
-    (token, core_id) = await hub.authenticate(host, servers)
+    (token, core_id, core_name) = await hub.authenticate(host, port, servers)
     if token is None:
         raise InvalidAuth
 
-    return {CONF_HOST: host, CONF_ROON_ID: core_id, CONF_API_KEY: token}
+    return {
+        CONF_HOST: host,
+        CONF_PORT: port,
+        CONF_ROON_ID: core_id,
+        CONF_ROON_NAME: core_name,
+        CONF_API_KEY: token,
+    }
 
 
 class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
@@ -109,33 +124,45 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
     def __init__(self):
         """Initialize the Roon flow."""
         self._host = None
+        self._port = None
         self._servers = []
 
     async def async_step_user(self, user_input=None):
-        """Handle getting host details from the user."""
+        """Get roon core details via discovery."""
 
-        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()
 
+        return await self.async_step_fallback()
+
+    async def async_step_fallback(self, user_input=None):
+        """Get host and port details from the user."""
+        errors = {}
+
         if user_input is not None:
             self._host = user_input["host"]
+            self._port = user_input["port"]
             return await self.async_step_link()
 
         return self.async_show_form(
-            step_id="user", data_schema=DATA_SCHEMA, errors=errors
+            step_id="fallback", data_schema=DATA_SCHEMA, errors=errors
         )
 
     async def async_step_link(self, user_input=None):
         """Handle linking and authenticting with the roon server."""
-
         errors = {}
         if user_input is not None:
+            # Do not authenticate if the host is already configured
+            self._async_abort_entries_match({CONF_HOST: self._host})
+
             try:
-                info = await authenticate(self.hass, self._host, self._servers)
+                info = await authenticate(
+                    self.hass, self._host, self._port, self._servers
+                )
+
             except InvalidAuth:
                 errors["base"] = "invalid_auth"
             except Exception:  # pylint: disable=broad-except

homeassistant/components/roon/const.py

@@ -5,6 +5,7 @@ AUTHENTICATE_TIMEOUT = 5
 DOMAIN = "roon"
 
 CONF_ROON_ID = "roon_server_id"
+CONF_ROON_NAME = "roon_server_name"
 
 DATA_CONFIGS = "roon_configs"
 

homeassistant/components/roon/manifest.json

@@ -3,7 +3,7 @@
   "name": "RoonLabs music player",
   "config_flow": true,
   "documentation": "https://www.home-assistant.io/integrations/roon",
-  "requirements": ["roonapi==0.0.38"],
+  "requirements": ["roonapi==0.1.1"],
   "codeowners": ["@pavoni"],
   "iot_class": "local_push",
   "loggers": ["roonapi"]

homeassistant/components/roon/server.py

@@ -2,15 +2,16 @@
 import asyncio
 import logging
 
-from roonapi import RoonApi
+from roonapi import RoonApi, RoonDiscovery
 
-from homeassistant.const import CONF_API_KEY, CONF_HOST, Platform
+from homeassistant.const import CONF_API_KEY, CONF_HOST, CONF_PORT, Platform
 from homeassistant.helpers.dispatcher import async_dispatcher_send
 from homeassistant.util.dt import utcnow
 
 from .const import CONF_ROON_ID, ROON_APPINFO
 
 _LOGGER = logging.getLogger(__name__)
+INITIAL_SYNC_INTERVAL = 5
 FULL_SYNC_INTERVAL = 30
 PLATFORMS = [Platform.MEDIA_PLAYER]
 
@@ -33,23 +34,38 @@ class RoonServer:
 
     async def async_setup(self, tries=0):
         """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]
-        # 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(
+        def get_roon_host():
-            ROON_APPINFO, token, host, blocking_init=False, core_id=core_id
+            host = self.config_entry.data.get(CONF_HOST)
-        )
+            port = self.config_entry.data.get(CONF_PORT)
+            if host:
+                _LOGGER.debug("static roon core host=%s port=%s", host, port)
+                return (host, port)
+
+            discover = RoonDiscovery(core_id)
+            server = discover.first()
+            discover.stop()
+            _LOGGER.debug("dynamic roon core core_id=%s server=%s", core_id, server)
+            return (server[0], server[1])
+
+        def get_roon_api():
+            token = self.config_entry.data[CONF_API_KEY]
+            (host, port) = get_roon_host()
+            return RoonApi(ROON_APPINFO, token, host, port, blocking_init=True)
+
+        hass = self.hass
+        core_id = self.config_entry.data.get(CONF_ROON_ID)
+
+        self.roonapi = await self.hass.async_add_executor_job(get_roon_api)
+
         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
+        self.roon_id = (
+            core_id if core_id is not None else self.config_entry.data[CONF_HOST]
+        )
 
         # initialize media_player platform
         hass.config_entries.async_setup_platforms(self.config_entry, PLATFORMS)
@@ -98,13 +114,14 @@ class RoonServer:
     async def async_do_loop(self):
         """Background work loop."""
         self._exit = False
+        await asyncio.sleep(INITIAL_SYNC_INTERVAL)
         while not self._exit:
             await self.async_update_players()
-            # await self.async_update_playlists()
             await asyncio.sleep(FULL_SYNC_INTERVAL)
 
     async def async_update_changed_players(self, changed_zones_ids):
         """Update the players which were reported as changed by the Roon API."""
+        _LOGGER.debug("async_update_changed_players %s", changed_zones_ids)
         for zone_id in changed_zones_ids:
             if zone_id not in self.roonapi.zones:
                 # device was removed ?
@@ -127,6 +144,7 @@ class RoonServer:
     async def async_update_players(self):
         """Periodic full scan of all devices."""
         zone_ids = self.roonapi.zones.keys()
+        _LOGGER.debug("async_update_players %s", zone_ids)
         await self.async_update_changed_players(zone_ids)
         # check for any removed devices
         all_devs = {}

homeassistant/components/roon/strings.json

@@ -1,10 +1,12 @@
 {
   "config": {
     "step": {
-      "user": {
+      "user": {},
-        "description": "Could not discover Roon server, please enter your the Hostname or IP.",
+      "fallback": {
+        "description": "Could not discover Roon server, please enter your Hostname and Port.",
         "data": {
-          "host": "[%key:common::config_flow::data::host%]"
+          "host": "[%key:common::config_flow::data::host%]",
+          "port": "[%key:common::config_flow::data::port%]"
         }
       },
       "link": {

requirements_all.txt

@@ -2076,7 +2076,7 @@ rokuecp==0.16.0
 roombapy==1.6.5
 
 # homeassistant.components.roon
-roonapi==0.0.38
+roonapi==0.1.1
 
 # homeassistant.components.rova
 rova==0.3.0

requirements_test_all.txt

@@ -1351,7 +1351,7 @@ rokuecp==0.16.0
 roombapy==1.6.5
 
 # homeassistant.components.roon
-roonapi==0.0.38
+roonapi==0.1.1
 
 # homeassistant.components.rpi_power
 rpi-bad-power==0.1.0

tests/components/roon/test_config_flow.py

@@ -1,9 +1,11 @@
 """Test the roon config flow."""
 from unittest.mock import patch
 
-from homeassistant import config_entries
+from homeassistant import config_entries, data_entry_flow
 from homeassistant.components.roon.const import DOMAIN
 
+from tests.common import MockConfigEntry
+
 
 class RoonApiMock:
     """Class to mock the Roon API for testing."""
@@ -18,8 +20,13 @@ class RoonApiMock:
         """Return the roon host."""
         return "core_id"
 
+    @property
+    def core_name(self):
+        """Return the roon core name."""
+        return "Roon Core"
+
     def stop(self):
-        """Stop socket and discovery."""
+        """Stop socket."""
         return
 
 
@@ -93,8 +100,10 @@ async def test_successful_discovery_and_auth(hass):
     assert result2["title"] == "Roon Labs Music Player"
     assert result2["data"] == {
         "host": None,
+        "port": None,
         "api_key": "good_token",
         "roon_server_id": "core_id",
+        "roon_server_name": "Roon Core",
     }
 
 
@@ -119,11 +128,11 @@ async def test_unsuccessful_discovery_user_form_and_auth(hass):
 
         # Should show the form if server was not discovered
         assert result["type"] == "form"
-        assert result["step_id"] == "user"
+        assert result["step_id"] == "fallback"
         assert result["errors"] == {}
 
         await hass.config_entries.flow.async_configure(
-            result["flow_id"], {"host": "1.1.1.1"}
+            result["flow_id"], {"host": "1.1.1.1", "port": 9331}
         )
         result2 = await hass.config_entries.flow.async_configure(
             result["flow_id"], user_input={}
@@ -134,10 +143,52 @@ async def test_unsuccessful_discovery_user_form_and_auth(hass):
     assert result2["data"] == {
         "host": "1.1.1.1",
         "api_key": "good_token",
+        "port": 9331,
+        "api_key": "good_token",
         "roon_server_id": "core_id",
+        "roon_server_name": "Roon Core",
     }
 
 
+async def test_duplicate_config(hass):
+    """Test user adding the host via the form for host that is already configured."""
+
+    CONFIG = {"host": "1.1.1.1"}
+
+    MockConfigEntry(domain=DOMAIN, unique_id="0123456789", data=CONFIG).add_to_hass(
+        hass
+    )
+
+    with patch(
+        "homeassistant.components.roon.config_flow.RoonApi",
+        return_value=RoonApiMock(),
+    ), patch(
+        "homeassistant.components.roon.config_flow.RoonDiscovery",
+        return_value=RoonDiscoveryFailedMock(),
+    ):
+
+        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"] == "fallback"
+        assert result["errors"] == {}
+
+        await hass.config_entries.flow.async_configure(
+            result["flow_id"], {"host": "1.1.1.1", "port": 9331}
+        )
+        result2 = await hass.config_entries.flow.async_configure(
+            result["flow_id"], user_input={}
+        )
+        await hass.async_block_till_done()
+
+        assert result2["type"] == data_entry_flow.RESULT_TYPE_ABORT
+        assert result2["reason"] == "already_configured"
+
+
 async def test_successful_discovery_no_auth(hass):
     """Test successful discover, but failed auth."""