Skip to content
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

Add 2D/3D Lidar Launch Arg in iris_with_lidar.launch.py #64

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tejalbarnwal
Copy link

Description

This PR addresses #63 and implements the following:

  • Adds two new models: iris_with_2d_lidar and iris_with_3d_lidar, which only include the lidar link and sensor.
  • Updates iris_with_lidar.launch.py and iris_maze.launch.py to select the appropriate model (2D or 3D) based on a launch argument.

Testing

Modify the lidar_dim argument passed to iris_with_lidar.launch.py in iris_maze.launch.py to either 2 or 3.
Any other value prevents the robot_state_publisher node from launching, leading to:

[rviz2-7] [INFO] [1732732583.933054374] [rviz]: Message Filter dropping message: frame 'base_scan' at time 2.773 for reason 'discarding message because the queue is full'

This occurs because no TF data is published.

Work in Progress

I am working on raising a ValueError if an invalid lidar_dim (other than 2 or 3) is passed. However, a challenge remains: Gazebo launches the iris model even if a ValueError is raised, and the launch process exits in the terminal. Manually killing the Gazebo process (ruby PID) is currently required to stop it. I am actively working on resolving this issue.

Potential Improvements

One concern is ensuring consistency between the model selected via the launch argument and the model referenced in the world files (e.g., iris_maze.sdf, iris_runway.sdf). For instance, iris_maze.sdf references iris_with_lidar and defines its pose in the environment, requiring manual verification.
A possible improvement could involve enabling the independent spawning of the iris model in the world, decoupling it from other Gazebo components, to simplify integration.

@srmainwaring
Copy link
Collaborator

Thanks for the PR @tejalbarnwal. I'm wondering whether we can save some duplication and future maintenance overhead by using an <include> element similar to how we do for the iris_with_gimbal model in the ardupilot_gazebo repo.

We would need to create additional models for the 2D and 3D Lidar, each would need to have a canonical link called base_scan to be consistent with the expected TF tree.

    <include merge="true">
      <uri>model://lidar_3d</uri>
      <name>lidar</name>
      <pose degrees="true">0 0 0 0 0 0</pose>    
    </include>

The remainder of the common SDF can be factored out.

We may only need a model expressed for either 2d or 3d, because in the launch file we can use standard python string replacement to substitute the desired model at launch time (using replace on <uri>model://lidar_3d</uri>).

To resolve the consistency concern we should use the gz launch spawn model capability and load the model into the maze at the desired location. There is an example of how to do this for multiple robots here: https://github.com/srmainwaring/ardupilot_gz/blob/prs/pr-multi-vehicle/ardupilot_gz_bringup/launch/robots/iris.launch.py

If you like I can help you rework this to implement the suggestions above.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Nov 28, 2024

This is off to a great start! Thanks for spending the time to submit your changes for a 3D Lidar Iris model! This is a nice capability addition that will go well with your bonxai demo.

I agree with Rhys on the requested changes, and can also help if you get stuck.

@tejalbarnwal
Copy link
Author

Hi,
I have implemented the requested changes to dynamically replace the <uri>model://lidar_3d</uri> string. Additionally, the launch files now use the ros_gz_sim create executable to spawn the iris_with_lidar model. Please let me know your feedback on the updates.

@tejalbarnwal
Copy link
Author

Hi @srmainwaring and @Ryanf55 ,
I would greatly appreciate any feedback on the latest commit I pushed, so we can refine it further for merging the PR at your convenience.

@tejalbarnwal
Copy link
Author

Hi @srmainwaring and @Ryanf55 ,
Just checking in, did you get a chance to look at the PR? Let me know if there is anything specific you would like me to address.

Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

Sorry for the very long delay, here's some initial feedback. Once I'm back at my desktop in a few days, I can provide more feedback by testing the changes. Nice improvements!

ardupilot_gz_bringup/launch/robots/iris_lidar.launch.py Outdated Show resolved Hide resolved
ardupilot_gz_gazebo/worlds/iris_maze.sdf Outdated Show resolved Hide resolved
@tejalbarnwal tejalbarnwal force-pushed the pr-iris-2d-3d-lidar-launch branch from 089b05b to 65d77ac Compare December 30, 2024 20:06
@tejalbarnwal
Copy link
Author

tejalbarnwal commented Dec 30, 2024

Hi @srmainwaring and @Ryanf55 , I have incorporated the changes you suggested into the code. Could you please review them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants