-
Notifications
You must be signed in to change notification settings - Fork 143
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
Update sam_bot_description for the new Gazebo #102
base: rolling
Are you sure you want to change the base?
Conversation
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.
First of all, a huge, huge thank you for taking this on, it really helps alot to get some support in updating documentation :-)
ros_type_name: "rosgraph_msgs/msg/Clock" | ||
gz_type_name: "gz.msgs.Clock" | ||
direction: GZ_TO_ROS | ||
|
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 we need TF as well?
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 the ekf node should publish the transforms instead of Gazebo. I have documented this 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.
I guess i haven't typically used EKF when using gazebo, I just let the diff drive plugin provide TF as pretty close to ground truth. I'm open to both ways, but just what I've usually done and makes it a little easier for folks to understand on a tutorial level
Also, it might help in the issues you've mentioned by just removing EKF from the situation 😉
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 guess having EKF would be a good idea for a real robot, but not really for Gazebo. I can add the bridge and then maybe make launching EKF an argument of the launch file. Then I can edit the tutorials accordingly.
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 guess having EKF would be a good idea for a real robot, but not really for Gazebo. I can add the bridge and then maybe make launching EKF an argument of the launch file. Then I can edit the tutorials accordingly.
Done in 7c1ddc8.
Let me know when you want me to take a look again! I don't want to nitpick mid-update unless you want me to :-) |
Go ahead with it, everything should be good to go. There is just one problem I have been trying to solve for a while now though:
It does not seem like the robot moves with every command (happens more on publishing a value on angular z) even though on running |
Update nav2_params.yaml Update to use stamped twist messages Add SDF description Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
fe3a78b
to
8afa689
Compare
@SteveMacenski this should be fixed with the latest commit. The only problem is that when the imu topic is included for the |
ros_type_name: "rosgraph_msgs/msg/Clock" | ||
gz_type_name: "gz.msgs.Clock" | ||
direction: GZ_TO_ROS | ||
|
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 guess i haven't typically used EKF when using gazebo, I just let the diff drive plugin provide TF as pretty close to ground truth. I'm open to both ways, but just what I've usually done and makes it a little easier for folks to understand on a tutorial level
Also, it might help in the issues you've mentioned by just removing EKF from the situation 😉
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'm a little unclear as to why there are <gazebo>
tags and sensor plugin definitions in the URDF. I think the URDF is just the non-GZ things and the SDF contains the gazebo and sensor plugins that attach to frames defined in the URDF. I think things like the IMU gazebo stuff should be removed to the SDF and only the IMU frame is created / linked in the URDF (and similarly for other sensors, diff drive plugin, joint state publisher, etc).
I don't think there needs to be / should be duplication. I'm not familiar with that much duplication in SDF and URDF
I haven't looked at this PR at all - but I will note that for a long time there have been gazebo tags in many URDFs - the URDF parser silently ignores them. The SDF has usually been more the description of the scene, not the robot. I'm not sure what the "best" practices are for new gazebo. |
For example, in
I think this model can/should be followed. I think this was always the 'best practice' even in ROS 1, but confusion / convenience / hacking led to them kind of bleeding together since the gazebo tags don't hurt anything for non-gazebo use as you mention If we're going to bleed them together, then I think its probably better to just use a singular file then and save the duplication maintenance work. You can actually see that we did that for the TB4 portions of that repository that were modified and pared down from the iRobot / Clearpath stacks. But I think its possible / reasonably easy, especially using those TB3 files as a baseline :-) |
In the turtlebot example, isn't the SRDF got a lot of duplicated things from the URDF (for instance, just the base_link definition is basically fully duplicated - if one gets updated but not the other, things get out of date)? The workflow I've seen in the past (again, ROS 1) was to create your base URDF, with no gazebo tags and then use xacro to include that and add the gazebo stuff in a more gazebo-specific urdf. I guess I'm assuming here that ROS 2 / new gazebo still has the tools to load a URDF/xacro directly (basically, it creates the SDF under the hood) - if that's gone, it's a moot point and users have to maintain separate SDF & URDF files. |
I think the intention is to long-term stop allowing or promoting users to use URDFs in this way due to the limiting nature of the URDF format w.r.t. SDF. With this said, I forget entirely where I heard that so I could just be making that up 😆 |
I mean, this is the team that built their own ROS-alternative for Gazebo internal comms, so... I feel like Gazebo keeps getting harder and harder to use, at the same time so many alternatives keep coming on the market. /rant |
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
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.
You bring up an interesting point about the composition containers and GZ. That's pretty new. Want to open a PR for Nav2's bringup to do that as well? I think that would be nice to have the bridge composed there as well now that this is an option!
@@ -0,0 +1,6 @@ | |||
--- | |||
- ros_topic_name: "/tf" |
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.
Why in a separate file?
Edit: I see now, do we need this if we always use the EKF? If not, then feel free to remove. I just thought it was nice to show TF even if we don't use it in this demonstration since it'll be a common-ish want.
container_name='ros_gz_container', | ||
create_own_container='False', | ||
use_composition='True', | ||
condition=UnlessCondition(LaunchConfiguration('ekf')) |
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.
Lets just recommend they use the EKF, why not :-)
Sure! Also about the EKF: the reason I have included the tf_bridge is that the ekf makes the robot rotate a bit too much on the ROS side (as can be seen in RViz). This doesn't happen when I give the EKF only the odom topic instead of both the odom and imu. I am not too sure of whether this is a problem with EKF or Gazebo. Will look into this more soon. |
Co-authored-by: Steve Macenski <[email protected]> Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Signed-off-by: Aarav Gupta <[email protected]>
Fixes ros-navigation/docs.nav2.org#608.
/demo/cmd_vel