Skip to content

feat: Add pinging to local client #627

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

Merged
Lash-L merged 3 commits into from
Dec 4, 2025
Merged

feat: Add pinging to local client #627

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c831705
feat: Add pinging to local client
Lash-L Dec 4, 2025
e562ac6
fix: reset keep_alive_task to None
Lash-L Dec 4, 2025
c002c36
chore: copilot test
Copilot Dec 4, 2025
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
29 changes: 29 additions & 0 deletions roborock/devices/local_channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
_LOGGER = logging.getLogger(__name__)
_PORT = 58867
_TIMEOUT = 5.0
_PING_INTERVAL = 10
Copy link
Copy Markdown
Contributor

@allenporter allenporter Dec 4, 2025

Choose a reason for hiding this comment

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

Can you use a timedelta here to be more explicit on the unit?

Suggested change
_PING_INTERVAL = 10
_PING_INTERVAL = datetime.timedelta(seconds=10)



@dataclass
Expand Down Expand Up @@ -58,6 +59,7 @@ def __init__(self, host: str, local_key: str):
self._subscribers: CallbackList[RoborockMessage] = CallbackList(_LOGGER)
self._is_connected = False
self._local_protocol_version: LocalProtocolVersion | None = None
self._keep_alive_task: asyncio.Task[None] | None = None
self._update_encoder_decoder(
LocalChannelParams(local_key=local_key, connect_nonce=get_next_int(10000, 32767), ack_nonce=None)
)
Expand Down Expand Up @@ -132,6 +134,28 @@ async def _hello(self):

raise RoborockException("Failed to connect to device with any known protocol")

async def _ping(self) -> None:
ping_message = RoborockMessage(
protocol=RoborockMessageProtocol.PING_REQUEST, version=self.protocol_version.encode()
)
await self._send_message(
roborock_message=ping_message,
request_id=ping_message.seq,
response_protocol=RoborockMessageProtocol.PING_RESPONSE,
)

async def _keep_alive_loop(self) -> None:
while self._is_connected:
try:
await asyncio.sleep(_PING_INTERVAL)
Copy link
Copy Markdown
Contributor

@allenporter allenporter Dec 4, 2025

Choose a reason for hiding this comment

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

Assuming you accept the above suggestion

Suggested change
await asyncio.sleep(_PING_INTERVAL)
await asyncio.sleep(_PING_INTERVAL.total_seconds())

if self._is_connected:
await self._ping()
except asyncio.CancelledError:
break
except Exception:
Copy link
Copy Markdown
Contributor

@allenporter allenporter Dec 4, 2025

Choose a reason for hiding this comment

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

Let's catch RoborockException (expected) separate from uncaught exceptions so we can log them separately e.g. "ping failed" vs "Uncaught exception" implying a bug/shouldn't happen case we need to fix. Similar to _background_reconnect in v1 channel?

_LOGGER.debug("Keep-alive ping failed", exc_info=True)
# Retry next interval
Comment on lines +147 to +157
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The new keep-alive functionality lacks test coverage. Consider adding tests to verify:

  1. That _keep_alive_task is created when connect() is called
  2. That the task is properly canceled when close() or _connection_lost() is called
  3. That the ping loop continues to execute periodically while connected
  4. That exceptions in the ping loop are handled gracefully without stopping the loop

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@Lash-L Lash-L Dec 4, 2025

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback


@property
def protocol_version(self) -> LocalProtocolVersion:
"""Return the negotiated local protocol version, or a sensible default."""
Expand Down Expand Up @@ -166,6 +190,7 @@ async def connect(self) -> None:
# Perform protocol negotiation
try:
await self._hello()
self._keep_alive_task = asyncio.create_task(self._keep_alive_loop())
Copy link
Copy Markdown
Contributor

@allenporter allenporter Dec 4, 2025

Choose a reason for hiding this comment

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

Move this outside the scope of the try/catch. It won't throw RoborockException

except RoborockException:
# If protocol negotiation fails, clean up the connection state
self.close()
Expand All @@ -177,6 +202,8 @@ def _data_received(self, data: bytes) -> None:

def close(self) -> None:
"""Disconnect from the device."""
if self._keep_alive_task:
self._keep_alive_task.cancel()
Comment thread
Lash-L marked this conversation as resolved.
if self._transport:
self._transport.close()
else:
Expand All @@ -187,6 +214,8 @@ def close(self) -> None:
def _connection_lost(self, exc: Exception | None) -> None:
"""Handle connection loss."""
_LOGGER.warning("Connection lost to %s", self._host, exc_info=exc)
if self._keep_alive_task:
self._keep_alive_task.cancel()
Comment thread
Lash-L marked this conversation as resolved.
self._transport = None
self._is_connected = False

Expand Down
Loading