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

feat(controller-server): publish_zero_velocity parameter jazzy backport #4687

Merged

Conversation

reinzor
Copy link
Contributor

@reinzor reinzor commented Sep 24, 2024

Backport of #4675

Copy link
Contributor

mergify bot commented Sep 24, 2024

@reinzor, 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.

@reinzor reinzor changed the title feat(controller-server): publish_zero_velocity parameter (#4675) feat(controller-server): publish_zero_velocity parameter jazzy backport Sep 24, 2024
@reinzor reinzor force-pushed the backport/publish-zero-velocity-jazzy branch from f633103 to f72b994 Compare September 24, 2024 12:41
velocity.header.frame_id = costmap_ros_->getBaseFrameID();
velocity.header.stamp = now();
publishVelocity(velocity);
if (get_parameter("publish_zero_velocity").as_bool()) {
Copy link
Member

Choose a reason for hiding this comment

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

You can store the variable in the class, that's OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to https://www.ros.org/reps/rep-0009.html:

You cannot...
  Add new non-static data members, even if they are protected or private

Also the get_parameter is a map lookup so should be fast.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose I haven't enforced that part of ABI, but perhaps I should be -- it would just make direct backporting any new feature virtually impossible since they would likely have some parameter set tied to them. I enforce API stability and behavior stability (i.e. not changing defaults or fundamentally changing the behavior on a module mid-distribution), but I think there's an argument that I should by enforcing stricter ABI as well. Though, its never come up and no one's had an issue.

This is fine.

nav2_controller/src/controller_server.cpp Show resolved Hide resolved
Co-authored-by: Steve Macenski <[email protected]>
Signed-off-by: Rein Appeldoorn <[email protected]>
@SteveMacenski SteveMacenski merged commit ef1330f into ros-navigation:jazzy Sep 25, 2024
7 of 8 checks passed
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