Skip to content

Simplify NodeInterface API mehotd call#129

Merged
ahcorde merged 5 commits intorollingfrom
ahcorde/rolling/simplify_api
Jul 17, 2025
Merged

Simplify NodeInterface API mehotd call#129
ahcorde merged 5 commits intorollingfrom
ahcorde/rolling/simplify_api

Conversation

@ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Jul 2, 2025

I simplify the std::shared_ptr NodeInterfaces to just the type NodeInterfaces. The motivation is to simplify how the API call is done:

auto node_interfaces = std::make_shared<rclcpp::node_interfaces::NodeInterfaces<
               rclcpp::node_interfaces::NodeBaseInterface,
               rclcpp::node_interfaces::NodeParametersInterface,
               rclcpp::node_interfaces::NodeTopicsInterface,
               rclcpp::node_interfaces::NodeLoggingInterface
             >>(
      node->get_node_base_interface(),
      node->get_node_parameters_interface(),
      node->get_node_topics_interface(),
      node->get_node_logging_interface()
             );
     
      PointCloudTransport(node_interfaces, ...);

now with these changes it's much more simpler:

     auto pct = PointCloudTransport(*node, ...);

FYI @elsayedelsheikh @SteveMacenski

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Jul 2, 2025
@ahcorde ahcorde mentioned this pull request Jul 2, 2025
@ahcorde
Copy link
Collaborator Author

ahcorde commented Jul 3, 2025

If you have some time available I appreciate your reviews @elsayedelsheikh @SteveMacenski

POINT_CLOUD_TRANSPORT_PUBLIC
Publisher create_publisher(
std::shared_ptr<rclcpp::node_interfaces::NodeInterfaces<
rclcpp::node_interfaces::NodeInterfaces<
Copy link
Member

@SteveMacenski SteveMacenski Jul 7, 2025

Choose a reason for hiding this comment

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

Suggestion: Given the repetition of this - you could typedef it to make this more readable. Though, I think also keeping it as-is is useful as a reference for later readers.

Its a trade off between accessibility to general ROS developers reading this and code cleanliness.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to keep it as it is to avoid possible name conflicts later on as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will also point out that this is changing a public interface, so we usually do a tick-tock deprecation (same goes for all of the public APIs below).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These APIs were added on rolling, I know I already released the package but I would claim that It was unestable.

{
advertiseImpl(
node_interfaces, base_topic,
*node_interfaces.get(), base_topic,
Copy link
Member

@SteveMacenski SteveMacenski Jul 7, 2025

Choose a reason for hiding this comment

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

Do we really want to bypass the reference counter here and similar places below?

Copy link
Member

Choose a reason for hiding this comment

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

@ahcorde there are a number of places this is done, I only pointed out one instance

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ups, I will fix it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from SteveMacenski July 8, 2025 12:20
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove node_interfaces declaration lines from 84:88

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from elsayedelsheikh July 9, 2025 08:33
Copy link
Contributor

@elsayedelsheikh elsayedelsheikh left a comment

Choose a reason for hiding this comment

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

LGTM!

@ahcorde
Copy link
Collaborator Author

ahcorde commented Jul 9, 2025

Pulls: #129, ros2/rviz#1526
Gist: https://gist.githubusercontent.com/ahcorde/01fb470b92dc6745e1b6f3c96c7c5b2e/raw/ad40e851de49cd6204a29730daefc24eb8212ff2/ros2.repos
BUILD args: --packages-above-and-dependencies point_cloud_transport
TEST args: --packages-above point_cloud_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16439

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit bf9dcd8 into rolling Jul 17, 2025
4 checks passed
@ahcorde ahcorde deleted the ahcorde/rolling/simplify_api branch July 17, 2025 07:54
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.

4 participants