Fix OpenDisplay connecting to every discovered device on startup (#178239)

homeassistant/components/opendisplay/config_flow.py

@@ -1,5 +1,6 @@
 """Config flow for OpenDisplay integration."""
 
+import asyncio
 from collections.abc import Mapping
 import logging
 from typing import TYPE_CHECKING, Any, override
@@ -11,6 +12,7 @@ from opendisplay import (
     BLEConnectionError,
     OpenDisplayDevice,
     OpenDisplayError,
+    parse_advertisement,
 )
 import probatio
 
@@ -31,6 +33,24 @@ _ENCRYPTION_KEY_VALIDATOR = probatio.All(
     str.strip, str.lower, probatio.Match(r"^[0-9a-f]{32}$")
 )
 
+CONNECT_TIMEOUT = 45
+
+# Firmware advertises as "OD" followed by the device id in hex.
+NAME_PREFIX = "OD"
+
+
+def _is_opendisplay(discovery_info: BluetoothServiceInfoBleak) -> bool:
+    """Return if the advertisement looks like an OpenDisplay device."""
+    if not discovery_info.name.startswith(NAME_PREFIX):
+        return False
+    if (data := discovery_info.manufacturer_data.get(MANUFACTURER_ID)) is None:
+        return False
+    try:
+        parse_advertisement(data)
+    except ValueError:
+        return False
+    return True
+
 
 class OpenDisplayConfigFlow(ConfigFlow, domain=DOMAIN):
     """Handle a config flow for OpenDisplay."""
@@ -48,47 +68,66 @@ class OpenDisplayConfigFlow(ConfigFlow, domain=DOMAIN):
         if ble_device is None:
             raise BLEConnectionError(f"Could not find connectable device for {address}")
 
-        async with OpenDisplayDevice(
-            mac_address=address, ble_device=ble_device, encryption_key=encryption_key
-        ) as device:
-            await device.read_firmware_version()
+        # A device that stops responding mid-probe would otherwise hold both the
+        # dialog and one of the adapter's connection slots indefinitely.
+        try:
+            async with (
+                asyncio.timeout(CONNECT_TIMEOUT),
+                OpenDisplayDevice(
+                    mac_address=address,
+                    ble_device=ble_device,
+                    encryption_key=encryption_key,
+                ) as device,
+            ):
+                await device.read_firmware_version()
+        except TimeoutError as err:
+            raise BLEConnectionError(
+                f"Connection probe exceeded {CONNECT_TIMEOUT}s"
+            ) from err
 
     @override
     async def async_step_bluetooth(
         self, discovery_info: BluetoothServiceInfoBleak
     ) -> ConfigFlowResult:
         """Handle the Bluetooth discovery step."""
+        if not _is_opendisplay(discovery_info):
+            return self.async_abort(reason="not_supported")
         await self.async_set_unique_id(discovery_info.address)
         self._abort_if_unique_id_configured()
         self._discovery_info = discovery_info
         self.context["title_placeholders"] = {"name": discovery_info.name}
 
-        try:
-            await self._async_test_connection(discovery_info.address)
-        except AuthenticationRequiredError:
-            return await self.async_step_encryption_key()
-        except OpenDisplayError:
-            return self.async_abort(reason="cannot_connect")
-        except Exception:
-            _LOGGER.exception("Unexpected error")
-            return self.async_abort(reason="unknown")
-
         return await self.async_step_bluetooth_confirm()
 
     async def async_step_bluetooth_confirm(
         self, user_input: dict[str, Any] | None = None
     ) -> ConfigFlowResult:
-        """Confirm discovery."""
+        """Confirm discovery and verify the device responds."""
         assert self._discovery_info is not None
+        errors: dict[str, str] = {}
 
-        if user_input is None:
-            self._set_confirm_only()
-            return self.async_show_form(
-                step_id="bluetooth_confirm",
-                description_placeholders=self.context["title_placeholders"],
-            )
+        # The device is only contacted once the user confirms: every unconfigured
+        # device in range is discovered on every restart, and probing them all
+        # would exhaust the adapter's connection slots.
+        if user_input is not None:
+            try:
+                await self._async_test_connection(self._discovery_info.address)
+            except AuthenticationRequiredError:
+                return await self.async_step_encryption_key()
+            except OpenDisplayError:
+                errors["base"] = "cannot_connect"
+            except Exception:
+                _LOGGER.exception("Unexpected error")
+                errors["base"] = "unknown"
+            else:
+                return self.async_create_entry(title=self._discovery_info.name, data={})
 
-        return self.async_create_entry(title=self._discovery_info.name, data={})
+        self._set_confirm_only()
+        return self.async_show_form(
+            step_id="bluetooth_confirm",
+            description_placeholders=self.context["title_placeholders"],
+            errors=errors,
+        )
 
     @override
     async def async_step_user(
@@ -125,7 +164,7 @@ class OpenDisplayConfigFlow(ConfigFlow, domain=DOMAIN):
                 address = discovery_info.address
                 if address in current_addresses or address in self._discovered_devices:
                     continue
-                if MANUFACTURER_ID in discovery_info.manufacturer_data:
+                if _is_opendisplay(discovery_info):
                     self._discovered_devices[address] = discovery_info
 
         if not self._discovered_devices:

homeassistant/components/opendisplay/strings.json

@@ -4,6 +4,7 @@
       "already_configured": "[%key:common::config_flow::abort::already_configured_device%]",
       "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
       "no_devices_found": "[%key:common::config_flow::abort::no_devices_found%]",
+      "not_supported": "Device not supported",
       "unknown": "[%key:common::config_flow::error::unknown%]"
     },
     "error": {

tests/components/opendisplay/__init__.py

@@ -28,6 +28,7 @@ V1_ADVERTISEMENT_DATA = b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x82\x72\x
 
 TEST_ADDRESS = "AA:BB:CC:DD:EE:FF"
 TEST_TITLE = "OpenDisplay 1234"
+TEST_NAME = "OD1A2B3C"
 ENCRYPTION_KEY = "aabbccddee112233aabbccddee112233"  # 32 hex chars = 16 bytes
 
 # Firmware version response: major=1, minor=2, patch=3, sha="abc123"
@@ -115,7 +116,7 @@ DEVICE_CONFIG = GlobalConfig(
 
 
 def make_service_info(
-    name: str | None = "OpenDisplay 1234",
+    name: str | None = TEST_NAME,
     address: str = "AA:BB:CC:DD:EE:FF",
     manufacturer_data: dict[int, bytes] | None = None,
 ) -> BluetoothServiceInfoBleak:
@@ -169,7 +170,7 @@ BUTTON_DEVICE_CONFIG = GlobalConfig(
 
 def make_v1_service_info(
     dynamic_data: bytes = b"\x00" * 11,
-    name: str | None = "OpenDisplay 1234",
+    name: str | None = TEST_NAME,
     address: str = TEST_ADDRESS,
 ) -> BluetoothServiceInfoBleak:
     """Create a v1 advertisement service info with a custom 11-byte dynamic block."""
@@ -215,8 +216,3 @@ def make_button_device_config(binary_inputs: list[BinaryInputs]) -> GlobalConfig
 
 
 VALID_SERVICE_INFO = make_service_info()
-
-NOT_OPENDISPLAY_SERVICE_INFO = make_service_info(
-    name="Other Device",
-    manufacturer_data={0x1234: b"\x00\x01"},
-)

tests/components/opendisplay/test_config_flow.py

@@ -1,6 +1,8 @@
 """Test the OpenDisplay config flow."""
 
+import asyncio
 from collections.abc import Generator
+from datetime import timedelta
 from unittest.mock import MagicMock, patch
 
 from opendisplay import (
@@ -13,13 +15,34 @@ from opendisplay import (
 import pytest
 
 from homeassistant import config_entries
+from homeassistant.components.bluetooth import BluetoothServiceInfoBleak
+from homeassistant.components.opendisplay.config_flow import CONNECT_TIMEOUT
 from homeassistant.components.opendisplay.const import CONF_ENCRYPTION_KEY, DOMAIN
 from homeassistant.core import HomeAssistant
 from homeassistant.data_entry_flow import FlowResultType
+from homeassistant.util import dt as dt_util
 
-from . import ENCRYPTION_KEY, NOT_OPENDISPLAY_SERVICE_INFO, VALID_SERVICE_INFO
+from . import (
+    ENCRYPTION_KEY,
+    OPENDISPLAY_MANUFACTURER_ID,
+    TEST_NAME,
+    VALID_SERVICE_INFO,
+    make_service_info,
+)
 
-from tests.common import MockConfigEntry
+from tests.common import MockConfigEntry, async_fire_time_changed
+
+UNSUPPORTED_SERVICE_INFOS = [
+    pytest.param(make_service_info(name="Other Device"), id="wrong_name"),
+    pytest.param(
+        make_service_info(manufacturer_data={0x1234: b"\x00\x01"}),
+        id="wrong_manufacturer_id",
+    ),
+    pytest.param(
+        make_service_info(manufacturer_data={OPENDISPLAY_MANUFACTURER_ID: b"\x00\x01"}),
+        id="malformed_advertisement",
+    ),
+]
 
 
 @pytest.fixture(autouse=True)
@@ -32,7 +55,9 @@ def mock_setup_entry() -> Generator[None]:
         yield
 
 
-async def test_bluetooth_discovery(hass: HomeAssistant) -> None:
+async def test_bluetooth_discovery(
+    hass: HomeAssistant, mock_opendisplay_device_class: MagicMock
+) -> None:
     """Test discovery via Bluetooth with a valid device."""
     result = await hass.config_entries.flow.async_init(
         DOMAIN,
@@ -41,17 +66,36 @@ async def test_bluetooth_discovery(hass: HomeAssistant) -> None:
     )
     assert result["type"] is FlowResultType.FORM
     assert result["step_id"] == "bluetooth_confirm"
+    # Discovery alone must not occupy a BLE connection slot.
+    mock_opendisplay_device_class.assert_not_called()
 
     result = await hass.config_entries.flow.async_configure(
         result["flow_id"], user_input={}
     )
 
     assert result["type"] is FlowResultType.CREATE_ENTRY
-    assert result["title"] == "OpenDisplay 1234"
+    assert result["title"] == TEST_NAME
     assert result["data"] == {}
     assert result["result"].unique_id == "AA:BB:CC:DD:EE:FF"
 
 
+@pytest.mark.parametrize("service_info", UNSUPPORTED_SERVICE_INFOS)
+async def test_bluetooth_discovery_not_opendisplay(
+    hass: HomeAssistant,
+    mock_opendisplay_device_class: MagicMock,
+    service_info: BluetoothServiceInfoBleak,
+) -> None:
+    """Test discovery aborts for devices that are not OpenDisplay devices."""
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN,
+        context={"source": config_entries.SOURCE_BLUETOOTH},
+        data=service_info,
+    )
+    assert result["type"] is FlowResultType.ABORT
+    assert result["reason"] == "not_supported"
+    mock_opendisplay_device_class.assert_not_called()
+
+
 async def test_bluetooth_discovery_already_configured(
     hass: HomeAssistant, mock_config_entry: MockConfigEntry
 ) -> None:
@@ -100,7 +144,7 @@ async def test_bluetooth_confirm_connection_error(
     exception: Exception,
     expected_reason: str,
 ) -> None:
-    """Test confirm step aborts when connection fails before showing the form."""
+    """Test the confirm step shows an error and allows a retry when connecting fails."""
     mock_opendisplay_device.__aenter__.side_effect = exception
 
     result = await hass.config_entries.flow.async_init(
@@ -108,27 +152,82 @@ async def test_bluetooth_confirm_connection_error(
         context={"source": config_entries.SOURCE_BLUETOOTH},
         data=VALID_SERVICE_INFO,
     )
+    assert result["type"] is FlowResultType.FORM
+    assert result["step_id"] == "bluetooth_confirm"
 
-    assert result["type"] is FlowResultType.ABORT
-    assert result["reason"] == expected_reason
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"], user_input={}
+    )
+
+    assert result["type"] is FlowResultType.FORM
+    assert result["step_id"] == "bluetooth_confirm"
+    assert result["errors"] == {"base": expected_reason}
+
+    mock_opendisplay_device.__aenter__.side_effect = None
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"], user_input={}
+    )
+    assert result["type"] is FlowResultType.CREATE_ENTRY
 
 
 async def test_bluetooth_confirm_ble_device_not_found(
     hass: HomeAssistant,
 ) -> None:
-    """Test confirm step aborts when BLE device is not found."""
+    """Test the confirm step reports an error when the BLE device is not found."""
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN,
+        context={"source": config_entries.SOURCE_BLUETOOTH},
+        data=VALID_SERVICE_INFO,
+    )
+
     with patch(
         "homeassistant.components.opendisplay.config_flow.async_ble_device_from_address",
         return_value=None,
     ):
-        result = await hass.config_entries.flow.async_init(
-            DOMAIN,
-            context={"source": config_entries.SOURCE_BLUETOOTH},
-            data=VALID_SERVICE_INFO,
+        result = await hass.config_entries.flow.async_configure(
+            result["flow_id"], user_input={}
         )
 
-    assert result["type"] is FlowResultType.ABORT
-    assert result["reason"] == "cannot_connect"
+    assert result["type"] is FlowResultType.FORM
+    assert result["errors"] == {"base": "cannot_connect"}
+
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"], user_input={}
+    )
+    assert result["type"] is FlowResultType.CREATE_ENTRY
+
+
+async def test_bluetooth_confirm_connection_timeout(
+    hass: HomeAssistant,
+    mock_opendisplay_device: MagicMock,
+) -> None:
+    """Test that a device which stops responding does not hang the flow."""
+
+    async def _never_returns(*args: object, **kwargs: object) -> None:
+        await asyncio.Event().wait()
+
+    mock_opendisplay_device.read_firmware_version.side_effect = _never_returns
+
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN,
+        context={"source": config_entries.SOURCE_BLUETOOTH},
+        data=VALID_SERVICE_INFO,
+    )
+
+    task = hass.async_create_task(
+        hass.config_entries.flow.async_configure(result["flow_id"], user_input={})
+    )
+    async_fire_time_changed(hass, dt_util.utcnow() + timedelta(seconds=CONNECT_TIMEOUT))
+    result = await task
+
+    assert result["type"] is FlowResultType.FORM
+    assert result["errors"] == {"base": "cannot_connect"}
+
+    mock_opendisplay_device.read_firmware_version.side_effect = None
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"], user_input={}
+    )
+    assert result["type"] is FlowResultType.CREATE_ENTRY
 
 
 async def test_user_step_with_devices(hass: HomeAssistant) -> None:
@@ -151,7 +250,7 @@ async def test_user_step_with_devices(hass: HomeAssistant) -> None:
     )
 
     assert result["type"] is FlowResultType.CREATE_ENTRY
