Skip to content

Adding additional docs on sensors and ROS 2 usage #433

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

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

SteveMacenski
Copy link
Contributor

@SteveMacenski SteveMacenski commented Mar 6, 2024

Summary

Adds some important documentation context and linking up of key information in tutorials. I wasn't sure how you handle version control since all the version are in the same branch, so I made the changes across the 3 current and future versions (and made sure for Fortress to use IGN instead of GZ in writing). If that's all automatically backported, let me know and I'll avoid that in the future.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Steve Macenski <stevenmacenski@gmail.com>
@github-actions github-actions bot added 🎵 harmonic Gazebo Harmonic 🏯 fortress Ignition Fortress labels Mar 6, 2024

For example:

```
ros2 run ros_ign_bridge parameter_bridge /TOPIC@ROS_MSG@IGN_MSG
ros2 run ros_ign_bridge parameter_bridge /scan@sensor_msgs/msg/LaserScan@ignition.msgs.LaserScan
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its important to show an actual use case when introducing topics like this. Otherwise, its not clear that ROS_MSG means sensor_msgs/msg/LaserScan and IGN_MSG means ignition.msgs.LaserScan (notable because one wants slashes and the other wants dots).

@@ -35,6 +37,16 @@ The ROS message type is followed by an `@`, `[`, or `]` symbol where:
Have a look at these [examples](https://github.com/ignitionrobotics/ros_ign/blob/ros2/ros_gz_bridge/README.md#example-1a-ignition-transport-talker-and-ros-2-listener)
explaining how to make communication connections from ROS to Ignition and vice versa.

It is also possible to use ROS Launch with the `ros_ign_bridge` and represent the topics in yaml format to be given to the bridge at launch time.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe its also important to introduce how to use this in a more practical application early on as well

@@ -8,7 +8,29 @@ to other models in our world. We will use three different sensors:
an IMU sensor, a Contact sensor and a Lidar sensor. We will also
learn how to launch multiple tasks with just one file using `ign_launch`.

You can find the final world of this tutorial [here](https://github.com/ignitionrobotics/docs/blob/master/fortress/tutorials/sensors/sensor_tutorial.sdf)
You can find the final world of this tutorial showing all these plugins in use [here.](https://github.com/gazebosim/docs/blob/master/ionic/tutorials/sensors/sensor_tutorial.sdf)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intent here is to early on point to helpful resources that folks might only be on this page to obtain (examples, tutorial source, and how to put it together for sensors)


## Preliminaries

When adding a `plugin` to an SDF file which does not currently contain one, the default plugins are not loaded. Before adding a sensor, make sure to add in a couple of logical defaults to your world so that it is possible to continue to use the GZ GUI:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a real issue I ran into that blocked me for several hours requiring a conversation with @azeey since there were no errors. I used the SDF exported by the GUI exporter which did not contain the plugins that were used by the GUI (which seems like a bug; shouldn't that export the SDF being full complete?). Thus, when I added sensor plugins, the SDF didn't load in the GUI anymore. This was due to missing plugins that this will help make sure others don't run into in the future while following this tutorial from their given SDF files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. I'm not an expert in this. But this sounds like something I might just assume and copy-paste whereas a non-developer might not know. So it seems a good addition to me.

@mabelzhang
Copy link
Contributor

mabelzhang commented Mar 6, 2024

For clarification - This is a feature / bug fix right? Could you please remove the "Forward port" and "Release" sections in the PR description?

I wasn't sure how you handle version control since all the version are in the same branch, so I made the changes across the 3 current and future versions

Yeah what you did is correct.

@mabelzhang
Copy link
Contributor

Maybe @quarkytale is the most familiar with other relevant tutorials to review parts of this one? No obligations.

Signed-off-by: Steve Macenski <stevenmacenski@gmail.com>
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few fixes, then I think this looks good. I haven't tested anything though.

The bits I didn't review are the parts about ros_gz_bridge and the sensor-specific parts, which I don't have too much experience with. If someone else wants to take a closer look at that, it would be good.


## Preliminaries

When adding a `plugin` to an SDF file which does not currently contain one, the default plugins are not loaded. Before adding a sensor, make sure to add in a couple of logical defaults to your world so that it is possible to continue to use the GZ GUI:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. I'm not an expert in this. But this sounds like something I might just assume and copy-paste whereas a non-developer might not know. So it seems a good addition to me.

SteveMacenski and others added 2 commits March 6, 2024 08:21
Co-authored-by: Mabel Zhang <mabel.m.zhang@gmail.com>
Signed-off-by: Steve Macenski <stevenmacenski@gmail.com>
Co-authored-by: Mabel Zhang <mabel.m.zhang@gmail.com>
Signed-off-by: Steve Macenski <stevenmacenski@gmail.com>
@SteveMacenski
Copy link
Contributor Author

Any blockers to having this merged now? This feels like something that can go in easily once the instructions are validated over each distribution (thanks Mabel!)

@mabelzhang mabelzhang merged commit c3b9950 into gazebosim:master Mar 11, 2024
@SteveMacenski
Copy link
Contributor Author

@mabelzhang when does this go online?

@azeey
Copy link
Contributor

azeey commented Mar 14, 2024

I just ran the deployment. Should be up pretty soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants