Skip to content

Initialize variables to avoid errors in the release build#758

Merged
mkhansenbot merged 1 commit intolifecyclefrom
fix-release-build
May 28, 2019
Merged

Initialize variables to avoid errors in the release build#758
mkhansenbot merged 1 commit intolifecyclefrom
fix-release-build

Conversation

@mjeronimo
Copy link

Description

  • In the release build, there were a few occurances of uninitialized variables. These values are set by the get_parameter call, but the compiler doesn't know that, and issues a warning. With warnings treated as errors, this resulted in a release build failure.

@mjeronimo mjeronimo added this to the D Turtle Release milestone May 25, 2019
@mjeronimo mjeronimo self-assigned this May 25, 2019
const nav2_lifecycle::LifecycleNode::SharedPtr & nh)
{
bool use_dwa;
bool use_dwa = true;
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is "navigation2" and not an extension of navigation, I'm of the opinion we're still too early in the development to be holding ourselves to backwards compatibility. I would just remove this parameter and checks entirely. We're not at a point of having LTS support for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Steve, please open another issue to remove use_dwa parameter. We would need to do validation on that change to make sure we don't break anything before we could merge it. This change is just to fix the build, I don't want to gate this for a bigger change.

Copy link
Member

Choose a reason for hiding this comment

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

@mkhansen-intel if you look at the function, all its doing is checking that value and then throwing an exception if its true. If you entirely remove the function, there shouldn't be any issues if DWB has ever worked, that parameter is never set to true. I'm not blocking it, but I am suggesting that potentially that is a better solution than to bandage the issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but there are other references to use_dwa in the code, see below.

mkhansen@mkhansen-desk:~/ros2_dev/navigation2_ws/src/navigation2/nav2_dwb_controller$ grep -r use_dwa
dwb_plugins/test/twist_gen.cpp:  nh->set_parameters({rclcpp::Parameter("use_dwa", true)});
dwb_plugins/test/twist_gen.cpp:  nh->set_parameters({rclcpp::Parameter("use_dwa", false)});
dwb_plugins/test/twist_gen.cpp:  nh->set_parameters({rclcpp::Parameter("use_dwa", true)});
dwb_plugins/test/twist_gen.cpp:  nh->set_parameters({rclcpp::Parameter("use_dwa", true)});
dwb_plugins/test/twist_gen.cpp:  nh->set_parameters({rclcpp::Parameter("use_dwa", true)});
dwb_plugins/src/limited_accel_generator.cpp:  bool use_dwa;
dwb_plugins/src/limited_accel_generator.cpp:  nh->get_parameter_or("use_dwa", use_dwa, true);
dwb_plugins/src/limited_accel_generator.cpp:  if (!use_dwa) {
dwb_plugins/src/limited_accel_generator.cpp:    throw nav_core2::PlannerException("Deprecated parameter use_dwa set to false. "
dwb_plugins/src/standard_traj_generator.cpp:  bool use_dwa;
dwb_plugins/src/standard_traj_generator.cpp:  nh->get_parameter_or("use_dwa", use_dwa, false);
dwb_plugins/src/standard_traj_generator.cpp:  if (use_dwa) {
dwb_plugins/src/standard_traj_generator.cpp:    throw nav_core2::PlannerException("Deprecated parameter use_dwa set to true. "
dwb_plugins/include/dwb_plugins/standard_traj_generator.hpp:   * @brief Check if the deprecated use_dwa parameter is set to the functionality that matches this class
dwb_plugins/include/dwb_plugins/standard_traj_generator.hpp:   * The functionality guarded by the use_dwa parameter has been split between this class and the derived
dwb_plugins/include/dwb_plugins/standard_traj_generator.hpp:   * LimitedAccelGenerator. If use_dwa was false, this class should be used. If it was true, then LimitedAccelGenerator.

As you see, there are tests and comments that would need to be removed also.

What should we do if someone sets that parameter? Ignore it without even a warning?
I'm not saying let's not remove it, I'm just saying I'd like someone (maybe @crdelsey or yourself) to make sure that we're not breaking something by removing the parameter entirely, and at the very least we should remove/change the tests that are using it and update any comments also.

Someone could probably do that in an hour. I just don't want to add that to this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added issue #759 for this, you're welcome to fix it. :)

Copy link
Collaborator

@mkhansenbot mkhansenbot left a comment

Choose a reason for hiding this comment

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

Built for me, thanks

@mkhansenbot
Copy link
Collaborator

Fixes issue #754

@mkhansenbot mkhansenbot merged commit 6d837b1 into lifecycle May 28, 2019
@mkhansenbot mkhansenbot deleted the fix-release-build branch May 28, 2019 04:33
Forsyth-Creations pushed a commit to Forsyth-Creations/navigation2 that referenced this pull request Feb 19, 2025
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.

4 participants