use_sim_time refactoring #3131

tonynajjar · 2022-08-21T14:44:11Z

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#3119)
Primary OS tested on	Ubuntu
Robotic platform tested on	Gazebo

Description of contribution in a few bullet points

I used SetParameter to set the use_sim_time parameter and removed the parameter from launch files

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

Check that any new parameters added are updated in navigation.ros.org
Check that any significant change is added to the migration guide
Check that any new features OR changes to existing behaviors are reflected in the tuning guide
Check that any new functions have Doxygen added
Check that any new features have test coverage
Check that any new plugins is added to the plugins page
If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

mergify · 2022-08-21T14:44:44Z

@tonynajjar, please properly fill in PR template in the future. @SteveMacenski, use this instead.

Check that any new parameters added are updated in navigation.ros.org
Check that any significant change is added to the migration guide
Check that any new features OR changes to existing behaviors are reflected in the tuning guide
Check that any new functions have Doxygen added
Check that any new features have test coverage
Check that any new plugins is added to the plugins page
If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

nav2_bringup/launch/localization_launch.py

nav2_bringup/launch/slam_launch.py

codecov · 2022-08-21T16:38:04Z

Codecov Report

Merging #3131 (868a03e) into main (1c74368) will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3131      +/-   ##
==========================================
+ Coverage   82.78%   83.05%   +0.27%     
==========================================
  Files         333      333              
  Lines       15078    15082       +4     
==========================================
+ Hits        12482    12527      +45     
+ Misses       2596     2555      -41

