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.

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

Jump to bottom

Migrate beluga_amcl to the new API #328

Merged
merged 11 commits into from
Feb 21, 2024
Merged

Migrate beluga_amcl to the new API #328

merged 11 commits into from
Feb 21, 2024

Conversation

nahueespinosa
Copy link
Member

@nahueespinosa nahueespinosa commented Feb 19, 2024 •
edited
Loading

Proposed changes

Builds on top of #327 to use the new range-based API through the Amcl class. This also refactors the initialization and publishing logic a bit. The code in the ROS 1 and ROS 2 implementations is more similar now.

Related to #279.

Type of change

  • 🐛 Bugfix (change which fixes an issue)
  • 🚀 Feature (change which adds functionality)
  • 📚 Documentation (change which fixes or extends documentation)

💥 Breaking change! This removes mixins from all user-facing implementations.

Checklist

  • Lint and unit tests (if any) pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All commits have been signed for DCO

@nahueespinosa nahueespinosa added enhancement New feature or request cpp Related to C++ code labels Feb 19, 2024
@nahueespinosa nahueespinosa self-assigned this Feb 19, 2024
@nahueespinosa nahueespinosa marked this pull request as ready for review February 20, 2024 00:01
hidmic
hidmic reviewed Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

First pass, minor comments. Code look solid. I have to double check the resampling logic to make sure we are not regressing.

beluga_amcl/include/beluga_amcl/amcl_node.hpp Outdated Show resolved Hide resolved
beluga_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
beluga_amcl/include/beluga_amcl/amcl_nodelet.hpp Outdated Show resolved Hide resolved
beluga_amcl/test/test_amcl_node.cpp Outdated Show resolved Hide resolved
@nahueespinosa nahueespinosa changed the title Migrate beluga_amcl to the new API Migrate beluga_amcl to the new API Feb 20, 2024
hidmic
hidmic reviewed Feb 20, 2024
View reviewed changes
beluga_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
beluga_amcl/src/amcl_nodelet.cpp Outdated Show resolved Hide resolved
beluga_amcl/test/test_amcl_node.cpp Outdated Show resolved Hide resolved
Base automatically changed from nahuel/add-amcl-impl to main February 21, 2024 12:50
@nahueespinosa
Migrate beluga_amcl to the new API
ed2d565 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Address code review comments from AMCL implementation
22b27b7 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Address code review comments
3e0fc7d 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Fix static analyzer errors
561c3f7 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
const all the things!
0cefddb 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Keep old initialization behavior
aa1bbbc 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Fix static analyzer errors
6abb693 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Add missing test cases
c2a5d23 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Initialize from map enables TF broadcast
8454718 
Functionality is the same, but this is better for consistency plus we get to
add a comment exactly where the "unexpected" part is.

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Fix ROS 1 build
abe2652 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa
Use try-catch instead of checking for nullptr
59035e3 
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
hidmic
hidmic approved these changes Feb 21, 2024
View reviewed changes
Copy link
Collaborator

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

beluga_amcl/include/beluga_amcl/amcl_nodelet.hpp Show resolved Hide resolved
@nahueespinosa nahueespinosa merged commit 7c60983 into main Feb 21, 2024
8 checks passed
@nahueespinosa nahueespinosa deleted the nahuel/rewrite-beluga-amcl branch February 21, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

Assignees

@nahueespinosa nahueespinosa

Labels
cpp Related to C++ code enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nahueespinosa @hidmic