Fix for pr2_calibration package to work #55

k-okada · 2014-10-24T16:48:48Z

this patch is based on 0915e8d, but updated some definition, because http://wiki.ros.org/urdf/XML/Transmission is not sufficient
for example
Add optional for joint/calibrarion, pr2 transmission #47 need to be modified
this patch is requred for pr2_calibration package

…al robot.xml from pr2 robot

isucan · 2014-10-25T16:02:36Z

urdf_parser_py/src/urdf_parser_py/urdf.py

+	xmlr.Element('actuator', Actuator),
+	])
+
+class PR2Compensator(xmlr.Object):


We shold not put pr2 specific things in the urdfdom

We shold not put pr2 specific things in the urdfdom

I agree with you, however PR2 requires several "extension" tags and old urdf parser could handle them because on groovy pr2 calibration worked.

What is the best way to solve this issue?

Not sure what the issue is, but this is inconsistent with the C++ parser. There is no code specific to the pr2 there. The python shold be an equivalent feature set. Could we maybe have this pr2-specific parsing done in the calibration package itself?

The problem is when we load pr2 urdf which contains pr2-specific tag and output to file, undefined tags are disappeared. Does anyone knows how to fix this?

TheDash · 2014-10-30T14:43:16Z

There are a couple solutions I see here:

Put the modified parser inside the pr2_calibration package and direct the PR2 to use that modified parser. This would mean someone would have to maintain that modified parser (perhaps).
Change the URDF of the PR2 so it compiles with Hydro environment and code.

I'm not entirely sure what your error is with the URDF though. Could you explain what is going wrong and what commands reproduce it?

TheDash · 2014-10-30T14:51:58Z

I looked through the URDF, it should require a restructuring of all the transmission elements. They need hardware interfaces and an actuator tag with the mechanical reduction inside of it. Shouldn't be that big of a deal, no? The hardware interface is only checked at run time if ros_control is running and shouldn't affect anything else.

adolfo-rt · 2014-10-30T15:04:17Z

What I understand from the implementation is that the duck-typing done here should take care of identifying whether a transmission specification is PR2- or ros_control-specific, according to the <transmission> element's contents. If I interpret this correctly, it should not be necessary to add ros_control-style transmissions to the PR2.

garaemon · 2014-10-30T15:05:32Z

Another idea is to store any "illegal" tags inside of transmission
instances and dump them into xml by just copying.

2014年10月30日木曜日、Devon [email protected]さんは書きました:

I looked through the URDF, it should require a restructuring of all the
transmission elements. They need hardware interfaces and an actuator tag
with the mechanical reduction inside of it. Shouldn't be that big of a
deal, no? The hardware interface is only checked at run time if ros_control
is running and shouldn't affect anything else.

—
Reply to this email directly or view it on GitHub
#55 (comment).

from iPhone

TheDash · 2014-10-30T15:08:25Z

@adolfo-rt pointed out something key.

Edit: here is the error for those interested PR2/pr2_mechanism#324 (comment)

garaemon · 2014-10-30T15:28:47Z

I still don't understand the key...

Anyway, I didn't see any errors when we compile xacro into urdf and urdf is
correctly generated (it had "illegal" tags). But I think xacro compilation
is not important here...

The error comes up only when I run PR2 realtime controller as described
here PR2/pr2_mechanism#324
after updating urdf file according to calibration process (it's not a xacro
compilation, but urdf re-generation).

2014年10月31日金曜日、Devon [email protected]さんは書きました:

@adolfo-rt https://github.com/adolfo-rt pointed out something key.
There should be no issues here because of that. Is there an error on your
side when compiling the pr2's .xacro, @garaemon
https://github.com/garaemon ?

—
Reply to this email directly or view it on GitHub
#55 (comment).

from iPhone

TheDash · 2014-10-30T15:37:35Z

Ok, if that solution at PR2/pr2_mechanism#324 fixes the bug I see no problem in adding it. Can you make a PR for it?

garaemon · 2014-10-31T01:08:43Z

Sorry, I'm a little bit confused.