-    assert result["title"] == "OpenDisplay 1234"
+    assert result["title"] == TEST_NAME
     assert result["data"] == {}
     assert result["result"].unique_id == "AA:BB:CC:DD:EE:FF"
 
@@ -171,11 +270,14 @@ async def test_user_step_no_devices(hass: HomeAssistant) -> None:
     assert result["reason"] == "no_devices_found"
 
 
-async def test_user_step_filters_unsupported(hass: HomeAssistant) -> None:
+@pytest.mark.parametrize("service_info", UNSUPPORTED_SERVICE_INFOS)
+async def test_user_step_filters_unsupported(
+    hass: HomeAssistant, service_info: BluetoothServiceInfoBleak
+) -> None:
     """Test user step filters out unsupported devices."""
     with patch(
         "homeassistant.components.opendisplay.config_flow.async_discovered_service_info",
-        return_value=[NOT_OPENDISPLAY_SERVICE_INFO],
+        return_value=[service_info],
     ):
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
@@ -265,6 +367,9 @@ async def test_bluetooth_discovery_encrypted_device(
         context={"source": config_entries.SOURCE_BLUETOOTH},
         data=VALID_SERVICE_INFO,
     )
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"], user_input={}
+    )
     assert result["type"] is FlowResultType.FORM
     assert result["step_id"] == "encryption_key"
 
@@ -292,6 +397,9 @@ async def test_bluetooth_discovery_encrypted_invalid_key_format(
         context={"source": config_entries.SOURCE_BLUETOOTH},
         data=VALID_SERVICE_INFO,
     )
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"], user_input={}
+    )
     assert result["step_id"] == "encryption_key"
 
     result = await hass.config_entries.flow.async_configure(
@@ -326,6 +434,9 @@ async def test_bluetooth_discovery_encrypted_wrong_key(
         context={"source": config_entries.SOURCE_BLUETOOTH},
         data=VALID_SERVICE_INFO,
     )
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"], user_input={}
+    )
     assert result["step_id"] == "encryption_key"
 
     result = await hass.config_entries.flow.async_configure(
@@ -500,3 +611,10 @@ async def test_reauth_invalid_key_format(
 
     assert result["type"] is FlowResultType.FORM
     assert result["errors"] == {CONF_ENCRYPTION_KEY: "invalid_key_format"}
+
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        user_input={CONF_ENCRYPTION_KEY: ENCRYPTION_KEY},
+    )
+    assert result["type"] is FlowResultType.ABORT
+    assert result["reason"] == "reauth_successful"