Update nav2_bringup readme for robot #351
Conversation
mkhansenbot
requested review from
SteveMacenski,
mhpanah,
mjeronimo and
orduno
|
Take another look. I've split the bringup into 2 parts and pushed 2 new launch files. Hopefully the README explains the new flow. |
| node_executable='amcl', | ||
| node_name='amcl', | ||
| output='screen', | ||
| parameters=[{ 'use_sim_time': use_sim_time}]), |
There was a problem hiding this comment.
We figured out that the use_sim_time parameter doesn't work, if I recall. You need to publish a parameter event after the node is up and listening.
We should just remove these and add another section to the readme saying run ros2 param set /gazebo use_sim_time true
There was a problem hiding this comment.
@crdelsey - the use_sim_time does get set, but not for the transform, I've updated the readme to add that. Please look at the last commit.
There was a problem hiding this comment.
Works for AMCL, but I'm getting errors now for world_model. I need to trigger some param update for the warning to stop.
WARN global_costmap: Costmap2DROS transform timeout. Current time: 1543355603.4700, global_pose stamp: 160.2020, tolerance: 0.3000, difference: 1543355443.2680
|
OK, I updated per Carlos' comment. It now says to set use_sim_time for the world_model node also. That worked for me. I do find it frustrating that we have to do that every time but for now all we can do is document it. |
This updates the readme with instructions for testing on a robot
mkhansenbot
force-pushed
the
nav2_bringup_readme
branch
from
48e3e84
to
f0f15f5
Compare
|
Just rebased, let me know if more changes are needed |
There was a problem hiding this comment.
Looks good to me, but I don't understand why there's 2 launch files. As they are required to be separated, then they should be named accordingly and not 1st 2nd. How about nav2_bringup_map_localization.launch.py and nav2_bringup_navigation.launch.py?
it also looks like the official naming for launch files is *.launch.py not _launch.py according to this doc
|
@SteveMacenski - I named them 1st and 2nd because they should preferrably be brought up in that order. The costmaps are in the 2nd one and they will wait indefinitely for the transforms to be available which are in the 1st one. Thus "bringup_1st" and "bringup_2nd". You 'can' use the nav2_bringup_launch.py file and just do it in one step, but it does make it harder to ensure that AMCL has localized correctly as the costmaps spew a ton of warning messages to the screen and can easily overwhelm the AMCL and map_server output. Please feel free to give these a try and if you like I can make more changes. As for using the _launch.py instead of .launch.py there was a Discourse thread about this I believe a month or so back saying _launch should be the convention. |
|
@mkhansen-intel to be explicit, you're referring to the error in this while loop? https://github.com/ros-planning/navigation2/blob/master/nav2_costmap_2d/src/costmap_2d_ros.cpp#L99 ? I can resolve that issue later today if that's the blocker for having 1 launch file |
|
@SteveMacenski - I think we may have stabilized things enough to try running from one bringup file. I can test that again tomorrow if you really think it's important. My focus here was on stability of bringup. I'll take another look tomorrow. |
|
|
||
| Set the World Model node to use simulation time | ||
|
|
||
| `ros2 param set /world_model use_sim_time True` |
There was a problem hiding this comment.
The global cost map doesn't get set. I still need to use ros2 param set /global_costmap use_sim_time True
There was a problem hiding this comment.
I have not needed to set that one but I'm testing again now with latest master branch
There was a problem hiding this comment.
OK, so I haven't seen exactly that /global_costmap needs to be set, but I have seen what I @crdelsey has also seen in that sometimes you have to set it twice. This whole setting of it is due to a bug in rclcpp I filed here:
ros2/rclcpp#595
Anyway, until that is fixed, we have to set the use_sim_time variable after everything has been loaded. In some cases, it doesn't seem to take for all nodes, so we have to set it a 2nd time.
In that case, we've seen you can simply set it again on /world_model and it should be OK. Again it's not perfect due to the bug above.
|
Update @SteveMacenski - Things are now stable enough that one launch file does seem to work most of the time, albeit with the caveat I just mentioned due to this bug: Sometimes we have to set the use_sim_time more than once before all the nodes respond to the event. Given that, I think the 'beginner' bringup should still be a 2-part bringup as I have here, so they can see AMCL localize correctly and make sure their transforms are working. However I will update the README again with more info stating this, and I will add the 'advanced' bringup of doing it all in one launch file. For those who have done the beginner and made that work, they could then move to the 1-part bringup after. Hopefully that should meet everyone's needs. |
Add notes and safety warnings Add more details why use_sim_time must be set dynamically Add 'Advanced bringup' instructions
|
OK @SteveMacenski @orduno and @crdelsey - I made a few more changes to the README to hopefully make things as clear as they can be at this point in time. Please take a look at the latest. Added notes and safety warnings |
|
@mkhansen-intel I dont think its important for this PR to make it all 1 launch file. I'm trying to figure out the reasons why and how I (or someone) can potentially resolve them. Can you indicate if that's the line that's a problem for you or not? Don't let that hold this PR up. It's not a big deal. |
|
@SteveMacenski - that line as well as the other messages from the Costmap 2D code that occur on every map update can overwhelm the screen output and make it hard to understand what is happening, until all the transforms are correct, AMCL receives valid scan data, and AMCL localizes. So I don't think you need to do anything, this is more of a way to help users get things correct on AMCL before proceeding. Once they have things working correctly using the 2 step flow, they can try the single step flow. I think it would be great if you tried both and let me know what you think. I found that this helped me when we were debugging issues. |
|
It would be nice if you could provide a snippet of the logs to what you're referring. I don't have time until after the holidays to get all this up and running but if you show me the logs that are being thrown by, I can probably resolve them. The while look needs a rate object to throttle and a macro wrapper for warn throttling I could write. If there's other areas we dont have a rate object throttling loop rates, we need to resolve those. We dont want essentially a |
* Update nav2_bringup readme for robot This updates the readme with instructions for testing on a robot * Add 2 launch files for bringup * Update README for 2 part bringup * Add setting use_sim_time to README * Set use_sim_time to true for world_model node * Update nav2_bringup README Add notes and safety warnings Add more details why use_sim_time must be set dynamically Add 'Advanced bringup' instructions
This updates the readme with instructions for testing on a robot