From 715d7f70f0aa5c08e587e9745b95d881af30719d Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Fri, 25 Feb 2022 17:15:45 +0100 Subject: [PATCH] Adjust SamsungTV abstraction layer (#67216) Co-authored-by: epenet --- .../components/samsungtv/__init__.py | 24 +-- homeassistant/components/samsungtv/bridge.py | 184 ++++++++++-------- .../components/samsungtv/test_media_player.py | 56 +++++- 3 files changed, 171 insertions(+), 93 deletions(-) diff --git a/homeassistant/components/samsungtv/__init__.py b/homeassistant/components/samsungtv/__init__.py index 508ed2f876f..c9ca282fdd1 100644 --- a/homeassistant/components/samsungtv/__init__.py +++ b/homeassistant/components/samsungtv/__init__.py @@ -39,7 +39,6 @@ from .const import ( LEGACY_PORT, LOGGER, METHOD_LEGACY, - METHOD_WEBSOCKET, ) @@ -134,7 +133,8 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: async def stop_bridge(event: Event) -> None: """Stop SamsungTV bridge connection.""" - await bridge.async_stop() + LOGGER.debug("Stopping SamsungTVBridge %s", bridge.host) + await bridge.async_close_remote() entry.async_on_unload( hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, stop_bridge) @@ -149,11 +149,11 @@ async def _async_create_bridge_with_updated_data( hass: HomeAssistant, entry: ConfigEntry ) -> SamsungTVLegacyBridge | SamsungTVWSBridge: """Create a bridge object and update any missing data in the config entry.""" - updated_data = {} - host = entry.data[CONF_HOST] - port = entry.data.get(CONF_PORT) - method = entry.data.get(CONF_METHOD) - info = None + updated_data: dict[str, str | int] = {} + host: str = entry.data[CONF_HOST] + port: int | None = entry.data.get(CONF_PORT) + method: str | None = entry.data.get(CONF_METHOD) + info: dict[str, Any] | None = None if not port or not method: if method == METHOD_LEGACY: @@ -162,7 +162,7 @@ async def _async_create_bridge_with_updated_data( # When we imported from yaml we didn't setup the method # because we didn't know it port, method, info = await async_get_device_info(hass, None, host) - if not port: + if not port or not method: raise ConfigEntryNotReady( "Failed to determine connection method, make sure the device is on." ) @@ -172,8 +172,8 @@ async def _async_create_bridge_with_updated_data( bridge = _async_get_device_bridge(hass, {**entry.data, **updated_data}) - mac = entry.data.get(CONF_MAC) - if not mac and bridge.method == METHOD_WEBSOCKET: + mac: str | None = entry.data.get(CONF_MAC) + if not mac: if info: mac = mac_from_device_info(info) else: @@ -197,7 +197,9 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: """Unload a config entry.""" unload_ok = await hass.config_entries.async_unload_platforms(entry, PLATFORMS) if unload_ok: - await hass.data[DOMAIN][entry.entry_id].async_stop() + bridge: SamsungTVBridge = hass.data[DOMAIN][entry.entry_id] + LOGGER.debug("Stopping SamsungTVBridge %s", bridge.host) + await bridge.async_close_remote() return unload_ok diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 74daf1d34e0..ddb3c0a4e9b 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -19,7 +19,6 @@ from homeassistant.const import ( CONF_NAME, CONF_PORT, CONF_TIMEOUT, - CONF_TOKEN, ) from homeassistant.core import CALLBACK_TYPE, HomeAssistant from homeassistant.helpers.device_registry import format_mac @@ -67,7 +66,7 @@ async def async_get_device_info( bridge = SamsungTVBridge.get_bridge(hass, METHOD_LEGACY, host, LEGACY_PORT) result = await bridge.async_try_connect() if result in (RESULT_SUCCESS, RESULT_AUTH_MISSING): - return LEGACY_PORT, METHOD_LEGACY, None + return LEGACY_PORT, METHOD_LEGACY, await bridge.async_device_info() return None, None, None @@ -97,7 +96,6 @@ class SamsungTVBridge(ABC): self.method = method self.host = host self.token: str | None = None - self._remote: Remote | None = None self._reauth_callback: CALLBACK_TYPE | None = None self._new_token_callback: CALLBACK_TYPE | None = None @@ -125,66 +123,17 @@ class SamsungTVBridge(ABC): async def async_get_app_list(self) -> dict[str, str] | None: """Get installed app list.""" + @abstractmethod async def async_is_on(self) -> bool: """Tells if the TV is on.""" - if self._remote is not None: - await self.async_close_remote() - - try: - remote = await self.hass.async_add_executor_job(self._get_remote) - return remote is not None - except ( - UnhandledResponse, - AccessDenied, - ConnectionFailure, - ): - # We got a response so it's working. - return True - except OSError: - # Different reasons, e.g. hostname not resolveable - return False + @abstractmethod async def async_send_key(self, key: str, key_type: str | None = None) -> None: """Send a key to the tv and handles exceptions.""" - try: - # recreate connection if connection was dead - retry_count = 1 - for _ in range(retry_count + 1): - try: - await self._async_send_key(key, key_type) - break - except ( - ConnectionClosed, - BrokenPipeError, - WebSocketException, - ): - # BrokenPipe can occur when the commands is sent to fast - # WebSocketException can occur when timed out - self._remote = None - except (UnhandledResponse, AccessDenied): - # We got a response so it's on. - LOGGER.debug("Failed sending command %s", key, exc_info=True) - except OSError: - # Different reasons, e.g. hostname not resolveable - pass @abstractmethod - async def _async_send_key(self, key: str, key_type: str | None = None) -> None: - """Send the key.""" - - @abstractmethod - def _get_remote(self, avoid_open: bool = False) -> Remote | SamsungTVWS: - """Get Remote object.""" - async def async_close_remote(self) -> None: """Close remote object.""" - try: - if self._remote is not None: - # Close the current remote connection - await self.hass.async_add_executor_job(self._remote.close) - self._remote = None - except OSError: - LOGGER.debug("Could not establish connection") def _notify_reauth_callback(self) -> None: """Notify access denied callback.""" @@ -214,6 +163,7 @@ class SamsungTVLegacyBridge(SamsungTVBridge): CONF_PORT: None, CONF_TIMEOUT: 1, } + self._remote: Remote | None = None async def async_mac_from_device(self) -> None: """Try to fetch the mac address of the TV.""" @@ -223,6 +173,21 @@ class SamsungTVLegacyBridge(SamsungTVBridge): """Get installed app list.""" return {} + async def async_is_on(self) -> bool: + """Tells if the TV is on.""" + return await self.hass.async_add_executor_job(self._is_on) + + def _is_on(self) -> bool: + """Tells if the TV is on.""" + if self._remote is not None: + self._close_remote() + + try: + return self._get_remote() is not None + except (UnhandledResponse, AccessDenied): + # We got a response so it's working. + return True + async def async_try_connect(self) -> str: """Try to connect to the Legacy TV.""" return await self.hass.async_add_executor_job(self._try_connect) @@ -258,7 +223,7 @@ class SamsungTVLegacyBridge(SamsungTVBridge): """Try to gather infos of this device.""" return None - def _get_remote(self, avoid_open: bool = False) -> Remote: + def _get_remote(self) -> Remote: """Create or return a remote control instance.""" if self._remote is None: # We need to create a new instance to reconnect. @@ -276,19 +241,43 @@ class SamsungTVLegacyBridge(SamsungTVBridge): pass return self._remote - async def _async_send_key(self, key: str, key_type: str | None = None) -> None: + async def async_send_key(self, key: str, key_type: str | None = None) -> None: """Send the key using legacy protocol.""" - return await self.hass.async_add_executor_job(self._send_key, key) + await self.hass.async_add_executor_job(self._send_key, key) def _send_key(self, key: str) -> None: """Send the key using legacy protocol.""" - if remote := self._get_remote(): - remote.control(key) + try: + # recreate connection if connection was dead + retry_count = 1 + for _ in range(retry_count + 1): + try: + if remote := self._get_remote(): + remote.control(key) + break + except (ConnectionClosed, BrokenPipeError): + # BrokenPipe can occur when the commands is sent to fast + self._remote = None + except (UnhandledResponse, AccessDenied): + # We got a response so it's on. + LOGGER.debug("Failed sending command %s", key, exc_info=True) + except OSError: + # Different reasons, e.g. hostname not resolveable + pass - async def async_stop(self) -> None: - """Stop Bridge.""" - LOGGER.debug("Stopping SamsungTVLegacyBridge") - await self.async_close_remote() + async def async_close_remote(self) -> None: + """Close remote object.""" + await self.hass.async_add_executor_job(self._close_remote) + + def _close_remote(self) -> None: + """Close remote object.""" + try: + if self._remote is not None: + # Close the current remote connection + self._remote.close() + self._remote = None + except OSError: + LOGGER.debug("Could not establish connection") class SamsungTVWSBridge(SamsungTVBridge): @@ -306,6 +295,7 @@ class SamsungTVWSBridge(SamsungTVBridge): super().__init__(hass, method, host, port) self.token = token self._app_list: dict[str, str] | None = None + self._remote: SamsungTVWS | None = None async def async_mac_from_device(self) -> str | None: """Try to fetch the mac address of the TV.""" @@ -328,6 +318,17 @@ class SamsungTVWSBridge(SamsungTVBridge): return self._app_list + async def async_is_on(self) -> bool: + """Tells if the TV is on.""" + return await self.hass.async_add_executor_job(self._is_on) + + def _is_on(self) -> bool: + """Tells if the TV is on.""" + if self._remote is not None: + self._close_remote() + + return self._get_remote() is not None + async def async_try_connect(self) -> str: """Try to connect to the Websocket TV.""" return await self.hass.async_add_executor_job(self._try_connect) @@ -356,8 +357,6 @@ class SamsungTVWSBridge(SamsungTVBridge): ) as remote: remote.open("samsung.remote.control") self.token = remote.token - if self.token is None: - config[CONF_TOKEN] = "*****" LOGGER.debug("Working config: %s", config) return RESULT_SUCCESS except WebSocketException as err: @@ -375,29 +374,47 @@ class SamsungTVWSBridge(SamsungTVBridge): return RESULT_CANNOT_CONNECT async def async_device_info(self) -> dict[str, Any] | None: + """Try to gather infos of this TV.""" + return await self.hass.async_add_executor_job(self._device_info) + + def _device_info(self) -> dict[str, Any] | None: """Try to gather infos of this TV.""" if remote := self._get_remote(avoid_open=True): with contextlib.suppress(HttpApiError, RequestsTimeout): - device_info: dict[str, Any] = await self.hass.async_add_executor_job( - remote.rest_device_info - ) + device_info: dict[str, Any] = remote.rest_device_info() return device_info return None - async def _async_send_key(self, key: str, key_type: str | None = None) -> None: + async def async_send_key(self, key: str, key_type: str | None = None) -> None: """Send the key using websocket protocol.""" - return await self.hass.async_add_executor_job(self._send_key, key, key_type) + await self.hass.async_add_executor_job(self._send_key, key, key_type) def _send_key(self, key: str, key_type: str | None = None) -> None: """Send the key using websocket protocol.""" if key == "KEY_POWEROFF": key = "KEY_POWER" - if remote := self._get_remote(): - if key_type == "run_app": - remote.run_app(key) - else: - remote.send_key(key) + try: + # recreate connection if connection was dead + retry_count = 1 + for _ in range(retry_count + 1): + try: + if remote := self._get_remote(): + if key_type == "run_app": + remote.run_app(key) + else: + remote.send_key(key) + break + except ( + BrokenPipeError, + WebSocketException, + ): + # BrokenPipe can occur when the commands is sent to fast + # WebSocketException can occur when timed out + self._remote = None + except OSError: + # Different reasons, e.g. hostname not resolveable + pass def _get_remote(self, avoid_open: bool = False) -> SamsungTVWS: """Create or return a remote control instance.""" @@ -437,7 +454,16 @@ class SamsungTVWSBridge(SamsungTVBridge): self._notify_new_token_callback() return self._remote - async def async_stop(self) -> None: - """Stop Bridge.""" - LOGGER.debug("Stopping SamsungTVWSBridge") - await self.async_close_remote() + async def async_close_remote(self) -> None: + """Close remote object.""" + await self.hass.async_add_executor_job(self._close_remote) + + def _close_remote(self) -> None: + """Close remote object.""" + try: + if self._remote is not None: + # Close the current remote connection + self._remote.close() + self._remote = None + except OSError: + LOGGER.debug("Could not establish connection") diff --git a/tests/components/samsungtv/test_media_player.py b/tests/components/samsungtv/test_media_player.py index 55d68453a38..eb7fb650a9a 100644 --- a/tests/components/samsungtv/test_media_player.py +++ b/tests/components/samsungtv/test_media_player.py @@ -274,6 +274,26 @@ async def test_update_off(hass: HomeAssistant, mock_now: datetime) -> None: assert state.state == STATE_OFF +async def test_update_off_ws( + hass: HomeAssistant, remotews: Mock, mock_now: datetime +) -> None: + """Testing update tv off.""" + await setup_samsungtv(hass, MOCK_CONFIGWS) + + state = hass.states.get(ENTITY_ID) + assert state.state == STATE_ON + + remotews.open = Mock(side_effect=WebSocketException("Boom")) + + next_update = mock_now + timedelta(minutes=5) + with patch("homeassistant.util.dt.utcnow", return_value=next_update): + async_fire_time_changed(hass, next_update) + await hass.async_block_till_done() + + state = hass.states.get(ENTITY_ID) + assert state.state == STATE_OFF + + @pytest.mark.usefixtures("remote") async def test_update_access_denied(hass: HomeAssistant, mock_now: datetime) -> None: """Testing update tv access denied exception.""" @@ -440,10 +460,21 @@ async def test_send_key_unhandled_response(hass: HomeAssistant, remote: Mock) -> assert state.state == STATE_ON -async def test_send_key_websocketexception(hass: HomeAssistant, remote: Mock) -> None: +async def test_send_key_websocketexception(hass: HomeAssistant, remotews: Mock) -> None: """Testing unhandled response exception.""" - await setup_samsungtv(hass, MOCK_CONFIG) - remote.control = Mock(side_effect=WebSocketException("Boom")) + await setup_samsungtv(hass, MOCK_CONFIGWS) + remotews.send_key = Mock(side_effect=WebSocketException("Boom")) + assert await hass.services.async_call( + DOMAIN, SERVICE_VOLUME_UP, {ATTR_ENTITY_ID: ENTITY_ID}, True + ) + state = hass.states.get(ENTITY_ID) + assert state.state == STATE_ON + + +async def test_send_key_os_error_ws(hass: HomeAssistant, remotews: Mock) -> None: + """Testing unhandled response exception.""" + await setup_samsungtv(hass, MOCK_CONFIGWS) + remotews.send_key = Mock(side_effect=OSError("Boom")) assert await hass.services.async_call( DOMAIN, SERVICE_VOLUME_UP, {ATTR_ENTITY_ID: ENTITY_ID}, True ) @@ -557,6 +588,12 @@ async def test_turn_off_websocket(hass: HomeAssistant, remotews: Mock) -> None: assert remotews.send_key.call_count == 1 assert remotews.send_key.call_args_list == [call("KEY_POWER")] + assert await hass.services.async_call( + DOMAIN, SERVICE_VOLUME_UP, {ATTR_ENTITY_ID: ENTITY_ID}, True + ) + # key not called + assert remotews.send_key.call_count == 1 + async def test_turn_off_legacy(hass: HomeAssistant, remote: Mock) -> None: """Test for turn_off.""" @@ -582,6 +619,19 @@ async def test_turn_off_os_error( assert "Could not establish connection" in caplog.text +async def test_turn_off_ws_os_error( + hass: HomeAssistant, remotews: Mock, caplog: pytest.LogCaptureFixture +) -> None: + """Test for turn_off with OSError.""" + caplog.set_level(logging.DEBUG) + await setup_samsungtv(hass, MOCK_CONFIGWS) + remotews.close = Mock(side_effect=OSError("BOOM")) + assert await hass.services.async_call( + DOMAIN, SERVICE_TURN_OFF, {ATTR_ENTITY_ID: ENTITY_ID}, True + ) + assert "Could not establish connection" in caplog.text + + async def test_volume_up(hass: HomeAssistant, remote: Mock) -> None: """Test for volume_up.""" await setup_samsungtv(hass, MOCK_CONFIG)