-
Notifications
You must be signed in to change notification settings - Fork 34
Add timer-based propagation functionality to AMCL #492
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
8479fd0
8086f76
1571ebe
9994687
4f7b10c
ce8b3f4
3ff9266
d13e2fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,6 +183,16 @@ AmclNode::AmclNode(const rclcpp::NodeOptions& options) : BaseAMCLNode{"amcl", "" | |
| "and ignore subsequent ones."; | ||
| declare_parameter("first_map_only", false, descriptor); | ||
| } | ||
|
|
||
| { | ||
| auto descriptor = rcl_interfaces::msg::ParameterDescriptor(); | ||
| descriptor.description = "Frequency in Hz for increased propagation rate. Set to 0 to disable."; | ||
| descriptor.floating_point_range.resize(1); | ||
| descriptor.floating_point_range[0].from_value = 0.0; | ||
| descriptor.floating_point_range[0].to_value = 100.0; | ||
| descriptor.floating_point_range[0].step = 0.0; | ||
| declare_parameter("increase_propagation", rclcpp::ParameterValue(0.0), descriptor); | ||
| } | ||
| } | ||
|
|
||
| AmclNode::~AmclNode() { | ||
|
|
@@ -251,6 +261,17 @@ void AmclNode::do_activate(const rclcpp_lifecycle::State&) { | |
| std::placeholders::_3), | ||
| common_service_qos, common_callback_group_); | ||
| RCLCPP_INFO(get_logger(), "Created request_nomotion_update service"); | ||
|
|
||
| // Setup increased propagation timer if enabled | ||
| { | ||
| const double propagation_freq = get_parameter("increase_propagation").as_double(); | ||
| if (propagation_freq > 0.0) { | ||
| auto period = std::chrono::duration<double>(1.0 / propagation_freq); | ||
| propagation_timer_ = | ||
| create_wall_timer(period, std::bind(&AmclNode::propagation_timer_callback, this), common_callback_group_); | ||
| RCLCPP_INFO(get_logger(), "Created propagation timer at %.1f Hz", propagation_freq); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void AmclNode::do_deactivate(const rclcpp_lifecycle::State&) { | ||
|
|
@@ -259,6 +280,7 @@ void AmclNode::do_deactivate(const rclcpp_lifecycle::State&) { | |
| laser_scan_filter_.reset(); | ||
| laser_scan_sub_.reset(); | ||
| global_localization_server_.reset(); | ||
| propagation_timer_.reset(); | ||
| if (likelihood_field_pub_) { | ||
| likelihood_field_pub_->on_deactivate(); | ||
| } | ||
|
|
@@ -466,26 +488,51 @@ void AmclNode::do_periodic_timer_callback() { | |
| } | ||
| } | ||
|
|
||
| void AmclNode::laser_callback(sensor_msgs::msg::LaserScan::ConstSharedPtr laser_scan) { | ||
| if (!particle_filter_) { | ||
| RCLCPP_WARN_THROTTLE( | ||
| get_logger(), *get_clock(), 2000, "Ignoring laser data because the particle filter has not been initialized"); | ||
| return; | ||
| } | ||
|
|
||
| auto AmclNode::get_base_pose_in_odom(const tf2::TimePoint& time) const -> std::optional<Sophus::SE2d> { | ||
| auto base_pose_in_odom = Sophus::SE2d{}; | ||
| try { | ||
| // Use the lookupTransform overload with no timeout since we're not using a dedicated | ||
| // tf thread. The message filter we are using avoids the need for it. | ||
| tf2::convert( | ||
| tf_buffer_ | ||
| ->lookupTransform( | ||
| get_parameter("odom_frame_id").as_string(), get_parameter("base_frame_id").as_string(), | ||
| tf2_ros::fromMsg(laser_scan->header.stamp)) | ||
| get_parameter("odom_frame_id").as_string(), get_parameter("base_frame_id").as_string(), time) | ||
| .transform, | ||
| base_pose_in_odom); | ||
| return base_pose_in_odom; | ||
| } catch (const tf2::TransformException& error) { | ||
| RCLCPP_ERROR(get_logger(), "Could not transform from odom to base: %s", error.what()); | ||
|
Comment on lines
-479
to
-488
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call wrapping this in a function. |
||
| RCLCPP_WARN(get_logger(), "Could not get base pose in odom: %s", error.what()); | ||
| return std::nullopt; | ||
| } | ||
| } | ||
|
|
||
| void AmclNode::propagation_timer_callback() { | ||
| if (!particle_filter_) { | ||
| RCLCPP_WARN_THROTTLE( | ||
| get_logger(), *get_clock(), 2000, "Ignoring propagation because the particle filter has not been initialized"); | ||
| return; | ||
| } | ||
|
|
||
| // Get current base pose in odom frame (latest available) | ||
| auto base_pose_in_odom = get_base_pose_in_odom(tf2::TimePointZero); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fbattocchia nit: can be |
||
| if (!base_pose_in_odom.has_value()) { | ||
| return; | ||
| } | ||
|
|
||
| // Force a propagation-only update (without sensor data) | ||
| particle_filter_->update_propagation(base_pose_in_odom.value()); | ||
|
|
||
| RCLCPP_INFO(get_logger(), "Forced propagation update executed"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this log will be too noisy, logging this message at high rate?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. Consider demoting it to DEBUG. |
||
| } | ||
|
|
||
| void AmclNode::laser_callback(sensor_msgs::msg::LaserScan::ConstSharedPtr laser_scan) { | ||
| if (!particle_filter_) { | ||
| RCLCPP_WARN_THROTTLE( | ||
| get_logger(), *get_clock(), 2000, "Ignoring laser data because the particle filter has not been initialized"); | ||
| return; | ||
| } | ||
|
|
||
| // Get base pose in odom frame at laser scan timestamp | ||
| auto base_pose_in_odom = get_base_pose_in_odom(tf2_ros::fromMsg(laser_scan->header.stamp)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it can happen that we get a lidar message with a timestamp that is previous to a get_base_pose_in_odom() update. Say, we execute get_base_pose_in_odom() at 10, 20, 30, 40, 50 and then we get a lidar message with timestamp 45. We might be adding a little bit more noise then. Maybe when we update the motion model from a separate thread then we should either
I tend to like more 1 or 2. 3 might add a lot more noise if the late arrivers are recurrent. Thoughts @fbattocchia @hidmic ?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I mean, we would be "rolling back" motion from 50 to 45, then forwarding again from 45 to 60 in the next timer event, thus adding extra "motion" and thus noise (proportional to motion).
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a very good point, and yes, this can absolutely happen. It's an out of sequence measurement (OOSM) case. A principled, general solution is probably out of scope for your purposes though (e.g. https://ieeexplore.ieee.org/document/8455401, but there is quite a bit of research on the topic).
馃 What if we don't touch the filter itself? If we let it evolve at lidar rates, we can still propagate the latest distribution up to current time on timer callback. Our control deltas wouldn't be any larger than they are at lidar rates. You could even do that deterministically on the latest estimate by applying an unscented transform with the motion nonlinear function (which we don't have, we just have a sampler, but it could be added) and spare the cost of sampled propagation.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. Can you provide an example of what it would look like? Another option is to do the the updates lazily: We don't update the filter in the timer callback, we just take note of the latest odometry motion at that time and queue it for later. When the lidar message arrives, we process the queue all the way up to the last timestamp that is before the lidar timestamp and leave any remaining one in the queue for later. We transfer the work of updating the filter to the lidar callback and force an ordered update sequence.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, I see now. I thought you wanted estimates at a rate higher than the lidar rate, but this is about processing odometry at a higher rate than lidar rate. If it were the former, what I meant is that you can always predict forward from the latest estimate (which is almost the same as relying on odometry given a fixed map to odom transform but for the explicit uncertainty modelling). But I sense it's the latter, so +100 to lazy updates. Maybe we can collect all odometry updates in a bigger control window.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a good idea to implement lazily updates. |
||
| if (!base_pose_in_odom.has_value()) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -505,7 +552,7 @@ void AmclNode::laser_callback(sensor_msgs::msg::LaserScan::ConstSharedPtr laser_ | |
|
|
||
| const auto update_start_time = std::chrono::high_resolution_clock::now(); | ||
| const auto new_estimate = particle_filter_->update( | ||
| base_pose_in_odom, // | ||
| base_pose_in_odom.value(), // | ||
| beluga_ros::LaserScan{ | ||
| laser_scan, | ||
| laser_pose_in_base, | ||
|
|
@@ -518,7 +565,7 @@ void AmclNode::laser_callback(sensor_msgs::msg::LaserScan::ConstSharedPtr laser_ | |
|
|
||
| if (new_estimate.has_value()) { | ||
| const auto& [base_pose_in_map, _] = new_estimate.value(); | ||
| last_known_odom_transform_in_map_ = base_pose_in_map * base_pose_in_odom.inverse(); | ||
| last_known_odom_transform_in_map_ = base_pose_in_map * base_pose_in_odom.value().inverse(); | ||
| last_known_estimate_ = new_estimate; | ||
|
|
||
| RCLCPP_INFO( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -210,6 +210,16 @@ class Amcl { | |||||
| /// Update the map used for localization. | ||||||
| void update_map(beluga_ros::OccupancyGrid map); | ||||||
|
|
||||||
| /// Update particles based on motion only (propagation only). | ||||||
| /** | ||||||
| * This method only performs the propagation step of the particle filter update, | ||||||
| * applying the motion model without any sensor correction. Useful for forced | ||||||
| * propagation at regular intervals without waiting for sensor data. | ||||||
| * | ||||||
| * \param base_pose_in_odom Base pose in the odometry frame. | ||||||
| */ | ||||||
| void update_propagation(Sophus::SE2d base_pose_in_odom); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fbattocchia consider making this just another
Suggested change
It is updating after all, it just uses less data. |
||||||
|
|
||||||
| /// Update particles based on motion and sensor information. | ||||||
| /** | ||||||
| * This method performs a particle filter update step using motion and sensor data. It evaluates whether | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,20 @@ void Amcl::update_map(beluga_ros::OccupancyGrid map) { | |
| std::visit([&](auto& sensor_model) { sensor_model.update_map(std::move(map)); }, sensor_model_); | ||
| } | ||
|
|
||
| void Amcl::update_propagation(Sophus::SE2d base_pose_in_odom) { | ||
| if (particles_.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| // Force propagation without checking update_policy_ | ||
| std::visit( | ||
| [&, this](auto& policy, auto& motion_model) { | ||
| particles_ |= beluga::actions::propagate(policy, motion_model(control_action_window_ << base_pose_in_odom)) | | ||
| beluga::actions::normalize(policy); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to normalize, since we only affected distribution density, but we left the weights unchanged.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
| }, | ||
| execution_policy_, motion_model_); | ||
| } | ||
|
|
||
| auto Amcl::update(Sophus::SE2d base_pose_in_odom, beluga_ros::LaserScan laser_scan) | ||
| -> std::optional<std::pair<Sophus::SE2d, Sophus::Matrix3d>> { | ||
| if (particles_.empty()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,4 +149,10 @@ TEST(TestAmcl, UpdateWithParticlesForced) { | |
| ASSERT_TRUE(estimate.has_value()); | ||
| } | ||
|
|
||
| TEST(TestAmcl, UpdatePropagationWithNoParticles) { | ||
| auto amcl = make_amcl(); | ||
| amcl.update_propagation(Sophus::SE2d{}); | ||
| ASSERT_EQ(amcl.particles().size(), 0); | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get that this manages to make the coverage checker think that the function was tested, but it does not look like its functionally testing anything. Let's do a propagation step with a few particles in the set.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I did the test to achieve coverage, I agree with it doesn't make much sense. What I need to test is that it doesn't do anything because there are no particles. I changed the function's logic so I wouldn't have to do that check. (Another option would be to return a bool and verify the return in the test.) |
||
| } // namespace | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbattocchia nit:
perhaps?