Reolink ONVIF move read to primary callback #91478
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
home-assistant
bot
added
cla-signed
code-quality
integration: reolink
small-pr
20 tasks
bdraco
reviewed
Apr 15, 2023
bdraco
approved these changes
Apr 16, 2023
bdraco
reviewed
Apr 16, 2023
bdraco
approved these changes
Apr 17, 2023
|
@starkillerOG Do your test stats still look good? |
|
@bdraco Yes, over a day I got 4000+ ONVIF motion/AI events and still 100% push, 0% poll, 0% lost. |
|
Awesome. It should use a bit less cpu time 👍 |
frenck
pushed a commit
that referenced
this pull request
Apr 17, 2023
* Move read to primary callback
* fix styling
* Do not raise on ConnectionResetError
* Split request.text() to .read() and decode("utf-8")
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Breaking change
Proposed change
Reolink cameras receive motion/AI detection events using ONVIF push messages.
First ONVIF pushes would get lost due to a ConnectionResetError in the "old implementation" HA 2023.4.4 and lower
This bugfix PR #91070 resolved that issue by properly shielding and catching these errors and using polling once data was lost this is the "current implementation", no events are lost which is the most important, but polling is slower and is heavier on the cameras.
This "new implementation" moves the reading back to the primary callback and properly cathes cancellation and ConnectionResetErrors, but in my tests was able to retrieve the message data in 100% of the about 300 callbacks.
Comparing performance of the diffrent implementations for each about 300 ONVIF push motion/AI people events:
Old implementation: 69% push 0% polling 31% lost
Current implementation: 64% push 36% polling 0% lost
New implementation: 100% push 0% polling 0% lost
I am currently running the "new implementation" in my production enviroment and will update the percentages if posible polling callbacks ocur over a few days (once I run it that long).Over more than a day with the "new implementation" in my production enviroment I got 4000+ ONVIF push motion/AI people events and still got 100% push 0% polling 0% lost.
I also tested this PR by adding
await asyncio.sleep(5.0)in various places to trigger the cancellations and mallformed the ONVIF message, and then got polling callbacks, so the fallback for older camera's (or slow once) is still working.Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: