Improve foscam library exception support #11701

i-am-shodan · 2018-01-16T12:47:19Z

Catches foscam exceptions that otherwise pollute the log. Many of these exception can safely be ignored

Description:

The Python Foscam library correctly sets motion detection on my Foscam FI9800P but throws an exception while doing so. I've 'fixed' this by adding an exception handler/logging. This will then allow people to suppress the messages via an entry in their configuration.

The upstream maintainer of the Python Foscam library is AWOL and hasn't responded to the actual problem: quatanium/foscam-python-lib#22

In time Python Foscam should be replaced, it's pretty bad and doesn't support many devices.

Related issue (if applicable): fixes #9484

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for `configuration.yaml` (if applicable):

Checklist:

[Y] The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

[n/a] Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

[n/a] Local tests with tox run successfully. Your PR cannot be merged unless tests pass
[n/a] New dependencies have been added to the REQUIREMENTS variable (example).
[n/a] New dependencies are only imported inside functions that use them (example).
[n/a] New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
[n/a] New files were added to .coveragerc.

If the code does not interact with devices:

[y] Local tests with tox run successfully. Your PR cannot be merged unless tests pass
[n/a] Tests have been added to verify that the new code works.

Catches foscam exceptions that otherwise pollute the log. Many of these exception can safely be ignored

homeassistant · 2018-01-16T12:47:20Z

Hi @i-am-shodan,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2018-01-16T12:47:28Z

homeassistant/components/camera/foscam.py

+            else:
+                self._motion_status = False
+        except:
+            _LOGGER.debug("An exception occured when communicating with the Foscam Camera")


line too long (91 > 79 characters)

houndci-bot · 2018-01-16T12:47:28Z

homeassistant/components/camera/foscam.py

+        try:
+            ret, err = self._foscam_session.disable_motion_detection()
+            if ret == FOSCAM_COMM_ERROR:
+                _LOGGER.debug("Unable to communicate with Foscam Camera: %s", err)


line too long (82 > 79 characters)

houndci-bot · 2018-01-16T12:47:28Z

homeassistant/components/camera/foscam.py

+            else:
+                self._motion_status = False
+        except:
+            _LOGGER.debug("An exception occured when communicating with the Foscam Camera")


line too long (91 > 79 characters)

houndci-bot · 2018-01-16T12:47:28Z

homeassistant/components/camera/foscam.py

+        try:
+            ret, err = self._foscam_session.enable_motion_detection()
+            if ret == FOSCAM_COMM_ERROR:
+                _LOGGER.debug("Unable to communicate with Foscam Camera: %s", err)


line too long (82 > 79 characters)

Fixed line length

homeassistant · 2018-01-16T13:27:03Z

Hi @gavinlam,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

balloob · 2018-01-18T05:24:52Z

homeassistant/components/camera/foscam.py

-        else:
+        try:
+            ret, err = self._foscam_session.enable_motion_detection()
+            if ret == FOSCAM_COMM_ERROR:


This code makes no sense. If it is an error, log that an error has occurred and then assume the command was successful because we're setting the attribute to True ?

If you loved this then you’ll love the constructor for this module. Who approved this in the first place???

So if this is supposed to be the "positive" result, we should not log "Unable to communicate"? I know it is not your code, but if you're touching it, let's at least just remove that log statement.

balloob · 2018-01-18T05:25:09Z

homeassistant/components/camera/foscam.py

+                self._motion_status = True
+            else:
+                self._motion_status = False
+        except:


Please specify the exceptions that Foscam can raise

Without having the broad range of Foscam devices to test against this isn’t really achievable to fix this bug. There are at least two cases noted in the upstream repo.

You should be able to see what the Python classes are of the exceptions that get raised

Looks like both the issues upstream result in TypeErrors. I'll add just those

balloob · 2018-01-18T05:25:25Z

homeassistant/components/camera/foscam.py

-            self._motion_status = True
-        else:
+        try:
+            ret, err = self._foscam_session.enable_motion_detection()


What's the use of returning an err code if it can also throw exceptions 🤔

A question to the original author perhaps. This patch is a sticking plaster to fix a long standing bug.

i-am-shodan · 2018-01-18T07:45:20Z

This whole module is a mess. The bigger question is why it raises an exception when the command actually works.

The goal of this patch is not to save this module from itself but to fix long standing bugs in it.

balloob · 2018-01-18T08:29:05Z

Oh I remember this lib now, it broke a release of ours. One day should indeed be replaced.

changed logging and catched only the TypeError effecting the library

balloob

Ok to merge when tests pass

gavinlam · 2018-01-19T13:04:38Z

I believe the author used the wrong email. I did not make this pull request.

…

On Fri, Jan 19, 2018 at 12:58 AM Paulus Schoutsen ***@***.***> wrote: ***@***.**** approved this pull request. Ok to merge when tests pass — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11701 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgEWvVanxO-RD5h62VhOR4rq7cSJHONks5tMC8JgaJpZM4RfuFP> .