Could you summarize what we should do?

2014年10月31日金曜日、Devon [email protected]さんは書きました:

Ok, if that solution at PR2/pr2_mechanism#324
PR2/pr2_mechanism#324 fixes the bug I see no
problem in adding it. Can you make a PR for it?

—
Reply to this email directly or view it on GitHub
#55 (comment).

from iPhone

k-okada · 2014-10-31T07:28:47Z

Change the URDF of the PR2 so it compiles with Hydro environment and code.

If I understand correctly, this won't work because this information is used in pr2's controller, which is different from ros_control, so it might be big change.

The problems is if we load ad write urdf ,then un-supported tag disappers.

    robot_params = UrdfParams(robot_description, config)
    outfile.write( robot_params.get_clean_urdf().to_xml_string() )

https://github.com/ros-perception/calibration/blob/hydro/calibration_estimation/src/calibration_estimation/multi_step_cov_estimator.py#L304
https://github.com/ros-perception/calibration/blob/hydro/calibration_estimation/src/calibration_estimation/multi_step_cov_estimator.py#L371

So if there're way to avoid this in xml_reflection, that would be great.

TheDash · 2014-11-03T15:31:44Z

Ah I see now. Parsing the URDF doesn't recognize that tag so output doesn't include it and that info is needed for PR2.

To summarize then our options are:

Modify the xml parser (e.g pr2_urdfdom) to include respective tags and have a PR2 version of that parser frozen in the PR2 repos.
Change URDF and change how controllers work
Update to ros_control for pr2

and finally one last hack:

add tags back to the file that gets generated by the urdf parser to add up for the fact that part of the code isn't being generated.

What do you think?

garaemon · 2014-11-07T06:42:12Z

Sorry for late

add tags back to the file that gets generated by the urdf parser to add up for the fact that part of the code isn't being generated.

I prefer this hack at this moment. Because we need to fix PR2-hydro calibration as soon as possible.
And after that, we should fix the problem by changing PR2 URDF definieiont and change how controllers work, however it may be not so easy.

In fact, merging this pull request is the fastest solution...

isucan · 2014-11-13T07:19:42Z

There is something I am missing here. We should not need to modify the parser for the URDF to get calibration to work. How did this work before?
@hsu

mikeferguson · 2014-11-13T07:22:47Z

@isucan -- the pr2_calibration and calibration packages predate the urdf_py stuff, and had their own hacky version inside. Maybe somebody updated the code to use this package not realizing that it wouldn't work?

isucan · 2014-11-13T07:39:46Z

I see. I am reluctant to put pr2 specific hacks though in the urdf parser. Maybe we could have a hacked parser in the pr2_calibration package? The more generic solution would be to use extension tags as for gazebo, and make those generically available in the parser. Then we could have the pr2_calibration use that directly.

garaemon · 2014-11-27T08:04:30Z

I see. I am reluctant to put pr2 specific hacks though in the urdf parser. Maybe we could have a hacked parser in the pr2_calibration package?

So, what should we do now to re-work pr2 calibration on hydro?

merge this pull request
copy python script into pr2_calibration?

isucan · 2014-11-27T08:08:36Z

We should put these changes in the pr2_calibration package.

TheDash · 2014-11-27T15:50:12Z

I'm not entirely sure how this change would propagate throughout the PR2 codebase, and what would be effected. Im thinking, a modified urdfdom that has the pr2's required hack inside of it, and renaming it pr2_urdfdom. Downsides of that mean no support for an up to date urdfdom, and another package to maintain.

The longer term solution would be to modify the PR2's urdf and its mechanism controllers to get around this nonsense.. but that has repercussions for the controller manager code which I'd rather not get into the thick of.

Garaemon, if this PR is not going to be accepted, would you like to make the modifications to the pr2_mechanism?

garaemon · 2014-11-28T07:43:33Z

if this PR is not going to be accepted, would you like to make the modifications to the pr2_mechanism?

@TheDash sounds good. But before that, as you said, I prefer to have pr2_urdfdom package as a hack.

TheDash · 2014-11-28T16:48:07Z

