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 robot_state to ROS 2 #59

Closed
wants to merge 11 commits into from

Conversation

vmayoral
Copy link
Contributor

Follows from #38 and particularly, after #38 (comment).

@vmayoral vmayoral mentioned this pull request Mar 30, 2019
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*********************************************************************/
* Software License Agreement (BSD License)
Copy link
Member

Choose a reason for hiding this comment

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

Please revert these kind of changes

@param timeout The timeout passed to the kinematics solver on each attempt
@param constraint A state validity constraint to be required for IK solutions */
bool setFromIK(const JointModelGroup* group, const geometry_msgs::Pose& pose, double timeout = 0.0,
const GroupStateValidityCallbackFn& constraint = GroupStateValidityCallbackFn(),
bool setFromIK(const JointModelGroup* group, const geometry_msgs::msg::Pose& pose, unsigned int attempts = 0,
Copy link
Member

Choose a reason for hiding this comment

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

The attempts parameter was removed so it shouldn't be added here again. This also accounts for the removed deprecated functions below.

const EigenSTL::vector_Isometry3d& attach_trans, const std::set<std::string>& touch_links,
const std::string& link_name,
const trajectory_msgs::JointTrajectory& detach_posture = trajectory_msgs::JointTrajectory());
void
Copy link
Member

Choose a reason for hiding this comment

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

Please revert

// ********************************************
// * Internal (hidden) functions
// ********************************************

namespace
{
static bool _jointStateToRobotState(const sensor_msgs::JointState& joint_state, RobotState& state)
// Logger
rclcpp::Logger LOGGER = rclcpp::get_logger("moveit").get_child("robot_state");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rclcpp::Logger LOGGER = rclcpp::get_logger("moveit").get_child("robot_state");
const rclcpp::Logger LOGGER = rclcpp::get_logger("moveit").get_child("robot_state");

Copy link
Member

Choose a reason for hiding this comment

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

Also, LOGGER is used outside this anonymous namespace so it should stay at the same position as LOGNAME before.

error = true;
continue;
}
// if frames do not mach, attempt to transform
Copy link
Member

Choose a reason for hiding this comment

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

revert

@@ -1644,6 +1633,9 @@ bool RobotState::setFromIK(const JointModelGroup* jmg, const EigenSTL::vector_Is
if (timeout < std::numeric_limits<double>::epsilon())
timeout = jmg->getDefaultIKTimeout();

if (attempts == 0)
Copy link
Member

Choose a reason for hiding this comment

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

revert

@@ -1652,32 +1644,66 @@ bool RobotState::setFromIK(const JointModelGroup* jmg, const EigenSTL::vector_Is
// Bijection
const std::vector<unsigned int>& bij = jmg->getKinematicsSolverJointBijection();

bool first_seed = true;
Copy link
Member

Choose a reason for hiding this comment

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

Fix code sync

ros::Publisher pub_aabb = nh.advertise<visualization_msgs::Marker>("/visualization_aabb", 10);
ros::Publisher pub_obb = nh.advertise<visualization_msgs::Marker>("/visualization_obb", 10);
rclcpp::init(argc, argv);
auto node = rclcpp::Node::make_shared("visualize_pr2");
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to create a shared pointer? I think rclcpp::Node node("visualize_pr2") would be sufficient.

ros::Publisher pub_obb = nh.advertise<visualization_msgs::Marker>("/visualization_obb", 10);
rclcpp::init(argc, argv);
auto node = rclcpp::Node::make_shared("visualize_pr2");
auto pub_aabb = node->create_publisher<visualization_msgs::Marker>("/visualization_aabb", rmw_qos_profile_default);
Copy link
Member

Choose a reason for hiding this comment

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

Where is rmw_qos_profile_default defined?

msg.type = visualization_msgs::Marker::CUBE;
msg.color.a = 0.5;
msg.lifetime.sec = 3000;
auto msg = std::make_shared<visualization_msgs::Marker>();
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 it's perfectly fine to use a reference instead of a shared pointer here.

@henningkayser
Copy link
Member

@ahcorde I see that you attempted to revert some of the unwanted changes. Unfortunately there still are a lot of these left. I strongly advocate to rebase this PR onto the current master branch or better recreate it and cherry-pick and verify the desired commits.

@ahcorde ahcorde mentioned this pull request May 16, 2019
@davetcoleman
Copy link
Member

Closing this in favor of @ahcorde 's #80

JafarAbdi pushed a commit to JafarAbdi/moveit2 that referenced this pull request Oct 28, 2019
…terface

Port planning_scene_interface to ROS2
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
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.

4 participants