Rover 2018 Model #369
Rover 2018 Model #369
Conversation
|
Most of the "totally new" files are actually just moves. The only real "new" files are the |
src/sb_gazebo/robots/macros.xacro
Outdated
| <xacro:macro name="cylinder_inertia" params="m r h"> | ||
| <inertia ixx="${m*(3*r*r+h*h)/12}" ixy = "0" ixz = "0" | ||
| iyy="${m*(3*r*r+h*h)/12}" iyz = "0" | ||
| izz="${m*r*r/2}" | ||
| /> | ||
| </xacro:macro> | ||
|
|
||
| <!-- Calculate the intertia settings for a box with given mass, radius, height --> |
There was a problem hiding this comment.
Instead of mass, radius, height...should it be mass, and position?
There was a problem hiding this comment.
changed to
<!-- Calculate the intertia settings for a box with given mass, x size, y size, z size -->
src/sb_gazebo/robots/macros.xacro
Outdated
| <xacro:macro name="box_inertia" params="m x y z"> | ||
| <inertia ixx="${m*(y*y+z*z)/12}" ixy = "0" ixz = "0" | ||
| iyy="${m*(x*x+z*z)/12}" iyz = "0" | ||
| izz="${m*(x*x+z*z)/12}" | ||
| /> | ||
| </xacro:macro> | ||
|
|
||
| <!-- Calculate the intertia settings for a sphere with given mass, radius, height --> |
There was a problem hiding this comment.
I believe height isn't included here
There was a problem hiding this comment.
Yup! Changed to
<!-- Calculate the intertia settings for a sphere with given mass, radius -->
| <xacro:property name="primary_rocker_radius" value="0.15"/> | ||
| <xacro:property name="primary_rocker_length" value="0.1"/> | ||
| <xacro:property name="primary_rocker_chassis_clearance" value="0.02"/> | ||
| <xacro:property name="primary_rocker_x_offset" value="0"/> |
There was a problem hiding this comment.
What are these offsets relative to?
There was a problem hiding this comment.
Hmm. So they're from the chassis. Not sure on the best way to doc. this, but added a comment above:
<!-- Primary rocker offset from it's parent joint (the chassis) -->
|
|
||
| <xacro:property name="primary_rocker_radius" value="0.15"/> | ||
| <xacro:property name="primary_rocker_length" value="0.1"/> | ||
| <xacro:property name="primary_rocker_chassis_clearance" value="0.02"/> |
There was a problem hiding this comment.
A comment explaining what chassis clearance corresponds to here would be good.
There was a problem hiding this comment.
added
<!-- The size of the gap between the chassis and the primary rocker -->
|
|
||
| <xacro:macro name="primary_rocker" params="lr_prefix y_reflect"> | ||
| <!-- TODO: Make this revolute model with a spring to resist rotation (but allow it) instead of just being fixed like it currently is --> | ||
| <joint name="${lr_prefix}_primary_rocker_to_chassis" type="revolute"> |
There was a problem hiding this comment.
A comment explaining what lr_prefix means would be good.
There was a problem hiding this comment.
Expanded all to left_right_prefix and added javadoc style comment to each macro.
| material_name="grey" | ||
| /> | ||
| </xacro:macro> | ||
| <primary_rocker_arm lr_prefix="L" fb_prefix="FRONT" x_reflect="1" y_reflect="1"/> |
There was a problem hiding this comment.
The above comment, "not knowing what lr_prefix means" could also be addressed by expanding L to LEFT.
There was a problem hiding this comment.
Changed L/R to left/right
| <gazebo reference="camera"> | ||
| <sensor type="depth" name="stereo_camera"> | ||
| <always_on>1</always_on> | ||
| <update_rate>30.0</update_rate> |
There was a problem hiding this comment.
What units is update rate in?
There was a problem hiding this comment.
Added a comment indicating it is in Hz
| </pointCloudTopicName> | ||
| <frameName>camera</frameName> | ||
| <pointCloudCutoff>0.5</pointCloudCutoff> | ||
| <distortionK1>0.00000001</distortionK1> |
There was a problem hiding this comment.
what does distortion mean?
There was a problem hiding this comment.
Ah, I'd never really looked into this too much before, but it's actually pretty cool. Changed to:
<!--
For more info on the below coefficients, see
https://www.mathworks.com/help/vision/ug/camera-calibration.html
-->
<!-- 1st, 2nd, and 3rd order image distortion coefficients -->
<distortionK1>0.00000001</distortionK1>
<distortionK2>0.00000001</distortionK2>
<distortionK3>0.00000001</distortionK3>
<!-- X and Y translation -->
<distortionT1>0.00000001</distortionT1>
<distortionT2>0.00000001</distortionT2>
| material_name="grey" | ||
| /> | ||
| </xacro:macro> | ||
| <primary_wheel lr_prefix="L" fb_prefix="FRONT" x_reflect="1" y_reflect="1"/> |
There was a problem hiding this comment.
Same as above, expanding to LEFT/RIGHT would make it easier to understand
There was a problem hiding this comment.
This is a great point in general. There are a few things here that are not named verbosely enough.
Changed to left/right to be more consistent with our naming scheme (lower snake case).
Also changed lr_prefix->left_right_prefix, fb_prefix->front_back_prefix.
| /> | ||
| </xacro:macro> | ||
| <primary_rocker_arm lr_prefix="L" fb_prefix="FRONT" x_reflect="1" y_reflect="1"/> | ||
| <primary_rocker_arm lr_prefix="R" fb_prefix="FRONT" x_reflect="1" y_reflect="-1"/> |
There was a problem hiding this comment.
Is x_reflect and y_reflect a multiplier? A comment describing what the values mean would be good here.
There was a problem hiding this comment.
Added javadoc style comments to each macro.
| <primary_rocker lr_prefix="L" y_reflect="1"/> | ||
| <primary_rocker lr_prefix="R" y_reflect="-1"/> | ||
|
|
||
| <xacro:macro name="primary_rocker_arm" params="lr_prefix fb_prefix x_reflect y_reflect"> |
There was a problem hiding this comment.
I think it would be helpful to have high-level comments describing what is happening in each xacro block. For example in this block, the arms are being connected to the rocker (I think).
There was a problem hiding this comment.
It might also be cleaner to put each of these blocks into their own xacro files, and include them back with xacro:include. That way we would have xacro file for each part, so we could easily change a specific part without having to search through this long file.
There was a problem hiding this comment.
After offline discussion, decided to leave the macros where they are.
Added javadoc style comments to each macro.
|
@RobynCastro - this should be good for another round of review! |
…wflake into gareth/rover-2018-model
…wflake into gareth/rover-2018-model
Please fill out the following before requesting review on this PR
Description
sb_gazebolaunch filesTesting Done
Resolved Issues
Part of #344
Review Checklist
(Please check every item to indicate your code complies with it (by changing
[ ]->[x]). This will hopefully save both you and the reviewer(s) a lot of time!)It is the reviewers responsibility to also make sure every item here has been covered
.cppand.hfile should have a comment at the start of it. See files insrc/sample_packagefor examples..hfile) should have a javadoc style comment at the start of them. For examples, see the classes defined insrc/sample_packageTODO(or similar) statements should either be completed or associated with a github issueFeel free to make additions of things that we should be checking to this file if you think there's something missing!!!!