Adjust SamsungTV abstraction layer (#67216)

Co-authored-by: epenet <epenet@users.noreply.github.com>

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 = {}
+    updated_data: dict[str, str | int] = {}
-    host = entry.data[CONF_HOST]
+    host: str = entry.data[CONF_HOST]
-    port = entry.data.get(CONF_PORT)
+    port: int | None = entry.data.get(CONF_PORT)
-    method = entry.data.get(CONF_METHOD)
+    method: str | None = entry.data.get(CONF_METHOD)
-    info = None
+    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)
+    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:
@@ -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
 
 

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():
+        try:
-            remote.control(key)
+            # 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:
+    async def async_close_remote(self) -> None:
-        """Stop Bridge."""
+        """Close remote object."""
-        LOGGER.debug("Stopping SamsungTVLegacyBridge")
+        await self.hass.async_add_executor_job(self._close_remote)
-        await self.async_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(
+                device_info: dict[str, Any] = remote.rest_device_info()
-                    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():
+        try:
-            if key_type == "run_app":
+            # recreate connection if connection was dead
-                remote.run_app(key)
+            retry_count = 1
-            else:
+            for _ in range(retry_count + 1):
-                remote.send_key(key)
+                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:
+    async def async_close_remote(self) -> None:
-        """Stop Bridge."""
+        """Close remote object."""
-        LOGGER.debug("Stopping SamsungTVWSBridge")
+        await self.hass.async_add_executor_job(self._close_remote)
-        await self.async_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")

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)
+    await setup_samsungtv(hass, MOCK_CONFIGWS)
-    remote.control = Mock(side_effect=WebSocketException("Boom"))
+    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)