Kinovarobotics / kinova-ros

ROS packages for Jaco2 and Mico robotic arms
BSD 3-Clause "New" or "Revised" License
365 stars 317 forks source link

[kinova_description] Omit limits for fixed or continuous joint types #347

Closed wxmerkt closed 3 years ago

wxmerkt commented 3 years ago

Resolves #289

felixmaisonneuve commented 3 years ago

Hi @wxmerkt,

I have looked at your PR. However, it is not that simple. You have added xacro:if in the kinova_Armjoint xacro:macro, but the macro still needs joint_lower_limit and joint_upper_limit and it will give an error if these fields are absent.

To be able to use the joint type continuous without having to define these 2 parameters, you would need to create a new xacro:macro, let's say kinova_revolute_armjoint. You would probably need a new one for the virtual joint too. Than we would have to change every armjoint that is revolute to this new macro. That means there will be two different macro used across the xacro files, which is just as confusing as having unnecessary limits in my opinion.

All of this represent quite a big change for little benefit at the end since URDF is suppose to ignore limits when joint type is continuous : http://wiki.ros.org/urdf/XML/joint.

Regards, Felix

wxmerkt commented 3 years ago

Hi Felix, Thank you for looking over this PR.

Indeed, at present it still requires setting some values for the joint_lower_limit and joint_upper_limit. To avoid this, we can set default values for these parameters and check that they are not omitted when required for a revolute joint. This would not require another macro to be maintained and it would adequately address the original issue. For instance, we would set a default value for the parameter with a dummy value which is unlikely to be encountered (joint_lower_limit:=123456789) and then check that the value has been set if required/for a revolute joint (<xacro:if value="${type == 'revolute' and joint_lower_limit == 123456789}"><xacro:ERROR_joint_lower_limit_has_not_been_set_for_revolute_joint/></xacro:if>). I've tested this and it works. (While not the primary target, applying the same concept to velocity/torque limit and updating the if-syntax for the fixed parameter to be directly inferred from type would reduce the parameter size of the URDFs further)

Would this be a workable solution?

Regards, Wolfgang

wxmerkt commented 3 years ago

Hi @felixmaisonneuve, I've just added two more commits to illustrate this idea: The first one adds validity checking for parameters passed to the kinova_armjoint macro - i.e., it also detects inconsistent type=='fixed' / fixed:=false settings and allows omission of position (if continuous) and velocity / torque limits (if fixed). The second is an example of omitting the limits as parameters in one URDF: The outputs before/after those two commits of the URDF are identical.

Please let me know if this sufficiently addresses the issue at hand and if I missed use cases.

Regards, Wolfgang

felixmaisonneuve commented 3 years ago

Hi @wxmerkt,

I understand what you are saying and it does make a lot of sense. What you did looks great. I think this could address the original issue and allow us to remove the limits when the joint is continuous.

I will make some testing to make sure it properly covers most of the use cases and doesn't break anything. If everything goes well I will probably merge your PR somewhere next week.

Thank you very much for your contributions, Felix

felixmaisonneuve commented 3 years ago

Hi @wxmerkt,

It took a bit longer than I expected, but I was able to test it, and everything worked great.

Thank you very much for your work. Felix