-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Four Bar Linkage example #13036
Four Bar Linkage example #13036
Conversation
/cc @mitiguy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @joemasterjohn)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 52 at r1 (raw file):
<link name='Crank'> <pose>0 0.15 0 0 0.3490658503988659 0</pose>
nit Can you document what these radians mean in degrees?
(In SDFormat 1.8, will try to see if we can put degrees as a rotation unit at some point! And would prefer to avoid the need for Xacro too :P)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 104 at r1 (raw file):
<link name='CrankCoupler'> <pose relative_to="Crank">0 0.1 -1 0 0.9023352232810684 0</pose>
nit Can you document what the radians mean in degrees?
examples/multibody/four_bar/asymmetric_linkage.sdf, line 165 at r1 (raw file):
<!-- The origin of the Crank frame is defined to be coincident with the origin of the x-z plane in the world frame. --> <pose>-1 0.35 0 0 0.6684611035138281 0</pose>
nit Same as above - degrees?
examples/multibody/four_bar/asymmetric_linkage.sdf, line 218 at r1 (raw file):
<link name='RockerCoupler'> <pose relative_to="Rocker">0 -0.1 -2 0 -2.558652683423687 0</pose>
nit Same as above - degrees?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know who you want to do feature / platform review?
I'm happy to serve as platform for Monday.
\cc @sherm1
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @joemasterjohn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't help but write a bunch of comments. Not a full review though.
Reviewable status: 29 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @joemasterjohn)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 5 at r1 (raw file):
<!-- This sdf file produces a model with the default parameters for an
nit, why do you say "default". Maybe just remove deaultt? This creates a model.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 10 at r1 (raw file):
Links AB, BC, CD, and DA complete the closed kinematic chain. The diagram is of a planar chain in the X-Z plane.
it'd help you said:
- what is x and what is z.
- Is there gravity in the z direction?
- Which world basis vector points away (or into) the page? Wy?
- How are the link frames defined?:
- Where are their origins located?
- How are their basis defined?
Note: in monogram notation if, for instance, a frame is called A, then Ax, Ay and Az denote the x/y/z unit vectors forming the basis of that frame A. Then what you'd like to add here is the convention you use to define the link's frames? is the Ax unit vector along the link? or Az?
examples/multibody/four_bar/asymmetric_linkage.sdf, line 27 at r1 (raw file):
Rocker and the RockerCoupler link. Since closed kinematic chains are not currently supported, the coupler link
We hope kinematic chains will be supported in the future. Even by then, this example will stay relevant and a useful demonstration on how to close a kinematic chain with a bushing. Therefore I'd recommend you remove anything having to do with non-supported functionality.
The rest of the text is great though, keep it.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 104 at r1 (raw file):
<link name='CrankCoupler'> <pose relative_to="Crank">0 0.1 -1 0 0.9023352232810684 0</pose>
This essentially defines the "link's frame". Could you add a comment on where the origina of that frame is and what orientation it has (e.g. origin coincident with the location of joint B's shaft, x axis along the link, z axis away from the page and y axis forming a right-handed triplet).
examples/multibody/four_bar/asymmetric_linkage.sdf, line 106 at r1 (raw file):
<pose relative_to="Crank">0 0.1 -1 0 0.9023352232810684 0</pose> <inertial> <pose>0 0 -0.5 0 0 0</pose>
these numbers confused me. Is the z axis of this link aligned with the coupler? A brief comment would help. Where is the link's origin?
examples/multibody/four_bar/passive_simulation.cc, line 5 at r1 (raw file):
#include <gflags/gflags.h> #include "drake/common/drake_assert.h"
not used.
examples/multibody/four_bar/passive_simulation.cc, line 26 at r1 (raw file):
using geometry::SceneGraph; using lcm::DrakeLcm;
not used. Double check headers and using
directives.
examples/multibody/four_bar/passive_simulation.cc, line 59 at r1 (raw file):
"Desired duration of the simulation in seconds."); DEFINE_string(integration_scheme, "runge_kutta3",
you can use MakeSimulatorFromGflags()
which adds a bunch of useful flags for you. Not all examples were updated to use it yet and that's you might have seen the "manual" version around.
examples/multibody/four_bar/passive_simulation.cc, line 73 at r1 (raw file):
DEFINE_bool(apply_force, false, "If 'true', applies an external spatial force of -200Nm in the " "world_x direction to the RockerCoupler link's origin.");
what do you mean here with "link's origin". In Paul's document the force was applied at joint.
examples/multibody/four_bar/passive_simulation.cc, line 75 at r1 (raw file):
"world_x direction to the RockerCoupler link's origin."); DEFINE_double(default_force_stiffness, 30000,
nit, once the user sets it, is not default anymore. Maybe just call force_stiffness
?
examples/multibody/four_bar/passive_simulation.cc, line 79 at r1 (raw file):
"LinearBushingRollPitchYaw ForceElement."); DEFINE_double(default_force_damping, 800,
ditto.
examples/multibody/four_bar/passive_simulation.cc, line 95 at r1 (raw file):
SceneGraph<double>& scene_graph = *builder.AddSystem<SceneGraph>(); scene_graph.set_name("scene_graph");
nit, the order is somehow wacky. Consider having this close to the place this is used.
Right now you have:
- create scene graph.
- setup of a bunch of parameters.
- creation of MBP.
- parsing.
It'd probably look better:
- setup flags.
- Create MBP/SG in a single call, with
AddMultibodyPlantSceneGraph()
examples/multibody/four_bar/passive_simulation.cc, line 100 at r1 (raw file):
// Make the desired maximum time step a fraction of the simulation time. const double max_time_step = simulation_time / 1000.0;
nit, MakeSimulatorFromGflags() would define this for you. Less code :-)
examples/multibody/four_bar/passive_simulation.cc, line 104 at r1 (raw file):
// The target accuracy determines the size of the actual time steps taken // whenever a variable time step integrator is used. const double target_accuracy = 0.001;
nit, MakeSimulatorFromGflags()
would define this for you.
examples/multibody/four_bar/passive_simulation.cc, line 132 at r1 (raw file):
// for stiffness and damping constants. These are hand tuned to fit the // linkages used in this example and make the simulation "look right". double k_xyz = FLAGS_default_force_stiffness;
nit const here and below.
examples/multibody/four_bar/passive_simulation.cc, line 133 at r1 (raw file):
// linkages used in this example and make the simulation "look right". double k_xyz = FLAGS_default_force_stiffness; double d_xyz = FLAGS_default_force_stiffness;
copy/paste, you want force_damping here. That's why changing damping "didn't affect much"!
examples/multibody/four_bar/passive_simulation.cc, line 137 at r1 (raw file):
double d_012 = FLAGS_default_torque_damping; Vector3<double> torque_stiffness_constants = {k_xyz, k_xyz, k_xyz};
nit const here and below.
examples/multibody/four_bar/passive_simulation.cc, line 155 at r1 (raw file):
four_bar.RegisterVisualGeometry( four_bar.GetBodyByName("CrankCoupler"), // Place it at the weld point, -1m down the Z-axis of the CrankCoupler
nit, could you comment which one the z-axis is? along the coupler?
examples/multibody/four_bar/passive_simulation.cc, line 169 at r1 (raw file):
torque_source->set_name("Applied Torque"); builder.Connect(torque_source->get_output_port(), four_bar.get_actuation_input_port());
nit, if none of your joints were actuated , this would not be needed. Shorter code.
But if you wanted it for future extensions, you could do our_bar.get_actuation_input_port().FixValue(&four_bar_context, 0.0)
(below when you get a context)
examples/multibody/four_bar/passive_simulation.cc, line 172 at r1 (raw file):
// Sanity check on the availability of the optional source id before using it. DRAKE_DEMAND(!!four_bar.get_source_id());
not needed. But if you want it, this should go before you register visual geometry, which MBP does under the hood with the already registered SG.
examples/multibody/four_bar/passive_simulation.cc, line 176 at r1 (raw file):
builder.Connect( four_bar.get_geometry_poses_output_port(), scene_graph.get_source_pose_port(four_bar.get_source_id().value()));
AddMultibodyPlantSceneGraph()
does this for you. Less code again :-)
examples/multibody/four_bar/passive_simulation.cc, line 225 at r1 (raw file):
systems::IntegratorBase<double>* integrator{nullptr}; if (FLAGS_integration_scheme == "implicit_euler") {
nit, MakeSimulatorFromGflags()
would do this for you.
examples/multibody/four_bar/passive_simulation.cc, line 257 at r1 (raw file):
// Some sanity checks: if (FLAGS_integration_scheme == "semi_explicit_euler") { DRAKE_DEMAND(integrator->get_fixed_step_mode() == true);
what are you trying to sanity check here?
- that'd reveal a bug in the integrators, hopefully our unit tests cover that.
- We have more than one fixed step integrator, why emi_explicit_euler?
That is, I'd remove that.
examples/multibody/four_bar/passive_simulation.cc, line 278 at r1 (raw file):
integrator->get_num_steps_taken()); DRAKE_DEMAND(integrator->get_num_step_shrinkages_from_error_control() == 0); }
I'd remove all these checks (though I know where they come from! sry for the bad examples I sprinkled around).
You might instead want to call PrintSimulatorStatistics()
which prints lots of useful stuff.
examples/multibody/four_bar/passive_simulation.cc, line 298 at r1 (raw file):
gflags::SetUsageMessage( "A simple four bar linkage demo using Drake's MultibodyPlant" "Launch drake-visualizer before running this example.");
nit, you might want to say something less general like: A four bar linkage demo demonstrating the use of a linear bushing as a way to model kinematic loops".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting that I got an email saying that I was assigned as one of the feature reviewers, but I do not see +@
in Review discussion. Perhaps @joemasterjohn used GitHub, instead of Reviewable, to assign reviewers? @joemasterjohn usually we do like this in Reviewable:
+@DamrongGuoy for feature review please.
Reviewable status: 29 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @joemasterjohn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Joe -- After you get a chance to respond to these comments, I am happy to help out -- as you see best.
If nothing else, I'll lend some writing help for documentation.
Reviewable status: 29 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @joemasterjohn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@EricCousineau-TRI for platform review on Monday, please (assuming feature review is far enough along)
Reviewable status: 29 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, and @joemasterjohn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor question to @joemasterjohn . I'm surprised to see minor details slightly different from Paul's FourBarLinkageForDrakeExample.pdf like:
- X-Z plane instead of X-Y plane
- Paul's doc said
links A, B, C and ground N
. The SDF file saidCrank, Coupler, Rocker, World?
(I am guessing the ground is the world.) - 10cm longer link (e.g., 1.1m instead of 1m)
- Paul's doc said qA ≈ 20.0°, qB ≈ 71.7°, qC ≈ 38.3°; however, the SDF uses radians, I think.
This is just my curiosity. Perhaps the differences are necessary due to the way Drake works. I never created SDF files myself.
I would sympathize with you if you had to create SDF files from scratch.
Checkpoint. Still early in the review.
Reviewable status: 29 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
a discussion (no related file):
Anyone has opinion about adding Paul's PDF file FourBarLinkageForDrakeExample.pdf into this directory multibody/four_bar/ ? I feel like the PDF helped me a lot in understanding what we try to do.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 10 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
it'd help you said:
- what is x and what is z.
- Is there gravity in the z direction?
- Which world basis vector points away (or into) the page? Wy?
- How are the link frames defined?:
- Where are their origins located?
- How are their basis defined?
Note: in monogram notation if, for instance, a frame is called A, then Ax, Ay and Az denote the x/y/z unit vectors forming the basis of that frame A. Then what you'd like to add here is the convention you use to define the link's frames? is the Ax unit vector along the link? or Az?
If Joe was like me, when I started, I was so confused about Frames and Bodies. I still have to look at this doc regularly: https://drake.mit.edu/doxygen_cxx/group__multibody__frames__and__bodies.html
For Joe, that doc said W stands for World, so we can say "frame W" for World's coordinates frame. And each body (Crank, Rocker, Coupler) has its own coordinates frame, possibly with one-letter name like frame F, frame G, frame H.
Now, the hard part is that we call the bodies "Crank", "Rocker", "Coupler" without one-letter aliases, and the letters A,B,C,D were taken for the joints. Perhaps we can take A,B,C,D back and have something like Paul's document:
Crank is body A
Coupler is body B
Rocker is body C
Ground is body N with frame N identified with World frame.
and the joints A, B, C, D in this SDF could be described indirectly like this in Paul's document?
CrankJoint coincides with A_o and N_o.
CrankCouplerJoint coincides with A_B and B_o.
RockerJoint coincides with C_o and N_C
As I write this comment, I am going back and forth many times, and I still do not like my proposal. Admittedly this is not easy. Perhaps this is a good time to ask @mitiguy for help?
examples/multibody/four_bar/asymmetric_linkage.sdf, line 45 at r1 (raw file):
+ - - - - - - - - - - - - + C (Coupler) 2m
I wonder how hard it is to make this picture closer to Paul's picture:
examples/multibody/four_bar/asymmetric_linkage.sdf, line 52 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you document what these radians mean in degrees?
(In SDFormat 1.8, will try to see if we can put degrees as a rotation unit at some point! And would prefer to avoid the need for Xacro too :P)
Yes, degrees would be easier to understand! I think Paul's pdf file said something like qA ≈ 20.0°, qB ≈ 71.7°, qC ≈ 38.3°
, where A=Crank, B=Coupler, C=Rocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for a Drake example we want to use user-friendly names like crank, coupler, rocker, and world. But the one-letter names are better for describing the math. Ideally we can bend Paul's notation and the code a little to bring them closer. An easy one is that we should use W in the pdf rather than N, since World is the term used everywhere in Drake and in the robotics field in general. Then maybe change one of the Drake names so all four have unique letters? How about crank, linker, rocker, world: C L R W. (The pdf could change to use those letters.)
Reviewable status: 29 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
a discussion (no related file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Anyone has opinion about adding Paul's PDF file FourBarLinkageForDrakeExample.pdf into this directory multibody/four_bar/ ? I feel like the PDF helped me a lot in understanding what we try to do.
I like this idea but I think the pdf and example should evolve towards each other a little more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments so far, everyone. I'll have some time to address them all over the weekend. But for now:
- X-Z plane just to match the default gravity vector in Drake
- The link names were just to introduce the user to historical naming conventions. I don't have much preference there.
- The actual
<pose>
elements of the links match the dimensions of Paul's notes. The visual elements look a little nicer (to me) with 0.05m margins around the revolute joints and the links out of plane with each other (looks a bit more physically realizable). Admittedly, the<inertia>
tags are set to match the visuals as well, but I can recalculate those and the example will match Paul's write-up in the important quantities. (Actually I'm not certain of the geometry implied in the write-up. Whether it's solid cylinders, thin rods, point masses at the midpoint of the links, or something else.) - I think SDF only accepts radians (correct me if I'm wrong). Paul's example measures each angle relative to what he calls the n_x axis, but it is much easier to express the
<pose>
of child links relative to the parent rather than the world. Fewer numbers in the SDF overall.
Reviewable status: 29 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be best if both the PDF and SDF can evolve closer.
Thank you @joemasterjohn for explanation. I agree with everything you said. Indeed X-Z plane is a constraint from Drake. It looks nicer to have the links out of plane with each other. The visual elements look nicer by adding 5cm margins. Expressing child links relative to the parent is easier (with perhaps explanation how it was derived from n_x measured in PDF).
I like C, L, R, W (crank, linker, rocker, world).
It looks like Alejandro already gave Joe enough to work on, and I'm not an expert in multibody. I'll wait for the next revision, which I believe will have great improvement.
Reviewable status: 30 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 10 at r1 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
If Joe was like me, when I started, I was so confused about Frames and Bodies. I still have to look at this doc regularly: https://drake.mit.edu/doxygen_cxx/group__multibody__frames__and__bodies.html
For Joe, that doc said W stands for World, so we can say "frame W" for World's coordinates frame. And each body (Crank, Rocker, Coupler) has its own coordinates frame, possibly with one-letter name like frame F, frame G, frame H.Now, the hard part is that we call the bodies "Crank", "Rocker", "Coupler" without one-letter aliases, and the letters A,B,C,D were taken for the joints. Perhaps we can take A,B,C,D back and have something like Paul's document:
Crank is body A Coupler is body B Rocker is body C Ground is body N with frame N identified with World frame.
and the joints A, B, C, D in this SDF could be described indirectly like this in Paul's document?
CrankJoint coincides with A_o and N_o. CrankCouplerJoint coincides with A_B and B_o. RockerJoint coincides with C_o and N_C
As I write this comment, I am going back and forth many times, and I still do not like my proposal. Admittedly this is not easy. Perhaps this is a good time to ask @mitiguy for help?
Please feel free to ignore my comment above. I think Sherm's suggestion C L R W (Crank, Link, Rocker, World) is a better solution.
examples/multibody/four_bar/passive_simulation.cc, line 91 at r1 (raw file):
"LinearBushingRollPitchYaw ForceElement."); int do_main() {
There are only two functions in this file; the 7-line main()
and this 198-line do_main()
. Could we split this function into several functions? F
Or @amcastro-tri 's suggestion about AddMultibodyPlantSceneGraph()
and PrintSimulatorStatistics()
can save lines of code enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 30 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 45 at r1 (raw file):
+ - - - - - - - - - - - - + C (Coupler) 2m
I'm lost about the direction of World's X and Z direction (Wx, Wz). Is it like this?
Wz
^
|
Wx <-----+
A D
+ - + - - - - - - - - - - - - - - - - - - - - - - - - + - - - - - +
\_ \
\_ \
(Crank) \_ \ (Rocker)
1m \_ \ 2m
\ (CrankCoupler) E \
B + - - - - - - - - - - + (RockerCoupler) \
+ - - - - - - - - - - - - + C
(Coupler)
2m
Drawing Wx and Wz would help a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joemasterjohn -- I think it may make sense to again update the picture and documentation to make this easier for everyone. Let's talk later today after the 1:30 meeting.
Reviewable status: 32 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 32 unresolved discussions, LGTM missing from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 10 at r1 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Please feel free to ignore my comment above. I think Sherm's suggestion C L R W (Crank, Link, Rocker, World) is a better solution.
Thanks for the link to those docs @DamrongGuoy. @joemasterjohn , please do not hesitate to ask if you have any trouble following our frames/notation conventions. Much easier to explain via VC than in a doc for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-@EricCousineau-TRI for now - feel free to reassign / use platform reviewer once feature review is complete!
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joemasterjohn I confirm what I see. The file asymmetric_linkage.sdf was deleted intentionally?
Just to be sure, you are using Reviewable, right? (https://reviewable.io/reviews/RobotLocomotion/drake/13036#-) I asked this trivial question because I do not see you answer any Reviewable comments.
Usually when I change the code according to a comment, I typed "Done" in the reply to the comment. Or I explain briefly why I do not agree with the comment.
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, @joemasterjohn you addressed this comment already. I would expect that you type "Done" or click on "Acknowledge".
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After that, Reviewable will tell @amcastro-tri that @joemasterjohn fixed it already, and Alejandro can unblock it (change -
to ✔️ ).
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes asymmetric_linkage.sdf was deleted intentionally. I decided to include just one four bar model, and have it match Paul's diagram exactly:
And yes, sorry, let me go through and resolve all of the comments I've addressed.
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still writing the README that will contain that image.
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice picture!
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is! All credit to @mitiguy.
Reviewable status: 32 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 33 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 6 at r1 (raw file):
This sdf file produces a model with the default parameters for an asymmetric four bar linkage.
I agree with Alejandro and also remove "asymmetric".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 24 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @DamrongGuoy, @EricCousineau-TRI, @joemasterjohn, and @mitiguy)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 6 at r1 (raw file):
Previously, mitiguy (Mitiguy) wrote…
I agree with Alejandro and also remove "asymmetric".
Now the sole sdf if named four_bar.sdf
examples/multibody/four_bar/asymmetric_linkage.sdf, line 10 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
Thanks for the link to those docs @DamrongGuoy. @joemasterjohn , please do not hesitate to ask if you have any trouble following our frames/notation conventions. Much easier to explain via VC than in a doc for sure
Ok, I rewrote the comments in the SDF to match the names in Paul's diagram, as well as expressing the frames in monogram notation. Please let me know if I've made any mistakes.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 27 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
We hope kinematic chains will be supported in the future. Even by then, this example will stay relevant and a useful demonstration on how to close a kinematic chain with a bushing. Therefore I'd recommend you remove anything having to do with non-supported functionality.
The rest of the text is great though, keep it.
Done.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 45 at r1 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I wonder how hard it is to make this picture closer to Paul's picture:
New draft should match Paul's picture.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 45 at r1 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I'm lost about the direction of World's X and Z direction (Wx, Wz). Is it like this?
Wz ^ | Wx <-----+ A D + - + - - - - - - - - - - - - - - - - - - - - - - - - + - - - - - + \_ \ \_ \ (Crank) \_ \ (Rocker) 1m \_ \ 2m \ (CrankCoupler) E \ B + - - - - - - - - - - + (RockerCoupler) \ + - - - - - - - - - - - - + C (Coupler) 2mDrawing Wx and Wz would help a lot.
I decided to remove the ASCII art because it was getting cluttered and the picture shows the details with much more clarity.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 104 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
This essentially defines the "link's frame". Could you add a comment on where the origina of that frame is and what orientation it has (e.g. origin coincident with the location of joint B's shaft, x axis along the link, z axis away from the page and y axis forming a right-handed triplet).
Done.
examples/multibody/four_bar/asymmetric_linkage.sdf, line 106 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
these numbers confused me. Is the z axis of this link aligned with the coupler? A brief comment would help. Where is the link's origin?
Done.
examples/multibody/four_bar/passive_simulation.cc, line 5 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
not used.
Done.
examples/multibody/four_bar/passive_simulation.cc, line 26 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
not used. Double check headers and
using
directives.
Done.
examples/multibody/four_bar/passive_simulation.cc, line 59 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
you can use
MakeSimulatorFromGflags()
which adds a bunch of useful flags for you. Not all examples were updated to use it yet and that's you might have seen the "manual" version around.
Done.
examples/multibody/four_bar/passive_simulation.cc, line 73 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
what do you mean here with "link's origin". In Paul's document the force was applied at joint.
Parameter removed to simplify the example.
examples/multibody/four_bar/passive_simulation.cc, line 79 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
ditto.
Done.
examples/multibody/four_bar/passive_simulation.cc, line 91 at r1 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
There are only two functions in this file; the 7-line
main()
and this 198-linedo_main()
. Could we split this function into several functions? FOr @amcastro-tri 's suggestion about
AddMultibodyPlantSceneGraph()
andPrintSimulatorStatistics()
can save lines of code enough?
I think do_main()
has been cut down a lot. How do you feel about the length now?
examples/multibody/four_bar/passive_simulation.cc, line 95 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, the order is somehow wacky. Consider having this close to the place this is used.
Right now you have:
- create scene graph.
- setup of a bunch of parameters.
- creation of MBP.
- parsing.
It'd probably look better:
- setup flags.
- Create MBP/SG in a single call, with
AddMultibodyPlantSceneGraph()
Changed. Now I have:
- Create MBPlant and SceneGraph
- Parse the sdf
- Add bushing and finalize model.
- Create Context and set initial conditions
- Simulate
What do you think?
examples/multibody/four_bar/passive_simulation.cc, line 133 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
copy/paste, you want force_damping here. That's why changing damping "didn't affect much"!
Done.
examples/multibody/four_bar/passive_simulation.cc, line 155 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, could you comment which one the z-axis is? along the coupler?
All visuals moved to SDF file.
examples/multibody/four_bar/passive_simulation.cc, line 172 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
not needed. But if you want it, this should go before you register visual geometry, which MBP does under the hood with the already registered SG.
Removed.
examples/multibody/four_bar/passive_simulation.cc, line 176 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
AddMultibodyPlantSceneGraph()
does this for you. Less code again :-)
Done.
examples/multibody/four_bar/passive_simulation.cc, line 257 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
what are you trying to sanity check here?
- that'd reveal a bug in the integrators, hopefully our unit tests cover that.
- We have more than one fixed step integrator, why emi_explicit_euler?
That is, I'd remove that.
Taken from previous example. Removed.
examples/multibody/four_bar/passive_simulation.cc, line 278 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I'd remove all these checks (though I know where they come from! sry for the bad examples I sprinkled around).
You might instead want to call
PrintSimulatorStatistics()
which prints lots of useful stuff.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 21 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @DamrongGuoy, @EricCousineau-TRI, and @mitiguy)
examples/multibody/four_bar/asymmetric_linkage.sdf, line 52 at r1 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Yes, degrees would be easier to understand! I think Paul's pdf file said something like
qA ≈ 20.0°, qB ≈ 71.7°, qC ≈ 38.3°
, where A=Crank, B=Coupler, C=Rocker.
I did you one better and acutally gave the trigonometry for the initial angles in the README. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 21 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @DamrongGuoy, @EricCousineau-TRI, and @mitiguy)
a discussion (no related file):
Previously, sherm1 (Michael Sherman) wrote…
I like this idea but I think the pdf and example should evolve towards each other a little more.
I think the example and the pdf match now. There are slight differences (e.g. links out of plane with each other to make the visualization nicer), but not much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 32 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @joemasterjohn)
examples/multibody/four_bar/README.md, line 3 at r12 (raw file):
# Four-Bar Linkage Example This planar four-bar linkage demonstrates how to use a bushing to approximate a closed kinematic chain. It loads an SDF model from the
BTW regarding suggestions for playing with this example that yield some intuition:
- change the force_stiffness to 300 (from 30000) and watch what happens to the bushing joint
- now rerun with force_damping set to zero and watch how the joint oscillates
- try setting
crank_torque
to 1000 and watch how the large forces interact with the bushing stiffness (assuming you elevate that to a command line parameter)
Consider suggesting some experiments like that to help the user see what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 32 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @joemasterjohn)
examples/multibody/four_bar/passive_simulation.cc, line 11 at r12 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I'm just curious. In general, do we have a policy on having
.h
file forexamples/
? I'm not suggesting to change anything. Just my curiosity.
A lot of the more complicated examples do have some header files. But I don't know of any policy about it. I suppose it could be useful to include a header file that can be scraped by Doxygen to help someone find the example. Still I think the examples are not really useful unless someone looks at the code, and Doxygen doesn't help with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 16 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @joemasterjohn and @sherm1)
examples/multibody/four_bar/four_bar.sdf, line 31 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: the previous two sentences are redundant since you already said X_WA was identity. If you still want to emphasize what that means, you could say "That is, ..." As written I was confused for a while because I assumed this was adding new information.
Ok, removed.
examples/multibody/four_bar/four_bar.sdf, line 56 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Do you mean these symbols to the joint angles? Just below they are used in
q_WA = q_AB = q_WC = 0
which indicates they are scalars. Please clarify this non-standard notation. If you are using the same symbol for two different meanings, that's asking for trouble. Why not call the jointsjoint_WA
and the corresponding coordinateq_WA
or something like that?
Yes, I used both to refer to the name of the joint, and its angle. Changed names to joint_X
and angles to q_X
.
examples/multibody/four_bar/four_bar.sdf, line 257 at r12 (raw file):
π − tan⁻¹(√15)
ok, thanks for typing it out so I didn't have to.
examples/multibody/four_bar/passive_simulation.cc, line 99 at r10 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Thank you. Now I get it. It's like "vegetarian dishes sans meat, eggs". Set to Satisfied now.
Yes, just me being a little too French lol.
examples/multibody/four_bar/passive_simulation.cc, line 11 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
A lot of the more complicated examples do have some header files. But I don't know of any policy about it. I suppose it could be useful to include a header file that can be scraped by Doxygen to help someone find the example. Still I think the examples are not really useful unless someone looks at the code, and Doxygen doesn't help with that.
I think of this comment as just a road sign pointing to the README if one were to get excited and open the code first. I'll make the change /**
-> /*
.
examples/multibody/four_bar/passive_simulation.cc, line 47 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: I found these names confusing for a while because I was looking for "translational" and "rotational" stiffness. But I see these names are closer to the bushing documentation. Please consider using those terms in the description, e.g.
force_stiffness Translational stiffness value ...
or
force_stiffness Force (translational) stiffness value
. Similarlytorque_stiffness Rotational stiffness ...
.
Done.
examples/multibody/four_bar/passive_simulation.cc, line 77 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
nit: in monogram notation it would be more natural to write those Bc and Cb -- we use one-capital-letter names for frames. "CB" looks like two frames in monogram, like when we write a rotation R_CB to orient frame B in frame C.
Done.
examples/multibody/four_bar/passive_simulation.cc, line 83 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
nit: consider "Bc_bushing" and "Cb_bushing".
Done
examples/multibody/four_bar/passive_simulation.cc, line 121 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: this line is unnecessary since you already said CreateDefaultContext() (that invokes SetDefaultContext() for you).
Done.
examples/multibody/four_bar/passive_simulation.cc, line 123 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
FYI a more object-oriented alternative, slightly nicer?
Context<double>& four_bar_context = four_bar.GetMyContextFromRoot(diagram_context.get());
I like it. Just checking, did you mean GetMyMutableContextFromRoot
?
examples/multibody/four_bar/passive_simulation.cc, line 126 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Oh, this is great -- I didn't realize you had an actuator. Can you make the applied torque a command line argument so users can play with it? Then the Tᴀ in the README drawing makes sense.
Sure thing. I had an example of a simple LeafSystem
that implemented the desired velocity controller mentioned in the writeup, but I thought it cluttered the example. I'll make an argument for applied constant torque.
examples/multibody/four_bar/passive_simulation.cc, line 134 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: per earlier comment, the names in the sdf look like generalized coordinate names, not joint names. However the variable names they get here look great!
Ok, I changed the names in the SDF to be consistent with the code. Also I put joint_
first just to be even more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r13.
Reviewable status: 13 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @joemasterjohn, and @sherm1)
examples/multibody/four_bar/passive_simulation.cc, line 123 at r12 (raw file):
Previously, joemasterjohn (Joe Masterjohn) wrote…
I like it. Just checking, did you mean
GetMyMutableContextFromRoot
?
Right! Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r13.
Reviewable status: 13 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy, @joemasterjohn, and @sherm1)
examples/multibody/four_bar/README.md, line 3 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW regarding suggestions for playing with this example that yield some intuition:
- change the force_stiffness to 300 (from 30000) and watch what happens to the bushing joint
- now rerun with force_damping set to zero and watch how the joint oscillates
- try setting
crank_torque
to 1000 and watch how the large forces interact with the bushing stiffness (assuming you elevate that to a command line parameter)Consider suggesting some experiments like that to help the user see what's going on.
I like Sherm's suggestions. I tried it, and it's very educational. I cannot do the crank_torque
though, at least not yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @DamrongGuoy, @joemasterjohn, and @sherm1)
examples/multibody/four_bar/passive_simulation.cc, line 143 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: per f2f these coordinate names are nice and match the figure. There is some danger of ambiguity with the one-letter tag but that could be made clear in documentation. The code here is clear because qA goes with WA_joint, qB goes with AB_joint, etc.
SDF and README changed to match the code.
examples/multibody/four_bar/README.md, line 104 at r11 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
actually. This is what I remember from my control days. People give away a bit of overshoot in exchange for a faster rise time. Typically if you are willing to accept say a 5% to 10% overshoot, you can get a faster response if you need to. I'm not advocating to include that here, I only added it for completeness to the discussion.
I still believe for our purposes ζ = 1 is best.
Changed to reflect the documentation from Paul in LinearBushingRollPitchYaw
.
examples/multibody/four_bar/README.md, line 3 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: please consider some more text here covering
- how to run the example
- what you'll see when you run it and what that means
- what options are available for changing the behavior (if that's relevant)
Done.
examples/multibody/four_bar/README.md, line 3 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW regarding suggestions for playing with this example that yield some intuition:
- change the force_stiffness to 300 (from 30000) and watch what happens to the bushing joint
- now rerun with force_damping set to zero and watch how the joint oscillates
- try setting
crank_torque
to 1000 and watch how the large forces interact with the bushing stiffness (assuming you elevate that to a command line parameter)Consider suggesting some experiments like that to help the user see what's going on.
Ok, I've added examples of how to run with the different parameters you suggested and what to expect to see for each.
examples/multibody/four_bar/README.md, line 10 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: replacing a pin joint with a bushing also requires force and torque stiffness and damping, except along the joint axis. It is only because this is a planar system that the other directions are already constrained. Since this is our only tutorial-like example for making closed-chain systems, please explain how that's done in general and then narrow it to this special case if necessary.
I note that in a comment in the code, but I'll adapt it for the readme.
examples/multibody/four_bar/README.md, line 12 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: not clear what's meant by "statics" here. I guess the system is at rest hanging in gravity -- consider a sentence just saying that. Since the system is symmetric this seems like sort of an obvious configuration which would make sense whether we talk about statics or not.
Relic from the original pdf, 'statics' -> 'model'.
examples/multibody/four_bar/README.md, line 30 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
The figure does not match the image I'm seeing in the visualizer. There is a triangular-shaped part in the image. There is also something call TA that is in the image and described as "motor torque" -- is there an analog to that in the code? Please make sure the image accurately reflects what you modeled.
Also the symbols used in the image ax ay etc. are not in Drake notation. We use Ax, Ay, Az for the axis unit vectors associated with a frame A. Please consider doing that here, or at least explain why this would use non-standard notation (and maybe provide a translation in that case?).
Ok, modified to be more consistent. No triangle in the code, and a floating coupler point added to the SDF. Axes changed in the image and README to be standard Drake notation.
examples/multibody/four_bar/README.md, line 30 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Also it looks like the figure is using different notation for the joint angles than the code uses -- q_B rather than q_AB for example. Per f2f, the one-letter form seems ambiguous since the links have joints at both ends.
Done. Went with joint names joint_WA, joint_AB, joint_WC
and joint angles qA, qB, qC respectively, to be succinct.
examples/multibody/four_bar/README.md, line 69 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
nit: internal symbol M_PI leaked out here -- how about π instead!
Done.
examples/multibody/four_bar/README.md, line 81 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: Bc and Cb are good point names but there is some inconsistency about whether the points are Bc Cb / BC CB / Bᴄ Cʙ (the last have small caps). Please use consistent notation between the sdf README and drawing (unless there is a good reason not to). I would suggest Bc and Cb (with normal letters) since that matches our standard notation for points (a capital letter, sometimes with a lower case letter as a modifier), and the font is available everywhere.
Good point. Changed to Bc and Cb.
examples/multibody/four_bar/README.md, line 125 at r12 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
How about just θₘₐₓ ?
I like θₘₐₓ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok finished with most of the suggestions. Just working on rewriting the last section of the README to reflect and point to the documentation in LinearBushingRollPitchYaw
.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @DamrongGuoy, @joemasterjohn, and @sherm1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r14.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @joemasterjohn, and @sherm1)
examples/multibody/four_bar/four_bar.sdf, line 176 at r14 (raw file):
<diffuse>0 0 1 1</diffuse> </material> </visual>
It is cool to see the blue ball!
examples/multibody/four_bar/README.md, line 3 at r12 (raw file):
Previously, joemasterjohn (Joe Masterjohn) wrote…
Ok, I've added examples of how to run with the different parameters you suggested and what to expect to see for each.
Thank you! I confirm each of these command-line parameters works now:
--force_stiffness
, --force_damping
, --applied_torque
(for crank_torque).
examples/multibody/four_bar/README.md, line 12 at r12 (raw file):
Previously, joemasterjohn (Joe Masterjohn) wrote…
Relic from the original pdf, 'statics' -> 'model'.
I guess it's "statics mechanics" (zero acceleration, F = m A = 0). I agree "Four-bar linkage model" sounds better than "Four-bar linkage statics".
examples/multibody/four_bar/README.md, line 26 at r14 (raw file):
To change the initial velocity of `joint_WA`, q̇A in radians/second :bazel run //examples/multibody/four_bar:passive_simulation -- --initial_velocity=<desired_velocity>
I like --initial_velocity
. I saw that increasing it from 3.0 to 4.0 radians/second let the blue ball make a complete cardioid. It's surprisingly nice to see the cardioid. I have mistakenly thought that it would make a circle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great now! Just a few minor comments, plus I agree some revision is needed to the last section of the README to reflect the discussion in #13106.
Reviewed 4 of 4 files at r14.
Reviewable status: 14 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri and @joemasterjohn)
examples/multibody/four_bar/four_bar.sdf, line 52 at r14 (raw file):
origin of that link's frame, oriented along the world's y-axis. joint_WA is the y-axis revolute joint between the parent world and the child A.
nit: line length <= 80 (META, I see 7 long lines sprinkled about)
examples/multibody/four_bar/README.md, line 30 at r12 (raw file):
Previously, joemasterjohn (Joe Masterjohn) wrote…
Ok, modified to be more consistent. No triangle in the code, and a floating coupler point added to the SDF. Axes changed in the image and README to be standard Drake notation.
Looks great now, thanks @mitiguy & @joemasterjohn !
examples/multibody/four_bar/README.md, line 116 at r14 (raw file):
In this example, we replace the pin joint at point **Bc** (see diagram) that connects links *B* and *C* with a `ForceElement::LinearBushingRollPitchYaw`(there are many other uses of a
minor:
- The decorated name is
drake::multibody::LinearBushingRollPitchYaw
. It is a force element but that's not part of the name. - Would be good to make this a clickable link in the README. The URL for the posted Doxygen for this is
https://drake.mit.edu/doxygen_cxx/classdrake_1_1multibody_1_1_linear_bushing_roll_pitch_yaw.html
and should be stable. In Markdown-speak, a clickable link is made with[label](url)
so this one can be:
[drake::multibody::LinearBushingRollPitchYaw](https://drake.mit.edu/doxygen_cxx/classdrake_1_1multibody_1_1_linear_bushing_roll_pitch_yaw.html)
(The line will end up over 80 characters but that's OK.)
I would suggestion replacing LinearBushingRollPitchYaw with the link wherever it occurs in this document. Later if @anchor
labels are used in the bushing class documentation you could make the links more specific to a particular section but for now just jumping to the class documentation seems fine.
examples/multibody/four_bar/README.md, line 129 at r14 (raw file):
direction, and three of the four revolute joints here are indeed modeled that way. However, in order to close the kinematic loop we have to use a bushing as "penalty method" substitute for hard constraints. That is, because
nit "as" -> "as a"
examples/multibody/four_bar/README.md, line 138 at r14 (raw file):
Similarly, would angular errors of 1 degree (say) be tolerable? We will give a procedure below for estimating a reasonable value of k to achieve a specified translational and rotational tolerance. Similarly, we need to choose
nit: "Similarly" -> "Also"
examples/multibody/four_bar/README.md, line 143 at r14 (raw file):
of 1ms (say) would be ignorable for your robot arm, which presumably has much larger time constants for important behaviors. We will give a procedure here for obtaining a reasonable d from k and your settling
nit: there appears to be a trailing blank and the end of most of the lines in this paragraph.
examples/multibody/four_bar/README.md, line 146 at r14 (raw file):
time tolerance. ## Estimate force stiffness [kx ky kz] from loading/displacement
minor: the subheadings should be smaller than the ##
main heading (you can use ###
to shrink them).
examples/multibody/four_bar/README.md, line 150 at r14 (raw file):
approximated via various methods (or a combination thereof). For example, one could specify a maximum bushing displacement in a direction (e.g., xMax), estimate a maximum directional load (Fx) that
nit: consider xₘₐₓ for consistency with θₘₐₓ used below
examples/multibody/four_bar/README.md, line 166 at r14 (raw file):
Values for k can be determined by choosing a characteristic mass m (which may be directionally dependent) and then choosing ωₙ. One way to pick ωₙ is to choose a settling_time which approximates the
nit: "settling_time" -> "settling time tₛ" (or something like that, then use tₛ in the expression below rather than the code identifier).
examples/multibody/four_bar/README.md, line 168 at r14 (raw file):
One way to pick ωₙ is to choose a settling_time which approximates the desired time for the bushing to settle to within 5% of an equilibrium solution, use ωₙ ≈ 5 / settling_time, and then k ≈ m ωₙ².
minor: this isn't quite right. Please see the discussion in #13106 or consult with @mitiguy.
examples/multibody/four_bar/README.md, line 173 at r14 (raw file):
Generally, a stiffer bushing more closely resembles an ideal revolute joint. However (depending on integrator) a stiffer bushing increase numerical integration time.
minor: this short section is oddly placed because it applies equally to the torque stiffness in the next section. Relocate to cover both cases?
examples/multibody/four_bar/README.md, line 192 at r14 (raw file):
Alternatively, a value for k₀ can be determined by choosing a characteristic moment of inertia I₀ (which is directionally dependent) and then choosing ωₙ (e.g., from setting_time), then using k₀ ≈ m ωₙ².
nit: "settling_time" -> "settling time"
examples/multibody/four_bar/README.md, line 192 at r14 (raw file):
Alternatively, a value for k₀ can be determined by choosing a characteristic moment of inertia I₀ (which is directionally dependent) and then choosing ωₙ (e.g., from setting_time), then using k₀ ≈ m ωₙ².
minor: k₀ ≈ I₀ ωₙ² (rather than m)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)
examples/multibody/four_bar/README.md, line 104 at r11 (raw file):
Previously, joemasterjohn (Joe Masterjohn) wrote…
Changed to reflect the documentation from Paul in
LinearBushingRollPitchYaw
.
Done.
examples/multibody/four_bar/README.md, line 116 at r14 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor:
- The decorated name is
drake::multibody::LinearBushingRollPitchYaw
. It is a force element but that's not part of the name.- Would be good to make this a clickable link in the README. The URL for the posted Doxygen for this is
https://drake.mit.edu/doxygen_cxx/classdrake_1_1multibody_1_1_linear_bushing_roll_pitch_yaw.html
and should be stable. In Markdown-speak, a clickable link is made with[label](url)
so this one can be:[drake::multibody::LinearBushingRollPitchYaw](https://drake.mit.edu/doxygen_cxx/classdrake_1_1multibody_1_1_linear_bushing_roll_pitch_yaw.html)
(The line will end up over 80 characters but that's OK.)
I would suggestion replacing LinearBushingRollPitchYaw with the link wherever it occurs in this document. Later if
@anchor
labels are used in the bushing class documentation you could make the links more specific to a particular section but for now just jumping to the class documentation seems fine.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All comments addressed, README modified to reflect #13106, reference to the documentation for LinearBushingRollPitchYaw
added. PTAL
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @DamrongGuoy, and @sherm1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @DamrongGuoy, @joemasterjohn, and @sherm1)
examples/multibody/four_bar/README.md, line 110 at r15 (raw file):
trigonometry. 𝐪ᴀ is one angle of a right triangle with its adjacent side measuring 1 m and its hypotenuse measuring 4 m. Hence, initially 𝐪ᴀ = tan⁻¹(√15) ≈ 1.318 radians ≈ 104.48°.
nit: 1.318 radians is about 75.52°, not 104.48°.
examples/multibody/four_bar/README.md, line 113 at r15 (raw file):
Because link *B* is parallel to **Ŵ**𝐱, 𝐪ᴀ and 𝐪ʙ are supplementary, hence the initial value is 𝐪ʙ = π - 𝐪ᴀ ≈ 1.823 radians ≈ 75.52°.
nit: 1.823 radians is about 104.48°, not 75.52°.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful -- platform
One nit and a couple of ignorable BTWs, PTAL.
Reviewed 1 of 1 files at r15.
Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri and @joemasterjohn)
examples/multibody/four_bar/README.md, line 166 at r15 (raw file):
| ----- | ---- | | m ẍ + dx ẋ + kx x = 0 | or alternatively as | | ẍ + 2 ζ ωₙ ẋ + ωₙ² x = 0 | where ωₙ = √(kx/m), ζ = dx / (2 √(m kx)) |
BTW an ignorable thought -- kx and dx in math expressions look like multiplication of two symbols. Because unicode does have a subscript x (though not y and z) you could write kₓ and dₓ here, which are easier to see as single symbols even though they don't quite match the text symbols kx dx above, which have to be written like that so kx ky kz and dx dy dz can look related. But y and z don't appear in this expression.
examples/multibody/four_bar/README.md, line 178 at r15 (raw file):
### Estimate force damping [dx dy dz] from mass and stiffness Once m and kx have been chosen, damping dx can be estimated by picking a damping ratio ζ (e.g., ζ ≈ 1, critical damping), then d ≈ 2 ζ √(m k).
nit k -> kx to match the rest of the notation
examples/multibody/four_bar/README.md, line 195 at r15 (raw file):
characteristic moment of inertia I₀ (which is directionally dependent) and then choosing ωₙ (e.g., from setting time), then using k₀ ≈ I₀ ωₙ². With k₀ available and a damping ratio ζ chosen, d₀ ≈ 2 ζ √(I₀ k₀).
BTW did you want to provide an example of how you used the above approach to get stiffness and damping coefficients for the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amcastro-tri you have one comment that is still blocking though I think it has been addressed.
Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri and @joemasterjohn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri and @DamrongGuoy)
examples/multibody/four_bar/README.md, line 110 at r15 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
nit: 1.318 radians is about 75.52°, not 104.48°.
Thank you. Somewhere along the road I swapped these values.
examples/multibody/four_bar/README.md, line 166 at r15 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW an ignorable thought -- kx and dx in math expressions look like multiplication of two symbols. Because unicode does have a subscript x (though not y and z) you could write kₓ and dₓ here, which are easier to see as single symbols even though they don't quite match the text symbols kx dx above, which have to be written like that so kx ky kz and dx dy dz can look related. But y and z don't appear in this expression.
Easy enough. Done.
examples/multibody/four_bar/README.md, line 195 at r15 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW did you want to provide an example of how you used the above approach to get stiffness and damping coefficients for the example?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more BTWs; still waiting for @amcastro-tri .
Reviewed 1 of 1 files at r16.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)
examples/multibody/four_bar/README.md, line 170 at r16 (raw file):
Values for kx can be determined by choosing a characteristic mass m (which may be directionally dependent) and then choosing ωₙ > 0 (speed of response). Rearranging ωₙ = √(kx/m) produces kx = m ωₙ².
BTW you may want to continue with kₓ dₓ in this paragraph (and some below) since it is still referring to the equations above.
examples/multibody/four_bar/README.md, line 177 at r16 (raw file):
For the included example code, a characteristic mass m = 20 kg was chosen with tₛ = 0.12 and ζ = 1 (critical damping). Thus ωₙ = -log(0.01) / 0.12 ≈ 38.38 and kx = (20)*(38.38)² ≈ 30000.
BTW consider whether ln(0.01)
would be more clear (though in C++ log() is the natural log so either way is fine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)
examples/multibody/four_bar/README.md, line 170 at r16 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW you may want to continue with kₓ dₓ in this paragraph (and some below) since it is still referring to the equations above.
Done.
examples/multibody/four_bar/README.md, line 177 at r16 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW consider whether
ln(0.01)
would be more clear (though in C++ log() is the natural log so either way is fine).
@mitiguy and I actually came to the same agreement yesterday. I had log() to match the bushing doc, but I like ln() here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks @joemasterjohn! I'm deeming Alejandro's blocking comment satisfactorily addressed. So this is ready to merge as soon as CI goes green.
Reviewed 1 of 1 files at r17.
Dismissed @amcastro-tri from a discussion.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks @sherm1!
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
👏 Congratulations, @joemasterjohn ! We now have a beautiful example of using a bushing to create a closed-topology mechanism. |
Adds a four bar linkage example program to
examples/multibody/four_bar
. Comes with two different four bar SDFs and some command line options to turn off and on several features. Demonstrates how to use theLinearBushingRollPitchYaw
ForceElement
to weld two links together and approximate a closed loop kinematic chain.//cc @amcastro-tri
This change is