Add parsing for reflected inertia parameters #14323

joemasterjohn · 2020-11-11T19:52:22Z

Modifies the SDF and URDF parser to take two custom tags:

drake:rotor_inertia
drake:gear_ratio
Used to set JointActuator::default_rotor_inertia_ and JointActuator::default_gear_ratio_.

In SDF, these custom tags are expected to appear within the <joint> tag, e.g.:

<model>
  ...
  <joint name="joint" type="revolute">
  ...
    <drake:rotor_inertia>1.5</drake:rotor_inertia>
    <drake:gear_ratio>300</drake:gear_ratio>
  ...

In URDF, these custom tags are expected to appear within the <actuator> tag within a <transmission> tag, e.g.:

<robot>
  ...
  <transmission>
    ...
    <actuator>
      ...
      <drake:rotor_inertia value="1.5" />
      <drake:gear_ratio value="300" />
      ...

Related to #12335

This change is

joemasterjohn

+@EricCousineau-TRI for feature review, please.

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

joemasterjohn

@EricCousineau-TRI: One question I had before you started reviewing was where would be the right place in public doxygen to share how custom tags should be formatted for both URDF and SDF?

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

EricCousineau-TRI · 2020-11-12T16:33:30Z

Er, good question on where to document this. At present, we don't have any existing centralized docs (see #13314 and #14107).

My suggestions:

Add a pure-doxygen file, parsing_doxygen.h, that describes custom tags. I would recommend using XPath for concise statements of where they belong, but full-form examples also help. I would recommend only adding your tags, with a TODO(#13314) to add in other tags.
Add your tags to random places in detail_urdf_parser.h and detail_sdf_parser.h. Not discoverable, but it's a start? (FTR, detail_scene_graph.h has some of its tags documented, but it's in internal and thus not discoverable to users).
Say "meh, too many options", and pass it up for now.

FYI @SeanCurtis-TRI

EricCousineau-TRI

- thanks!

Reviewed 4 of 4 files at r1.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @joemasterjohn)

multibody/parsing/detail_urdf_parser.cc, line 572 at r1 (raw file):

  if (rotor_inertia_node) {
    double rotor_inertia;
    if (!ParseScalarAttribute(rotor_inertia_node, "value", &rotor_inertia)) {

BTW I confirmed that the use of <tag>{value}</tag> in SDFormat and <tag value="{value}"/> in URDF is consistent with the hydroelastic tags.

multibody/parsing/test/detail_sdf_parser_test.cc, line 1147 at r1 (raw file):

    auto [plant, scene_graph] = ParseTestString(R"(
    <model name='ReflectedInertiaModel'>
      <link name='A'/>

nit Please use either ' or " consistently within one document.

multibody/parsing/test/detail_urdf_parser_test.cc, line 742 at r1 (raw file):

    <?xml version="1.0"?>
    <robot name="">
      <link name="A"></link>

nit Above this, <link name="X"/> is used, rather than <link name="X"></link>.

joemasterjohn

Thanks @EricCousineau-TRI. Comments addressed.

@rpoyner-tri and I talked about the possibility of your first suggestion. For now, I've added it as an item in the TODO list on #13314 and hopefully all custom tags can be consolidated at once.

Reviewable status: needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

multibody/parsing/detail_urdf_parser.cc, line 572 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW I confirmed that the use of <tag>{value}</tag> in SDFormat and <tag value="{value}"/> in URDF is consistent with the hydroelastic tags.

Thanks. I think I saw that convention when adding the bushing tags, and followed my nose here.

multibody/parsing/test/detail_sdf_parser_test.cc, line 1147 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Please use either ' or " consistently within one document.

Done. Thanks, I was being sloppy.

multibody/parsing/test/detail_urdf_parser_test.cc, line 742 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Above this, <link name="X"/> is used, rather than <link name="X"></link>.

Done.

joemasterjohn

+@jwnimmer-tri for platform review, please.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI and @jwnimmer-tri)

SeanCurtis-TRI

One of my OKRs is to solve the question of how does the relevant documentation get exposed to the real world. It's a non-trivial problem based on several desirable but mutually exclusive constraints. If you solve it before I do, I'll steal your answer. If I solve it before you do, feel free to steal my answer.

If you just want to create an issue relating to the discoverability of MBP documentation (akin to geometry's -- e.g., #14107 and #13314) and defer it, I'd be ok with that.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI and @jwnimmer-tri)

SeanCurtis-TRI

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI and @jwnimmer-tri)

multibody/parsing/detail_urdf_parser.cc, line 572 at r1 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Thanks. I think I saw that convention when adding the bushing tags, and followed my nose here.

As long as I keep getting burned by this, I'll keep adding gentle reminders.

When using "BTW" or "FYI", reviewable sets your status to "informing". That means it wants acknowledgment from everyone who is part of the review (putting red on my dashboard). If you explicitly set your self to discussing when using one of those shorthands, then it's kept between you and the author.

This has been your BTW PSA for today.

jwnimmer-tri

Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: 4 unresolved discussions (waiting on @joemasterjohn)

multibody/parsing/detail_sdf_parser.cc, line 256 at r2 (raw file):

    }

    drake::log()->trace("sdf_parser: Add JointActuator parameters.");

What is this logging for? It's logging anytime we add an actuator, but claims to be logging that we've added the parameters which we might have not, if the if-checks were false.

multibody/parsing/test/detail_sdf_parser_test.cc, line 1143 at r2 (raw file):

GTEST_TEST(SdfParser, ReflectedInertiaParametersParsing) {
  // Test successful parsing of both parameters

nit Missing punctuation.

multibody/parsing/test/detail_sdf_parser_test.cc, line 1145 at r2 (raw file):

  // Test successful parsing of both parameters
  {
    auto [plant, scene_graph] = ParseTestString(R"(

nit See https://drake.mit.edu/styleguide/cppguide.html#Raw_String_Literals for required formatting that's missing here (and the rest of the new code, below).

multibody/parsing/test/detail_sdf_parser_test.cc, line 1161 at r2 (raw file):

        <drake:gear_ratio>300.0</drake:gear_ratio>
      </joint>
    </model>)");

BTW There is enough large boilerplate here (3x) that using a fmt string for this literal would help cut it down a lot (with a {} placeholder for the variable content, to be substituted for each test case).

multibody/parsing/test/detail_urdf_parser_test.cc, line 737 at r2 (raw file):

GTEST_TEST(MultibodyPlantUrdfParserTest, ReflectedInertiaParametersParsing) {
  // Test successful parsing of both parameters

nit Missing punctuation.

joemasterjohn

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

multibody/parsing/detail_sdf_parser.cc, line 256 at r2 (raw file):