Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make amcrest integration more robust #30843

Merged
merged 3 commits into from Feb 6, 2020

Conversation

@pnbruckner
Copy link
Contributor

pnbruckner commented Jan 16, 2020

Description:

The amcrest integration has had some lingering communication issues for a while. E.g., when taking a snapshot (i.e., camera.snapshot service, or for camera "stream" when not using the stream component), every once in a while the operation would fail. Even when it worked it was not uncommon to see a lot of retries (and associated WARNINGs in the log.)

This PR includes many improvements, including in the amcrest package it uses, to enhance robustness. It also eliminates communicating with the camera during integration setup to avoid delays caused if the camera is offline, with the added benefit that the entity will always be created, but will go to unavailable if not online (and will become usable as soon as it does become accessible.)

List of changes:

  • 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.
  • Make online sensor actually communicate with camera instead of just showing availability status. Necessary if no other sensors configured and not taking snapshots (i.e., no other communication going on with camera.)
  • 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.

Related issue (if applicable): possibly #30523

Pull request with documentation for home-assistant.io (if applicable): N/A

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
@project-bot project-bot bot added this to Needs review in Dev Jan 16, 2020
@pnbruckner pnbruckner force-pushed the pnbruckner:amcrest-update branch from e58346a to 02c9f15 Jan 16, 2020
@MartinHjelmare MartinHjelmare changed the title Make amcrest integration more robust, fix lingering, intermittent errors Make amcrest integration more robust Jan 17, 2020
@pnbruckner pnbruckner changed the title Make amcrest integration more robust WIP: Make amcrest integration more robust Jan 17, 2020
@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Jan 17, 2020

Please do not review yet. I thought of a better way to implement the binary sensor. I wasn't too crazy about all those if statements!

@MartinHjelmare MartinHjelmare moved this from Needs review to Incoming in Dev Jan 17, 2020
@pnbruckner pnbruckner force-pushed the pnbruckner:amcrest-update branch from b777997 to 57cd699 Jan 17, 2020
@pnbruckner pnbruckner moved this from Incoming to Needs review in Dev Jan 17, 2020
@pnbruckner pnbruckner changed the title WIP: Make amcrest integration more robust Make amcrest integration more robust Jan 17, 2020
@pnbruckner pnbruckner requested a review from MartinHjelmare Jan 20, 2020
@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Jan 21, 2020

Sorry, if it wasn't obvious, this is ready for review now. 😃

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 22, 2020

When moving cards in our dev project, please make sure to not put them last in a list with an automation card. The automation card must be last in the list, otherwise it will stop working.

@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Jan 22, 2020

@MartinHjelmare sorry about that. But I didn't move the card from the dev project page. I simply used the Dev Project drop-down at the top-right of this PR page and selected the "Needs review" option from the list. I had no reason to suspect that that would do the wrong thing.

Is there somewhere where project administrative guidelines and/or procedures are documented? I'd like to help out more, but I'm afraid to mess things up (case in point: trying to make a simple change to a PR as I did here.)

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 22, 2020

No worries. Usually this is not a problem. I guess we should document it somewhere in the developer docs.

@pnbruckner pnbruckner force-pushed the pnbruckner:amcrest-update branch from 57cd699 to 454a949 Jan 27, 2020
@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Jan 28, 2020

@MartinHjelmare I was wondering if you, or someone else, would have time to review this. I understand it's not small or trivial, but I've put in a lot of work to improve the robustness of this integration. I've been testing it on my own system for weeks now and it has definitely reduced the number of warnings, errors and command failures. I'd like to see users benefit from these changes.

Also, I'm working on changing the motion detection sensor from polling to streaming (which will both reduce network traffic, as well as make the sensor much more responsive), but I'd like to get this resolved before submitting that PR.

Thanks for you consideration! I know you're very busy.

@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Jan 29, 2020
@pnbruckner pnbruckner requested a review from amelchio Jan 29, 2020
@pnbruckner pnbruckner force-pushed the pnbruckner:amcrest-update branch from 454a949 to a26c7ba Jan 30, 2020
@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Jan 30, 2020

FYI, the force push was because I had incorrectly named an async method _get_image. I changed it to _async_get_image. I also rebased since it's been a while.

@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Jan 31, 2020

Would it help if I removed the following?

  • Make online sensor actually communicate with camera instead of just showing availability status. Necessary if no other sensors configured and not taking snapshots (i.e., no other communication going on with camera.)

That would make the PR smaller and easier to understand. And I have another change to the binary sensors queued up anyway.

Actually, I think I'll go ahead and do that. I'll hopefully push another commit later today or early next week...

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 31, 2020

I started reading this PR earlier in the week but it was too complicated to understand in one commute ride.

So it sounds good to try to trim it down a bit.

- 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.
@pnbruckner pnbruckner force-pushed the pnbruckner:amcrest-update branch from a26c7ba to 3e8d914 Jan 31, 2020
@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Jan 31, 2020

Sorry, I did a force push because it didn't look like anyone had started reading it yet. But basically all that did was to effectively revert all the changes to binary_sensor.py, as well as one related line that was added to const.py. I also took the opportunity to change one line in camera.py -- I simplified await asyncio.wait({self._snapshot_task}) to await self._snapshot_task. (Still learning the nuances of asyncio.)

I appreciate you looking at this. Let me know if you have any questions.

@pnbruckner

This comment has been minimized.

Copy link
Contributor Author

pnbruckner commented Feb 5, 2020

@MartinHjelmare do you think you'll be able to review this before too long? Or would it be ok if I just went ahead and merged it? I've tested this extensively so I'm pretty sure it works without causing any bad side effects. Also I have another PR ready to submit that converts the binary sensors from polling to streaming, and I have other improvements I'd like to make (remove old, unneeded, effectively dead code, add config flow, etc.), but I'm kind of stalled here. Kind of hard to keep momentum when one change takes this long to get done. 😄

Remove leading underscore from _CannotSnapshot
Copy link
Member

MartinHjelmare left a comment

Thanks!

Dev automation moved this from By Code Owner to Reviewer approved Feb 6, 2020
Add docstring for now public Exception class.
@pnbruckner pnbruckner merged commit d1e7ade into home-assistant:dev Feb 6, 2020
10 checks passed
10 checks passed
CI Build #20200206.74 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 61e41f0...75ca755
Details
codecov/project 94.65% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Feb 6, 2020
@lock lock bot locked and limited conversation to collaborators Feb 8, 2020
@pnbruckner pnbruckner deleted the pnbruckner:amcrest-update branch Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.