Removed var

i-am-shodan · 2018-01-19T13:58:17Z

Can we ignore the shorter if statement. It will result in a long line that hound will throw up

balloob · 2018-01-21T21:00:09Z

Long line? This seems well below 80 chars:

            self._motion_status = ret == FOSCAM_COMM_ERROR

i-am-shodan · 2018-01-21T21:07:46Z

If I was to implement the lint warning
************* Module homeassistant.components.camera.foscam
R: 80,12: The if statement can be replaced with 'var = bool(test)' (simplifiable-if-statement)
R: 93,12: The if statement can be replaced with 'var = bool(test)' (simplifiable-if-statement)

it would make the line over 80 chars.
Also 80 char line length is pretty old fashioned.

tschmidty69 · 2018-01-22T20:32:19Z

80 character length is pretty standard and not a required part of the pep standards but in any case that is what the project uses.

baloob gave you a perfect answer to replace 4 lines of code with one nice short line.

* upstream/master: (465 commits) Update pyhomematic to 0.1.38 (home-assistant#11936) Implement Alexa temperature sensors (home-assistant#11930) Fixed rfxtrx binary_sensor KeyError on missing optional device_class (home-assistant#11925) Allow setting climate devices to AUTO mode via Google Assistant (home-assistant#11923) fixes home-assistant#11848 (home-assistant#11915) Add "write" service to system_log (home-assistant#11901) Update frontend to 20180126.0 Version 0.62 Allow separate command and state OIDs and payloads in SNMP switch (home-assistant#11075) Add ERC20 tokens to etherscan.io sensor (home-assistant#11916) Report scripts and groups as scenes to Alexa (home-assistant#11900) Minor fix to configuration validation and related log line. (home-assistant#11898) Multi-Room Support for Greenwave Reality (home-assistant#11364) Added Xeoma camera platform (home-assistant#11619) Improve foscam library exception support (home-assistant#11701) Iota wallet (home-assistant#11398) New venstar climate component (home-assistant#11639) Update python-wink version and multiple wink fixes/updates. (home-assistant#11833) Use API to discover Hue if no bridges specified (home-assistant#11909) Clarify emulated hue warning (home-assistant#11910) ...

Improve foscam library exception support

3963701

Catches foscam exceptions that otherwise pollute the log. Many of these exception can safely be ignored

homeassistant added cla-needed platform: camera.foscam labels Jan 16, 2018

houndci-bot reviewed Jan 16, 2018

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Jan 16, 2018

Fixed line length

0514943

Fixed line length

homeassistant added cla-needed cla-signed and removed cla-needed labels Jan 16, 2018

balloob reviewed Jan 18, 2018

View reviewed changes

Changed exception and log handling

dae839f

changed logging and catched only the TypeError effecting the library

homeassistant added the cla-signed label Jan 18, 2018

balloob approved these changes Jan 19, 2018

View reviewed changes

Removed unused var

b797240

Removed var

Fix remaining issue

c2f7807

fabaff merged commit 502ebd2 into home-assistant:dev Jan 25, 2018

balloob mentioned this pull request Jan 26, 2018

0.62 #11921

Merged

home-assistant locked and limited conversation to collaborators May 29, 2018

ghost added integration: foscam and removed platform: camera.foscam labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve foscam library exception support #11701

Improve foscam library exception support #11701

i-am-shodan commented Jan 16, 2018

homeassistant commented Jan 16, 2018

houndci-bot Jan 16, 2018

houndci-bot Jan 16, 2018

houndci-bot Jan 16, 2018

houndci-bot Jan 16, 2018

homeassistant commented Jan 16, 2018

balloob Jan 18, 2018

i-am-shodan Jan 18, 2018

balloob Jan 18, 2018

balloob Jan 18, 2018

i-am-shodan Jan 18, 2018

balloob Jan 18, 2018

i-am-shodan Jan 18, 2018

balloob Jan 18, 2018

i-am-shodan Jan 18, 2018

i-am-shodan commented Jan 18, 2018

balloob commented Jan 18, 2018

balloob left a comment

gavinlam commented Jan 19, 2018 via email

i-am-shodan commented Jan 19, 2018

balloob commented Jan 21, 2018

i-am-shodan commented Jan 21, 2018

tschmidty69 commented Jan 22, 2018

Improve foscam library exception support #11701

Improve foscam library exception support #11701

Conversation

i-am-shodan commented Jan 16, 2018

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

homeassistant commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homeassistant commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-am-shodan commented Jan 18, 2018

balloob commented Jan 18, 2018

balloob left a comment

Choose a reason for hiding this comment

gavinlam commented Jan 19, 2018 via email

i-am-shodan commented Jan 19, 2018

balloob commented Jan 21, 2018

i-am-shodan commented Jan 21, 2018

tschmidty69 commented Jan 22, 2018

Example entry for `configuration.yaml` (if applicable):