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

Port moveit_core/utils to ROS 2 #68

Merged
merged 5 commits into from May 21, 2019

Conversation

vmayoral
Copy link
Member

@vmayoral vmayoral commented Mar 31, 2019

Initial iteration was made on a different "master" status

@vmayoral
Copy link
Member Author

vmayoral commented Apr 8, 2019

See #62 (comment)

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

The changes look good! I would only vote to remove xmlrpc_casts.cpp completely as explained below.

@@ -2,34 +2,37 @@ set(MOVEIT_LIB_NAME moveit_utils)

add_library(${MOVEIT_LIB_NAME}
src/lexical_casts.cpp
src/xmlrpc_casts.cpp
# src/xmlrpc_casts.cpp # TODO: solve the conflicts with xmlrpc dependencies
Copy link
Member

Choose a reason for hiding this comment

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

I think xmlrpc_casts.cpp will not be used anymore since XML-RPC is not supported in ROS 2.
parseDouble() and isArray() should be redundant in the future but we have to check whereisStruct() is used and if we need to reimplement it somehow.
Let's remove xmlrpc_casts.cpp and add TODOs everywhere where it is used.
These parts will need to be changed anyway since the same missing XML-RPC dependencies are used.

@ahcorde
Copy link
Contributor

ahcorde commented May 16, 2019

@henningkayser can you review it?

@ahcorde
Copy link
Contributor

ahcorde commented May 20, 2019

@henningkayser friendly ping

@henningkayser
Copy link
Member

henningkayser commented May 20, 2019

@ahcorde removing these files won't be enough since the functionality needs to be reimplemented eventually. Those files that xmlrpc_casts.h will fail to compile which is just terrible to debug and could slow down the port.

  • moveit_plugins/moveit_simple_controller_manager/src/follow_joint_trajectory_controller_handle.cpp
  • moveit_plugins/moveit_simple_controller_manager/src/moveit_simple_controller_manager.cpp
  • moveit_core/kinetmatic_constraints/src/utils.cpp

Please fix related includes and add a descriptions and TODOs for all function calls that need to be replaced somehow. We should keep a list of all those references, either in a new issue or maybe inside the CMakeLists.txt of the utils package. Ideally we would replace the implementation right now but I believe that we still need to have some further discussions on how we handle parameter config files.

@ahcorde
Copy link
Contributor

ahcorde commented May 20, 2019

we decided to remove xmlrpc because in ROS 2 is not used anymore. This concept of getting parameters with xmlrpc has disappeared. There are new ways the get parameters though ROS 2 and DDS.

We have replaced all these calls in the code with the corresponding new ROS 2.0 code. I think it's better to discuss if we can include new abstractions for getting parameters than maintaining this old ROS code.

@henningkayser
Copy link
Member

henningkayser commented May 20, 2019

we decided to remove xmlrpc because in ROS 2 is not used anymore. This concept of getting parameters with xmlrpc has disappeared. There are new ways the get parameters though ROS 2 and DDS.

Yes, that's why I suggested to remove xmlrpc_casts.cpp.

We have replaced all these calls in the code with the corresponding new ROS 2.0 code. I think it's better to discuss if we can include new abstractions for getting parameters than maintaining this old ROS code.

I wasn't aware that this was already implemented. Is it still under review? If yes I would suggest adding related descriptions and would +1 this PR

@ahcorde
Copy link
Contributor

ahcorde commented May 20, 2019

You can open this (huge) PR and have a look to moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp. You will see the changes.

Most of these changes are still pending of been ported. But You will find a TODO (anasarrak) or (ahcorde). The parameters which were ported are the most relevant.

Copy link
Member

@mlautman mlautman left a comment

Choose a reason for hiding this comment

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

One small question. +1 after the comment has been resolved

@mlautman mlautman merged commit 238ba19 into ros-planning:master May 21, 2019
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.

None yet

4 participants