Skip to content

Fix: Add negative check (Issue #5765) #5766

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.

Sign up for GitHub

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

Open
r0s4ngeles wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Fix: Add negative check (Issue #5765) #5766

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
83e489b
Add negative check
r0s4ngeles Dec 6, 2025
68c46f3
Add negative check
r0s4ngeles Dec 6, 2025
e4327a6
Fix result error
r0s4ngeles Dec 6, 2025
a97b0d2
Change return to break
r0s4ngeles Dec 8, 2025
5090659
Change validation
r0s4ngeles Dec 8, 2025
9ae9170
Change validation
r0s4ngeles Dec 8, 2025
20e8cec
Fix velocity test
r0s4ngeles Dec 9, 2025
4631cc9
Fix amcl_node
r0s4ngeles Dec 10, 2025
c1060ce
Fix amcl_node
r0s4ngeles Dec 10, 2025
db2aea7
Fix amcl error log
r0s4ngeles Dec 10, 2025
d6ab181
Merge branch 'ros-navigation:main' into fix/zero-division-validation-…
r0s4ngeles Dec 10, 2025
ffb66f0
Fix amcl test
r0s4ngeles Dec 10, 2025
f8f3b2a
Fix error
r0s4ngeles Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions nav2_amcl/src/amcl_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,6 +1021,13 @@ rcl_interfaces::msg::SetParametersResult AmclNode::validateParameterUpdatesCallb
param_name.c_str(), parameter.as_double());
result.successful = false;
}
if (parameter.as_double() <= 0.0 && param_name == "save_pose_rate") {
Copy link
Collaborator

@mini-1235 mini-1235 Dec 9, 2025

Choose a reason for hiding this comment

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

We should also move this to the first condition, and make the second condition elif

RCLCPP_WARN(
get_logger(), "The value of parameter '%s' is incorrectly set to %f, "
"it should be >0. Ignoring parameter update.",
param_name.c_str(), parameter.as_double());
result.successful = false;
}
} else if (param_type == ParameterType::PARAMETER_INTEGER) {
if (parameter.as_int() <= 0.0 && param_name == "resample_interval") {
RCLCPP_WARN(
Expand Down
12 changes: 12 additions & 0 deletions nav2_amcl/test/test_dynamic_parameters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,18 @@ TEST(WPTest, test_dynamic_parameters)
results = rec_param->set_parameters_atomically({rclcpp::Parameter("min_particles", 2000)});
rclcpp::spin_until_future_complete(amcl->get_node_base_interface(), results);
EXPECT_EQ(amcl->get_parameter("min_particles").as_int(), 100);

// Test zero save_pose_rate rejection
results = rec_param->set_parameters_atomically({rclcpp::Parameter("save_pose_rate", 0.0)});
rclcpp::spin_until_future_complete(amcl->get_node_base_interface(), results);
EXPECT_FALSE(results.get().successful);
EXPECT_EQ(amcl->get_parameter("save_pose_rate").as_double(), 2.0);

// Test negative save_pose_rate rejection
results = rec_param->set_parameters_atomically({rclcpp::Parameter("save_pose_rate", -1.0)});
rclcpp::spin_until_future_complete(amcl->get_node_base_interface(), results);
EXPECT_FALSE(results.get().successful);
EXPECT_EQ(amcl->get_parameter("save_pose_rate").as_double(), 2.0);
}

int main(int argc, char ** argv)
Expand Down
12 changes: 11 additions & 1 deletion nav2_velocity_smoother/src/velocity_smoother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,17 @@ VelocitySmoother::dynamicParametersCallback(std::vector<rclcpp::Parameter> param

if (param_type == ParameterType::PARAMETER_DOUBLE) {
if (param_name == "smoothing_frequency") {
smoothing_frequency_ = parameter.as_double();
double smoothing_frequency = parameter.as_double();
if (smoothing_frequency <= 0.0) {
RCLCPP_WARN(
get_logger(),
"smoothing_frequency must be positive, received: %f. Ignoring update.",
smoothing_frequency);
result.successful = false;
result.reason = "smoothing_frequency must be greater than 0.0";
break;
}
smoothing_frequency_ = smoothing_frequency;
if (timer_) {
timer_->cancel();
timer_.reset();
Expand Down
12 changes: 12 additions & 0 deletions nav2_velocity_smoother/test/test_velocity_smoother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,18 @@ TEST(VelocitySmootherTest, testDynamicParameter)
rclcpp::spin_until_future_complete(smoother->get_node_base_interface(), results);
EXPECT_FALSE(results.get().successful);

// Test zero smoothing_frequency_ rejection
results = rec_param->set_parameters_atomically(
{rclcpp::Parameter("smoothing_frequency_", 0.0)});
rclcpp::spin_until_future_complete(smoother->get_node_base_interface(), results);
EXPECT_FALSE(results.get().successful);

// Test negative smoothing_frequency_ rejection
results = rec_param->set_parameters_atomically(
{rclcpp::Parameter("smoothing_frequency_", -1.0)});
rclcpp::spin_until_future_complete(smoother->get_node_base_interface(), results);
EXPECT_FALSE(results.get().successful);

// test full state after major changes
smoother->deactivate(state);
smoother->cleanup(state);
Expand Down
Loading