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

Asyncio conversion #723

Merged
merged 49 commits into from Jul 24, 2023
Merged

Asyncio conversion #723

merged 49 commits into from Jul 24, 2023

Conversation

mkmer
Copy link
Contributor

@mkmer mkmer commented May 26, 2023

Description:

Convert the API to utilize aiohttp and asyncIO
I've spent some time converting to asyncIO - this is the first step toward updating the HA integration to asyncIO, and eventually if we figure out the subscription end points, will allow for event base updates.

It's a big change... so it will probably take some time to get through the proposed changes (assuming you agree with this direction).

Related issue (if applicable): fixes #

Checklist:

  • Local tests with tox run successfully PR cannot be meged unless tests pass
  • Changes tested locally to ensure platform still works as intended
  • Tests added to verify new code works

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #723 (28aa242) into dev (4e0c251) will increase coverage by 16.48%.
The diff coverage is 99.35%.

@@             Coverage Diff             @@
##              dev     #723       +/-   ##
===========================================
+ Coverage   83.15%   99.64%   +16.48%     
===========================================
  Files           8        8               
  Lines        1413     1396       -17     
===========================================
+ Hits         1175     1391      +216     
+ Misses        238        5      -233     
Flag Coverage Δ
unittests 99.64% <99.35%> (+16.48%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
blinkpy/camera.py 98.38% <97.70%> (+24.92%) ⬆️
blinkpy/api.py 100.00% <100.00%> (+37.70%) ⬆️
blinkpy/auth.py 100.00% <100.00%> (+6.17%) ⬆️
blinkpy/blinkpy.py 100.00% <100.00%> (+15.50%) ⬆️
blinkpy/helpers/util.py 100.00% <100.00%> (+9.18%) ⬆️
blinkpy/sync_module.py 100.00% <100.00%> (+10.66%) ⬆️

... and 1 file with indirect coverage changes

@fronzbot
Copy link
Owner

100% on board with this. And a huge huge thanks for working on it. It's something I've always wanted to add to the library but gave up due to lack of time.

Hopefully I'll be able to start looking at the changes a bit this weekend 🤞

@mkmer
Copy link
Contributor Author

mkmer commented May 27, 2023

Since you agree with the concept, I'm going to keep pecking away at the tests. I've been finding a few coding errors as create I them (Usually missing an await for one of the converted functions).

Copy link
Owner

@fronzbot fronzbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the changes looks pretty easy to understand. There's a few questions I have related to my lack of understanding about how async works. Ultimately this is a pretty big shift so my plan will be to pull your changes down locally just so I can do some spot checking of functionality on my end, and then probably do an rc release for more wide-spread testing and stuff.

blinkpy/auth.py Outdated Show resolved Hide resolved
blinkpy/auth.py Show resolved Hide resolved
blinkpy/camera.py Show resolved Hide resolved
blinkpy/camera.py Outdated Show resolved Hide resolved
blinkpy/camera.py Outdated Show resolved Hide resolved
blinkpy/camera.py Show resolved Hide resolved
blinkpy/camera.py Show resolved Hide resolved
blinkpy/camera.py Show resolved Hide resolved
blinkpy/sync_module.py Outdated Show resolved Hide resolved
blinkpy/sync_module.py Outdated Show resolved Hide resolved
@mkmer
Copy link
Contributor Author

mkmer commented Jun 2, 2023

Found a couple of mistakes in the image formatting while testing the HA integration.
If you're interested in testing, I have the HA integration "ready to go":
https://github.com/mkmer/HomeAssistant_Core.git
branch is "Blink_asyncio".
I punted with 0.23.0 in the manifest - just as a place holder.

@mkmer
Copy link
Contributor Author

mkmer commented Jun 20, 2023

After 3 weeks of running as a custom component, It looks like moving to asyncIO version fixes the partial picture download that was reported here: home-assistant/core#91227.

@fronzbot
Copy link
Owner

Awesome! Exactly what would be expected, so glad to hear that. Real life got in the way a bit so I haven't been able to test on my end, but I should be able to run some tests this week as a sanity check and then merge 🤞

@mkmer
Copy link
Contributor Author

mkmer commented Jun 22, 2023

Found the wrapper was removing force from the refresh function. It was impossible to force a refresh. Then the usual failure to be complete on my part :)

@mkmer
Copy link
Contributor Author

mkmer commented Jun 22, 2023

In the process of trying to figure out why things are so slow to update, I discovered we need a data coordinator. I've created a second branch in the repo with said data coordinator - please try that one out too...
https://github.com/mkmer/HomeAssistant_Core.git
Branch = coordinator

@mkmer
Copy link
Contributor Author

mkmer commented Jul 20, 2023

How's the review going?
Would really love to get the HA integration moving with asyncIO :)

@fronzbot
Copy link
Owner

I have tested very little, to be honest. Life and work have been hectic, unfortunately. You've been running this for awhile now, though, right? If you think it's good to go, I'll merge it.

@mkmer
Copy link
Contributor Author

mkmer commented Jul 21, 2023

It has been running flawlessly for me, BUT I only have doorbells on a sync module. I have no way to verify any other combinations IRL. If the added tests are any good, everything "should" work, but it wouldn't hurt to try some other combinations.

If not, let her fly with a PyPi release and I'll see if we can get the HA integration update through. If something is broken we can fix it on the fly :)

I've also added a user friendly wxPython based blinksync.py which can be used to download/delete files from a sync module via a selection dialog. For now it only uses the first system found.

@fronzbot fronzbot merged commit 9129e76 into fronzbot:dev Jul 24, 2023
8 checks passed
@fronzbot
Copy link
Owner

Just made a pre-release to pypi: blinkpy==0.22.0.dev0

Not a valid release to push to home-assistant, but at least allows testing where the version can just be updated once the release is pushed to the master branch.

@mkmer mkmer deleted the asyncio_conversion branch July 24, 2023 13:48
@mkmer
Copy link
Contributor Author

mkmer commented Jul 25, 2023

Thoughts on when we will be ready for a full release? (I'll keep quiet till then :) )

@mkmer mkmer mentioned this pull request Jul 26, 2023
3 tasks
@fronzbot fronzbot mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants