Skip to content

[Humble] Implement Unified Node Interface (NodeInterfaces class) (backport #2041) #3002

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.

Sign up for GitHub

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

Jump to bottom
Merged
ahcorde merged 2 commits into from
Dec 18, 2025
Merged

[Humble] Implement Unified Node Interface (NodeInterfaces class) (backport #2041) #3002

ahcorde merged 2 commits into from
Dec 18, 2025

Conversation

@fabianhirmann
Copy link
Contributor

@fabianhirmann fabianhirmann commented Dec 2, 2025

Description

This is a backport of #2041 to Humble to use the new nice NodeInterfaces class there as well.

Fixes #2309

Is this user-facing behavior change?

No, previously existing behavior is not changed.

Did you use Generative AI?

No.

Additional Information

Interestingly the NodeInterfaces class is also already documented for ROS Humble but has not been backported to Humble yet:
https://docs.ros.org/en/humble/Tutorials/Intermediate/Using-Node-Interfaces-Template-Class.html

alsora
alsora approved these changes Dec 6, 2025
View reviewed changes
Copy link
Collaborator

@alsora alsora left a comment

Choose a reason for hiding this comment

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

Looks good to me.
The PR is made of only additions and no changes to existing classes so it's safe to backport.

@ahcorde
Copy link
Contributor

ahcorde commented Dec 8, 2025

Pulls: #3002
Gist: https://gist.githubusercontent.com/ahcorde/c0e6f29e8411e6cf7f531da6b3b2ac28/raw/4d0f5b268275411fab4ce323121d9c484220cfe6/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp rclcpp_lifecycle
TEST args: --packages-above rclcpp rclcpp_lifecycle
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17709

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

ahcorde
ahcorde requested changes Dec 8, 2025
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I can see here some build errors https://ci.ros2.org/job/ci_linux-rhel/6214/console

@fabianhirmann
Copy link
Contributor Author

fabianhirmann commented Dec 9, 2025

Thank you @ahcorde for your comment! I think I fixed the issue now. I am still wondering that the same issue did not appear locally on my system (Ubuntu 22.04) as well as at the PR on the rolling branch but I believe it must be because of different compilers or compiler options. In any case, please have another look on it!

@ahcorde
Copy link
Contributor

ahcorde commented Dec 9, 2025 •
edited
Loading

Pulls: #3002
Gist: https://gist.githubusercontent.com/ahcorde/95e3be6be08b5dc37b6e5ad52653ef7c/raw/4d0f5b268275411fab4ce323121d9c484220cfe6/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp rclcpp_lifecycle
TEST args: --packages-above rclcpp rclcpp_lifecycle
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17720

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

@fabianhirmann fabianhirmann mentioned this pull request Dec 9, 2025
Merged
@ahcorde
Copy link
Contributor

ahcorde commented Dec 9, 2025

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

@fabianhirmann
Copy link
Contributor Author

fabianhirmann commented Dec 17, 2025

Hi @ahcorde!

Is there something blocking this PR from getting merged? Or is there something else I can do to support the progress of this PR?

Thanks,
Fabian

ahcorde
ahcorde requested changes Dec 17, 2025
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Hi @ahcorde!

Is there something blocking this PR from getting merged? Or is there something else I can do to support the progress of this PR?

Thanks, Fabian

There are some consistent tests failures, can you check if these tests are related with this PR ?

@jmachowinski
Copy link
Collaborator

jmachowinski commented Dec 17, 2025

This seems to miss the changes done recently to the signal handling. A rebase should fix the tests.

methylDragon and others added 2 commits December 17, 2025 11:42
@methylDragon @wjwwood @fabianhirmann
Implement Unified Node Interface (NodeInterfaces class) (ros2#2041)
98f8934 
Co-authored-by: William Woodall <[email protected]>
Co-authored-by: methylDragon <[email protected]>
Signed-off-by: Fabian Hirmann <[email protected]>
@fabianhirmann
node interfaces: fix unintentionally invoked copy constructor by expl…
9d97c24 
…icitly getting a reference using a lambda instead of a conditional operator where the type is NodeT instead of NodeT &

Signed-off-by: Fabian Hirmann <[email protected]>
@fabianhirmann fabianhirmann force-pushed the feature/unified-node-interface branch from b41fe0c to 9d97c24 Compare December 17, 2025 10:43
@fabianhirmann
Copy link
Contributor Author

fabianhirmann commented Dec 17, 2025

This seems to miss the changes done recently to the signal handling. A rebase should fix the tests.

Thank you @jmachowinski! I rebased my branch now. @ahcorde Could you trigger another build? Thanks!

In the meanwhile I also checked the failed tests and the comment of @jmachowinski makes sense. The following tests failed:

  • 21 - test_sigint_handler_before_shutdown__rmw_connextdds
  • 23 - test_sigterm_handler_before_shutdown__rmw_connextdds
  • 48 - test_sigint_handler_before_shutdown__rmw_cyclonedds_cpp
  • 50 - test_sigterm_handler_before_shutdown__rmw_cyclonedds_cpp
  • 75 - test_sigint_handler_before_shutdown__rmw_fastrtps_cpp
  • 77 - test_sigterm_handler_before_shutdown__rmw_fastrtps_cpp

@ahcorde
Copy link
Contributor

ahcorde commented Dec 17, 2025 •
edited
Loading

Pulls: #3002
Gist: https://gist.githubusercontent.com/ahcorde/fb9a0932af7680b9af0c08d494aa901d/raw/4d0f5b268275411fab4ce323121d9c484220cfe6/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp rclcpp_lifecycle
TEST args: --packages-above rclcpp rclcpp_lifecycle
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17818

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

@fabianhirmann
Copy link
Contributor Author

fabianhirmann commented Dec 18, 2025

Pulls: #3002 Gist: https://gist.githubusercontent.com/ahcorde/fb9a0932af7680b9af0c08d494aa901d/raw/4d0f5b268275411fab4ce323121d9c484220cfe6/ros2.repos BUILD args: --packages-above-and-dependencies rclcpp rclcpp_lifecycle TEST args: --packages-above rclcpp rclcpp_lifecycle ROS Distro: humble Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17818
* Linux Build Status
* Linux-aarch64 Build Status
* Linux-rhel Build Status
* Windows Build Status

Thank you @ahcorde for running the build again! I see that linux-rhel is still unstable but I do not see any relation to my PR.

The errors at linux-rhel are in the following packages where I believe in both cases it is because RHEL has an older version of python (3.6 compared to 3.10 on Ubuntu 22.04):

  • message_filters
  • test_launch_ros
    • yaml.FullLoader does not exist
    • This code is already there for quite a while so I am not sure if this ever worked on RHEL to be honest
    • The exact error is:
      ________________ TestNode.test_launch_node_with_parameter_dict _________________
      15:46:04 test/test_launch_ros/actions/test_node.py:229: in test_launch_node_with_parameter_dict
      15:46:04 expanded_parameters_dict = yaml.load(h, Loader=yaml.FullLoader)
      15:46:04 E AttributeError: module 'yaml' has no attribute 'FullLoader'

@ahcorde ahcorde merged commit 4e834fa into ros2:humble Dec 18, 2025
3 checks passed
@fabianhirmann fabianhirmann mentioned this pull request Dec 18, 2025
Closed
This was referenced Dec 18, 2025
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahcorde ahcorde ahcorde approved these changes

@alsora alsora alsora approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fabianhirmann @ahcorde @jmachowinski @alsora @methylDragon