From 00780988993a8c2489e71882af7b64ac68b0be72 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 07:34:08 +0000 Subject: [PATCH 01/23] Make async_is_on abstract --- homeassistant/components/samsungtv/bridge.py | 57 ++++++++++++++------ 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 74daf1d34e04eb..483930bdec18ae 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -97,7 +97,7 @@ def __init__( self.method = method self.host = host self.token: str | None = None - self._remote: Remote | None = None + self._remote: Remote | SamsungTVWS | None = None self._reauth_callback: CALLBACK_TYPE | None = None self._new_token_callback: CALLBACK_TYPE | None = None @@ -125,24 +125,9 @@ async def async_mac_from_device(self) -> str | None: 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 async def async_send_key(self, key: str, key_type: str | None = None) -> None: """Send a key to the tv and handles exceptions.""" @@ -223,6 +208,25 @@ async def async_get_app_list(self) -> dict[str, str]: """Get installed app list.""" return {} + 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 + 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) @@ -328,6 +332,25 @@ def _get_app_list(self) -> dict[str, str] | None: return self._app_list + 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 + 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) From 86a09f31015968d37a7a952074546f9542ea41f7 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 07:37:12 +0000 Subject: [PATCH 02/23] Make async_send_key abstract --- homeassistant/components/samsungtv/bridge.py | 74 +++++++++++++------- 1 file changed, 49 insertions(+), 25 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 483930bdec18ae..1749c243bec5d4 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -129,33 +129,9 @@ async def async_get_app_list(self) -> dict[str, str] | None: async def async_is_on(self) -> bool: """Tells if the TV is on.""" + @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: @@ -280,6 +256,30 @@ def _get_remote(self, avoid_open: bool = False) -> Remote: pass return self._remote + 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 + 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) @@ -408,6 +408,30 @@ async def async_device_info(self) -> dict[str, Any] | None: return None + 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 + 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) From 0d62cec29548242f4dff3d76184325b24e87d759 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 07:38:49 +0000 Subject: [PATCH 03/23] Make async_close_remote abstract --- homeassistant/components/samsungtv/bridge.py | 28 +++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 1749c243bec5d4..33e45252de3a00 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -137,15 +137,9 @@ async def async_send_key(self, key: str, key_type: str | None = None) -> None: def _get_remote(self, avoid_open: bool = False) -> Remote | SamsungTVWS: """Get Remote object.""" + @abstractmethod 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.""" @@ -289,6 +283,16 @@ def _send_key(self, key: str) -> None: if remote := self._get_remote(): remote.control(key) + 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") + async def async_stop(self) -> None: """Stop Bridge.""" LOGGER.debug("Stopping SamsungTVLegacyBridge") @@ -484,6 +488,16 @@ def _get_remote(self, avoid_open: bool = False) -> SamsungTVWS: self._notify_new_token_callback() return self._remote + 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") + async def async_stop(self) -> None: """Stop Bridge.""" LOGGER.debug("Stopping SamsungTVWSBridge") From fbef38ef7c680de19a631d4f3472f26ebcd704e2 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 07:53:05 +0000 Subject: [PATCH 04/23] Remove _get_remote from abstraction --- homeassistant/components/samsungtv/bridge.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 33e45252de3a00..50bbe63cb5ddc9 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -97,7 +97,6 @@ def __init__( self.method = method self.host = host self.token: str | None = None - self._remote: Remote | SamsungTVWS | None = None self._reauth_callback: CALLBACK_TYPE | None = None self._new_token_callback: CALLBACK_TYPE | None = None @@ -133,10 +132,6 @@ async def async_is_on(self) -> bool: async def async_send_key(self, key: str, key_type: str | None = None) -> None: """Send a key to the tv and handles exceptions.""" - @abstractmethod - def _get_remote(self, avoid_open: bool = False) -> Remote | SamsungTVWS: - """Get Remote object.""" - @abstractmethod async def async_close_remote(self) -> None: """Close remote object.""" @@ -169,6 +164,7 @@ def __init__( 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.""" @@ -232,7 +228,7 @@ async def async_device_info(self) -> None: """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. @@ -314,6 +310,7 @@ def __init__( 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.""" From 4d811614aee26162d073b50314be4afc3989080d Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 08:06:53 +0000 Subject: [PATCH 05/23] Only use executor on top level abstraction --- homeassistant/components/samsungtv/bridge.py | 82 +++++++++++--------- 1 file changed, 46 insertions(+), 36 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 50bbe63cb5ddc9..3cd9c7d7fb2544 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -175,12 +175,16 @@ async def async_get_app_list(self) -> dict[str, str]: 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: - await self.async_close_remote() + self._close_remote() try: - remote = await self.hass.async_add_executor_job(self._get_remote) + remote = self._get_remote() return remote is not None except ( UnhandledResponse, @@ -247,13 +251,18 @@ def _get_remote(self) -> Remote: return self._remote async def async_send_key(self, key: str, key_type: str | None = None) -> None: + """Send a key to the tv and handles exceptions.""" + await self.hass.async_add_executor_job(self._send_key, key) + + def _send_key(self, key: str) -> 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) + if remote := self._get_remote(): + remote.control(key) break except ( ConnectionClosed, @@ -270,21 +279,16 @@ async def async_send_key(self, key: str, key_type: str | None = None) -> None: # Different reasons, e.g. hostname not resolveable pass - 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) - - def _send_key(self, key: str) -> None: - """Send the key using legacy protocol.""" - if remote := self._get_remote(): - remote.control(key) - 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 - await self.hass.async_add_executor_job(self._remote.close) + self._remote.close() self._remote = None except OSError: LOGGER.debug("Could not establish connection") @@ -292,7 +296,7 @@ async def async_close_remote(self) -> None: async def async_stop(self) -> None: """Stop Bridge.""" LOGGER.debug("Stopping SamsungTVLegacyBridge") - await self.async_close_remote() + await self.hass.async_add_executor_job(self._close_remote) class SamsungTVWSBridge(SamsungTVBridge): @@ -334,12 +338,16 @@ def _get_app_list(self) -> dict[str, str] | None: 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: - await self.async_close_remote() + self._close_remote() try: - remote = await self.hass.async_add_executor_job(self._get_remote) + remote = self._get_remote() return remote is not None except ( UnhandledResponse, @@ -399,24 +407,36 @@ def _try_connect(self) -> str: 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: """Send a key to the tv and handles exceptions.""" + 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 a key to the tv and handles exceptions.""" + if key == "KEY_POWEROFF": + key = "KEY_POWER" 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) + if remote := self._get_remote(): + if key_type == "run_app": + remote.run_app(key) + else: + remote.send_key(key) break except ( ConnectionClosed, @@ -433,20 +453,6 @@ async def async_send_key(self, key: str, key_type: str | None = None) -> None: # Different reasons, e.g. hostname not resolveable pass - 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) - - 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) - def _get_remote(self, avoid_open: bool = False) -> SamsungTVWS: """Create or return a remote control instance.""" if self._remote is None: @@ -486,11 +492,15 @@ def _get_remote(self, avoid_open: bool = False) -> SamsungTVWS: return self._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 - await self.hass.async_add_executor_job(self._remote.close) + self._remote.close() self._remote = None except OSError: LOGGER.debug("Could not establish connection") @@ -498,4 +508,4 @@ async def async_close_remote(self) -> None: async def async_stop(self) -> None: """Stop Bridge.""" LOGGER.debug("Stopping SamsungTVWSBridge") - await self.async_close_remote() + await self.hass.async_add_executor_job(self._close_remote) From 04177b8df091fdf5987260aed4bc499a591189ac Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 08:16:29 +0000 Subject: [PATCH 06/23] Make async_stop abstract --- homeassistant/components/samsungtv/bridge.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 3cd9c7d7fb2544..1b997963696380 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -136,6 +136,10 @@ async def async_send_key(self, key: str, key_type: str | None = None) -> None: async def async_close_remote(self) -> None: """Close remote object.""" + @abstractmethod + async def async_stop(self) -> None: + """Stop Bridge.""" + def _notify_reauth_callback(self) -> None: """Notify access denied callback.""" if self._reauth_callback is not None: @@ -296,7 +300,7 @@ def _close_remote(self) -> None: async def async_stop(self) -> None: """Stop Bridge.""" LOGGER.debug("Stopping SamsungTVLegacyBridge") - await self.hass.async_add_executor_job(self._close_remote) + await self.async_close_remote() class SamsungTVWSBridge(SamsungTVBridge): @@ -508,4 +512,4 @@ def _close_remote(self) -> None: async def async_stop(self) -> None: """Stop Bridge.""" LOGGER.debug("Stopping SamsungTVWSBridge") - await self.hass.async_add_executor_job(self._close_remote) + await self.async_close_remote() From e1f0179de40f54358fda3338fc9da690049e7bb8 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 08:19:30 +0000 Subject: [PATCH 07/23] Adjust docstrings --- homeassistant/components/samsungtv/bridge.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 1b997963696380..dc7ee6de7c465d 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -255,11 +255,11 @@ def _get_remote(self) -> Remote: return self._remote async def async_send_key(self, key: str, key_type: str | None = None) -> None: - """Send a key to the tv and handles exceptions.""" + """Send the key using legacy protocol.""" await self.hass.async_add_executor_job(self._send_key, key) def _send_key(self, key: str) -> None: - """Send a key to the tv and handles exceptions.""" + """Send the key using legacy protocol.""" try: # recreate connection if connection was dead retry_count = 1 @@ -424,11 +424,11 @@ def _device_info(self) -> dict[str, Any] | None: return None async def async_send_key(self, key: str, key_type: str | None = None) -> None: - """Send a key to the tv and handles exceptions.""" + """Send the key using websocket protocol.""" 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 a key to the tv and handles exceptions.""" + """Send the key using websocket protocol.""" if key == "KEY_POWEROFF": key = "KEY_POWER" try: From 9e0cc746d643f4e8b37f25b6e4a99a9b261180bf Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 08:23:35 +0000 Subject: [PATCH 08/23] Adjust exception handling --- homeassistant/components/samsungtv/bridge.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index dc7ee6de7c465d..0b09abeac3a762 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -190,11 +190,7 @@ def _is_on(self) -> bool: try: remote = self._get_remote() return remote is not None - except ( - UnhandledResponse, - AccessDenied, - ConnectionFailure, - ): + except (UnhandledResponse, AccessDenied): # We got a response so it's working. return True except OSError: @@ -353,11 +349,7 @@ def _is_on(self) -> bool: try: remote = self._get_remote() return remote is not None - except ( - UnhandledResponse, - AccessDenied, - ConnectionFailure, - ): + except ConnectionFailure: # We got a response so it's working. return True except OSError: @@ -443,16 +435,12 @@ def _send_key(self, key: str, key_type: str | None = None) -> None: remote.send_key(key) 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 From 77c7ec7b67dd38b6fc40bfc81def8d1f038cbb90 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 08:28:05 +0000 Subject: [PATCH 09/23] Adjust exception handling --- homeassistant/components/samsungtv/bridge.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 0b09abeac3a762..b3871cc779f790 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -264,13 +264,8 @@ def _send_key(self, key: str) -> None: if remote := self._get_remote(): remote.control(key) break - except ( - ConnectionClosed, - BrokenPipeError, - WebSocketException, - ): + except (ConnectionClosed, BrokenPipeError): # 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. From 100d67713d5416402d19a2f407c466ba65b4064c Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 08:35:15 +0000 Subject: [PATCH 10/23] Fix test --- tests/components/samsungtv/test_media_player.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/components/samsungtv/test_media_player.py b/tests/components/samsungtv/test_media_player.py index 55d68453a3814e..d3184e50cb8a32 100644 --- a/tests/components/samsungtv/test_media_player.py +++ b/tests/components/samsungtv/test_media_player.py @@ -440,10 +440,10 @@ 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 ) From 79c784671d4bdeaf09769a452b9aa321b7081084 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 10:38:13 +0000 Subject: [PATCH 11/23] Minor code cleanup --- homeassistant/components/samsungtv/bridge.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index b3871cc779f790..a00f40f80ddbc0 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -188,8 +188,7 @@ def _is_on(self) -> bool: self._close_remote() try: - remote = self._get_remote() - return remote is not None + return self._get_remote() is not None except (UnhandledResponse, AccessDenied): # We got a response so it's working. return True @@ -342,8 +341,7 @@ def _is_on(self) -> bool: self._close_remote() try: - remote = self._get_remote() - return remote is not None + return self._get_remote() is not None except ConnectionFailure: # We got a response so it's working. return True From a432fa070001896f3ecdec39ffeedfc7f3b8bb2c Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:07:58 +0000 Subject: [PATCH 12/23] OSError already handled in _get_remote --- homeassistant/components/samsungtv/bridge.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index a00f40f80ddbc0..bc2fe50800c09b 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -192,9 +192,6 @@ def _is_on(self) -> bool: except (UnhandledResponse, AccessDenied): # We got a response so it's working. return True - except OSError: - # Different reasons, e.g. hostname not resolveable - return False async def async_try_connect(self) -> str: """Try to connect to the Legacy TV.""" From 8cdbbd4166e978bb3e4602ddbf1bba5c1069be91 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:14:38 +0000 Subject: [PATCH 13/23] OSError already handled in _get_remote (ws) --- homeassistant/components/samsungtv/bridge.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index bc2fe50800c09b..8df8cff5605b07 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -342,9 +342,6 @@ def _is_on(self) -> bool: except ConnectionFailure: # We got a response so it's working. return True - except OSError: - # Different reasons, e.g. hostname not resolveable - return False async def async_try_connect(self) -> str: """Try to connect to the Websocket TV.""" From e6aa15e480d9a1a9fffb85b11f30221146c8e974 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:16:52 +0000 Subject: [PATCH 14/23] Use async_get_device_info for legacy --- homeassistant/components/samsungtv/bridge.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 8df8cff5605b07..9e4eb6e05900bd 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -67,7 +67,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 From 0da3d1e1cb03d107636e86455c2d822862268331 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:22:57 +0000 Subject: [PATCH 15/23] Add type hints --- homeassistant/components/samsungtv/__init__.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/homeassistant/components/samsungtv/__init__.py b/homeassistant/components/samsungtv/__init__.py index 508ed2f876ffc9..01f5b5d2b0c631 100644 --- a/homeassistant/components/samsungtv/__init__.py +++ b/homeassistant/components/samsungtv/__init__.py @@ -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,7 +172,7 @@ async def _async_create_bridge_with_updated_data( bridge = _async_get_device_bridge(hass, {**entry.data, **updated_data}) - mac = entry.data.get(CONF_MAC) + mac: str | None = entry.data.get(CONF_MAC) if not mac and bridge.method == METHOD_WEBSOCKET: if info: mac = mac_from_device_info(info) From 95b938b3309a359b51634690511c6926f1a6a8ec Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:23:21 +0000 Subject: [PATCH 16/23] Use async_mac_from_device for legacy --- homeassistant/components/samsungtv/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/homeassistant/components/samsungtv/__init__.py b/homeassistant/components/samsungtv/__init__.py index 01f5b5d2b0c631..b9fe3a80d80eb6 100644 --- a/homeassistant/components/samsungtv/__init__.py +++ b/homeassistant/components/samsungtv/__init__.py @@ -39,7 +39,6 @@ LEGACY_PORT, LOGGER, METHOD_LEGACY, - METHOD_WEBSOCKET, ) @@ -173,7 +172,7 @@ async def _async_create_bridge_with_updated_data( bridge = _async_get_device_bridge(hass, {**entry.data, **updated_data}) mac: str | None = entry.data.get(CONF_MAC) - if not mac and bridge.method == METHOD_WEBSOCKET: + if not mac: if info: mac = mac_from_device_info(info) else: From 72c97e9863793d11c3baced4a54284e4cdadbb57 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:26:56 +0000 Subject: [PATCH 17/23] ConnectionFailure already handled in _get_remote (ws) --- homeassistant/components/samsungtv/bridge.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 9e4eb6e05900bd..1b109ff31b6d09 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -337,11 +337,7 @@ def _is_on(self) -> bool: if self._remote is not None: self._close_remote() - try: - return self._get_remote() is not None - except ConnectionFailure: - # We got a response so it's working. - return True + return self._get_remote() is not None async def async_try_connect(self) -> str: """Try to connect to the Websocket TV.""" From 0f3b100f665916774dbb55048eae172e84b7fbdf Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:41:44 +0000 Subject: [PATCH 18/23] Remove token from debug output --- homeassistant/components/samsungtv/bridge.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/homeassistant/components/samsungtv/bridge.py b/homeassistant/components/samsungtv/bridge.py index 1b109ff31b6d09..62b3314f0af029 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -19,7 +19,6 @@ CONF_NAME, CONF_PORT, CONF_TIMEOUT, - CONF_TOKEN, ) from homeassistant.core import CALLBACK_TYPE, HomeAssistant from homeassistant.helpers.device_registry import format_mac @@ -367,8 +366,6 @@ def _try_connect(self) -> str: ) 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: From d55e4f984460e1eab137f4f6936913db3e4a55ca Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 14:55:06 +0000 Subject: [PATCH 19/23] Drop async_stop (dup of async_close_remote) --- homeassistant/components/samsungtv/__init__.py | 7 +++++-- homeassistant/components/samsungtv/bridge.py | 14 -------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/homeassistant/components/samsungtv/__init__.py b/homeassistant/components/samsungtv/__init__.py index b9fe3a80d80eb6..c9ca282fdd1a31 100644 --- a/homeassistant/components/samsungtv/__init__.py +++ b/homeassistant/components/samsungtv/__init__.py @@ -133,7 +133,8 @@ def new_token_callback() -> None: 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) @@ -196,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 62b3314f0af029..ddb3c0a4e9b940 100644 --- a/homeassistant/components/samsungtv/bridge.py +++ b/homeassistant/components/samsungtv/bridge.py @@ -135,10 +135,6 @@ async def async_send_key(self, key: str, key_type: str | None = None) -> None: async def async_close_remote(self) -> None: """Close remote object.""" - @abstractmethod - async def async_stop(self) -> None: - """Stop Bridge.""" - def _notify_reauth_callback(self) -> None: """Notify access denied callback.""" if self._reauth_callback is not None: @@ -283,11 +279,6 @@ def _close_remote(self) -> None: except OSError: LOGGER.debug("Could not establish connection") - async def async_stop(self) -> None: - """Stop Bridge.""" - LOGGER.debug("Stopping SamsungTVLegacyBridge") - await self.async_close_remote() - class SamsungTVWSBridge(SamsungTVBridge): """The Bridge for WebSocket TVs.""" @@ -476,8 +467,3 @@ def _close_remote(self) -> None: self._remote = None except OSError: LOGGER.debug("Could not establish connection") - - async def async_stop(self) -> None: - """Stop Bridge.""" - LOGGER.debug("Stopping SamsungTVWSBridge") - await self.async_close_remote() From 34ce49f9526cd19bb0a16fda03d68ebaf69b11bc Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 15:08:53 +0000 Subject: [PATCH 20/23] Add test for OSError in ws close --- tests/components/samsungtv/test_media_player.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/components/samsungtv/test_media_player.py b/tests/components/samsungtv/test_media_player.py index d3184e50cb8a32..c9e2e1a328dbc1 100644 --- a/tests/components/samsungtv/test_media_player.py +++ b/tests/components/samsungtv/test_media_player.py @@ -582,6 +582,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) From df79c14b0df05f6a4a9bf6ec49769679d318db91 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 15:15:11 +0000 Subject: [PATCH 21/23] Add test for OSError in ws send_key --- tests/components/samsungtv/test_media_player.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/components/samsungtv/test_media_player.py b/tests/components/samsungtv/test_media_player.py index c9e2e1a328dbc1..294255f145104b 100644 --- a/tests/components/samsungtv/test_media_player.py +++ b/tests/components/samsungtv/test_media_player.py @@ -451,6 +451,17 @@ async def test_send_key_websocketexception(hass: HomeAssistant, remotews: Mock) 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 + ) + state = hass.states.get(ENTITY_ID) + assert state.state == STATE_ON + + async def test_send_key_os_error(hass: HomeAssistant, remote: Mock) -> None: """Testing broken pipe Exception.""" await setup_samsungtv(hass, MOCK_CONFIG) From fad868b6a9957c7d4383a45b927d02b744b4d63f Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 15:20:37 +0000 Subject: [PATCH 22/23] Add test for WebSocketException in ws get_remote --- .../components/samsungtv/test_media_player.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/components/samsungtv/test_media_player.py b/tests/components/samsungtv/test_media_player.py index 294255f145104b..bee0d46db346b0 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.""" From 2342cec4773767a75604ae9d5593ff118c582859 Mon Sep 17 00:00:00 2001 From: epenet Date: Fri, 25 Feb 2022 15:32:36 +0000 Subject: [PATCH 23/23] Add tests for send_key when power_off_in_progress --- tests/components/samsungtv/test_media_player.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/components/samsungtv/test_media_player.py b/tests/components/samsungtv/test_media_player.py index bee0d46db346b0..eb7fb650a9a839 100644 --- a/tests/components/samsungtv/test_media_player.py +++ b/tests/components/samsungtv/test_media_player.py @@ -588,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."""