Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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
3 changes: 2 additions & 1 deletion beluga_amcl/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ See [Beluga AMCL documentation](https://ekumen-os.github.io/beluga/packages/belu

### Subscribed Topics

The subscribed topic names can be changed with the parameters `map_topic`, `scan_topic` and `initial_pose_topic`.
The subscribed topic names can be changed with the parameters `map_topic`, `scan_topic`, `odom_topic` and `initial_pose_topic`.

| Topic | Type | Description |
|------------------|-------------------------------------------|-----------------------------------------------------------------------------|
| `map` | `nav_msgs/OccupancyGrid` | Input topic for map updates. |
| `scan` | `sensor_msgs/LaserScan` | Input topic for laser scan updates. |
| `odom` | `nav_msgs/Odometry` | Input topic for odometry updates (when use_odometry_propagation is enabled). |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fbattocchia nit:

Suggested change
| `odom` | `nav_msgs/Odometry` | Input topic for odometry updates (when use_odometry_propagation is enabled). |
| `odom` | `nav_msgs/Odometry` | Input topic for odometry updates (when use_odometry_propagation is enabled). |

and align the other rows.

| `initial_pose` | `geometry_msgs/PoseWithCovarianceStamped` | Input topic for pose mean and covariance to initialize the particle filter. |

### Published Topics
Expand Down
7 changes: 7 additions & 0 deletions beluga_amcl/config/Amcl.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ gen.add(
default="scan"
)

gen.add(
"odom_topic", str_t, 0,
"Topic to subscribe to in order to "
"receive the odometry data for localization.",
default="odom"
)

gen.add(
"min_particles", int_t, 0,
"Minimum allowed number of particles.",
Expand Down
16 changes: 16 additions & 0 deletions beluga_amcl/include/beluga_amcl/amcl_node.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@

#include <beluga/beluga.hpp>
#include <beluga_ros/amcl.hpp>
#include <deque>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fbattocchia nit: this one goes up with the other C++ standard headers. Wonder why the linter didn't complain 馃

#include <nav_msgs/msg/odometry.hpp>
#include "beluga_amcl/message_filters.hpp"
#include "beluga_amcl/ros2_common.hpp"

Expand All @@ -60,6 +62,9 @@ class AmclNode : public BaseAMCLNode {
~AmclNode() override;

protected:
/// Type Buffer for queued odometry motions (timestamp, pose)
using OdometryMotion = std::pair<tf2::TimePoint, Sophus::SE2d>;

/// Callback for lifecycle transitions from the INACTIVE state to the ACTIVE state.
void do_activate(const rclcpp_lifecycle::State&) override;

Expand Down Expand Up @@ -91,6 +96,12 @@ class AmclNode : public BaseAMCLNode {
/// Callback for laser scan updates.
void laser_callback(sensor_msgs::msg::LaserScan::ConstSharedPtr);

/// Callback for odometry updates.
void odometry_callback(nav_msgs::msg::Odometry::ConstSharedPtr);

/// Processes and removes from the buffer all odometry actions up to a given time point.
void process_buffered_odometry_until(std::deque<OdometryMotion>& buffer, const tf2::TimePoint& until);

/// Callback for pose (re)initialization.
void do_initial_pose_callback(geometry_msgs::msg::PoseWithCovarianceStamped::SharedPtr) override;

Expand Down Expand Up @@ -143,6 +154,9 @@ class AmclNode : public BaseAMCLNode {
/// Connection for laser scan updates filter and callback.
::message_filters::Connection laser_scan_connection_;

/// Odometry updates subscription.
rclcpp::Subscription<nav_msgs::msg::Odometry>::SharedPtr odom_sub_;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit, but for you to know:

In this node it does not really matter because this is a lifecycle node and thus subscribers get destroyed before the wrapping class does, but its a good habit to put anything that has a callback int the very last position in the class members declarations.

The reason is so that whenever the class gets destroyed, those are the first to be destroyed, and that includes rendering them unable to execute their callbacks.

If they are not last, some datastructure their callbacks depend on might be destroyed first concurrently with something triggering the callback, and that will usually cause a crash.

This is a frequent cause of flaky tests and nodes that randomly seem to crash when being stopped.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it's good to know

/// Particle filter instance.
std::unique_ptr<beluga_ros::Amcl> particle_filter_;
/// Last known pose estimate, if any.
Expand All @@ -151,6 +165,8 @@ class AmclNode : public BaseAMCLNode {
std::optional<Sophus::SE2d> last_known_odom_transform_in_map_;
/// Whether to broadcast transforms or not.
bool enable_tf_broadcast_{false};
/// Buffer for queued odometry motions (timestamp, pose)
std::deque<OdometryMotion> odometry_motion_buffer_;
};

} // namespace beluga_amcl
Expand Down
52 changes: 52 additions & 0 deletions beluga_amcl/src/amcl_node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,12 @@ 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 = "Set true to enable odometry-driven filter propagation.";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fbattocchia nit:

Suggested change
descriptor.description = "Set true to enable odometry-driven filter propagation.";
descriptor.description = "Enable odometry-driven filter propagation.";

declare_parameter("use_odometry_propagation", rclcpp::ParameterValue(false), descriptor);
}
}

AmclNode::~AmclNode() {
Expand Down Expand Up @@ -251,6 +257,18 @@ 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");

{
// Subscribe to odometry topic to buffer odometry motions
if (get_parameter("use_odometry_propagation").as_bool()) {
odom_sub_ = create_subscription<nav_msgs::msg::Odometry>(
get_parameter("odom_topic").as_string(), rclcpp::SensorDataQoS(),
std::bind(&AmclNode::odometry_callback, this, std::placeholders::_1), common_subscription_options_);

RCLCPP_INFO(get_logger(), "Subscribed to odom_topic: %s", odom_sub_->get_topic_name());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
RCLCPP_INFO(get_logger(), "Subscribed to odom_topic: %s", odom_sub_->get_topic_name());
RCLCPP_INFO(get_logger(), "Subscribed to odometry topic: %s", odom_sub_->get_topic_name());

odometry_motion_buffer_.clear();
}
}
}

void AmclNode::do_deactivate(const rclcpp_lifecycle::State&) {
Expand All @@ -259,6 +277,7 @@ void AmclNode::do_deactivate(const rclcpp_lifecycle::State&) {
laser_scan_filter_.reset();
laser_scan_sub_.reset();
global_localization_server_.reset();
odom_sub_.reset();
if (likelihood_field_pub_) {
likelihood_field_pub_->on_deactivate();
}
Expand All @@ -268,6 +287,7 @@ void AmclNode::do_cleanup(const rclcpp_lifecycle::State&) {
particle_filter_.reset();
likelihood_field_pub_.reset(); // attaching likelihood_field_pub_ lifespan to particle_filter_ lifespan
enable_tf_broadcast_ = false;
odometry_motion_buffer_.clear();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should also clear this whenever the particle filter is initialized/created.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm clearing the buffer in do_activate() where the odometry subscription is performed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, but that only happens when the node starts. The filter might be reinitialized multiple times afterwards, whenever the map is updated or when the user sends a pose intialization message. Look for calls to initialize_from_estimate() in amcl_node.cpp

}

auto AmclNode::get_initial_estimate() const -> std::optional<std::pair<Sophus::SE2d, Eigen::Matrix3d>> {
Expand Down Expand Up @@ -466,13 +486,45 @@ void AmclNode::do_periodic_timer_callback() {
}
}

void AmclNode::odometry_callback(nav_msgs::msg::Odometry::ConstSharedPtr odom) {
if (!particle_filter_) {
RCLCPP_WARN_THROTTLE(
get_logger(), *get_clock(), 2000,
"Ignoring odometry data because the particle filter has not been initialized");
return;
}
// Use the odometry message timestamp and pose
const auto time = tf2_ros::fromMsg(odom->header.stamp);
auto base_pose_in_odom = Sophus::SE2d{};
tf2::convert(odom->pose.pose, base_pose_in_odom);
odometry_motion_buffer_.emplace_back(time, base_pose_in_odom);
}

void AmclNode::process_buffered_odometry_until(std::deque<OdometryMotion>& buffer, const tf2::TimePoint& until) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fbattocchia nit^2: do we need to pass the buffer explicitly? The call site would read better if we didn't.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@hidmic The parameter remained from when I implemented this function in amcl_ros and I forgot to remove it. Thank you for pointing that out.

while (!buffer.empty()) {
const auto& [odom_time, odom_pose] = buffer.front();
if (odom_time > until) {
break;
}
particle_filter_->update(odom_pose);
buffer.pop_front();
}
}

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;
}

// If use_odometry_propagation is enabled, process odometry buffer up to lidar timestamp
if (get_parameter("use_odometry_propagation").as_bool()) {
const auto laser_scan_stamp = tf2_ros::fromMsg(laser_scan->header.stamp);
process_buffered_odometry_until(odometry_motion_buffer_, laser_scan_stamp);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit, but if odometry driven prop is off, the queue will be empty anyway.

@fbattocchia-ekumen fbattocchia-ekumen Sep 3, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@glpuga Removing the if statement would make the code more compact, but in my opinion, less explicit. Keeping it helps clarify that this action is only performed when use_odometry_propagation is enabled, which can be beneficial for someone reading or maintaining the code. But replacing the if with a comment can serve the same purpose, so I'll do that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok, I'm ok with it staying.


// Get base pose in odom frame at laser scan timestamp
auto base_pose_in_odom = Sophus::SE2d{};
try {
// Use the lookupTransform overload with no timeout since we're not using a dedicated
Expand Down
6 changes: 6 additions & 0 deletions beluga_amcl/src/ros2_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ BaseAMCLNode::BaseAMCLNode(
this->declare_parameter("scan_topic", rclcpp::ParameterValue("scan"), descriptor);
}

{
auto descriptor = rcl_interfaces::msg::ParameterDescriptor();
descriptor.description = "Topic to subscribe to in order to receive odometry messages for motion propagation.";
this->declare_parameter("odom_topic", rclcpp::ParameterValue("odom"), descriptor);
}

{
auto descriptor = rcl_interfaces::msg::ParameterDescriptor();
descriptor.description = "Minimum allowed number of particles.";
Expand Down
159 changes: 159 additions & 0 deletions beluga_amcl/test/test_amcl_node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,15 @@ class AmclNodeUnderTest : public beluga_amcl::AmclNode {

/// Return the last known estimate. Throws if there is no estimate.
const auto& estimate() { return last_known_estimate_.value(); }

/// Check if odom_sub_ is created
bool has_odom_sub() const { return odom_sub_ != nullptr; }

/// Expose odometry callback for testing
void odometry_callback(nav_msgs::msg::Odometry::ConstSharedPtr odom) { AmclNode::odometry_callback(odom); }

/// Expose odometry_motion_buffer_ for testing
const auto& odometry_motion_buffer() const { return odometry_motion_buffer_; }
};

/// Base node fixture class with common utilities.
Expand Down Expand Up @@ -698,6 +707,156 @@ TEST_F(TestNode, TransformValue) {
EXPECT_NEAR(transform.so2().log(), 0.0, 0.01);
}

TEST_F(TestNode, OdomSubNotCreatedWhenPropagationDisabled) {
amcl_node_->set_parameter(rclcpp::Parameter{"use_odometry_propagation", false});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fbattocchia nit: shall we check that the default value (when unset) is false too?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@hidmic No, I don't verify that it's the default value, but if I delete this line, I can verify two things: that the default value is false and that the odometry subscription is not created if it is false.

amcl_node_->configure();
amcl_node_->activate();
tester_node_->publish_map();
ASSERT_TRUE(wait_for_initialization());

// odom_sub_ should not be created when propagation is disabled
EXPECT_EQ(amcl_node_->has_odom_sub(), false);
}

TEST_F(TestNode, OdomSubCreatedWhenPropagationEnabled) {
amcl_node_->set_parameter(rclcpp::Parameter{"use_odometry_propagation", true});
amcl_node_->configure();
amcl_node_->activate();
tester_node_->publish_map();
ASSERT_TRUE(wait_for_initialization());

// odom_sub_ should be created when propagation is enabled
EXPECT_EQ(amcl_node_->has_odom_sub(), true);
}

TEST_F(TestNode, OdometryPropagationConsumesBuffer) {
amcl_node_->set_parameter(rclcpp::Parameter{"use_odometry_propagation", true});
amcl_node_->set_parameter(rclcpp::Parameter{"set_initial_pose", true});
amcl_node_->configure();
amcl_node_->activate();
tester_node_->publish_map();
ASSERT_TRUE(wait_for_initialization());

// Publish several odometry messages to fill the buffer
for (int i = 0; i < 5; ++i) {
tester_node_->publish_odometry(i * 0.1, 0.0);
spin_for(20ms, amcl_node_, tester_node_);
}

// Check that odometry_motion_buffer_ has values before laser scan
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 5);

// Configure TF so get_base_pose_in_odom works by publishing a laser scan with transform
tester_node_->publish_laser_scan_with_odom_to_base(Sophus::SE2d{});

// Wait for the callback to process the buffer
spin_for(100ms, amcl_node_, tester_node_);

// Check that odometry_motion_buffer_ was consumed up to the lidar timestamp
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 0);
Comment on lines +754 to +755

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add two more odometry messages: one with a timestamp equal than the one in the lidar, and one later, and test that we consume all but the one with the later timestamp.

Receiving lidar and odometry messages with the same timestamp is a frequent thing in simulators.

}

TEST_F(TestNode, OdometryBufferConsumedWhenOdomTimestampsLessThanLidar) {
amcl_node_->set_parameter(rclcpp::Parameter{"use_odometry_propagation", true});
amcl_node_->set_parameter(rclcpp::Parameter{"set_initial_pose", true});
amcl_node_->configure();
amcl_node_->activate();
tester_node_->publish_map();
ASSERT_TRUE(wait_for_initialization());

// Publish several odometry messages to fill the buffer
for (int i = 0; i < 5; ++i) {
tester_node_->publish_odometry(i * 0.1, 0.0);
spin_for(20ms, amcl_node_, tester_node_);
}

// Check that odometry_motion_buffer_ has values before laser scan
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 5);

// Configure TF so get_base_pose_in_odom works by publishing a laser scan with transform
tester_node_->publish_laser_scan_with_odom_to_base(Sophus::SE2d{});

// Wait for the callback to process the buffer
spin_for(100ms, amcl_node_, tester_node_);

// Check that odometry_motion_buffer_ was consumed up to the lidar timestamp
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 0);
}

TEST_F(TestNode, OdometryBufferConsumedWhenOdomTimestampsEqualThanLidar) {
amcl_node_->set_parameter(rclcpp::Parameter{"use_odometry_propagation", true});
amcl_node_->set_parameter(rclcpp::Parameter{"set_initial_pose", true});
amcl_node_->configure();
amcl_node_->activate();
tester_node_->publish_map();
ASSERT_TRUE(wait_for_initialization());

rclcpp::Time timestamp;
// Publish several odometry messages to fill the buffer
for (int i = 0; i < 5; ++i) {
timestamp = tester_node_->now();
tester_node_->publish_odometry(i * 0.1, 0.0, 1.0, timestamp);
spin_for(20ms, amcl_node_, tester_node_);
}

// Check that odometry_motion_buffer_ has values before laser scan
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 5);

// Configure TF so get_base_pose_in_odom works by publishing a laser scan with transform
tester_node_->publish_laser_scan_with_odom_to_base(Sophus::SE2d{}, timestamp);

// Wait for the callback to process the buffer
spin_for(100ms, amcl_node_, tester_node_);

// Check that odometry_motion_buffer_ was consumed up to the lidar timestamp
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 0);
}

