-
Notifications
You must be signed in to change notification settings - Fork 26
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
Uses a Semaphore to limit API calls to one at a time to avoid KLF issues #353
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
==========================================
- Coverage 81.96% 81.90% -0.07%
==========================================
Files 77 77
Lines 3388 3393 +5
==========================================
+ Hits 2777 2779 +2
- Misses 611 614 +3 ☔ View full report in Codecov by Sentry. |
This looks different to what I'm trying to merge with #350. My fork already has the Semaphore feature. I meantioned that here: #331 (comment). |
We should find the best approach :) both introduce an The other question is, where where we should make use of the Semaphore: I think it is more explicit to do this within the sender (so |
And while reading my old own code: You can clearly see a C++ guy doing Python things ... |
I'll note that #350 also has a lot of other stuff there, which will make review of that one hard. This PR even if it duplicates work might be a good one to merge first just to reduce the volume there. |
I've renamed it to
|
# We check for connection before entering the semaphore section | ||
# because otherwise we might try to connect, which calls this, and we get stuck on | ||
# the semaphore. | ||
await self.pyvlx.check_connected() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you exclude check_connected() from Semaphore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less a matter of excluding, more a matter of "do before". So, inside check_connected
is sometimes (if not connected yet) a call to connect
, which calls self.klf200.password_enter
which calls do_api_call
...
Without checking for connection before, the first call to do_api_call
(a GetAllNodesInformation
typically) gets stuck because the semaphore has been activated, but it can't connect.
The magic that makes this not loop entirely is that the first thing connect
does is call await self.connection.connect()
so when check_connected
gets called as part of the password_enter
do_api_call
, it doesn't call connect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, you'right, I don't mean exclude, but before Semaphore. Especially, why not checking connection "inside" Semaphore?
What happen if you receive a burst of commands from HA while the pyvlx is not connected? Don't you trigger the connect function several times? Why not making this check within the Semaphore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while but now I got it... If the check_connected would be inside Semaphore (i.e. during command send) and in case it needs to connect first, the do_api_call
from await self.connection.connect()
would be locked by Semaphore as the current command send is not returned.
...let's merge this PR.
On the other hand, I just try to merge my fork as it is used by my velux custom_component and over the time lots of users contributed with several PRs there. |
| Wrapping CommandSend doesn't capture all the sends e.g. GetNodeInformation, ActivateScene, etc, etc convinces me. @pawlizio : fine for you too? |
Regarding Semaphore within do_api_call yes, I considered this also already in my PR. However I'm not yet convinced on the comment for check_connected() before Semaphore. |
ty! |
Fixes #331. I've tested this a few times with a set of 6 blinds, and if I do
node.close()
for all of them in parallel without this, I get roughly half closing (sometimes 2, sometimes 4-5). With this change they appear pretty stable all 6 happening. They don't quite run all at the same time, there's a bit of a gap between them (1s maybe?), but that's a lot better than them not closing.