Removing topic name parameter configuration and replacing with const strings #2777
Conversation
…, replace with const string
…replace with const string
…uration via parameters and using const strings instead
|
Please target the |
Gaurang-1402
changed the title
There was a problem hiding this comment.
Nice work @Gaurang-1402! Just small comments and then this is good to go
| @@ -271,12 +264,11 @@ ServoParameters ServoParameters::get(const std::string& ns, | |||
|
|
|||
| // ROS Parameters | |||
| parameters.use_gazebo = node_parameters->get_parameter(ns + ".use_gazebo").as_bool(); | |||
| parameters.status_topic = node_parameters->get_parameter(ns + ".status_topic").as_string(); | |||
| parameters.status_topic = "/servo_server/status"; | |||
There was a problem hiding this comment.
Can you make these constants in the anonymous namespace above like Logger and then assign the constant to the parameters member? Something like
namespace {
const std::string SERVO_STATUS_TOPIC = "/servo_server/status";
}
...
parameters.status_topic = SERVO_STATUS_TOPIC;
| @@ -44,29 +48,69 @@ API changes in MoveIt releases | |||
|
|
|||
| ## ROS Melodic | |||
|
|
|||
| - Migration to ``tf2`` API. | |||
There was a problem hiding this comment.
Why are these changes necessary?
|
This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete. |
github-actions
bot
added
the
stale
|
@Gaurang-1402 what's the current status of this PR? It would also be great if you could target the 'main' branch. We can backport fixes and non-breaking changes to 'humble' |
github-actions
bot
removed
the
stale
|
This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete. |
github-actions
bot
added
the
stale
|
Closing this due to inactivity. Feel free to re-open if you want to continue working on it again |
Description
This PR introduces changes to streamline the setup process for the
moveit_cpp::PlanningSceneMonitorOptionsandmoveit_servo::ServoParametersby utilizing constant strings for topic names instead of parameter-based configurations. The changes ensure that topic names are now consistent and predefined. This modification also implies that users who need customized topic names will now have to use topic remapping in their launch files or at the ROS node level.These updates are in direct response to the issue discussed in #2714
Checklist
In the tutorials, I see some files where changes need to be made to remove topic names from parameters, for example here:
https://github.com/ros-planning/moveit2_tutorials/blob/main/doc/examples/moveit_cpp/config/moveit_cpp.yaml
I'm confused about the sequence of changes though since these are 2 separate repos. Do I make these changes before this PR is merged, do I make 2 simultaneous PR or do I raise an issue on that repo? Some guidance would be helpful to proceed on this point.
Done included in the commit
As this PR stands, there are no scenarios where the code fails without this PR because it is an API enhancement. Yet, if needed, I can create tests- please let me know.
Not Applicable