Skip to content

Conversation

ottojo
Copy link
Contributor

@ottojo ottojo commented Jul 14, 2025

This adds constructors using NodeInterfaces to TwistSubscriber. This allows use outside of nav2 with non-lifecycle nodes. It would also allow use in places where NodeInterfaces are already used, or places where a node handle is available in any other form than shared_ptr<LifecycleNode>.


Basic Info

Info Please fill out this column
Ticket(s) this addresses (none)
Primary OS tested on Ubuntu 24.04
Robotic platform tested on TBD
Does this PR contain AI generated software? No
Was this PR description generated by AI software? No

Description of contribution in a few bullet points

  • Adds constructors using NodeInterfaces to TwistSubscriber
  • Adds NodeInterfaces version of declare_parameter_if_not_declared (used in ctor)
  • Change all uses to use new constructor, requiring passing the node by reference instead of shared_ptr. I would like to revert that if and when i find a way to make the new and old constructors non-ambiguous. I'm not quite sure why they are, as far as i can tell, only ever one is applicable in any situation.

Description of documentation updates required from your changes

  • TODO: Check for any doxygen that needs updating

Description of how this change was tested

  • TODO: Add appropriate unit tests if both constructors are kept.
  • TODO: Add unit test for stamped-only constructor, i think there are none currently.

Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists
  • Should this be backported to current distributions? If so, tag with backport-*.

@ottojo ottojo force-pushed the twist_subscriber_nodeinterfaces branch from a790049 to f0b0be6 Compare July 14, 2025 16:20
Copy link
Contributor

mergify bot commented Jul 14, 2025

@ottojo, all pull requests must be targeted towards the main development branch.
Once merged into main, it is possible to backport to @jazzy, but it must be in main
to have these changes reflected into new distributions.

Copy link

codecov bot commented Jul 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
nav2_behaviors/plugins/assisted_teleop.cpp 90.24% <ø> (ø)
...2_collision_monitor/src/collision_monitor_node.cpp 97.09% <ø> (-0.02%) ⬇️
nav2_util/include/nav2_util/node_utils.hpp 93.75% <ø> (ø)
nav2_util/include/nav2_util/twist_subscriber.hpp 100.00% <100.00%> (ø)
nav2_util/src/node_utils.cpp 83.87% <100.00%> (+1.72%) ⬆️
nav2_velocity_smoother/src/velocity_smoother.cpp 94.39% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

@ottojo heed the cpplint failing job

return output;
}

void declare_parameter_if_not_declared(
Copy link
Member

Choose a reason for hiding this comment

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

Please put this in the new nav2_ros_common package where the other parameter utilities are relocated

qos,
std::forward<TwistStampedCallbackT>(TwistStampedCallback));
twist_stamped_sub_ =
rclcpp::create_subscription<geometry_msgs::msg::TwistStamped>(
Copy link
Member

@SteveMacenski SteveMacenski Jul 14, 2025

Choose a reason for hiding this comment

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

Must use node-> per https://docs.nav2.org/migration/Kilted.html#new-nav2-ros-common-nav2-lifecycle-node or nav2_ros_common::interfaces::create_subscription

@SteveMacenski
Copy link
Member

@ottojo these comments are more aligned with the main version of these rather than Jazzy. What's your intent here for the future? Some of the changes cannot be forward ported past Kilted, so perhaps its worth looking at solutions now while working on this to get these changes in main as well so that you can have them in the future as well automatically.

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.

2 participants