[WIP] Enforce joint limits by hsd-dev · Pull Request #223 · UniversalRobots/Universal_Robots_ROS_Driver

hsd-dev · 2020-07-07T13:33:20Z

Related to #198
[WIP] since have not tested.

gavanderhoorn · 2020-07-07T14:20:31Z

I'll let @fmauch do the detailed review, but two high-level comments:

was this code copied from somewhere? I seem to recognise it as the code used in the ros_control boilerplate for loading limits from the parameter server. If it was copied (doesn't matter from where), this fact should be recorded somewhere -- probably with a copyright statement and attribution somewhere
to make the diff easier to follow: try to avoid whitespace and layout changes (unless clang format asks/demands it)

Edit: looks like the format_check test failed.

hsd-dev · 2020-07-07T14:23:21Z

was this code copied from somewhere?

Yes from the boilerplate. There are very few examples out there :)

gavanderhoorn · 2020-07-07T15:05:05Z

was this code copied from somewhere?

Yes from the boilerplate. There are very few examples out there :)

Then we should mention that somewhere.

It might even be a good idea to put that code in a separate .cpp. That would make it even easier to add a license block at the top of the file.

gavanderhoorn · 2020-07-07T15:27:24Z

Will you do that @ipa-hsd ? (just making sure)

hsd-dev · 2020-07-07T15:30:01Z

Will you do that @ipa-hsd ?

Yes working on it.

hsd-dev · 2020-07-07T15:50:46Z

All checks passed, but not tested yet and updating as per @gavanderhoorn's comments.
Do Not Merge.

fmauch

This roughly looks quite good.

Because of the missing vector initialization I cannot test this currently, but I'll be happy to test this once that's done.

fmauch · 2020-07-07T15:42:39Z

