Skip to content

Commit b98f0fd

Browse files
author
Jehan Shah
committed
Fix robot status hang issue with best-effort publishing
- Replace blocking status publishing with non-blocking best-effort approach - Remove complex retry logic and timeouts that could cause indefinite hangs - Add get_state command handler to respond to InOrbit status requests - Simplify implementation: direct client.publish() calls without threading - InOrbit's multi-layer detection system handles status failures gracefully Resolves connector hanging issues by eliminating wait_for_publish() calls that could block indefinitely during network instability.
1 parent 2d2590f commit b98f0fd

3 files changed

Lines changed: 166 additions & 324 deletions

File tree

inorbit_edge/robot.py

Lines changed: 45 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -473,18 +473,22 @@ def _on_connect(self, client, userdata, flags, reason_code, properties):
473473
)
474474
return
475475

476-
# Send robot online status with retries
476+
# Send robot online status (best effort)
477+
# If this fails and InOrbit is getting data, it will detect discrepancy
478+
# and request a status update.
477479
try:
478-
self._retry_operation(
479-
operation=lambda: self.send_robot_status(online=True),
480-
operation_name="Send online status",
481-
max_retries=3,
480+
self.client.publish(
481+
self._get_robot_subtopic(subtopic=MQTT_SUBTOPIC_STATE),
482+
f"1|{self.robot_api_key}|{self.agent_version}|{self.robot_name}",
483+
qos=1,
484+
retain=True,
482485
)
486+
self.logger.debug("Online status sent successfully")
483487
except Exception as e:
484-
self.logger.error(f"Connection failed: unable to send online status: {e}")
485-
# This will cause the connection to be considered failed
486-
# The connector should catch this and restart
487-
raise RuntimeError(f"Connection failed: unable to send online status: {e}")
488+
self.logger.debug(
489+
f"Online status failed: {e} - InOrbit will detect via data messages "
490+
f"and request a status update"
491+
)
488492

489493
# Subscribe to interesting topics
490494
self.client.subscribe(
@@ -627,6 +631,8 @@ def _handle_in_cmd(self, msg):
627631
self._handle_load_module(args[1], args[2])
628632
if args[0] == "unload_module" and len(args) >= 2:
629633
self._handle_unload_module(args[1])
634+
if args[0] == "get_state":
635+
self._handle_get_state()
630636

631637
def _handle_load_module(self, module_name, run_level):
632638
"""Handles a load_module command"""
@@ -638,6 +644,26 @@ def _handle_unload_module(self, module_name):
638644
if module_name == INORBIT_MODULE_CAMERAS:
639645
self._stop_cameras_streaming()
640646

647+
def _handle_get_state(self):
648+
"""Handles a get_state command by re-sending online status.
649+
650+
This is called when InOrbit detects data from a robot marked as offline
651+
and requests the robot to re-send its online status.
652+
"""
653+
self.logger.info("Received request to re-send online status")
654+
655+
# Send online status directly (non-blocking, no threading needed)
656+
try:
657+
self.client.publish(
658+
self._get_robot_subtopic(subtopic=MQTT_SUBTOPIC_STATE),
659+
f"1|{self.robot_api_key}|{self.agent_version}|{self.robot_name}",
660+
qos=1,
661+
retain=True,
662+
)
663+
self.logger.debug("Online status response sent successfully")
664+
except Exception as e:
665+
self.logger.debug(f"Online status response failed: {e}")
666+
641667
def _start_cameras_streaming(self):
642668
"""Start streaming on all registered cameras"""
643669
with self.camera_streaming_mutex:
@@ -929,87 +955,6 @@ def _resend_modules(self):
929955
qos=1,
930956
)
931957

932-
def _retry_operation(self, operation, operation_name, max_retries=3, delay=0.5):
933-
"""Generic retry wrapper with fixed delay - keeps it simple and fast.
934-
935-
Args:
936-
operation: Callable that performs the operation
937-
operation_name: String describing the operation for logging
938-
max_retries: Maximum number of attempts
939-
delay: Delay between retries in seconds
940-
941-
Returns:
942-
Result of the operation if successful
943-
944-
Raises:
945-
RuntimeError: If all retry attempts fail
946-
"""
947-
for attempt in range(max_retries):
948-
try:
949-
result = operation()
950-
if attempt > 0:
951-
self.logger.debug(
952-
f"{operation_name} succeeded on attempt {attempt + 1}"
953-
)
954-
return result
955-
except Exception as e:
956-
if attempt < max_retries - 1:
957-
self.logger.warning(
958-
f"{operation_name} failed "
959-
f"(attempt {attempt + 1}/{max_retries}): {e}. "
960-
f"Retrying in {delay}s..."
961-
)
962-
time.sleep(delay)
963-
else:
964-
self.logger.error(
965-
f"{operation_name} failed after {max_retries} attempts: {e}"
966-
)
967-
raise RuntimeError(
968-
f"{operation_name} failed after {max_retries} attempts: {e}"
969-
)
970-
971-
def send_robot_status(self, online=True):
972-
"""Send robot online/offline status.
973-
974-
This method blocks until either the message
975-
is sent, publish times out, or the client errors out.
976-
977-
Args:
978-
online (bool): True to mark robot as online, False for offline.
979-
Defaults to True.
980-
981-
Raises:
982-
RuntimeError: If status message fails to publish.
983-
"""
984-
robot_status = "1" if online else "0"
985-
986-
# Every time we connect or disconnect to the service, send
987-
# updated status including online/offline bit
988-
status_message = "{}|{}|{}|{}".format(
989-
robot_status, self.robot_api_key, self.agent_version, self.robot_name
990-
)
991-
ret = self.publish(
992-
self._get_robot_subtopic(subtopic=MQTT_SUBTOPIC_STATE),
993-
status_message,
994-
qos=1,
995-
retain=True,
996-
)
997-
self.logger.info("Publishing status {}. ret = {}.".format(robot_status, ret))
998-
999-
try:
1000-
ret.wait_for_publish(timeout=5.0)
1001-
published = ret.is_published()
1002-
except Exception as e:
1003-
self.logger.warning(f"Failed to wait for status publish: {e}")
1004-
published = False
1005-
1006-
self.logger.info(
1007-
"Robot status '{}' published: {:b}.".format(robot_status, published)
1008-
)
1009-
1010-
if not published:
1011-
raise RuntimeError(f"Failed to publish robot status '{robot_status}'")
1012-
1013958
def _is_connected(self):
1014959
return self.client.is_connected()
1015960

@@ -1082,18 +1027,19 @@ def disconnect(self):
10821027
self.logger.info("Ending robot session")
10831028
self._stop_cameras_streaming()
10841029

1085-
# Try to send offline status with retries, but don't fail disconnect
1086-
# Offline status is best effort
1030+
# Send offline status (best effort, non-blocking)
1031+
# InOrbit will detect offline via data absence if this fails
10871032
try:
1088-
self._retry_operation(
1089-
operation=lambda: self.send_robot_status(online=False),
1090-
operation_name="Send offline status",
1091-
max_retries=3,
1033+
self.client.publish(
1034+
self._get_robot_subtopic(subtopic=MQTT_SUBTOPIC_STATE),
1035+
f"0|{self.robot_api_key}|{self.agent_version}|{self.robot_name}",
1036+
qos=1,
1037+
retain=True,
10921038
)
1093-
self.logger.info("Offline status sent successfully")
1039+
self.logger.debug("Offline status sent successfully")
10941040
except Exception as e:
1095-
self.logger.warning(
1096-
f"Failed to send offline status during disconnect: {e}. "
1041+
self.logger.debug(
1042+
f"Offline status failed: {e} - InOrbit will detect via data absence"
10971043
)
10981044

10991045
# TODO: Unsubscribe from topics

inorbit_edge/tests/test_robot_session_callbacks.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,3 +255,40 @@ def test_robot_session_handles_map_requests(
255255
msg.payload = MapRequest(label="map_id", data_hash=123).SerializeToString()
256256
robot_session._on_message(None, None, msg)
257257
robot_session.client._publish_map_bytes.assert_not_called()
258+
259+
260+
def test_responds_to_status_request_from_inorbit(
261+
mock_mqtt_client, mock_inorbit_api, mock_sleep
262+
):
263+
"""Test that robot responds when InOrbit requests status update."""
264+
robot_session = RobotSession(
265+
robot_id="id_123",
266+
robot_name="name_123",
267+
api_key="apikey_123",
268+
)
269+
270+
# connect robot_session, so it populates properties with API response data
271+
robot_session.connect()
272+
robot_session._on_connect(None, None, None, 0, None)
273+
274+
# Clear previous publish calls from connect
275+
robot_session.client.publish.reset_mock()
276+
277+
# Simulate InOrbit requesting status update
278+
get_state_msg = b"get_state"
279+
robot_session._handle_in_cmd(get_state_msg)
280+
281+
# Should respond with current status
282+
robot_session.client.publish.assert_called()
283+
call_args = robot_session.client.publish.call_args
284+
285+
# Verify response format
286+
topic = call_args[0][0]
287+
message = call_args[0][1]
288+
289+
assert "state" in topic
290+
assert message.startswith("1|") # Should report online
291+
assert call_args[1]["qos"] == 1
292+
assert call_args[1]["retain"] is True
293+
294+

0 commit comments

Comments
 (0)