ros-perception / image_common

Common code for working with images in ROS
http://www.ros.org/wiki/image_common
129 stars 223 forks source link

support lifecycle nodes #167

Closed Karsten1987 closed 9 months ago

Karsten1987 commented 4 years ago

closes #108

Currently still in draft mode. Call for participation.

The current architecture of image transport is pretty complex for easily integrating lifecycle nodes as rclcpp::Nodes are passed along pretty heavily. Hence, all functions are basically overloaded to deal with their individual node interfaces to avoid templating.

flynneva commented 3 years ago

@Karsten1987 this might be a bit beyond my depth but i wouldn't mind trying to help out here. it looks like you had a direction you were going and just need someone to implement. more than happy to help if you point me in the right direction

Karsten1987 commented 3 years ago

@flynneva thanks for picking this up.

The basic idea here is as follows: Right now, the package is receiving an externally initiated rclcpp::Node in its constructor rather then itself being a node, e.g:

  IMAGE_TRANSPORT_PUBLIC
  CameraPublisher(
    rclcpp::Node * node,
    const std::string & base_topic,
    rmw_qos_profile_t custom_qos = rmw_qos_profile_default);

While this makes quite a bit of sense, it prohibits though to initialize it with a rclcpp_lifecycle::LifecycleNode. One way to work around this is to template the constructor to accept any type of node and essentially work with their NodeInterfaces.

  IMAGE_TRANSPORT_PUBLIC
  CameraPublisher(
    rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_interface,
    rclcpp::node_interfaces::NodeTopicsInterface::SharedPtr node_topics_interface,
    rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_interface,
    const std::string & base_topic,
    rmw_qos_profile_t custom_qos = rmw_qos_profile_default);

  template<class NodeT>
  CameraPublisher(
    NodeT && node,
    const std::string & base_topic,
    rmw_qos_profile_t custom_qos = rmw_qos_profile_default)
  : CameraPublisher(
      node->get_node_base_interface(),
      node->get_node_topics_interface(),
      node->get_node_logging_interface(),
      base_topic, custom_qos)

That's basically what I started here. It's a bit tedious do to do and touches quite a bit of files along the way.

flynneva commented 3 years ago

@Karsten1987 sounds great and thank you for the more in-depth information. I had already started to work towards this and will pick it up later this week. it does seem like a lot of work but I think should be done in order to make these tools usable for anyone who wants to use lifecycle nodes.

my plan right now is to first help out with the uncrustify issues for the ros2 branch...then pick this topic up. hopefully later this week you'll see some more contributions from me here 😄

Rayman commented 3 years ago

I have added lifecycle support to CameraInfoManager. Should I create a new PR or make a PR against this one?

Karsten1987 commented 3 years ago

@Rayman I won't really work on this in the near future. It might be easier for you to open a new PR - incorporate my existing commits from this PR if helpful - and I'll close out this PR.

wodtko commented 3 years ago

Is there any progress on this topic? I would be very interested in the LifecycleNode Support, and the approach looks promising.

saching13 commented 1 year ago

is it still stalled ?

ahcorde commented 9 months ago

As @Karsten1987 commented here https://github.com/ros-perception/image_common/pull/167#issuecomment-842449965 he will not put more effort in this PR. If someone want to reopen a new PR feel free to do it. Happy to review it