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

Add method to populate SoftJointLimits from ROS parameter server. #292

Conversation

@miguelprada
Copy link
Contributor

miguelprada commented Nov 22, 2017

Addresses #291.

If this is well accepted I will extend the existing tests.

}

// Soft position limits
double soft_lower_limit;

This comment has been minimized.

Copy link
@bmagyar

bmagyar Nov 23, 2017

Member

I'd prefer using soft_limits_nh.hasParam(XYZ) and calling getParam to copy directly onto the limits object. This saves us a line for each move and simplifies the code.

This comment has been minimized.

Copy link
@miguelprada

miguelprada Nov 23, 2017

Author Contributor

Reasonable enough.

This makes me wonder whether getParam is guaranteed not to modify the value of the output variable if the parameter does not exist, which would further simplify the code. A quick test suggests that it doesn't, but since it's not mentioned in the documentation at all, I guess it's better not to rely on that behavior.

@bmagyar bmagyar requested a review from ipa-mdl Nov 23, 2017
@bmagyar

This comment has been minimized.

Copy link
Member

bmagyar commented Nov 29, 2017

@miguelprada , please go ahead with the tests.

@ipa-mdl

This comment has been minimized.

Copy link
Contributor

ipa-mdl commented Dec 3, 2017

I would prefer to upload the soft limits to joint_limits as well, enabled by has_soft_limits. This way all limits for each joint can be configured in one place.

@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 4, 2017

@ipa-mdl I was also in doubt about how to namespace the parameters. What do you think now?

I also set non-existent values to zero, and made k_velocity a requirement, as here (and as specified in the URDF joint element).

}

// Safety k_position
if (limits_nh.hasParam("k_position"))

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Dec 5, 2017

Contributor

This could be simplified to

limits_nh.param("k_position", soft_limits.k_position, 0.0);
}

// Required k_velocity
if (limits_nh.hasParam("k_velocity"))

This comment has been minimized.

Copy link
@ipa-mdl

ipa-mdl Dec 5, 2017

Contributor
if (!limits_nh.getParam("k_velocity", soft_limits.k_velocity)) 
{
  ROS_DEBUG_STREAM(...);
  return false;
}

?

@ipa-mdl

This comment has been minimized.

Copy link
Contributor

ipa-mdl commented Dec 5, 2017

I like this namespace layout!

But I don't like that many braces ;)
Feel free to adapt to my suggestions, or not.

PS: I really wonder why roscpp calls hasParam and getParam in param() as well..

@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 5, 2017

I like this namespace layout!

But I don't like that many braces ;)
Feel free to adapt to my suggestions, or not.

Done!

PS: I really wonder why roscpp calls hasParam and getParam in param() as well..

Maybe something related to my prior comment?

@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 5, 2017

While writing the tests, it came to me that setting the optional values to their default value (i.e. 0.0) if not found may not be the desired behavior. This makes it impossible to use the parameter server to override just some of the existing values in the URDF, such as here.

If partial override is allowed, then it is also not clear to me whether the existence of a k_velocity should be required, or whether whichever values are found should be updated. And in the latter case, when the function should return false (or whether it should at all).

I think my preferred solution would be:

  • Return false iff no k_velocity is found in parameter server and existing value is 0.0
  • Update only the values found in parameter server, leaving the rest unchanged

Didn't change anything yet. Will wait for your comments @bmagyar @ipa-mdl.

@ipa-mdl

This comment has been minimized.

Copy link
Contributor

ipa-mdl commented Dec 16, 2017

This makes it impossible to use the parameter server to override just some of the existing values in the URDF, such as here.

I am not sure if it makes sense to change only one specific value of the soft limits, because they are coupled.

Didn't change anything yet. Will wait for your comments

How do you like the following logic?

bool ok = false;
if(nh.param("has_soft_limits", false)){
  if(!nh.hasParam("k_velocity") || soft_limits.k_velocity == 0.0){
    ROS_ERROR("k_velocity is required");
    return false;
  }

  ok = nh.getParam("k_velocity", soft_limits.k_velocity) || ok;
  ok = nh.getParam("k_position", soft_limits.k_position) || ok;
  ok = nh.getParam("soft_lower_limit", soft_limits.soft_lower_limit) || ok;
  ok = nh.getParam("soft_upper_limit", soft_limits.soft_upper_limit) || ok;

 if(!ok) {
    ROS_WARN("No soft limits found");
 }

}
return ok;
@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 18, 2017