TEST_F(TestNode, OdometryBufferConsumedWhenOdomTimestampsMajorThanLidar) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fbattocchia nit:

Suggested change
TEST_F(TestNode, OdometryBufferConsumedWhenOdomTimestampsMajorThanLidar) {
TEST_F(TestNode, OdometryBufferConsumedWhenOdomTimestampsGreaterThanLidar) {

amcl_node_->set_parameter(rclcpp::Parameter{"use_odometry_propagation", true});
amcl_node_->set_parameter(rclcpp::Parameter{"set_initial_pose", true});
amcl_node_->configure();
amcl_node_->activate();
tester_node_->publish_map();
ASSERT_TRUE(wait_for_initialization());

rclcpp::Time timestamp;
// Publish several odometry messages to fill the buffer
for (int i = 0; i < 5; ++i) {
timestamp = tester_node_->now();
tester_node_->publish_odometry(i * 0.1, 0.0, 1.0, timestamp);
spin_for(20ms, amcl_node_, tester_node_);
}

const auto newtimestamp = tester_node_->now();
tester_node_->publish_odometry(5 * 0.1, 0.0, 1.0, newtimestamp);
spin_for(20ms, amcl_node_, tester_node_);

// Check that odometry_motion_buffer_ has values before laser scan
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 6);

// Configure TF so get_base_pose_in_odom works by publishing a laser scan with transform
tester_node_->publish_laser_scan_with_odom_to_base(Sophus::SE2d{}, timestamp);

// Wait for the callback to process the buffer
spin_for(100ms, amcl_node_, tester_node_);

// Check that odometry_motion_buffer_ was consumed up to the lidar timestamp
EXPECT_EQ(amcl_node_->odometry_motion_buffer().size(), 1);
}

TEST_F(TestNode, OdomSubWithoutParticles) {
// Verify that particle filter remains uninitialized
EXPECT_FALSE(amcl_node_->particle_filter() != nullptr);

// Call odometry callback when particle filter is not initialized
const nav_msgs::msg::Odometry::SharedPtr odom = std::make_shared<nav_msgs::msg::Odometry>();
amcl_node_->odometry_callback(odom);

// The callback should have returned early without filling the queue
EXPECT_TRUE(amcl_node_->odometry_motion_buffer().empty());
}

class TestParameterValue : public ::testing::TestWithParam<rclcpp::Parameter> {};

INSTANTIATE_TEST_SUITE_P(
Expand Down
Loading
Loading