Modernize / Modularize xacro

[rhaschke]

This is the new PR replacing #109 / #113 already announced in #113 (review).

Summary

The key goal followed by the restructuring is a simple and scalable approach to specify different hand variants.
Until, for each variant essentially a separate xacro file was required. With this PR, we can essentially resolve everything using either full_hand.urdf.xacro or hand_lite.urdf.xacro. Using the macro parameter fingers one can specify which fingers are needed. With parameters [tip|mid|prox]_sensor(s) one can choose individual (tactile) sensors. The already existing variable hand_type can now also be used to specify muscle or muscletrans hands.

Changes w.r.t. #109 / #113:

• Removed redundant definition of pi, causing warnings: redefining global symbol: pi
• Removed unused meshes in ubi_tactiles
• Factored out Simplify URDF test #118 and Simplify scripts to visually test/inspect arm and hand models #119
• Added a compatibility-test to ensure that the old robot xacros produce the same results as the new ones.
  The test introduced in F#98 modular xacros #109 was just ensuring that the xacros can be processed at all.

TODOs:

• Approval from @guihomework: Modernize / Modularize xacro #120 (comment)
• Fix the remaining problem of some lost xmlns attributes (see Modernize / Modularize xacro #120 (comment)).
  Thanks to @guihomework analysis, I was able to fix the remaining problem with a fix in xacro:
  • Merge Fix handling of xml namespaces imported via xacro:include in a macro ros/xacro#287
• Drop commits marked as TEMP. They just illustrate the progress of the compatibility test.
• Squash some commits (d891721, 51bf390, ceacb17) into the main one (929fac3). I kept them to illustrate my changes.
• Squash the fixing commits
  I kept the original, unsquashed version of this PR in https://github.com/ubi-agni/sr_common/tree/PR%23120-long.
  One can easily validate the backward compatibility there by inspecting the history of test/robots.urdf
• Merge Issue a deprecation warning when old parameters are used ubi-agni/sr_common#1
  • Merge Expose more functions ros/xacro#283
• Merge Drop useless bumpers ubi-agni/sr_common#2 ?
• Merge Improve readability ubi-agni/sr_common#3
  • Merge Allow greedy property evaluation ros/xacro#284
• To further improve readability, I suggest replacing the list variables with dotified dicts, e.g. tip_sensors[2] becomes tip_sensor.mf. Argument would be passed as tip_sensors=${dict(th='bio', mf='std', rf='eli')}.
  To simplify for the common case of identical values, we could also think of some internal property re-assignment to allow for tip_sensors=bio.
• @beatrizleon decide and @rhaschke? implement the hand_type naming: Modernize / Modularize xacro #120 (comment)
• Release xacro
  • Melodic: xacro: 1.13.14-1 in 'melodic/distribution.yaml' [bloom] ros/rosdistro#30921
  • Noetic: xacro: 1.14.10-1 in 'noetic/distribution.yaml' [bloom] ros/rosdistro#30922
  • Sync planned for Oct 13th: https://discourse.ros.org/t/preparing-for-noetic-sync-2021-09-23/22378/5

[rhaschke]

@beatrizleon: How can I access the results of the failing CI jobs at AWS?

[rhaschke]

I added a series of commits fixing most compatibility-test issues. The only remaining issue are some lost xmlns attributes as illustrated in this comparison showing the differences between generated URDF files from test/robots.old vs. robots. That's what the compatibility test compares as well. I'm not sure where these attributes got lost.
@guihomework, maybe you have an idea?

The good news is that processing the new robot URDFs yields the same results as in a741b66.
That's already much better than the results from the original PR, shown in this and following diffs.

[guihomework]

That's what the compatibility test compares as well. I'm not sure where these attributes got lost. @guihomework, maybe you have an idea?

I think I traced the problem to the presence of the include of the full_hand (defining the macro shadowhand) within the definition of the macro shadowhand there https://github.com/shadow-robot/sr_common/pull/120/files#diff-d505b43845bf4e41bf9a24883f9c24aef30837e1f512253f29313889990ce805R14

I was first surprised a macro redefinition was possible, so I tried to rename one of the macro either the exterior, or the interior one, but it does not change anything as long the include is inside the outer macro redefinition
Of course with the same macro name, one cannot put the include outside the new definition. It fails with
invalid parameter "fingers"
because the macro call inside, wants to call the newly defined macro (recursivity issue ahead), instead of the included one.

I find this already too tricky for me, so cannot provide a fix. I hope this pin pointing suffices to guide to a possible true cause (within xacro itself I suppose)

[guihomework]

@rhaschke I think I am done looking at the code and answering your questions. Everything I found was bad understanding of the code. sorry for the noise.
the compat test is not python2.7 compatible so I cannot test it myself under melodic, even calling it with python3 and some of the rospkg for python3 we have fails on Bionic.
I suppose it does not matter, @beatrizleon and @devops-shadowrobot will confirm it does what they need.

[rhaschke]

I think I traced the problem to the presence of the include of the full_hand within the definition of the macro shadowhand.

Thanks for tracing this down! I will have a look.

[rhaschke]

Indeed, the culprit was an issue in xacro: namespaces imported within a macro were not lifted. Fixed via ros/xacro#285.

[guihomework]

Indeed, the culprit was an issue in xacro: namespaces imported within a macro were not lifted. Fixed via ros/xacro#285.

I would never have generated such a case my self as I would never have attempt redefining a macro that has the same name that itself calls the macro name.
This "courage" of doing so requires a great knowledge on how xacro effectively works to know it will do what is desired.

[rhaschke]

The problem was actually more fundamental: Any namespaces imported by a xacro:include within a macro were ignored - even if the include file doesn't do anything else than providing those namespaces. See the corresponding unit test I added in xacro for an example.
Redefining macros (or properties) is perfectly fine in xacro. As macros create a new scope, all symbols newly defined within the macro don't even affect the parent scope. That is, the original macro stays untouched when doing so.

[rhaschke]

I think I am done looking at the code and answering your questions.

Thanks a lot. Do you mind to also formally approve this PR if you are fine with it?

[guihomework]

I think I am done looking at the code and answering your questions.

Thanks a lot. Do you mind to also formally approve this PR if you are fine with it?

I cannot test it easily immediately, I need to update my noetic/focal install first. I did not prepare my PR for noetic so cannot know what else can happen without testing.

[guihomework]

The problem was actually more fundamental: Any namespaces imported by a xacro:include within a macro were ignored - even if the include file doesn't do anything else than providing those namespaces. See the corresponding unit test I added in xacro for an example. Redefining macros (or properties) is perfectly fine in xacro. As macros create a new scope, all symbols newly defined within the macro don't even affect the parent scope. That is, the original macro stays untouched when doing so.

Not trivial to me, but probably fine for python developers.

[rhaschke]

I cannot test it easily immediately, I need to update my noetic/focal install first.

Note that the noetic-devel branches of this package as well as of xacro both work fine under Melodic as well. No need to install Noetic/Focal for testing.

[beatrizleon]

@rhaschke We were working on refactoring this repo with the help of @guihomework with the following purposes:

1. Be able to use one xacro to build all our hands (removing the need to create a xacro per hand configuration inside robots folder)
2. Have a way to easily specify the number of fingers and tactile sensors
3. Introduce a way to specify the hand version differentiating the meshes on each of them.

I had a work in progress branch: https://github.com/shadow-robot/sr_common/tree/F_refactoring_meshes. It was based on the changes that @guihomework did on his proposed branch which were really helpful. With this new refactoring, I will create a new branch based on this one and show you what is the idea. It will be great to improve this as it has lots of deprecated code, unused meshes, etc

Thanks both for you help with this. It is going slowly because of my lack of time to spend coding but hopefully with all your help, we will be able to finish this refactoring soon.

[beatrizleon]

I checked the CI error and the issue is:
Copyright check failure: There are 1 files without copyright notices: ./sr_description/test/test_compatibility.py

[rhaschke]

I had a work in progress branch: https://github.com/shadow-robot/sr_common/tree/F_refactoring_meshes, based on #109.
With this new refactoring, I will create a new branch based on this one and show you what is the idea. It will be great to improve this as it has lots of deprecated code, unused meshes, etc.

I will shortly rebase this PR onto the latest noetic-devel and apply the announced squashes.

[rhaschke]

I factored out the ubi tactile sensor definitions and merged ubi-agni#3

[rhaschke]

@guihomework: This selfCollide tag is part of the unless clause here, while in all other places it's outside.
Is this one wrong here?

[guihomework]

yes it makes no sense to remove the selfcollide here and not at other places, yet another mistake of mine.

side note : I am not convinced anymore by the current set of true/false on the self collide in the fingers, but I think it is a tricky matter (at least I remember it was not obvious) and if the current set works one should not try change it.

[guihomework]

I factored out the ubi tactile sensor definitions.

I don't know why this is necessary, since shadow robot company supports the ubi sensors, even in the driver.

[rhaschke]

I don't know why this is necessary, since shadow robot company supports the ubi sensors, even in the driver.

As you said yourself: they can only be used with our custom urdf repos, which will never merged into public.

[guihomework]

I don't know why this is necessary, since shadow robot company supports the ubi sensors, even in the driver.

As you said yourself: they can only be used with our custom urdf repos, which will never merged into public.

Not exactly what I said : I said that the taxels cannot be used without our tactile_toolbox, and could maybe be removed, but the visual/collision of the ubi could stay there as they do not need the tactile_toolbox. They were merged by shadow there shadow-robot/sr-ros-interface#253 and there was a distinction in the urdf for ubi that I had added already at shadow-robot/sr-ros-interface#147 and was also accepted upstream.

We always wanted to have as maximum things merged upstream as possible. If this an effort to now clean up their repo from tip sensor they do not sell, sure, then this the reason, but it is not a problem to have our visual tips even without tactile_toolbox.

EDIT: one advantage of having ubi remain in the possible sensors, is that when they upgrade the package, they will take care of keeping the ubi sensor as part of the list/future new way of handling sensors, when if they do not see it, we will have more issues to upgrade in the future.

[rhaschke]

As a maintainer, I wouldn't like to have external sources in my repo that I cannot validate.
From my point of view, it is cleaner that we maintain our sensor addons on our side.
Yes, my point of view has changed on this since 2014 - due to much more experience in maintaining repositories.

[beatrizleon]

I am working now in a branch, so give me a few days to finish this.

About the hand_type, what we would like to have is:
E: hand_e (dexterous hand)
G: hand_lite
G-:hand_extra_lite (I am not sure about this, probably we don't need it as it is a one finger less than G)

I am modifying them in the process_hand_type_parameter.xacro file.

I wanted to ask you if you are still using a muscle or muscletrans hand? As I said before, we have only few clients with that and we are not currently supporting any. Same for the shadow arm. I would like to know if you need that we keep support for those

[guihomework]

@beatrizleon in place of the team in Paris UPMC who has a ShadowArm and won't probably notice those discussions here, I would say, either they should fork to keep it (please announce it to the customers), or your company should create a legacy sr_description package, with things that you supported in the past, but won't anymore. This way it stays visible (and not hidden in history that is no easy to find), in the state they are now.
I personally like companies who still provide old drivers for winXP etc in a legacy section of their website. It is just a matter of dragging the files along but they require less maintenance and are separated for the supported ones.

Regarding the muscle hands in my current team, I think they were never used with ROS, but the same about the meshes and URDFs as for the Shadow Muscle arm applies IMHO. If for any reason someone would like to integrate the muscles hands in ROS on his own, finding the resources without digging in history would be nice.

[rhaschke]

I confirm that we never used the muscle hands with ROS (and probably won't do it).

About the hand_type, what we would like to have is:
E: hand_e (dexterous hand)
G: hand_lite
G-:hand_extra_lite (I am not sure about this, probably we don't need it as it is a one finger less than G)

If hand_lite and extra_lite only differ in a finger less, there is no need for another hand type. Fingers can be added or removed in a modular fashion in the xacro. I think you just need to distinguish the two different forearms.

[beatrizleon]

Thanks both. I will create the legacy repo and delete arm and muscle hands as it will be easier for us to not keep this compatibility with things that nobody needs.

[rhaschke]

Dear @beatrizleon, skimming through your changes in 417b7a0, I noticed the following potential issues:

• It seems like you removed the std sensor type. This was intended to be used as the default for Gazebo.
• You renamed the hand_type motor to E in process_hand_type_parameter.xacro, but not yet in all other locations.
• It looks like you moved many mesh files into an archive subfolder. However, git didn't recognized that they were renamed.

In general, it was rather hard to review your single commit. It would be helpful if you could split commits into smaller units with a commit message clearly stating what has changed.