Impacted Files	Coverage Δ
...tmap_2d/include/nav2_costmap_2d/costmap_2d_ros.hpp	`85.71% <ø> (ø)`
...clude/nav2_behavior_tree/bt_action_server_impl.hpp	`75.55% <100.00%> (+0.27%)`	⬆️
nav2_controller/src/controller_server.cpp	`52.09% <100.00%> (+0.16%)`	⬆️
nav2_costmap_2d/src/costmap_2d_ros.cpp	`88.00% <100.00%> (+0.04%)`	⬆️
nav2_planner/src/planner_server.cpp	`72.47% <100.00%> (+0.12%)`	⬆️
...v2_util/include/nav2_util/simple_action_server.hpp	`83.97% <0.00%> (+26.28%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

SteveMacenski

Generally LGTM! This is worth an update to the migration guide for - mentioning you can remove the use_sim_time from your param files.

SteveMacenski · 2022-08-22T16:49:57Z

nav2_bringup/launch/localization_launch.py

    load_nodes = GroupAction(
        condition=IfCondition(PythonExpression(['not ', use_composition])),
        actions=[
+            SetParameter("use_sim_time", use_sim_time),


does SetParameter work globally or can it be used for a single node? Also, does that work if the parameter in question is inside of the yaml file also (clearly it works if its not inside the yaml file)? I ask because this could be a really nice solution for the yaml_filename thing too if we can set it for only the map server if there is a non-trivial value set in the launch configuration.

It would be great not to require users to set yaml_filename in the yaml if we're going to override it always in launch using the CLI or the launch configuration defaults. This could be that solution, as long as it works if the parameter is in the yaml file too.

In general though, that's a problem with this PR if it doesn't work with the param in the yaml too, since users' yaml files right now will have it from previous clones they're working under

does SetParameter work globally or can it be used for a single node?

It applies the parameter for all nodes within the GroupAction. See here

Also, does that work if the parameter in question is inside of the yaml file also

yes, the parameter in the yaml would override

For the refactoring of yaml_filename, I'd prefer if we can open a separate issue. I might take it but can't commit to it right now

Totally understood, just asking the question, it sounds like this would be a solution for it too. Duly noted.

Leaving this un-resolved as a reminder to myself

nav2_bringup/launch/slam_launch.py

nav2_controller/src/controller_server.cpp

nav2_planner/src/planner_server.cpp

nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp

nav2_bringup/launch/slam_launch.py

tonynajjar

In general I think this approach to using sim time should be made more public/popular in the ROS2 community. I can't find the discussions anymore but I remember many complaints from people switching from ROS1 on having to set this parameter manually for every node.

mergify · 2022-08-23T16:27:40Z

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

SteveMacenski · 2022-08-23T17:11:08Z

In general I think this approach to using sim time should be made more public/popular in the ROS2 community. I can't find the discussions anymore but I remember many complaints from people switching from ROS1 on having to set this parameter manually for every node.

Once this is merged, you should post about it with a link! Awareness is 25% of the battle.

SteveMacenski · 2022-08-23T18:07:23Z

Other commits are showing up in this PR now - please rebase on main with only these comments / conflict resolutions

tonynajjar · 2022-08-23T18:14:05Z

Not sure I see what you mean, I merged master into my branch to fix the conflicts. The changes in "Files changed" are only my changes.
Should I have rebased instead and why?

nav2_costmap_2d/include/nav2_costmap_2d/costmap_2d_ros.hpp

SteveMacenski · 2022-08-23T18:16:25Z

Oh sorry, I was a little quick on the draw there, this is fine

mergify · 2022-08-24T21:42:28Z

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

tonynajjar · 2022-08-24T21:53:41Z

This is worth an update to the migration guide

ros-navigation/docs.nav2.org#339

nav2_controller/src/controller_server.cpp

* initial commit * fix * fixes * revert * fix * linting * fixes * fix format * fixing pycodestyle * revert typo * add use_sim_time to costmap * add parameter description * fix defaults * fix formatting

Tony Najjar and others added 7 commits August 18, 2022 22:05

initial commit

c73b0b3

fix

2ed939b

fixes

518a7b6

revert

eb2482b

Merge branch 'ros-planning:main' into use-sim-time-refactoring

47c5846

fix

48b1c81

linting

34b951f

tonynajjar marked this pull request as draft August 21, 2022 14:44

tonynajjar mentioned this pull request Aug 21, 2022

Replace config use_sim_time with SetUseSimTime in launch file #3119

Closed

fixes

60b84a1

tonynajjar commented Aug 21, 2022

View reviewed changes

nav2_bringup/launch/localization_launch.py Show resolved Hide resolved

nav2_bringup/launch/localization_launch.py Show resolved Hide resolved

nav2_bringup/launch/slam_launch.py Show resolved Hide resolved

tonynajjar marked this pull request as ready for review August 21, 2022 15:45

SteveMacenski requested changes Aug 22, 2022

View reviewed changes

SteveMacenski linked an issue Aug 22, 2022 that may be closed by this pull request

Replace config use_sim_time with SetUseSimTime in launch file #3119

Closed

Tony Najjar added 3 commits August 22, 2022 21:52

fix format

1fcd63b

fixing pycodestyle

e5564d4

revert typo

ef4adc4

tonynajjar commented Aug 22, 2022

View reviewed changes

tonynajjar requested a review from SteveMacenski August 22, 2022 20:28

Tony Najjar added 2 commits August 23, 2022 19:24

Merge remote-tracking branch 'origin/HEAD' into use-sim-time-refactoring

2554851

add use_sim_time to costmap

6281bcf

SteveMacenski reviewed Aug 23, 2022

View reviewed changes

nav2_costmap_2d/include/nav2_costmap_2d/costmap_2d_ros.hpp Outdated Show resolved Hide resolved

add parameter description

262dee8

fix defaults

971d4be

tonynajjar mentioned this pull request Aug 24, 2022

use_sim_time from launch files docs ros-navigation/docs.nav2.org#339

Merged

Merge remote-tracking branch 'origin/main' into use-sim-time-refactoring

fb11fb3

tonynajjar requested a review from SteveMacenski August 24, 2022 21:55

SteveMacenski approved these changes Aug 25, 2022

View reviewed changes

nav2_controller/src/controller_server.cpp Outdated Show resolved Hide resolved

SteveMacenski mentioned this pull request Aug 25, 2022

Use SetParameter Launch API to set the yaml filename for map_server #3149

Closed

fix formatting

868a03e

SteveMacenski approved these changes Aug 25, 2022

View reviewed changes

SteveMacenski merged commit 7703c81 into ros-navigation:main Aug 25, 2022

tonynajjar mentioned this pull request Mar 3, 2023

Humble backport of MPPI controller #3439

Merged

7 tasks

AlexeyMerzlyakov mentioned this pull request Jun 7, 2023

Refactor sim_time and composable node usage in Collision Monitor #3604

Merged

7 tasks

mini-1235 mentioned this pull request Jul 19, 2025

Remove use_sim_time from yaml ros-navigation/docs.nav2.org#730

Merged

use_sim_time refactoring #3131

use_sim_time refactoring #3131

Uh oh!

Conversation

tonynajjar commented Aug 21, 2022

Basic Info

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

Uh oh!

mergify bot commented Aug 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SteveMacenski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

tonynajjar Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonynajjar left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 23, 2022

Uh oh!

SteveMacenski commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Aug 23, 2022

Uh oh!

tonynajjar commented Aug 23, 2022

Uh oh!

Uh oh!

SteveMacenski commented Aug 23, 2022

Uh oh!

mergify bot commented Aug 24, 2022

Uh oh!

tonynajjar commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 21, 2022 •

edited

Loading

SteveMacenski left a comment •

edited

Loading

tonynajjar Aug 22, 2022 •

edited

Loading

SteveMacenski commented Aug 23, 2022 •

edited

Loading

tonynajjar commented Aug 24, 2022 •

edited

Loading