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

feat(marker_radar_lidar_calibrator): marker_radar_lidar_calibrator support for different radar msgs and transformation algorithms #180

Open
wants to merge 442 commits into
base: tier4/universe
Choose a base branch
from

Conversation

vividf
Copy link
Contributor

@vividf vividf commented Jul 23, 2024

Description

Same description as #161 after rebasing on the documentation branch.

Our previous tools assumed that radar has no elevation and we provided two algorithms:

  • Yaw-only calibration
  • x,y, yaw calibration

With elevation, we can get the full 6D pose. However, some sensors may impose restrictions on the orientation. In particular, the ARS548 imposes roll=0 for its object interface. We need an algorithm that easily accommodates this new restriction.

With this PR, we provide four algorithms

  • Yaw-only calibration (yaw_only_rotation_2d)
  • x,y, yaw calibration (svd_2d)
  • 3d calibration (svd_3d)
  • 3d transformation with roll is restricted to zero (roll_zero_3d)

and three different input msg-type for radar

  • radar_tracks
  • radar_scan
  • radar_cloud

Additionally, this PR also includes the xx1 gen2 project in the sensor_calibration_manager.

Related links

Tests performed

Notes for reviewers

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

vividf and others added 30 commits May 29, 2024 11:22
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
note: this was note made as another PR since it also involved documentation

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
@vividf vividf requested a review from knzo25 October 7, 2024 07:10
@knzo25
Copy link
Collaborator

knzo25 commented Oct 28, 2024

@vividf
Was this addressed? If so can you tell me the commit?

@vividf
Everywhere in the code that a .z() = 0 exists, it is only applicable to the old 2D versions of the algorithm. This one is an important error, and any result that comes with it has a non negligible error
I was worried that the error that I saw using this tool was too high. Hopefully this one fixes it...

vividf and others added 2 commits October 28, 2024 19:30
Signed-off-by: vividf <[email protected]>
…tor_support_radars_and_transformation_algorithms
@vividf
Copy link
Contributor Author

vividf commented Oct 28, 2024

@knzo25
Before I fixed it in a62aae7 but seems that error is still high.

I just found out that I didn't fix the yaw error function so I just fixed it in 64d8e53.

The error is a bit lower than before but still high (I will send PM to you on slack)

@knzo25
Copy link
Collaborator

knzo25 commented Oct 28, 2024

Thanks, just now I am catching up on the radar calibration, to make list of the next steps

@knzo25
Copy link
Collaborator

knzo25 commented Nov 12, 2024

@vividf

  • pre-commit does not pass
  • This PR should be rebased if possible
  • XX1 Gen2 unrelated to the radar should be in another PR
  • Since we are going to (probably) end up restricting the calibration FOV with a cone, it should both put as a marker (guideline) and also not accept samples outside
  • (Calibrator data) Variance indicators will be vital, since using single value of elevation in the reflectors end up in a degenerate optimization problem (for example, in the camera-lidar, each tag has 4 points, so the issue does not arise)
  • We had talked before, but we need a button to delete a reflector by index. In the same way, it would be convenient to have a cancel a pair addition.
  • Camera-lidar, base-lidar, and lidar-lidar have ways to save the data used for calibration for offline analysis. Please add an option (potentially a service), to save the calibration points
  • The radar markers were implemented as a line with a square, representing the fact that we do not know the z component of RadarTrack (ARS408). The same is not true for 4D radars (with elevation). Please propose a new way to visualize it

<arg name="radar_frame"/>
<arg name="lidar_frame"/>
<arg name="msg_type"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

for msg_type and transformation_type, please provide alternatives with the choice attribute

<arg name="input_lidar_pointcloud" default="/sensing/lidar/front_lower/pointcloud_raw"/>
<arg name="input_radar_objects" default="/sensing/radar/front_center/objects_raw"/>
<arg name="input_radar_msg" default="/sensing/radar/front_center/objects_raw"/>
<arg name="msg_type" default="objects_raw"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

for msg_type and transformation_type, please provide alternatives with the choice attribute

Copy link
Collaborator

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

Checked calibration with X2Gen2 data.
Made general comments since the results are not near the required level yet.
Will be updating tickets later

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.

3 participants