I am not sure if it makes sense to change only one specific value of the soft limits, because they are coupled.

I think I agree.

How do you like the following logic?

I find it a bit inconsistent. Whereas, in getJointLimits, false will only be returned if the limits namespace does not exist, your suggested logic would return false whenever any of the parameters is not found. Also, even though false would be returned, any existing values in the parameter server would be written to soft_limits.

How about?

if(nh.hasParam("k_position") && nh.hasParam("k_velocity") && nh.hasParam("soft_lower_limit") && nh.hasParam("soft_upper_limit"))
{
  nh.getParam("k_position", soft_limits.k_position);
  nh.getParam("k_velocity", soft_limits.k_velocity);
  nh.getParam("soft_lower_limit", soft_limits.min_position);
  nh.getParam("soft_upper_limit", soft_limits.max_position);
  return true;
}
return false;

This way, the values will be overriden and true will be returned if, and only if, all values are specified in the parameter server.

@bmagyar

This comment has been minimized.

Copy link
Member

bmagyar commented Dec 18, 2017

miguelprada added 2 commits Dec 18, 2017
Behavior of getSoftJointLimits is changed to only update the soft limits
structure if, and only if, the complete set of parameters is found. Otherwise
nothing is changed and the function returns false.
@bmagyar

This comment has been minimized.

Copy link
Member

bmagyar commented Dec 18, 2017

I'm wondering if it's a good idea to print a warning when not all limits are specified. May be handy to save some people from gray hair but slightly verbose.

@ipa-mdl

This comment has been minimized.

Copy link
Contributor

ipa-mdl commented Dec 18, 2017

@miguelprada: Your version is fine for me. However, I still think we should add has_soft_limits to be consistent with the existing functions.

@bmagyar

This comment has been minimized.

Copy link
Member

bmagyar commented Dec 18, 2017

As we are done with discussing naming and print details I've put my green light on it.

@ipa-mdl please merge at your convenience.

@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 18, 2017

@miguelprada: Your version is fine for me. However, I still think we should add has_soft_limits to be consistent with the existing functions.

A slight difference here is that, unlike JointLimits with has_position_limits, etc., SoftJointLimits doesn't have a has_soft_limits member.

I can see the need for it in JointLimits, since an instance can hold some but not other limit specifications. This is not the case with SoftJointLimits; it either has everything or nothing, and this can be already deduced from the return value of the function.

I won't refuse to add it if you really prefer to have it for consistence, though. If that's the case, would you rather add it to the SoftJointLimits as a member as well @ipa-mdl? And what would be your preferred semantics for the return value of getSoftJointLimits then?

@ipa-mdl

This comment has been minimized.

Copy link
Contributor

ipa-mdl commented Dec 18, 2017

has_soft_limits would just disable the parsing and make getSoftJointLimits return false.
I don't think there is a direct need for adding it to the structure.
We have used the has_*_limits flags disable limits for certain robots or environments, so this might be handy for soft limits as well. I don't insist on adding it.

Copy link
Contributor

ipa-mdl left a comment

Current state LGTM.

@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 18, 2017

We have used the has_*_limits flags disable limits for certain robots or environments, so this might be handy for soft limits as well. I don't insist on adding it.

I see your point and I think it's fair. Give me a while and I'll add it.

@miguelprada miguelprada force-pushed the tecnalia-medical-robotics:soft_limits_rosparam branch from e352562 to 73f0317 Dec 18, 2017
@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 18, 2017

Sorry for the dirty amend, but I had forgotten to update the tests in e352562 and I feel like this PR already has way more commits than it should.

@bmagyar

This comment has been minimized.

Copy link
Member

bmagyar commented Dec 18, 2017

Probably we should squash-merge it.

@ipa-mdl

This comment has been minimized.

Copy link
Contributor

ipa-mdl commented Dec 18, 2017

@miguelprada: if you agree, I will squash-merge. If not, just go ahead and rebase accordingly.

@miguelprada

This comment has been minimized.

Copy link
Contributor Author

miguelprada commented Dec 18, 2017

Sure, go ahead @ipa-mdl.

@ipa-mdl ipa-mdl merged commit 2d7ef90 into ros-controls:kinetic-devel Dec 18, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.