Skip to content
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.

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

support lifecycle nodes #167

Closed

Conversation

Karsten1987
Copy link
Contributor

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.

Signed-off-by: Karsten Knese <[email protected]>
@flynneva
Copy link

@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
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor

Rayman commented May 17, 2021

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

@Karsten1987
Copy link
Contributor Author

@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
Copy link

wodtko commented Oct 18, 2021

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

@audrow audrow changed the base branch from ros2 to rolling June 28, 2022 14:15
@ilidar ilidar mentioned this pull request Nov 5, 2022
@saching13
Copy link

is it still stalled ?

@ahcorde ahcorde added the ros2 Issues or Pull Requests targeting ROS2 label Jan 18, 2024
@ahcorde
Copy link
Collaborator

ahcorde commented Jan 18, 2024

As @Karsten1987 commented here #167 (comment) 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ros2 Issues or Pull Requests targeting ROS2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants