Fix being unable to change StandardTrajectoryGenerator parameter vtheta_samples #1619

sloretz · 2020-03-31T04:33:18Z

Basic Info

Info	Please fill out this column
Ticket(s) this addresses
Primary OS tested on	Ubuntu Bionic
Robotic platform tested on	simulated turtlebot

Description of contribution in a few bullet points

On eloquent-devel changing vtheta_samples has no effect; the controller always uses the default number of 20
The root case is loadParameterWithDeprecation() not declaring parameters
Existing test did not catch this bug because it uses set_parameters(), which automatically declares parameters.
Existing test further used node options that are not used by the real ControllerServer node
This PR fixes the test to use same node options as ControllerServer
This PR fixes the test to pass parameters via overrides rather than using set_parameters()
This PR fixes loadParameterWithDeprecation() to declare parameters; a consequence of which is it can only warn about deprecated parameters if the set value is different from the default value.

Description of documentation updates required from your changes

None

Future work that may be required in bullet points

This PR is a cherry-pick of a fix I made for the eloquent-devel branch see [Eloquent] Fix vtheta_samples not having any effect. #1620
I've actually only tested using eloquent-devel, so I don't know for sure this bug exists on master
- I'm relying on your CI to do that for me 🙃

SteveMacenski

LGTM, waiting on CI

SteveMacenski · 2020-03-31T04:51:20Z

Your build failed

SteveMacenski · 2020-03-31T05:08:21Z

nav2_dwb_controller/dwb_plugins/test/twist_gen.cpp

-  auto nh = makeTestNode("sim_time");
+  auto nh = makeTestNode("sim_time",
+    rclcpp::Parameter("dwb.sim_time", sim_time),
+    rclcpp::Parameter("dwb.linear_granularity", 0.5)});
  const double sim_time = 2.5;


This needs to be before make node, since the variable is not yet defined.

Thank you! Moved sim_time creation up in 928af55

SteveMacenski · 2020-04-13T18:26:48Z

@sloretz just following up here

SteveMacenski · 2020-04-22T21:59:13Z

@sloretz is this something you're going to finish up? You have a bunch of test failures

SteveMacenski · 2020-08-12T21:46:50Z

@sloretz friendly ping, in a week I'll pl on marking the PR as stale. Any feedback on timeline or even a 'I'm not going to be able to finish this' is appreciated :-)

sloretz · 2020-08-17T17:01:14Z

friendly ping, in a week I'll pl on marking the PR as stale. Any feedback on timeline or even a 'I'm not going to be able to finish this' is appreciated :-)

@SteveMacenski Making time for this this week :)

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

codecov · 2020-08-21T17:19:30Z

Codecov Report

Merging #1619 into main will decrease coverage by 0.41%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1619      +/-   ##
==========================================
- Coverage   75.79%   75.38%   -0.42%     
==========================================
  Files         224      224              
  Lines       10962    10962              
==========================================
- Hits         8309     8264      -45     
- Misses       2653     2698      +45

Flag	Coverage Δ
#project	`75.38% <ø> (-0.42%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...r/nav_2d_utils/include/nav_2d_utils/parameters.hpp	`36.84% <ø> (ø)`
nav2_recoveries/plugins/spin.cpp	`0.00% <0.00%> (-89.48%)`	⬇️
nav2_util/src/node_utils.cpp	`85.71% <0.00%> (-14.29%)`	⬇️
nav2_navfn_planner/src/navfn_planner.cpp	`87.11% <0.00%> (-6.14%)`	⬇️
...lifecycle_manager/src/lifecycle_manager_client.cpp	`87.80% <0.00%> (-2.44%)`	⬇️
...av2_costmap_2d/src/footprint_collision_checker.cpp	`88.37% <0.00%> (-2.33%)`	⬇️
nav2_amcl/src/amcl_node.cpp	`84.56% <0.00%> (+0.15%)`	⬆️
nav2_navfn_planner/src/navfn.cpp	`91.34% <0.00%> (+0.48%)`	⬆️
nav2_controller/src/nav2_controller.cpp	`88.78% <0.00%> (+2.69%)`	⬆️
..._dwb_controller/dwb_core/src/dwb_local_planner.cpp	`81.41% <0.00%> (+5.74%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed8ddaf...1ac0b63. Read the comment docs.

sloretz · 2020-08-21T17:36:05Z

@SteveMacenski CI LGTM! Codecov shows now change in coverage for the one file affected by this PR. I rebased and fixed the issues. The only changes since @crdelsey's review are c3ad9c4 9013a9b 1ac0b63

SteveMacenski

Thanks Shane for getting this done 😄

…ta_samples (#1619) * Fix tests declaring parameters real nodes don't Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix loadParameterWithDeprecation not getting initial parameter values Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Create sim_time variable before using it Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Line length < 100 Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Add missing { Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Linter fixes Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * sim_granularity -> time_granularity Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Linter fix Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* waypoint_follower node has _rclcpp_node as well as base node (#1940) * Add #include <vector> for vector<> (#1946) To fix cpplint * Add 'angles' dependency to nav2_costmap_2d package.xml (#1947) * transform goal to costmap frame (#1949) The plan recieved is usually in global frame, but our local costmap is often in odom frame. This fixes a regression from #1857 * Add mutex lock into inflation layer to avoid thread issue in updating footprint (#1952) Signed-off-by: Daisuke Sato <daisukes@cmu.edu> * Fix being unable to change StandardTrajectoryGenerator parameter vtheta_samples (#1619) * Fix tests declaring parameters real nodes don't Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix loadParameterWithDeprecation not getting initial parameter values Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Create sim_time variable before using it Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Line length < 100 Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Add missing { Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Linter fixes Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * sim_granularity -> time_granularity Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Linter fix Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * update version to 0.4.3 * removing redundant dep on angles Co-authored-by: Ruffin <roxfoxpox@gmail.com> Co-authored-by: Sarthak Mittal <sarthakmittal2608@gmail.com> Co-authored-by: Michael Ferguson <mfergs7@gmail.com> Co-authored-by: Daisuke Sato <43101027+daisukes@users.noreply.github.com> Co-authored-by: Shane Loretz <sloretz@openrobotics.org>

…ta_samples (ros-navigation#1619) * Fix tests declaring parameters real nodes don't Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix loadParameterWithDeprecation not getting initial parameter values Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Create sim_time variable before using it Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Line length < 100 Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Add missing { Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Linter fixes Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * sim_granularity -> time_granularity Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Linter fix Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz mentioned this pull request Mar 31, 2020

[Eloquent] Fix vtheta_samples not having any effect. #1620

Closed

SteveMacenski approved these changes Mar 31, 2020

View reviewed changes

SteveMacenski reviewed Mar 31, 2020

View reviewed changes

ghost approved these changes May 19, 2020

View reviewed changes

SteveMacenski changed the base branch from master to main August 12, 2020 21:34

sloretz added 5 commits August 17, 2020 16:01

Fix tests declaring parameters real nodes don't

7704c4f

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Fix loadParameterWithDeprecation not getting initial parameter values

e6a210f

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Create sim_time variable before using it

deded44

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Line length < 100

6ef51eb

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Add missing {

1211944

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz force-pushed the sloretz/fix_loadParameterWithDeprecation branch from e1103b2 to 1211944 Compare August 21, 2020 15:39

sloretz added 3 commits August 21, 2020 08:44

Linter fixes

c3ad9c4

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sim_granularity -> time_granularity

9013a9b

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Linter fix

1ac0b63

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

SteveMacenski approved these changes Aug 21, 2020

View reviewed changes

SteveMacenski merged commit d191e0a into ros-navigation:main Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix being unable to change StandardTrajectoryGenerator parameter vtheta_samples #1619

Fix being unable to change StandardTrajectoryGenerator parameter vtheta_samples #1619

Uh oh!

sloretz commented Mar 31, 2020 •

edited

Loading

Uh oh!

SteveMacenski left a comment

Uh oh!

SteveMacenski commented Mar 31, 2020

Uh oh!

SteveMacenski Mar 31, 2020

Uh oh!

sloretz Mar 31, 2020

Uh oh!

SteveMacenski commented Apr 13, 2020

Uh oh!

SteveMacenski commented Apr 22, 2020

Uh oh!

SteveMacenski commented Aug 12, 2020

Uh oh!

sloretz commented Aug 17, 2020

Uh oh!

codecov bot commented Aug 21, 2020

Uh oh!

sloretz commented Aug 21, 2020

Uh oh!

SteveMacenski left a comment

Uh oh!

Uh oh!

Fix being unable to change StandardTrajectoryGenerator parameter vtheta_samples #1619

Fix being unable to change StandardTrajectoryGenerator parameter vtheta_samples #1619

Uh oh!

Conversation

sloretz commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

SteveMacenski left a comment

Choose a reason for hiding this comment

Uh oh!

SteveMacenski commented Mar 31, 2020

Uh oh!

SteveMacenski Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

sloretz Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

SteveMacenski commented Apr 13, 2020

Uh oh!

SteveMacenski commented Apr 22, 2020

Uh oh!

SteveMacenski commented Aug 12, 2020

Uh oh!

sloretz commented Aug 17, 2020

Uh oh!

codecov bot commented Aug 21, 2020

Codecov Report

Uh oh!

sloretz commented Aug 21, 2020

Uh oh!

SteveMacenski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sloretz commented Mar 31, 2020 •

edited

Loading