Make amcrest integration more robust (#30843)

- Bump amcrest package to 1.5.6. Includes networking improvements, no longer
  communicates during Http.__init__(), and allows running snapshot command
  without using stream mode.
- Handle login errors better, and not just at startup.
- Increase network connect & read timeout to 6.05 seconds.
- Increase network read timeout to 20 seconds for snapshot command.
- Run snapshot command in separate task, that cannot be cancelled, to eliminate
  possibility of two snapshot commands running simultaneously (since
  AmcrestCam.async_camera_image can be cancelled.) Also makes sure any exceptions
  from the command are caught properly.

homeassistant/components/amcrest/__init__.py

@@ -35,7 +35,15 @@ from homeassistant.helpers.service import async_extract_entity_ids
 
 from .binary_sensor import BINARY_SENSORS
 from .camera import CAMERA_SERVICES, STREAM_SOURCE_LIST
-from .const import CAMERAS, DATA_AMCREST, DEVICES, DOMAIN, SERVICE_UPDATE
+from .const import (
+    CAMERAS,
+    COMM_RETRIES,
+    COMM_TIMEOUT,
+    DATA_AMCREST,
+    DEVICES,
+    DOMAIN,
+    SERVICE_UPDATE,
+)
 from .helpers import service_signal
 from .sensor import SENSORS
 
@@ -111,38 +119,56 @@ class AmcrestChecker(Http):
         self._wrap_name = name
         self._wrap_errors = 0
         self._wrap_lock = threading.Lock()
+        self._wrap_login_err = False
         self._unsub_recheck = None
         super().__init__(
-            host, port, user, password, retries_connection=1, timeout_protocol=3.05
+            host,
+            port,
+            user,
+            password,
+            retries_connection=COMM_RETRIES,
+            timeout_protocol=COMM_TIMEOUT,
         )
 
     @property
     def available(self):
         """Return if camera's API is responding."""
-        return self._wrap_errors <= MAX_ERRORS
+        return self._wrap_errors <= MAX_ERRORS and not self._wrap_login_err
+
+    def _start_recovery(self):
+        dispatcher_send(self._hass, service_signal(SERVICE_UPDATE, self._wrap_name))
+        self._unsub_recheck = track_time_interval(
+            self._hass, self._wrap_test_online, RECHECK_INTERVAL
+        )
 
     def command(self, cmd, retries=None, timeout_cmd=None, stream=False):
         """amcrest.Http.command wrapper to catch errors."""
         try:
             ret = super().command(cmd, retries, timeout_cmd, stream)
+        except LoginError as ex:
+            with self._wrap_lock:
+                was_online = self.available
+                was_login_err = self._wrap_login_err
+                self._wrap_login_err = True
+            if not was_login_err:
+                _LOGGER.error("%s camera offline: Login error: %s", self._wrap_name, ex)
+            if was_online:
+                self._start_recovery()
+            raise
         except AmcrestError:
             with self._wrap_lock:
                 was_online = self.available
-                self._wrap_errors += 1
+                errs = self._wrap_errors = self._wrap_errors + 1
-                _LOGGER.debug("%s camera errs: %i", self._wrap_name, self._wrap_errors)
                 offline = not self.available
-            if offline and was_online:
+            _LOGGER.debug("%s camera errs: %i", self._wrap_name, errs)
+            if was_online and offline:
                 _LOGGER.error("%s camera offline: Too many errors", self._wrap_name)
-                dispatcher_send(
+                self._start_recovery()
-                    self._hass, service_signal(SERVICE_UPDATE, self._wrap_name)
-                )
-                self._unsub_recheck = track_time_interval(
-                    self._hass, self._wrap_test_online, RECHECK_INTERVAL
-                )
             raise
         with self._wrap_lock:
             was_offline = not self.available
             self._wrap_errors = 0
+            self._wrap_login_err = False
         if was_offline:
             self._unsub_recheck()
             self._unsub_recheck = None
@@ -152,6 +178,7 @@ class AmcrestChecker(Http):
 
     def _wrap_test_online(self, now):
         """Test if camera is back online."""
+        _LOGGER.debug("Testing if %s back online", self._wrap_name)
         try:
             self.current_time
         except AmcrestError:
@@ -167,14 +194,9 @@ def setup(hass, config):
         username = device[CONF_USERNAME]
         password = device[CONF_PASSWORD]
 
-        try:
+        api = AmcrestChecker(
-            api = AmcrestChecker(
+            hass, name, device[CONF_HOST], device[CONF_PORT], username, password
-                hass, name, device[CONF_HOST], device[CONF_PORT], username, password
+        )
-            )
-
-        except LoginError as ex:
-            _LOGGER.error("Login error for %s camera: %s", name, ex)
-            continue
 
         ffmpeg_arguments = device[CONF_FFMPEG_ARGUMENTS]
         resolution = RESOLUTION_LIST[device[CONF_RESOLUTION]]

homeassistant/components/amcrest/camera.py

@@ -1,11 +1,11 @@
 """Support for Amcrest IP cameras."""
 import asyncio
 from datetime import timedelta
+from functools import partial
 import logging
 
 from amcrest import AmcrestError
 from haffmpeg.camera import CameraMjpeg
-from urllib3.exceptions import HTTPError
 import voluptuous as vol
 
 from homeassistant.components.camera import (
@@ -26,9 +26,11 @@ from homeassistant.helpers.dispatcher import async_dispatcher_connect
 from .const import (
     CAMERA_WEB_SESSION_TIMEOUT,
     CAMERAS,
+    COMM_TIMEOUT,
     DATA_AMCREST,
     DEVICES,
     SERVICE_UPDATE,
+    SNAPSHOT_TIMEOUT,
 )
 from .helpers import log_update_error, service_signal
 
@@ -90,6 +92,10 @@ async def async_setup_platform(hass, config, async_add_entities, discovery_info=
     async_add_entities([AmcrestCam(name, device, hass.data[DATA_FFMPEG])], True)
 
 
+class CannotSnapshot(Exception):
+    """Conditions are not valid for taking a snapshot."""
+
+
 class AmcrestCam(Camera):
     """An implementation of an Amcrest IP camera."""
 
@@ -112,12 +118,11 @@ class AmcrestCam(Camera):
         self._motion_recording_enabled = None
         self._color_bw = None
         self._rtsp_url = None
-        self._snapshot_lock = asyncio.Lock()
+        self._snapshot_task = None
         self._unsub_dispatcher = []
         self._update_succeeded = False
 
-    async def async_camera_image(self):
+    def _check_snapshot_ok(self):
-        """Return a still image response from the camera."""
         available = self.available
         if not available or not self.is_on:
             _LOGGER.warning(
@@ -125,15 +130,46 @@ class AmcrestCam(Camera):
                 self.name,
                 "offline" if not available else "off",
             )
+            raise CannotSnapshot
+
+    async def _async_get_image(self):
+        try:
+            # Send the request to snap a picture and return raw jpg data
+            # Snapshot command needs a much longer read timeout than other commands.
+            return await self.hass.async_add_executor_job(
+                partial(
+                    self._api.snapshot,
+                    timeout=(COMM_TIMEOUT, SNAPSHOT_TIMEOUT),
+                    stream=False,
+                )
+            )
+        except AmcrestError as error:
+            log_update_error(_LOGGER, "get image from", self.name, "camera", error)
+            return None
+        finally:
+            self._snapshot_task = None
+
+    async def async_camera_image(self):
+        """Return a still image response from the camera."""
+        _LOGGER.debug("Take snapshot from %s", self._name)
+        try:
+            # Amcrest cameras only support one snapshot command at a time.
+            # Hence need to wait if a previous snapshot has not yet finished.
+            # Also need to check that camera is online and turned on before each wait
+            # and before initiating shapshot.
+            while self._snapshot_task:
+                self._check_snapshot_ok()
+                _LOGGER.debug("Waiting for previous snapshot from %s ...", self._name)
+                await self._snapshot_task
+            self._check_snapshot_ok()
+            # Run snapshot command in separate Task that can't be cancelled so
+            # 1) it's not possible to send another snapshot command while camera is
+            #    still working on a previous one, and
+            # 2) someone will be around to catch any exceptions.
+            self._snapshot_task = self.hass.async_create_task(self._async_get_image())
+            return await asyncio.shield(self._snapshot_task)
+        except CannotSnapshot:
             return None
-        async with self._snapshot_lock:
-            try:
-                # Send the request to snap a picture and return raw jpg data
-                response = await self.hass.async_add_executor_job(self._api.snapshot)
-                return response.data
-            except (AmcrestError, HTTPError) as error:
-                log_update_error(_LOGGER, "get image from", self.name, "camera", error)
-                return None
 
     async def handle_async_mjpeg_stream(self, request):
         """Return an MJPEG stream."""

homeassistant/components/amcrest/const.py

@@ -6,6 +6,9 @@ DEVICES = "devices"
 
 BINARY_SENSOR_SCAN_INTERVAL_SECS = 5
 CAMERA_WEB_SESSION_TIMEOUT = 10
+COMM_RETRIES = 1
+COMM_TIMEOUT = 6.05
 SENSOR_SCAN_INTERVAL_SECS = 10
+SNAPSHOT_TIMEOUT = 20
 
 SERVICE_UPDATE = "update"

homeassistant/components/amcrest/manifest.json

@@ -2,7 +2,7 @@
   "domain": "amcrest",
   "name": "Amcrest",
   "documentation": "https://www.home-assistant.io/integrations/amcrest",
-  "requirements": ["amcrest==1.5.3"],
+  "requirements": ["amcrest==1.5.6"],
   "dependencies": ["ffmpeg"],
   "codeowners": ["@pnbruckner"]
 }

requirements_all.txt

@@ -220,7 +220,7 @@ alpha_vantage==2.1.3
 ambiclimate==0.2.1
 
 # homeassistant.components.amcrest
-amcrest==1.5.3
+amcrest==1.5.6
 
 # homeassistant.components.androidtv
 androidtv==0.0.39