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

Added nav2 simple commander with the new Gazebo #3637

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jun 19, 2023

Basic Info

Info Please fill out this column
Ticket(s) this addresses #2997

Description of contribution in a few bullet points

I created a new package called nav2_gz_simple_commander to replicate the package nav2_simple_commander but this time with the new Gazebo.

I have some issues that maybe you can help me to solve. The odom frame should be locate close to the map frame (compared with the gazebo-classic simulation), but for some reason is located where the robots is spawned and sometimes the robot is lost. You can check this in the gif. any thoughts?

NOTE: You need to add this repository to your workspace https://github.com/ahcorde/aws-robomaker-small-warehouse-world

Test it

ros2 launch nav2_gz_simple_commander nav_to_pose_example_launch.py

nav2_lost
nav2_lost2

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@SteveMacenski
Copy link
Member

SteveMacenski commented Jun 19, 2023

The only differences are the launch files, no? If so, delete this package and just add the complimentary launch files to the existing package. I don't think there's any reason to have an entirely separate package.

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 20, 2023

The only differences are the launch files, no? If so, delete this package and just add the complimentary launch files to the existing package. I don't think there's any reason to have an entirely separate package.

Done here 6e708e9

Any thoughts about why the robot is lost ?

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@SteveMacenski
Copy link
Member

Probably some of the URDF things I pointed out in the other PR. A 5m lidar is completely insufficient to be localizing in a warehouse of that size.

Otherwise, your changes look good if you make the updates for the rest of these, but I'd recommend just waiting until we get the other PR done since this is blocked by that anyway

@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 20, 2023

Probably some of the URDF things I pointed out in the other PR. A 5m lidar is completely insufficient to be localizing in a warehouse of that size.

Otherwise, your changes look good if you make the updates for the rest of these, but I'd recommend just waiting until we get the other PR done since this is blocked by that anyway

I noticed that in this case the warehouse.world file contains a different instance of the turtlebot3 which is defined inside the file. In particular the model working on Gazebo classic has a 3.5m lidar.

the only different that I have between models are the type caster wheel, one is ball and in the other one if fixed.

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@SteveMacenski
Copy link
Member

SteveMacenski commented Jun 20, 2023

I can't answer that for you then, maybe the Ignition model you selected has problems?

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@SteveMacenski
Copy link
Member

SteveMacenski commented Jun 22, 2023

Figure out the reason and/or run each of these launch files to verify they work?

This PR I'll broadly just give a once over and merge as long as someone's run them and the demos still work

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 22, 2023

Figure out the reason and/or run each of these launch files to verify they work?

This PR I'll broadly just give a once over and merge as long as someone's run them and the demos still work

I checked all the new launch files and they have the same behaviour as Gazebo classic.

@SteveMacenski
Copy link
Member

Approved beyond the minor linting problems needed for CI to not have errors

@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 22, 2023

humm the CI failures look odd, I can see two tests failing in nav2_gz_simple_commander.test.test_copyright but this package was removed, same happens with nav2_gz_simple_commander.test.test_flake8.

should I need to fix linters that not included in the files that I included ?

@SteveMacenski
Copy link
Member

Might be a caching issue, change all 3 instances of v14 to v15 in this file and push it: https://github.com/ros-planning/navigation2/blob/main/.circleci/config.yml

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@SteveMacenski
Copy link
Member

That worked

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@@ -0,0 +1,486 @@
<?xml version="1.0" ?>
Copy link
Member

Choose a reason for hiding this comment

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

How does this URDF differ from that in nav2_bringup once the other PR is merged? is this necessary?

get_package_share_directory('turtlebot3_gazebo'), '..')

# Clock bridge
clock_bridge = Node(
Copy link
Member

Choose a reason for hiding this comment

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

It just occurred to me that we abstracted out the gz tb3 robot simulation launch file to deal with the gz specifics / bridges and whatnot. Can't that be used in all these launch files as well if we send it the updated starting pose?

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 made the PRs independent, because the tb3 for Gazebo classic is defined in the world file inside this package. We are using a two versions of the tb3, which are a little bit different between them.

But I'm happy to use the new resources included in nav2_bringup. What fo you prefer ?

  • Keep models independent from nav2_bringup
  • Use the same models here and in nav2_bringup

Copy link
Member

Choose a reason for hiding this comment

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

Same - unless there are key differences... which I don't believe there should be? I think when I made this I may have made the lidar for the simple commander range much higher to deal with the larger nature of the space that would need to be kept.

So I'd say I'd rather re-use as much as possible. Where there are changes where they're not the same, err on the side of less restrictive (e.g. longer ranges and such)

@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2023

This pull request is in conflict. Could you fix it @ahcorde?

@SteveMacenski
Copy link
Member

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.

None yet

2 participants