Skip to content

Conversation

@maciejmajek
Copy link
Member

@maciejmajek maciejmajek commented Apr 1, 2025

Purpose

Sometimes, destroying the subscribers lead to crash of the node/executor. An effort has ben made to allow persistent subscribers.

Proposed Changes

Enhances the logic behind handling destroy_subscribers in ROS2TopicAPI
ROS2ARIConnector docstring

Issues

  • Links to relevant issues

Testing

from rai.communication.ros2 import ROS2ARIConnector
from rai.tools.ros2 import ReceiveROS2MessageTool
import rclpy
import time
import subprocess

rclpy.init()

proc = subprocess.Popen(["ros2", "topic", "pub", "topic", "std_msgs/String", f"data: 'Hello {time.time()}'"])

connector = ROS2ARIConnector(destroy_subscribers=True)
tool = ReceiveROS2MessageTool(connector=connector)
try:
    for i in range(10):
        msg = tool.run(tool_input={"topic": "topic", "timeout_sec": 2.0})
        print(msg)
finally:
    connector.shutdown()
    rclpy.shutdown()
    proc.terminate()

@maciejmajek maciejmajek requested a review from rachwalk April 1, 2025 19:07
Copy link
Collaborator

@rachwalk rachwalk left a comment

Choose a reason for hiding this comment

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

LGTM and makes sense - good job; I have some comments regarding docstring for ARIConnector, merge after applying them.

Whether to destroy subscribers after receiving a message, by default False.
If True, the subscriber will be destroyed after the message is received.
If False, may lead to performance issues due to unneded subscribers.
It has been observed that destroying subscribers may lead to a crash of the node/executor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line should be moved to notes and expanded upon. "It has been observed.... may", do you know anything more about this behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

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

Expanded note

Comment on lines 59 to 74
_node : Node
The ROS2 node instance.
_topic_api : ROS2TopicAPI
API for handling ROS2 topic operations.
_service_api : ROS2ServiceAPI
API for handling ROS2 service operations.
_actions_api : ROS2ActionAPI
API for handling ROS2 action operations.
_tf_buffer : Buffer
Buffer for storing TF transforms.
tf_listener : TransformListener
Listener for TF transforms.
_executor : MultiThreadedExecutor
ROS2 executor for running the node.
_thread : Thread
Thread running the executor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_node : Node
The ROS2 node instance.
_topic_api : ROS2TopicAPI
API for handling ROS2 topic operations.
_service_api : ROS2ServiceAPI
API for handling ROS2 service operations.
_actions_api : ROS2ActionAPI
API for handling ROS2 action operations.
_tf_buffer : Buffer
Buffer for storing TF transforms.
tf_listener : TransformListener
Listener for TF transforms.
_executor : MultiThreadedExecutor
ROS2 executor for running the node.
_thread : Thread
Thread running the executor.
tf_listener : TransformListener
Listener for TF transforms.

Protected and private Attributes should not be listed in the docstring documenting the classes public API. If any of these should become part of the public API please remove the _ prefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed tf_listener, to _tf_listener, as there is no need to expose the listener.
removed Attributes section of the docstring

@maciejmajek maciejmajek merged commit 0675ff6 into development Apr 2, 2025
5 checks passed
@maciejmajek maciejmajek deleted the feat/handle-destroy-subscribers branch April 2, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants