From d1e7ade6dbd3fbbba91e6c0edde55cdcbdfeac21 Mon Sep 17 00:00:00 2001 From: Phil Bruckner Date: Thu, 6 Feb 2020 14:44:48 -0600 Subject: [PATCH] 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 | 62 +++++++++++++------ homeassistant/components/amcrest/camera.py | 60 ++++++++++++++---- homeassistant/components/amcrest/const.py | 3 + .../components/amcrest/manifest.json | 2 +- requirements_all.txt | 2 +- 5 files changed, 95 insertions(+), 34 deletions(-) diff --git a/homeassistant/components/amcrest/__init__.py b/homeassistant/components/amcrest/__init__.py index d1e1aafa6f3..5578d350e22 100644 --- a/homeassistant/components/amcrest/__init__.py +++ b/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 - _LOGGER.debug("%s camera errs: %i", self._wrap_name, self._wrap_errors) + errs = self._wrap_errors = self._wrap_errors + 1 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._hass, service_signal(SERVICE_UPDATE, self._wrap_name) - ) - self._unsub_recheck = track_time_interval( - self._hass, self._wrap_test_online, RECHECK_INTERVAL - ) + self._start_recovery() 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( - 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 + api = AmcrestChecker( + hass, name, device[CONF_HOST], device[CONF_PORT], username, password + ) ffmpeg_arguments = device[CONF_FFMPEG_ARGUMENTS] resolution = RESOLUTION_LIST[device[CONF_RESOLUTION]] diff --git a/homeassistant/components/amcrest/camera.py b/homeassistant/components/amcrest/camera.py index 8ff6403c566..0e64d4fefc9 100644 --- a/homeassistant/components/amcrest/camera.py +++ b/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): - """Return a still image response from the camera.""" + def _check_snapshot_ok(self): 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.""" diff --git a/homeassistant/components/amcrest/const.py b/homeassistant/components/amcrest/const.py index 98d613634b5..38ff8a8894e 100644 --- a/homeassistant/components/amcrest/const.py +++ b/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" diff --git a/homeassistant/components/amcrest/manifest.json b/homeassistant/components/amcrest/manifest.json index 8b2d72effa6..38e19e4ec26 100644 --- a/homeassistant/components/amcrest/manifest.json +++ b/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"] } diff --git a/requirements_all.txt b/requirements_all.txt index 92dd07500b2..be6b266e9d2 100644 --- a/requirements_all.txt +++ b/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