-
Notifications
You must be signed in to change notification settings - Fork 17
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
[WIP] Planning_scene_monitor (current_state_monitor) #29
Conversation
It's a still WIP, problems with tf2_buffer_ there is no _removeTransformsChangedListener and _addTransformsChangedListener on geometry2 for ros2, think how to change it. the appropriate change for ros::WallTime is std::chrono::system_clock since it's not implemented yet find something to replace ros::WallDuration
if (tf_buffer_ && !robot_model_->getMultiDOFJointModels().empty()) | ||
{ | ||
tf_connection_.reset(new TFConnection( | ||
tf_buffer_->_addTransformsChangedListener(boost::bind(&CurrentStateMonitor::tfCallback, this)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davetcoleman and @mlautman I'm trying to replace the _addTransformsChangedListener call since on ros2 geometry this function is not implemented, I would like some input about how to procede with that.
There is a similar function called addTransformableCallback but it has nothing to do with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @henningkayser, @davetcoleman, @mlautman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why _addTransformsChangedListener()
is not implemented?
@IanTheEngineer made this change 10 months ago, here: moveit/moveit@736251d
Looking at that PR, it required the following PRs to pass:
This PR depends on the following upstream API changes for Melodic:
ros/geometry#163 (Exposing tf2_ros::Buffer
object from TransformListener)
ros/geometry2#292 (tf2 Type conversions)
ros/geometry2#294 (Additional tf2 Type conversions)
ros-visualization/rviz#1236 (Exposing tf2 without ROS deprecation warnings)
So likely one of those needs to be forward ported to ROS2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why
_addTransformsChangedListener()
is not implemented?
Look at the moveit/moveit#830 (comment), on the porting procedure they just left it apart as it seems to be "deprecated".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addTransformsChangedListener API has been replaced by the TransformableRequest interface. An example of using it is in the tf2::MessageFilters: http://docs.ros.org/jade/api/tf2_ros/html/c++/message__filter_8h_source.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IanTheEngineer can you help shed some light on this question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @mlautman, @davetcoleman
I have to finish this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anasarrak I don't know if I have any more information than you do at this point.. Have you had the chance to hunt down the dependencies that Dave referenced to see if any of them might be holding up the port? Perhaps @tfoote's comment can help shed some light here: moveit/moveit#830 (comment)
missing_states.push_back(joint->getName()); | ||
result = false; | ||
} | ||
} | ||
return result; | ||
} | ||
|
||
bool planning_scene_monitor::CurrentStateMonitor::waitForCurrentState(const ros::Time t, double wait_time) const | ||
{ | ||
ros::WallTime start = ros::WallTime::now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::chrono::duration
is sufficient I think. We were supposed to have to/from functions for converting between std::chrono::duration
and rclcpp::Duration
, but I don't know if they got implemented or not. It would be a good thing to contribute if they're missing.
I don't know what the context here is, so I can't advise you. Please give me an exact question or scenario and I can help more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically the question was: What is the best replacemanet for ros::WallDuration/ros::WallTime since they are not implemented for ros2. You have already answered me with, use std::chrono::system_clock as replacement for ros::walltime and std::chrono::duration for ros::WallDuration, right?
Much of the functionality that provides boost are implemented on C++11
ament_export_include_directories(include) | ||
ament_export_libraries(${libraries}) | ||
|
||
ament_package() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A stray COLCON_IGNORE
was committed in the following file:
moveit_ros/planning/COLCON_IGNORE
|
||
/** @brief Get the current state | ||
* @return Returns the current state */ | ||
robot_state::RobotStatePtr getCurrentState() const; | ||
|
||
std::pair<robot_state::RobotStatePtr, rclcpp::Time> getCurnode_rentStateAndTime() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name of this function is broken. also, is this function needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here f12ad0e, thanks for this nice catch on the mispelling (it's still a WIP). Also, it's needed on
std::pair<robot_state::RobotStatePtr, ros::Time> state = current_state_monitor_->getCurrentStateAndTime(); |
if (tf_buffer_ && !robot_model_->getMultiDOFJointModels().empty()) | ||
{ | ||
tf_connection_.reset(new TFConnection( | ||
tf_buffer_->_addTransformsChangedListener(boost::bind(&CurrentStateMonitor::tfCallback, this)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why _addTransformsChangedListener()
is not implemented?
@IanTheEngineer made this change 10 months ago, here: moveit/moveit@736251d
Looking at that PR, it required the following PRs to pass:
This PR depends on the following upstream API changes for Melodic:
ros/geometry#163 (Exposing tf2_ros::Buffer
object from TransformListener)
ros/geometry2#292 (tf2 Type conversions)
ros/geometry2#294 (Additional tf2 Type conversions)
ros-visualization/rviz#1236 (Exposing tf2 without ROS deprecation warnings)
So likely one of those needs to be forward ported to ROS2.
moveit_ros/planning/planning_scene_monitor/src/current_state_monitor.cpp
Outdated
Show resolved
Hide resolved
Update TODO for the demos Co-Authored-By: anasarrak <anasarrak@gmail.com>
RCLCPP_WARN(logger, "default_collision_operations is not an array"); | ||
return; | ||
} | ||
for(auto & parameter : parameter_robot_description->get_parameters({robot_description_ + "_planning/default_collision_operations"})){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like please some input about these changes @ahcorde
Changes on: -planning_interface: CMakelists updated so it only compiles move_group_interface -move_group_interface: Changes on cmakelist, msgs and commented out all the code, letting available plan and execute, ill be focusing more on these changes once planning_scene_monitor PR is closed. - planning_scene_interface: Changes on the msgs on the .h, still has to be modified the .cpp - py_binding_tools: The cmakelist is prepared to compile with colcon.
@anasarrak, what's the status of this? |
The waitForAction function has some callbacks that has to be reviewed, added some TODO and a logger so it prints something when we launch it
Changed both publishers to rclcpp publishers, Added a TODO, since trajectory_execution_manager is not ported yet, I have hardcoded the topic name
Sync Move group with planning_last
Due to the conflicts with master I'll split this pr on different PRs, since planning_scene_monitor needs to be ported for the different modules, for example plan_execution, trajectory_execution_manager... I prefer to merge it on master as it is and fix it on the future. |
No description provided.