[topic_tools] Abstract class to implement connection based transport

[wkentaro]

No description provided.

[wkentaro]

ConnectionBased means the node does not subscribe unless there are any child subscribers of that node.

[dirk-thomas]

I don't understand what you mean with:

ConnectionBased means the node does not subscribe unless there are any child subscribers of that node.

Please provide a detailed description what the use case is you are trying to address, the expected behavior of the provided patch and why this should be added in the ROS core (instead of being handled in user land code).

[wkentaro]

ConnectionBased is the same function as lazy of throttle in topic_tools. https://github.com/ros/ros_comm/blob/indigo-devel/tools/topic_tools/src/throttle.cpp#L83-L88
It subscribes and process something only when there is at least a subscriber. This behavior is good for conserve the cpu and memory in particular when the node is doing image processing.
At this time, I think each author of each node uses event listener of connection/disconnection and subscribe/unsubscribe.
like

• https://github.com/ros/ros_comm/blob/indigo-devel/tools/topic_tools/src/throttle.cpp#L86
• https://github.com/ros/ros_comm/blob/indigo-devel/tools/topic_tools/src/throttle.cpp#L117
• https://github.com/ros-perception/perception_pcl/blob/jade-devel/pcl_ros/src/pcl_ros/filters/filter.cpp#L179
• https://github.com/ros-perception/vision_opencv/blob/indigo/opencv_apps/src/nodelet/lk_flow_nodelet.cpp#L286-L291
• https://github.com/ros-perception/image_pipeline/blob/indigo/image_rotate/src/nodelet/image_rotate_nodelet.cpp#L238-L243

This seems a common practice enough to create an abstract class for it, that's why I think this should be included in topic_tools.
We have also c++ version of this kind of class in my lab package.
https://github.com/jsk-ros-pkg/jsk_common/blob/master/jsk_topic_tools/src/connection_based_nodelet.cpp#L74

[dirk-thomas]

The patch makes use of sensor_msgs within topic_tools. I don't think that is a viable dependency for this core repository.

Also topic_tools does currently not depend on six and I am not convinced it is necessary for this PR.

The remaining class ConnectionBasedTransport seems to be overly complicated for the desired task. E.g. _ever_subscribed and _connection_status can be easily modeled as one variable with the following value:

• None: never been subscribed
• False: currently not subscribed but has been subscribed before
• True: currently subscribed

The class embeds decisions like getting all the configuration option from ros parameters. It is not possible to e.g. pass them as argument.

Regarding the naming if the class: when talking about transport connection-based as a clearly defined meaning. Therefore I think the current name is confusing - something with lazy seems to be more appropriate.

Based on the above comments I am not sure if this should be integrated into topic_tools. It might be better to place it into a separate package which still allow the code to be reused.

[dirk-thomas]

@ros-pull-request-builder retest this please

[wkentaro]

I will change the commit like below, is these not enough to justify this to be merged?

• Use std_msgs instead of sensor_msgs as the example.
• Rename the class to be more common name (lazy)
• Stop using enum and simplify the flag
• Add python-six as run_depend (dependency on six is removed)

[dirk-thomas]

I don't think a run dependency on Python six should be added since this code has been released for a very long time without. Can you update the code to not require six?

The other three options would be great.

[dirk-thomas]

Thank you for the patch and iterating on this. I have cherry-picked the changes to the kinetic-devel branch: f8f0d38

[wkentaro]

Thank you!.