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.

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

Revise timestamping #18

Merged
merged 14 commits into from
Nov 19, 2024
Merged

Revise timestamping #18

merged 14 commits into from
Nov 19, 2024

Conversation

benemer
Copy link
Member

@benemer benemer commented Nov 15, 2024

There are some inconsistencies in our timestamping that this PR addresses to fix.

First, we currently query the TF tree for the odometry from the beginning to the end of the current scan. This is incorrect because the sensor might drop frames, and we, in that case, ignore the odometry during that time.

Second, we assume that the timestamps of the points are absolute, which is not necessarily the case.

We can still not cover one case: if the timestamp of the ROS message is the beginning of the scan and no timestamps are provided for the points. In such a case, there is no way to query the correct odometry from TF because the exact timestamp at the scan's end is unknown.

@benemer benemer added the help wanted Extra attention is needed label Nov 15, 2024
@tizianoGuadagnino
Copy link
Collaborator

Introduce the TimeStampsHandler module

I now introduce the TimeStampsHandler module, which implements the logic discussed with @benemer, that is:

  1. If the per-point timestamp is not provided or the `PointCloud2' msg is stamped at the end of the scan, we use the msg stamp itself to query the tf for the odometry or
  2. If the per-point timestamps are provided and the message is stamped at the beginning of the scan (as it should), then we add the scan duration to the message stamp to query the TF for the corresponding wheel odometry.

In both cases, the end stamp of one odometry query will become the begin stamp of the following query.

The only breaking point I see until now is if the scan is stamped at the end and the relative timestamps in the PointField have a negative value. In that case, our motion compensation will break very severely... This shouldn't happen (last famous words).

@benemer
Copy link
Member Author

benemer commented Nov 18, 2024

The current implementation misses the combination of stamping at the beginning of the scan and having relative timestamps. Right now, we assume you stamp at the end, even though you can get the duration from the timestamps.

Also, having negative timestamps for de-skewing does not work when using the KISS deskewing because we subtract the mid_pose_timestamp=0.5 from it, see here.

@benemer benemer removed the help wanted Extra attention is needed label Nov 19, 2024
@benemer benemer marked this pull request as ready for review November 19, 2024 10:17
@benemer benemer merged commit 27d76e3 into main Nov 19, 2024
11 checks passed
@benemer benemer deleted the benedikt/revise_timestamping branch November 19, 2024 10:18
tizianoGuadagnino added a commit that referenced this pull request Nov 19, 2024
* Query from last stamp in case of frame drops

* Rename

* WIP

* Fix conversions

* Fix build

* Make CI happy

* Add TimeStampHandler module to handle this madness

* Minor renaming

* Try to take care of absolute timestamping of points + scan stamped at
the end

* Comments

* Should fix all cases that came to my mind at this time

* Some comments to make stuff more clear

* Remove useless case, we need to fix deskewing

* Fix condition for clarity

---------

Co-authored-by: tizianoGuadagnino <[email protected]>
tizianoGuadagnino added a commit that referenced this pull request Nov 21, 2024
* Query from last stamp in case of frame drops

* Rename

* WIP

* Fix conversions

* Fix build

* Make CI happy

* Add TimeStampHandler module to handle this madness

* Minor renaming

* Try to take care of absolute timestamping of points + scan stamped at
the end

* Comments

* Should fix all cases that came to my mind at this time

* Some comments to make stuff more clear

* Remove useless case, we need to fix deskewing

* Fix condition for clarity

---------

Co-authored-by: tizianoGuadagnino <[email protected]>
tizianoGuadagnino added a commit that referenced this pull request Nov 22, 2024
* We didnt have proper flagging in the online node

* Clean launch common args

* Add new threshold

* Update CMakeLists.txt (#7)

Make *USE_SYSTEM_SOPHUS* consistent with the rest of the options.

* Update README.md (#6)

More accurate statement in the readme, the system does not need to follow a unicycle motion model, just the pose correction needs to do that, which applies to much more robotics platforms.

* Add python checks to pre-commit (#9)

* Add black and isort

* Format

* use black for isort

* Fixing ROS launch system (#8)

* We didnt have proper flagging in the online node

* Clean launch common args

* Fix formatting python

* Tiny modification, but makes sense

* Renaming and add configuration for the correspondence threshold

* Fix pipeline

* Consistency in the CMakeLists

* Remove ternary operator and make the two functions consistent

* Add 2d lidar support, need testing

* Restore ros utils to master

* Revise timestamping (#18)

* Query from last stamp in case of frame drops

* Rename

* WIP

* Fix conversions

* Fix build

* Make CI happy

* Add TimeStampHandler module to handle this madness

* Minor renaming

* Try to take care of absolute timestamping of points + scan stamped at
the end

* Comments

* Should fix all cases that came to my mind at this time

* Some comments to make stuff more clear

* Remove useless case, we need to fix deskewing

* Fix condition for clarity

---------

Co-authored-by: tizianoGuadagnino <[email protected]>

* Kinematic ICP Threshold (#15)

* We didnt have proper flagging in the online node

* Clean launch common args

* Add new threshold

* Update CMakeLists.txt (#7)

Make *USE_SYSTEM_SOPHUS* consistent with the rest of the options.

* Update README.md (#6)

More accurate statement in the readme, the system does not need to follow a unicycle motion model, just the pose correction needs to do that, which applies to much more robotics platforms.

* Add python checks to pre-commit (#9)

* Add black and isort

* Format

* use black for isort

* Fixing ROS launch system (#8)

* We didnt have proper flagging in the online node

* Clean launch common args

* Fix formatting python

* Tiny modification, but makes sense

* Renaming and add configuration for the correspondence threshold

* Fix pipeline

* Consistency in the CMakeLists

* Remove ternary operator and make the two functions consistent

* Update cpp/kinematic_icp/correspondence_threshold/CorrespondenceThreshold.hpp

Co-authored-by: Benedikt Mersch <[email protected]>

* Renaming according to Ben's review

* Why a ref

* Odometry Regularization (#19)

* Natural extension to make also the odometry regularization optional

* Consistency in the CMakeLists

* Remove ternary operator and make the two functions consistent

* Update cpp/kinematic_icp/correspondence_threshold/CorrespondenceThreshold.hpp

Co-authored-by: Benedikt Mersch <[email protected]>

* Renaming according to Ben's review

* Why a ref

* Use the same default

---------

Co-authored-by: Benedikt Mersch <[email protected]>
Co-authored-by: Benedikt Mersch <[email protected]>

---------

Co-authored-by: Benedikt Mersch <[email protected]>
Co-authored-by: Benedikt Mersch <[email protected]>

* We didnt have proper flagging in the online node

* Add new threshold

* Renaming and add configuration for the correspondence threshold

* Fix pipeline

* fixx

* Include timestamps from the projected laser

---------

Co-authored-by: Benedikt Mersch <[email protected]>
Co-authored-by: Benedikt Mersch <[email protected]>
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