Skip to content

Conversation

@silanus23
Copy link
Contributor

@silanus23 silanus23 commented Oct 13, 2025


Basic Info

Info Please fill out this column
Ticket(s) this addresses #5037
Primary OS tested on Ubuntu
Robotic platform tested on nav2 bringup
Does this PR contain AI generated software? util test
Was this PR description generated by AI software? nope

Description of contribution in a few bullet points

Description of documentation updates required from your changes

BT node addition
max_error parameter
If there is a document for it maybe upcoming xml

Description of how this change was tested

I tried to take inspiration from is_battery_low condition node. I created unit tests too.


Future work that may be required in bullet points

  • There is an upcoming xml I will put this in
<RateController hz="3.0">
          <Fallback name="PathTrackingRecoveryPlanner">
            <IsWithinPathTrackingBounds max_tracking_error="2.0" />
            <Fallback name="TieredReplanning">
              <ComputePathToPose goal="{goal}" path="{path}" planner_id="{selected_planner}" error_code_id="{compute_path_error_code}" error_msg="{compute_path_error_msg}"/>
              <Sequence>
                <ClearEntireCostmap name="ClearGlobalCostmap-Tier2" service_name="global_costmap/clear_entirely_global_costmap"/>
                <RetryUntilSuccessful num_attempts="1">
                  <ComputePathToPose goal="{goal}" path="{path}" planner_id="{selected_planner}" error_code_id="{compute_path_error_code}" error_msg="{compute_path_error_msg}"/>
                </RetryUntilSuccessful>
              </Sequence>
            </Fallback>
          </Fallback>
        </RateController>

place this in navigating path through pose xml.

Note:

I had to take out every file with copy paste to a new branch. Cause I had created this branch as a sub branch of the last messy branch. This was the only clean choice that can save me.
I think this will solve my cycling comments and confused commits problem.

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.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
  • Should this be backported to current distributions? If so, tag with backport-*.

@mergify
Copy link
Contributor

mergify bot commented Oct 13, 2025

@silanus23, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

<input_port name="battery_topic">Topic for battery info</input_port>
</Condition>

<Decorator ID="IsWithinPathTrackingBounds">
Copy link
Member

Choose a reason for hiding this comment

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

For docs: this needs to be having its configuration guide page + add to Navigation Plugins table + migration guide with the larger feature description

Signed-off-by: silanus23 <[email protected]>
Signed-off-by: silanus23 <[email protected]>
Signed-off-by: silanus23 <[email protected]>
Signed-off-by: silanus23 <[email protected]>
Signed-off-by: silanus23 <[email protected]>
@silanus23 silanus23 force-pushed the path-trackin-conditon-node branch from 61389fd to 3ea85d8 Compare October 24, 2025 10:11
Signed-off-by: silanus23 <[email protected]>
@codecov
Copy link

codecov bot commented Oct 24, 2025

Codecov Report

❌ Patch coverage is 82.97872% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ition/is_within_path_tracking_bounds_condition.cpp 80.95% 8 Missing ⚠️
Files with missing lines Coverage Δ
...ition/is_within_path_tracking_bounds_condition.hpp 100.00% <100.00%> (ø)
...ition/is_within_path_tracking_bounds_condition.cpp 80.95% <80.95%> (ø)

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

My sincere apologoes for the gross delay - ROSCon + vacation + time to get back to speed meant that some things didn't get my time until today. Overall this looks great and its just small quibbles and we should be able to merge this soon!

Can this be moved from a draft? Did you test this to work well?

For docs: this needs to be having its configuration guide page + add to Navigation Plugins table + migration guide with the larger feature description



+   if (!getInput("max_error_left", max_error_left_)) {
+     RCLCPP_ERROR(logger_, "max_error_left parameter not provided");
+     return BT::NodeStatus::FAILURE;
+   }
+ 
+   if (max_error_left_ < 0.0) {
+     RCLCPP_WARN(logger_, "max_error_left should be positive, using absolute value");
+     max_error_left_ = std::abs(max_error_left_);
+   }
+ 
+   if (!getInput("max_error_right", max_error_right_)) {
+     RCLCPP_ERROR(logger_, "max_error_right parameter not provided");
+     return BT::NodeStatus::FAILURE;
+   }
+ 
+   if (max_error_right_ < 0.0) {
+     RCLCPP_WARN(logger_, "max_error_right should be positive, using absolute value");
+     max_error_right_ = std::abs(max_error_right_);
+   }

These lines should be covered in tests

: BT::ConditionNode(condition_name, conf),
last_error_(std::numeric_limits<double>::max())
{
auto node = config().blackboard->get<nav2::LifecycleNode::SharedPtr>("node");
Copy link
Member

Choose a reason for hiding this comment

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

Set logger_ = node->get_logger();

rclcpp::Logger logger_{rclcpp::get_logger("is_within_path_tracking_bounds_node")};
rclcpp::CallbackGroup::SharedPtr callback_group_;
rclcpp::executors::SingleThreadedExecutor callback_group_executor_;
rclcpp::Subscription<nav2_msgs::msg::TrackingFeedback>::SharedPtr tracking_feedback_sub_;
Copy link
Member

Choose a reason for hiding this comment

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

Use nav2::Subscription

};
}

private:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private:
protected:

double max_error_left_{1.5};
std::chrono::milliseconds bt_loop_duration_;

void trackingFeedbackCallback(const nav2_msgs::msg::TrackingFeedback::SharedPtr msg);
Copy link
Member

Choose a reason for hiding this comment

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

Move this before the variables; all methods before all variables please. Also needs doxygen documentation for the API

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.

2 participants