Julius2342 / pyvlx

PyVLX - controling VELUX windows with Python via KLF 200
GNU Lesser General Public License v3.0
74 stars 26 forks source link

Uses a Semaphore to limit API calls to one at a time to avoid KLF issues #353

Closed palfrey closed 8 months ago

palfrey commented 9 months ago

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.

codecov-commenter commented 9 months ago

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (936e23b) 81.96% compared to head (d25c6de) 81.90%.

Files Patch % Lines
pyvlx/api/api_event.py 0.00% 8 Missing :warning:
pyvlx/pyvlx.py 75.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pawlizio commented 9 months ago

This looks different to what I'm trying to merge with https://github.com/Julius2342/pyvlx/pull/350. My fork already has the Semaphore feature. I meantioned that here: https://github.com/Julius2342/pyvlx/issues/331#issuecomment-1825176957.

Julius2342 commented 9 months ago

We should find the best approach :)

both introduce an asyncio.Semaphore member within PyVLX . I'am not 100% convinced about the name parallel_commands - bc, yes, this is the intention, but it does not actually express what it is doing. I would have named the member semaphore though.

The other question is, where where we should make use of the Semaphore: CommandSend.send() (function for sending frames to the bus) or ApiEvent.do_api_call() (Baseclass of all frames).

I think it is more explicit to do this within the sender (so do_api_call) and less hidden, but im open to other opinions?

Julius2342 commented 9 months ago

And while reading my old own code: You can clearly see a C++ guy doing Python things ...

palfrey commented 9 months ago

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).

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.

palfrey commented 9 months ago

both introduce an asyncio.Semaphore member within PyVLX . I'am not 100% convinced about the name parallel_commands - bc, yes, this is the intention, but it does not actually express what it is doing. I would have named the member semaphore though.

I've renamed it to api_call_semaphore as I think that gives the clearest indication of what it does to my mind.

The other question is, where where we should make use of the Semaphore: CommandSend.send() (function for sending frames to the bus) or ApiEvent.do_api_call() (Baseclass of all frames).

I think it is more explicit to do this within the sender (so do_api_call) and less hidden, but im open to other opinions?

do_api_call also nicely wraps both send and receive. Wrapping CommandSend doesn't capture all the sends e.g. GetNodeInformation, ActivateScene, etc, etc

pawlizio commented 9 months ago

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).

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.

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. Finally I would appreciate if we could merge them soon so that I could switch my custom_component back to official pyvlx. Next step would be then, bringing my custom_component into HA default velux integration as it provides much more features.

Julius2342 commented 9 months ago

| Wrapping CommandSend doesn't capture all the sends e.g. GetNodeInformation, ActivateScene, etc, etc

convinces me. @pawlizio : fine for you too?

pawlizio commented 9 months ago

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.

Julius2342 commented 8 months ago

ty!