+                                            std::size_t joint_id)
+{
+  // Default values
+  joint_position_lower_limits_[joint_id] = -std::numeric_limits<double>::max();


As far as I can see, this vector doesn't get resized anywhere, which would making this call illegal.

fmauch · 2020-07-07T15:44:00Z

+  bool has_soft_limits = false;
+
+  // Get limits from URDF
+  if (urdf_model_ == NULL)


Suggested change

if (urdf_model_ == NULL)

if (urdf_model_ == nullptr)

fmauch · 2020-07-07T15:44:40Z

+  urdf::JointConstSharedPtr urdf_joint = urdf_model_->getJoint(joint_names_[joint_id]);
+
+  // Get main joint limits
+  if (urdf_joint == NULL)


Suggested change

if (urdf_joint == NULL)

if (urdf_joint == nullptr)

fmauch · 2020-07-07T15:47:29Z

+    if (urdf_joint->type != urdf::Joint::CONTINUOUS)
+      ROS_WARN_STREAM("Joint " << joint_names_[joint_id]
+                               << " does not have a URDF "
+                                  "position limit");


This isn't only referring to position limits, right? We could change the output that no limits are configured for that joint in the URDF.

fmauch · 2020-07-07T15:57:54Z

+    {
+      ROS_INFO_STREAM("Waiting for model URDF on the ROS param server at location: " << nh.getNamespace()
+                                                                                     << param_name);
+      nh.getParam(param_name, urdf_string);
+    }


This block doesn't make sense to me. In which situation should searchParam fail but getParam work? Am I missing anything?

You are right. Removed it for the next commit.

fmauch · 2020-07-20T12:29:43Z

@ipa-hsd Any plans / resources to continue this?

hsd-dev · 2020-07-21T12:54:54Z

@fmauch sorry for the delay. Was occupied last week. I have separated the code from the boilerplate example in a separate file. Please take a look if that make sense because I have not made it as a separate class, but just a separate .cpp

@gavanderhoorn I have also added the original license from the example.

fmauch · 2020-07-23T09:14:19Z

+   * \brief Applies joint limits & soft joint limits on position, velocity & effort defined in URDF / parameter server
+   *
+   */
+  void registerJointLimits(ros::NodeHandle& robot_hw_nh, const hardware_interface::JointHandle& joint_handle_position,


Does this function apply the limits or register the limits? I guess, the docstring needs to be adapted.

fmauch · 2020-07-23T09:16:19Z

  std::string input_recipe_filename;

+  // Load URDF file from parameter server
+  loadURDF(robot_hw_nh, "robot_description");


This should probably be the root_nh

fmauch · 2020-07-23T09:17:49Z

+ *********************************************************************/
+
+/* Author: Dave Coleman
+   Desc:   Helper ros_control hardware interface that loads configurations


I think a note where this file comes from and that you added modifications to it (if you did) might be nice to add here.

gavanderhoorn · 2020-08-27T18:42:07Z

@ipa-hsd: would you see a chance to finish this?

hsd-dev · 2020-10-09T10:10:08Z

@fmauch a1422f1 addresses your previous comments.

hsd-dev · 2020-10-09T10:17:18Z

Any idea about:

  Starting function 'before_init'
  E: Failed to fetch http://packages.ros.org/ros/ubuntu/dists/xenial/InRelease  Clearsigned file isn't valid, got 'NOSPLIT' (does the network require authentication?)
  E: Some index files failed to download. They have been ignored, or old ones used instead.

gavanderhoorn · 2020-10-09T12:51:17Z

@ipa-hsd: could be ros-infrastructure/answers.ros.org#209.

hsd-dev · 2020-10-12T09:12:03Z

The checks look good now.

ljden · 2021-06-21T00:25:14Z

What's the status on this?

github-actions · 2023-04-08T22:37:44Z

This PR hasn't made any progress for quite some time and will be closed soon. Please comment if it is still relevant.

fmauch · 2023-04-11T10:58:44Z

though not being updated in a while, this topic is still important.

github-actions · 2023-07-13T22:38:06Z

This PR hasn't made any progress for quite some time and will be closed soon. Please comment if it is still relevant.

loads URDF model from parameter server

8828a43

hsd-dev mentioned this pull request Jul 7, 2020

Add support for ros_control joint limits? #198

Closed

gavanderhoorn reviewed Jul 7, 2020

View reviewed changes

Comment thread ur_robot_driver/include/ur_robot_driver/ros/hardware_interface.h Outdated

hsd-dev force-pushed the joint_limits branch from 189d744 to 3520361 Compare July 7, 2020 14:16

hsd-dev force-pushed the joint_limits branch from 3520361 to c99685c Compare July 7, 2020 15:35

enforcing joint limits from urdf / parameter server

72463b3

hsd-dev force-pushed the joint_limits branch from c99685c to 72463b3 Compare July 7, 2020 15:38

fmauch suggested changes Jul 7, 2020

View reviewed changes

fmauch added the wrid20 label Jul 7, 2020

hsd-dev force-pushed the joint_limits branch from e2ad636 to 7d01042 Compare July 21, 2020 12:59

Separated boilterplate example in a separate cpp with license

7315fef

hsd-dev force-pushed the joint_limits branch from 7d01042 to 7315fef Compare July 21, 2020 13:11

fmauch reviewed Jul 23, 2020

View reviewed changes

Added note to document changes from original file

a1422f1

restarting CI checks

7a4fcfb

gavanderhoorn mentioned this pull request Sep 23, 2021

Parameter naming in joint_limits.yaml UniversalRobots/Universal_Robots_ROS2_Driver#168

Closed

github-actions Bot added the Stale label Apr 8, 2023

github-actions Bot removed the Stale label Apr 11, 2023

github-actions Bot added the Stale label Jul 13, 2023

Conversation

hsd-dev commented Jul 7, 2020

Uh oh!

Uh oh!

gavanderhoorn commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsd-dev commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavanderhoorn commented Jul 7, 2020

Uh oh!

gavanderhoorn commented Jul 7, 2020

Uh oh!

hsd-dev commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsd-dev commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmauch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmauch commented Jul 20, 2020

Uh oh!

hsd-dev commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavanderhoorn commented Aug 27, 2020

Uh oh!

hsd-dev commented Oct 9, 2020

Uh oh!

hsd-dev commented Oct 9, 2020

Uh oh!

gavanderhoorn commented Oct 9, 2020

Uh oh!

hsd-dev commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljden commented Jun 21, 2021

Uh oh!

github-actions Bot commented Apr 8, 2023

Uh oh!

fmauch commented Apr 11, 2023

Uh oh!

github-actions Bot commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gavanderhoorn commented Jul 7, 2020 •

edited

Loading

hsd-dev commented Jul 7, 2020 •

edited

Loading

hsd-dev commented Jul 7, 2020 •

edited

Loading

hsd-dev commented Jul 7, 2020 •

edited

Loading

hsd-dev commented Oct 12, 2020 •

edited

Loading