Skip to content
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.

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

fix(pose_instability_detector): fix a rare error #7681

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2023- Autoware Foundation

Check notice on line 1 in localization/pose_instability_detector/src/pose_instability_detector.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

✅ No longer an issue: Large Method

PoseInstabilityDetector::clip_out_necessary_twist is no longer above the threshold for lines of code
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -354,32 +354,38 @@
start_twist.header.stamp = start_time;
result_deque.push_front(start_twist);
} else {
if (result_deque.size() < 2) {
return result_deque;
}
// If the first element is earlier than start_time, interpolate the first element
rclcpp::Time time0 = rclcpp::Time(result_deque[0].header.stamp);
rclcpp::Time time1 = rclcpp::Time(result_deque[1].header.stamp);
double ratio = (start_time - time0).seconds() / (time1 - time0).seconds();
Twist twist0 = result_deque[0].twist.twist;
Twist twist1 = result_deque[1].twist.twist;
result_deque[0].twist.twist.linear.x = twist1.linear.x * ratio + twist0.linear.x * (1 - ratio);
result_deque[0].twist.twist.linear.y = twist1.linear.y * ratio + twist0.linear.y * (1 - ratio);
result_deque[0].twist.twist.linear.z = twist1.linear.z * ratio + twist0.linear.z * (1 - ratio);
result_deque[0].twist.twist.angular.x =
twist1.angular.x * ratio + twist0.angular.x * (1 - ratio);
result_deque[0].twist.twist.angular.y =
twist1.angular.y * ratio + twist0.angular.y * (1 - ratio);
result_deque[0].twist.twist.angular.z =
twist1.angular.z * ratio + twist0.angular.z * (1 - ratio);

result_deque[0].header.stamp = start_time;
}

// If the last element is earlier than end_time, add the last element to the back of the
// result_deque
if (rclcpp::Time(result_deque.back().header.stamp) < end_time) {
TwistWithCovarianceStamped end_twist = *end_it;
end_twist.header.stamp = end_time;
result_deque.push_back(end_twist);
} else {
if (result_deque.size() < 2) {
return result_deque;

Check warning on line 387 in localization/pose_instability_detector/src/pose_instability_detector.cpp

View check run for this annotation

Codecov / codecov/patch

localization/pose_instability_detector/src/pose_instability_detector.cpp#L387

Added line #L387 was not covered by tests
}

Check warning on line 388 in localization/pose_instability_detector/src/pose_instability_detector.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ New issue: Complex Method

PoseInstabilityDetector::clip_out_necessary_twist has a cyclomatic complexity of 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.

Check notice on line 388 in localization/pose_instability_detector/src/pose_instability_detector.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

ℹ Getting worse: Bumpy Road Ahead

PoseInstabilityDetector::clip_out_necessary_twist increases from 2 to 4 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
// If the last element is later than end_time, interpolate the last element
rclcpp::Time time0 = rclcpp::Time(result_deque[result_deque.size() - 2].header.stamp);
rclcpp::Time time1 = rclcpp::Time(result_deque[result_deque.size() - 1].header.stamp);
Expand Down
53 changes: 53 additions & 0 deletions localization/pose_instability_detector/test/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,59 @@ TEST_F(TestPoseInstabilityDetector, output_warn_when_twist_is_too_small) // NOL
EXPECT_TRUE(diagnostic_status.level == diagnostic_msgs::msg::DiagnosticStatus::WARN);
}

TEST_F(TestPoseInstabilityDetector, does_not_crash_even_if_abnormal_odometry_data_comes) // NOLINT
{
// [Condition] There is no twist_msg between the two target odometry_msgs.
// Normally this doesn't seem to happen.
// As far as I can think, this happens when the odometry msg stops (so the next timer callback
// will refer to the same odometry msg, and the timestamp difference will be calculated as 0)
// This test case shows that an error occurs when two odometry msgs come in close succession and
// there is no other odometry msg.
// Referring again, this doesn't normally seem to happen in usual operation.

builtin_interfaces::msg::Time timestamp{};

// send the twist message1
timestamp.sec = 0;
timestamp.nanosec = 0;
helper_->send_twist_message(timestamp, 0.2, 0.0, 0.0);

// send the first odometry message after the first twist message
timestamp.sec = 0;
timestamp.nanosec = 5e8 + 1;
helper_->send_odometry_message(timestamp, 10.0, 0.0, 0.0);

// process the above message (by timer_callback)
helper_->received_diagnostic_array_flag = false;
while (!helper_->received_diagnostic_array_flag) {
executor_.spin_some();
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}

// send the second odometry message before the second twist message
timestamp.sec = 0;
timestamp.nanosec = 5e8 + 1e7;
helper_->send_odometry_message(timestamp, 12.0, 0.0, 0.0);

// send the twist message2
timestamp.sec = 1;
timestamp.nanosec = 0;
helper_->send_twist_message(timestamp, 0.2, 0.0, 0.0);

executor_.spin_some();

// process the above messages (by timer_callback)
TaikiYamada4 marked this conversation as resolved.
Show resolved Hide resolved
helper_->received_diagnostic_array_flag = false;
while (!helper_->received_diagnostic_array_flag) {
executor_.spin_some();
std::this_thread::sleep_for(std::chrono::milliseconds(10));
}

// This test is OK if pose_instability_detector does not crash. The diagnostics status is not
// checked.
SUCCEED();
}

int main(int argc, char ** argv)
{
rclcpp::init(argc, argv);
Expand Down
Loading