Reimplemented server response-waiting functionalities inside the methods of move_group_interface #2863
Conversation
…ods of move_group_interface - Reimplemented getPlannerParams, setPlannerParams, getInterfaceDescription, getInterfaceDescriptions, plan, execute, move and computeCartesianPath methods of move_group_interface using rclcpp's own api - Removed callback_thread_
|
Hello @sjahr , it seems ready, but not ready for merging, i need a second eye. I only spin now when the methods like plan, execute etc. are called instead of spinning during lifetime of MoveGroupInterface instance. But I want to ask a question.
|
| @@ -192,18 +192,22 @@ class MOVEIT_MOVE_GROUP_INTERFACE_EXPORT MoveGroupInterface | |||
| unsigned int getVariableCount() const; | |||
|
|
|||
| /** \brief Get the descriptions of all planning plugins loaded by the action server */ | |||
| bool getInterfaceDescriptions(std::vector<moveit_msgs::msg::PlannerInterfaceDescription>& desc) const; | |||
| bool getInterfaceDescriptions(std::vector<moveit_msgs::msg::PlannerInterfaceDescription>& desc, | |||
There was a problem hiding this comment.
Do you mind adding documentation for the new duration parameter? I know the current documentation doesn't do that but this is a good chance to improve that
| * target. | ||
| * This call is not blocking (does not wait for the execution of the trajectory to complete). | ||
| * \param [in] server_timeout period of time duration for waiting server's response. Function returns TIME_OUT in case | ||
| * that server doesn't answer whether the request is accepted or rejected within certain period of time \param [in] |
There was a problem hiding this comment.
Unfortunately, pre-commit regularly messes with the doxygen formatting. Once the PR is ready to be merged and approved you'll need to fix issues like this
| } | ||
|
|
||
| const auto& response = future_response.get(); |
There was a problem hiding this comment.
Just to be save, you should maybe check for the future validity before trying to get it, right?
| return false; | ||
| } | ||
|
|
||
| const auto& response = future_response.get(); |
|
|
||
| response = future_response.get(); | ||
| for (unsigned int i = 0, end = response->params.keys.size(); i < end; ++i) | ||
| result[response->params.keys[i]] = response->params.values[i]; |
There was a problem hiding this comment.
Prefere .at(i) over [i] to get an out of bounds segfault if something fails
| @@ -782,15 +826,16 @@ class MoveGroupInterface::MoveGroupInterfaceImpl | |||
| } | |||
|
|
|||
| moveit::core::MoveItErrorCode execute(const moveit_msgs::msg::RobotTrajectory& trajectory, bool wait, | |||
| const std::vector<std::string>& controllers = std::vector<std::string>()) | |||
| const std::vector<std::string>& controllers, | |||
There was a problem hiding this comment.
Sanity check, you're removing this because it is properly set in the header right?
| server_timeout.to_chrono<std::chrono::duration<double>>()) != | ||
| rclcpp::FutureReturnCode::SUCCESS) | ||
| { | ||
| RCLCPP_ERROR(logger_, "MoveGroupInterface::execute() get sent call failed :("); |
There was a problem hiding this comment.
You should make this error message more descriptive. Try to explain what failed and why
There was a problem hiding this comment.
You'll probably need to update all error messages like this
|
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
|
This pull request is in conflict. Could you fix it @CihatAltiparmak? |
github-actions
bot
removed
the
stale
Description
Firstly, this pull request is created in order to solve this issue #2859 . I still have to apply some test such that this PR works for move2_tutorials and so on. I already have tested this PR on rviz but i need to try this part more to see if any corruption occurs for this PR.
I haven't tested computeCartesianMethod, getPlannerParams, setPlannerParams(acutally there is a posibility that i tested getPlannerParam and setPlannerParam implicitly while testing rviz and getInterfaceDescriptions, or else rviz gets stuck related to node spining)
I have removed callback_thread_ because it's not necessary to yield CPU continuously during move_group_interface to work. Instead of this, i just made a implementation for spinning only when client send a request from move_group_interface. I also have added private node for move_group_interface to handle its client jobs.
I also have to give anonymous node name to pnode_ . I will work on this as well.
I need some review from variable naming to logic.
Checklist