Ok, we can put it inside the pr2_calibration stack. Let's continue the discussion at PR2/pr2_calibration#5 If you make a PR of the pr2_calibration with the pr2_urdfdom included, that would be sufficient

garaemon · 2014-12-02T07:30:47Z

I solved this issue by PR2/pr2_calibration#6

garaemon · 2014-12-02T07:34:30Z

it's just a hack

jacquelinekay · 2015-12-09T18:57:09Z

This pull request is a year old and appears to be fixed by an external hack. Hydro is EOL'd. We are also trying to move away from PR2-specfiic logic in ROS. Thus I am closing this PR.

support DockTypedFactory, copied from 0915e8d

eca4ed7

This was referenced Oct 24, 2014

fix for pr2 transmission #54

Closed

Add optional for joint/calibrarion, pr2 transmission #47

Closed

add PR2 transmission, this is similar to 484a34d, but confirmed on re…

174c1d6

…al robot.xml from pr2 robot

k-okada force-pushed the fix_for_pr2 branch from c42c193 to 174c1d6 Compare October 24, 2014 16:54

isucan reviewed Oct 25, 2014
View reviewed changes

garaemon mentioned this pull request Oct 30, 2014

size of joints of PR2GripperTransmission on hydro PR2/pr2_mechanism#324

Open

garaemon mentioned this pull request Dec 2, 2014

Fix PR2 calibration by using hacked version of urdf_parser_py by overwriting PYTHONPATH in the shell scripts. PR2/pr2_calibration#6

Merged

jacquelinekay closed this Dec 9, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for pr2_calibration package to work #55

Fix for pr2_calibration package to work #55

k-okada commented Oct 24, 2014

isucan Oct 25, 2014

garaemon Oct 26, 2014

isucan Oct 26, 2014

k-okada Oct 30, 2014

TheDash commented Oct 30, 2014

TheDash commented Oct 30, 2014

adolfo-rt commented Oct 30, 2014

garaemon commented Oct 30, 2014

TheDash commented Oct 30, 2014

garaemon commented Oct 30, 2014

TheDash commented Oct 30, 2014

garaemon commented Oct 31, 2014

k-okada commented Oct 31, 2014

TheDash commented Nov 3, 2014

garaemon commented Nov 7, 2014

isucan commented Nov 13, 2014

mikeferguson commented Nov 13, 2014

isucan commented Nov 13, 2014

garaemon commented Nov 27, 2014

isucan commented Nov 27, 2014

TheDash commented Nov 27, 2014

garaemon commented Nov 28, 2014

TheDash commented Nov 28, 2014

garaemon commented Dec 2, 2014

garaemon commented Dec 2, 2014

jacquelinekay commented Dec 9, 2015

Fix for pr2_calibration package to work #55

Fix for pr2_calibration package to work #55

Conversation

k-okada commented Oct 24, 2014

isucan Oct 25, 2014

Choose a reason for hiding this comment

garaemon Oct 26, 2014

Choose a reason for hiding this comment

isucan Oct 26, 2014

Choose a reason for hiding this comment

k-okada Oct 30, 2014

Choose a reason for hiding this comment

TheDash commented Oct 30, 2014

TheDash commented Oct 30, 2014

adolfo-rt commented Oct 30, 2014

garaemon commented Oct 30, 2014

TheDash commented Oct 30, 2014

garaemon commented Oct 30, 2014

TheDash commented Oct 30, 2014

garaemon commented Oct 31, 2014

k-okada commented Oct 31, 2014

TheDash commented Nov 3, 2014

garaemon commented Nov 7, 2014

isucan commented Nov 13, 2014

mikeferguson commented Nov 13, 2014

isucan commented Nov 13, 2014

garaemon commented Nov 27, 2014

isucan commented Nov 27, 2014

TheDash commented Nov 27, 2014

garaemon commented Nov 28, 2014

TheDash commented Nov 28, 2014

garaemon commented Dec 2, 2014

garaemon commented Dec 2, 2014

jacquelinekay commented Dec 9, 2015