[multibody] Parse screw joints from URDF #18191

scpeters · 2022-10-27T01:12:27Z

This adds support for parsing screw joints from URDF using a custom <drake:joint/> element. The screw thread pitch is specified with a value attribute in a custom <drake:screw_thread_pitch/> element. To maintain parity with the SDFormat parsing test, a continuous joint is added to the URDF parsing test to match the changes to the SDFormat test from #17956.

This change is

jwnimmer-tri · 2022-10-31T19:06:20Z

@amcastro-tri and @EricCousineau-TRI you reviewed #17956; would either of you like to sign up for this review likewise?

EricCousineau-TRI

feature, thanks! Just a nit on consistency of error message
+@jwnimmer-tri for platform review Tues, please!

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri and @scpeters)

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

        Error(*screw_thread_pitch_node,
              fmt::format("Joint {}'s drake:screw_thread_pitch does not have"
                          " a \"value\" attribute.", name));

nit These error messages do not appear consistent with those above.

I see elements denoted as <element>, and attributes denoted as 'attribute'.

EricCousineau-TRI

+@EricCousineau-TRI

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri and @scpeters)

jwnimmer-tri

platform.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @scpeters)

a discussion (no related file):
The parser's documentation needs updating to reflect the new elements/attributes in this PR.

FYI See https://drake.mit.edu/documentation_instructions.html for local preview.

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

    throw_on_custom_joint(true);
    ParseJointDynamics(node, &damping);
    // Parse screw thread pitch

nit Missing punctuation on a complete sentence.

Suggestion:

// Parse screw thread pitch.

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

    ParseJointDynamics(node, &damping);
    // Parse screw thread pitch
    double screw_thread_pitch;

Looking around within this parsing function, it seems to me like the local style is that long-winded branchy code like this new thread-pitch parsing should be delegated to helper functions, so that the main if-else chain in this function is straightforward and easy to read.

Also because I think it will help in that refactor -- in case of bad input, I think it's fine for the pitch-parsing helper to fire off an Error and then return some nominal value like 0.001 or 0.0 that will placate the ScrewJoint constructor so that we can still add the Joint to the plant. That will likely make the control flow easier, and having the Joint object with the correct name in the tree might reduce additional spurious error messages later on.

scpeters

Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The parser's documentation needs updating to reflect the new elements/attributes in this PR.

FYI See https://drake.mit.edu/documentation_instructions.html for local preview.

Ok, I've tried updating it.

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Missing punctuation on a complete sentence.

I ended up removing this comment

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Looking around within this parsing function, it seems to me like the local style is that long-winded branchy code like this new thread-pitch parsing should be delegated to helper functions, so that the main if-else chain in this function is straightforward and easy to read.

Also because I think it will help in that refactor -- in case of bad input, I think it's fine for the pitch-parsing helper to fire off an Error and then return some nominal value like 0.001 or 0.0 that will placate the ScrewJoint constructor so that we can still add the Joint to the plant. That will likely make the control flow easier, and having the Joint object with the correct name in the tree might reduce additional spurious error messages later on.

Good call; I moved the logic to a ParseScrewJointThreadPitch method

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

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit These error messages do not appear consistent with those above.

I see elements denoted as <element>, and attributes denoted as 'attribute'.

Ok, I updated to follow this style

EricCousineau-TRI

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @scpeters)

multibody/parsing/detail_urdf_parser.cc line 420 at r2 (raw file):

void UrdfParser::ParseScrewJointThreadPitch(XMLElement* node,
                                            double* screw_thread_pitch) {
  // default value of 1 mm / rev

I'm not sure if I get this comment.

How is there a default value if this attribute is required?
Also, 0.0 != 1?

jwnimmer-tri

+@rpoyner-tri for parsing_doxygen.h SME feature review, please.

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @rpoyner-tri, and @scpeters)

multibody/parsing/detail_urdf_parser.cc line 420 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I'm not sure if I get this comment.

How is there a default value if this attribute is required?
Also, 0.0 != 1?

Maybe a clearer comment would be "Always set a value for the output-only argument, even if parsing fails."

multibody/parsing/parsing_doxygen.h line 714 at r2 (raw file):

@ref reflected_inertia "Reflected Inertia"

*/

nit I think we need two new @subsections here documenting the new attribute, so that the @refs above actually go somewhere.

rpoyner-tri

Reviewed 1 of 3 files at r2.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @jwnimmer-tri)

multibody/parsing/parsing_doxygen.h line 714 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I think we need two new @subsections here documenting the new attribute, so that the @refs above actually go somewhere.

Actually, a @subsection tag_drake_screw_thread_pitch drake:screw_thread_pitch and a @subsubsection tag_drake_screw_thread_pitch_semantics Semantics.

jwnimmer-tri

Reviewed all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

A <link> never contains a <joint>, so remove the incorrect link/joint XPath expressions.

scpeters

Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @rpoyner-tri)

