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

Check if connection is running before connecting. #296

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Check if connection is running before connecting. #296

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c0dcbd
Check if connection is running before connecting.
ilar Apr 2, 2024
b957a66
Merge branch 'main' into fix-non-continuous
ilar May 25, 2024
db0692f
Merge branch 'main' into fix-non-continuous
ilar Jun 8, 2024
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
5 changes: 3 additions & 2 deletions roombapy/roomba.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,11 @@ def periodic_connection(self) -> None:
# only one connection thread at a time!
if self.periodic_connection_running:
return
self.periodic_connection_running = True
while not self.stop_connection:
try:
self._connect()
if not self.periodic_connection_running:
self._connect()
self.periodic_connection_running = True
Comment on lines -161 to +165
Copy link

Choose a reason for hiding this comment

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

Are you sure this does what you think it does? This looks like a mutex to me and with this you're defeating the purpose of the mutex?

Copy link
Author

Choose a reason for hiding this comment

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

self.periodic_connection_running is just the current state of whether or not we have a current active connection.

self.stop_connection is the mutex-like that actually controls whether or not the connection SHOULD be stopped.

If stop_connection == False and periodic_connection_running == False, we need to restart the connection- anything else we just let pass through, because we're either already connected and should be, or stopping the connection and falling out of the while loop.

Copy link

@jan-xyz jan-xyz May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm it doesn't look like this. self.periodic_connection_running is just prohibiting multiple while-loops from running. With the change it will be possible to start it multiple times.

Copy link
Author

Choose a reason for hiding this comment

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

Why would multiple while loops be running? That would require calling periodic_connection multiple times (which would raise a runtime error, as you can only start() a thread no more than once).

Even if we did call it more than once, periodic_connection still has the guard against that on line 159.

Finally, a key takeaway here is that we're not running _connect() from periodic_connection any more than we already were- we're running it less, as we're not running it where we would have before- that is, when we already have a connection, which is the entire cause of the issue people have with 'periodic mode' constantly reconnecting. That's the only real change here.

except RoombaConnectionError as error:
self.periodic_connection_running = False
self.on_disconnect(MQTT_ERROR_MESSAGES[7])
Expand Down