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

[WIP] Planning_scene_monitor (current_state_monitor) #29

Closed
wants to merge 97 commits into from

Conversation

anasarrak
Copy link

No description provided.

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
@anasarrak anasarrak requested a review from ahcorde March 13, 2019 12:59
if (tf_buffer_ && !robot_model_->getMultiDOFJointModels().empty())
{
tf_connection_.reset(new TFConnection(
tf_buffer_->_addTransformsChangedListener(boost::bind(&CurrentStateMonitor::tfCallback, this))));
Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

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".

Copy link
Author

Choose a reason for hiding this comment

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

moveit/moveit#830 (comment)

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

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?

Copy link
Author

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...

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();
Copy link
Author

Choose a reason for hiding this comment

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

@wjwwood As you mentioned on moveit#31 the walltime calls should be replaced with std::chrono::system_clock, so I guess that the correct use of WallDuration should be something like std::chrono::duration or is there a better way to do it, like just using rclcpp::Duration calls?

Copy link

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.

Copy link
Author

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?

ament_export_include_directories(include)
ament_export_libraries(${libraries})

ament_package()

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

moveit_ros/planning/planning_scene_monitor/CMakeLists.txt Outdated Show resolved Hide resolved

/** @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;

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?

Copy link
Author

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))));

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.

vmayoral pushed a commit that referenced this pull request Mar 16, 2019
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"})){
Copy link
Author

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.
@vmayoral
Copy link

@anasarrak, what's the status of this?

@anasarrak
Copy link
Author

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.

@anasarrak anasarrak closed this Apr 8, 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.

6 participants