Skip to content

Conversation

Talhanc
Copy link
Contributor

@Talhanc Talhanc commented Oct 5, 2025

Purpose

This branch is created as a clean starting point for the ESKF implementation from earlier this year.
The goal is to complete the ESKF and potentially improve its performance.

Objectives

  • Implement possibility for different measurement types
  • Potentially look at faster and cheaper methods for model discretization.

Talhanc and others added 27 commits February 9, 2025 19:47
…ses msg imu/data_raw and /orca/pose as the dvl info
@Talhanc Talhanc self-assigned this Oct 5, 2025
@Talhanc Talhanc linked an issue Oct 5, 2025 that may be closed by this pull request
3 tasks
Copy link
Contributor

@Andeshog Andeshog left a comment

Choose a reason for hiding this comment

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

Talha is back 🗣️

Comment on lines +3 to +5
imu_topic: /imu/data_raw
dvl_topic: /orca/twist
odom_topic: odom
Copy link
Contributor

Choose a reason for hiding this comment

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

These can go to orca.yaml in auv_setup

Copy link
Contributor

Choose a reason for hiding this comment

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

newdrone.yaml 👀 name not decided yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let this be a thing we do semi last

Comment on lines 39 to 40

struct state_quat {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use same style as for classes, i.e.

struct StateQuat

Comment on lines +112 to +114
std_msgs::msg::Float64 nis_msg;
nis_msg.data = eskf_->NIS_;
nis_pub_->publish(nis_msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this value is not used/always needed, could it be put under a debug flag?

@Talhanc Talhanc requested a review from chrstrom October 6, 2025 07:05
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.92%. Comparing base (41850c4) to head (c6f948e).

Files with missing lines Patch % Lines
navigation/eskf/src/eskf.cpp 0.00% 116 Missing ⚠️
navigation/eskf/src/eskf_ros.cpp 0.00% 96 Missing ⚠️
navigation/eskf/src/eskf_utils.cpp 0.00% 33 Missing ⚠️
navigation/eskf/include/eskf/typedefs.hpp 0.00% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
- Coverage   18.77%   16.92%   -1.85%     
==========================================
  Files          38       42       +4     
  Lines        2376     2635     +259     
  Branches      426      428       +2     
==========================================
  Hits          446      446              
- Misses       1815     2074     +259     
  Partials      115      115              
Flag Coverage Δ
unittests 16.92% <0.00%> (-1.85%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
navigation/eskf/include/eskf/typedefs.hpp 0.00% <0.00%> (ø)
navigation/eskf/src/eskf_utils.cpp 0.00% <0.00%> (ø)
navigation/eskf/src/eskf_ros.cpp 0.00% <0.00%> (ø)
navigation/eskf/src/eskf.cpp 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Q3rkses Q3rkses self-requested a review October 7, 2025 15:12
@kluge7 kluge7 requested a review from Copilot October 7, 2025 15:44
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements an Error-state Kalman Filter (ESKF) for navigation, adding a complete ROS 2 node implementation with support for IMU and DVL sensor fusion. The implementation provides odometry estimation using quaternion-based state representation for the nominal state and Euler angles for the error state.

  • Implements core ESKF algorithm with IMU prediction and DVL measurement updates
  • Adds ROS 2 integration with configurable parameters and topic subscriptions
  • Includes automated testing infrastructure for the new ESKF node

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
navigation/eskf/src/eskf.cpp Core ESKF algorithm implementation with state prediction and measurement updates
navigation/eskf/src/eskf_ros.cpp ROS 2 node wrapper providing sensor callbacks and odometry publishing
navigation/eskf/src/eskf_node.cpp Main entry point for the ESKF ROS node
navigation/eskf/src/eskf_utils.cpp Utility functions for quaternion operations and mathematical helpers
navigation/eskf/include/eskf/*.hpp Header files defining classes, typedefs, and function declarations
navigation/eskf/config/eskf_params.yaml Configuration parameters for noise models and sensor topics
navigation/eskf/launch/eskf.launch.py Launch file for starting the ESKF node with parameters
navigation/eskf/package.xml ROS package definition with dependencies
navigation/eskf/CMakeLists.txt Build configuration for the ESKF package
tests/ros_node_tests/eskf_node_test.sh Integration test script for ESKF node functionality
.github/workflows/ros-node-tests.yml CI workflow update to include ESKF node testing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@Q3rkses Q3rkses left a comment

Choose a reason for hiding this comment

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

Talha comeback 👀?

Comment on lines +3 to +5
imu_topic: /imu/data_raw
dvl_topic: /orca/twist
odom_topic: odom
Copy link
Contributor

Choose a reason for hiding this comment

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

newdrone.yaml 👀 name not decided yet

// @brief Calculate the NIS
// @param innovation: Innovation vector
// @param S: Innovation covariance matrix
void NIS(const Eigen::Vector3d& innovation, const Eigen::Matrix3d& S);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest thinking about visualization from the start. Don't know if you want to implement it seperately or inside this library e.g calculate ANIS function, print covariance elipse function ... , but you should not neglect it. Would be nice to have some nice visuals on propagation of error state and maybe when / how covariances blow up.

"I've probably spent 80% of my time on visualization and understanding and 20% of my time doing math and programming" -Edmund

Probably helps with debugging later if you have put thought into testing from the start. But then again i am no expert on this (yet, give me 4 weeks ¯_(ツ)_/¯ )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to look a this later on, but yes a good way to plot would be nice. The issue though is that i would like to have a plot of the estimates with there uncertainty, NEES and NIS given that we have ground truth. This will make it easier for you to test. The issue is that NIS upper and lower bounds depends on the amount of measurements, which can become a problem ( i feel so ) if we have different dimensions and different measurements.

@vortexntnu vortexntnu deleted a comment from Copilot AI Oct 8, 2025
@vortexntnu vortexntnu deleted a comment from Copilot AI Oct 8, 2025
@vortexntnu vortexntnu deleted a comment from Copilot AI Oct 8, 2025
@vortexntnu vortexntnu deleted a comment from Copilot AI Oct 8, 2025
@vortexntnu vortexntnu deleted a comment from Copilot AI Oct 8, 2025
@chrstrom
Copy link
Member

@Talhanc any reason for making the ros-eskf class be so very stateful? I'd expect that the non-ros eskf class maintained most of the state, only exposing things through getters (leaving the ros-layer mostly for conversions etc)

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.

[TASK] Implement ESKF for sensor fusion of DVL and IMU

4 participants