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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Nov 27, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2296681
Uses a Semaphore to limit API calls to one at a time to avoid KLF issues
palfrey Nov 25, 2023
6140e5b
Fix pydocstyle issues
palfrey Nov 25, 2023
82639d7
And fix the flake8 spacing issues
palfrey Nov 25, 2023
d25c6de
Rename semaphore to api_call_semaphore
palfrey Nov 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions pyvlx/api/api_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,18 @@ def __init__(self, pyvlx: "PyVLX", timeout_in_seconds: int = 10):

async def do_api_call(self) -> None:
"""Start. Sending and waiting for answer."""
self.pyvlx.connection.register_frame_received_cb(self.response_rec_callback)
await self.send_frame()
await self.start_timeout()
await self.response_received_or_timeout.wait()
await self.stop_timeout()
self.pyvlx.connection.unregister_frame_received_cb(self.response_rec_callback)
# 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()
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator

@pawlizio pawlizio Nov 27, 2023 •
edited
Loading

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.


async with self.pyvlx.api_call_semaphore:
self.pyvlx.connection.register_frame_received_cb(self.response_rec_callback)
await self.send_frame()
await self.start_timeout()
await self.response_received_or_timeout.wait()
await self.stop_timeout()
self.pyvlx.connection.unregister_frame_received_cb(self.response_rec_callback)

async def handle_frame(self, frame: FrameBase) -> bool:
"""Handle incoming API frame, return True if this was the expected frame."""
Expand Down
2 changes: 1 addition & 1 deletion pyvlx/api/get_all_nodes_information.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@


class GetAllNodesInformation(ApiEvent):
"""Class for retrieving node informationfrom API."""
"""Class for retrieving node information from API."""

def __init__(self, pyvlx: "PyVLX"):
"""Initialize SceneList class."""
Expand Down
9 changes: 7 additions & 2 deletions pyvlx/pyvlx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def __init__(self,
self.version = None
self.protocol_version = None
self.klf200 = Klf200Gateway(pyvlx=self)
self.api_call_semaphore = asyncio.Semaphore(1) # Limit parallel commands

async def connect(self) -> None:
"""Connect to KLF 200."""
Expand All @@ -67,10 +68,14 @@ async def reboot_gateway(self) -> None:
PYVLXLOG.warning("KLF 200 reboot initiated")
await self.klf200.reboot()

async def send_frame(self, frame: FrameBase) -> None:
"""Send frame to API via connection."""
async def check_connected(self) -> None:
"""Check we're connected, and if not, connect."""
if not self.connection.connected:
await self.connect()

async def send_frame(self, frame: FrameBase) -> None:
"""Send frame to API via connection."""
await self.check_connected()
self.connection.write(frame)

async def disconnect(self) -> None:
Expand Down
Loading