multibody/parsing/detail_urdf_parser.cc line 420 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Maybe a clearer comment would be "Always set a value for the output-only argument, even if parsing fails."

Ok, I've applied Jeremy's suggestion.

multibody/parsing/parsing_doxygen.h line 714 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Actually, a @subsection tag_drake_screw_thread_pitch drake:screw_thread_pitch and a @subsubsection tag_drake_screw_thread_pitch_semantics Semantics.

Ok, I've added the requested subsections and fixed a few incorrect XPath expressions that I noticed elsewhere in the file

EricCousineau-TRI

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @rpoyner-tri)

rpoyner-tri

Reviewed 3 of 5 files at r1, 1 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @scpeters)

EricCousineau-TRI · 2022-11-03T14:43:46Z

multibody/parsing/parsing_doxygen.h

@@ -708,4 +711,19 @@ object. Units are kg⋅m² for revolute joints, and kg for prismatic joints.
 @see drake::multibody::JointActuator, @ref tag_drake_gear_ratio,
 @ref reflected_inertia "Reflected Inertia"

+@subsection tag_drake_screw_thread_pitch drake:screw_thread_pitch
+
+- SDFormat path: `//model/joint/drake:screw_thread_pitch`


I'll file chaser PR to fix this to //model/joint/screw_thread_pitch given that it's part of SDFormat proper

Parse screw joints in URDF Add URDF continuous joint parsing test parsing_doxygen: Fix invalid link/joint hierarchy A <link> never contains a <joint>, so remove the incorrect link/joint XPath expressions.

scpeters mentioned this pull request Oct 27, 2022

Add SDFormat and URDF parsing for screw joint #17765

Closed

2 tasks

EricCousineau-TRI assigned jwnimmer-tri Nov 1, 2022

EricCousineau-TRI reviewed Nov 1, 2022

View reviewed changes

EricCousineau-TRI self-assigned this Nov 1, 2022

EricCousineau-TRI reviewed Nov 1, 2022

View reviewed changes

jwnimmer-tri added the release notes: feature This pull request contains a new feature label Nov 1, 2022

jwnimmer-tri reviewed Nov 1, 2022

View reviewed changes

Add URDF continuous joint parsing test

a2fcd07

scpeters force-pushed the parsing_urdf_screw_joint branch from 876a0ef to 36d1126 Compare November 2, 2022 08:23

scpeters commented Nov 2, 2022

View reviewed changes

EricCousineau-TRI reviewed Nov 2, 2022

View reviewed changes

jwnimmer-tri assigned rpoyner-tri Nov 2, 2022

jwnimmer-tri reviewed Nov 2, 2022

View reviewed changes

rpoyner-tri reviewed Nov 2, 2022

View reviewed changes

jwnimmer-tri reviewed Nov 2, 2022

View reviewed changes

scpeters added 2 commits November 2, 2022 21:23

Parse screw joints in URDF

5de5c1f

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

parsing_doxygen: fix invalid link/joint hierarchy

c27890b

A <link> never contains a <joint>, so remove the incorrect link/joint XPath expressions.

scpeters force-pushed the parsing_urdf_screw_joint branch from 36d1126 to c27890b Compare November 3, 2022 04:26

scpeters commented Nov 3, 2022

View reviewed changes

EricCousineau-TRI reviewed Nov 3, 2022

View reviewed changes

rpoyner-tri reviewed Nov 3, 2022

View reviewed changes

EricCousineau-TRI added the status: squashing now https://drake.mit.edu/reviewable.html#curated-commits label Nov 3, 2022

EricCousineau-TRI merged commit b23faa4 into RobotLocomotion:master Nov 3, 2022

EricCousineau-TRI reviewed Nov 3, 2022

View reviewed changes

EricCousineau-TRI mentioned this pull request Nov 3, 2022

parsing_doxygen: Clarify that SDFormat uses screw_thread_pitch #18255

Merged

scpeters deleted the parsing_urdf_screw_joint branch January 11, 2023 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multibody] Parse screw joints from URDF #18191

[multibody] Parse screw joints from URDF #18191

scpeters commented Oct 27, 2022 •

edited by sherm1

Loading

jwnimmer-tri commented Oct 31, 2022

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

jwnimmer-tri left a comment

scpeters left a comment

EricCousineau-TRI left a comment

jwnimmer-tri left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

scpeters left a comment

EricCousineau-TRI left a comment

rpoyner-tri left a comment

EricCousineau-TRI Nov 3, 2022

[multibody] Parse screw joints from URDF #18191

[multibody] Parse screw joints from URDF #18191

Conversation

scpeters commented Oct 27, 2022 • edited by sherm1 Loading

jwnimmer-tri commented Oct 31, 2022

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

EricCousineau-TRI Nov 3, 2022

Choose a reason for hiding this comment

scpeters commented Oct 27, 2022 •

edited by sherm1

Loading