From a9c5aafe920c0b89322fd31ae27ef34592354ead Mon Sep 17 00:00:00 2001 From: g4bri3lDev Date: Wed, 30 Sep 2026 18:05:29 +0200 Subject: [PATCH] Fix OpenDisplay connecting to every discovered device on startup (#178239) --- .../components/opendisplay/config_flow.py | 85 +++++++--- .../components/opendisplay/strings.json | 1 + tests/components/opendisplay/__init__.py | 10 +- .../opendisplay/test_config_flow.py | 152 ++++++++++++++++-- 4 files changed, 201 insertions(+), 47 deletions(-) diff --git a/homeassistant/components/opendisplay/config_flow.py b/homeassistant/components/opendisplay/config_flow.py index 0be022903739..22b364908357 100644 --- a/homeassistant/components/opendisplay/config_flow.py +++ b/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: diff --git a/homeassistant/components/opendisplay/strings.json b/homeassistant/components/opendisplay/strings.json index fa52cdd60f44..84a3d9bb9fcd 100644 --- a/homeassistant/components/opendisplay/strings.json +++ b/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": { diff --git a/tests/components/opendisplay/__init__.py b/tests/components/opendisplay/__init__.py index e664db999069..b081fe8655c0 100644 --- a/tests/components/opendisplay/__init__.py +++ b/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"}, -) diff --git a/tests/components/opendisplay/test_config_flow.py b/tests/components/opendisplay/test_config_flow.py index 4c7b4cf23a61..cdbc80bc0b60 100644 --- a/tests/components/opendisplay/test_config_flow.py +++ b/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"