Cam + IMU with RS calibration

[v0n0]

As discussed in #64 I am integrating RS code into cam + IMU calibration.

One of the first things I noted is a possible error in the code. At https://github.com/othlu/kalibr/blob/master/aslam_offline_calibration/kalibr/python/kalibr_rs_camera_calibration/RsCalibrator.py#L316 we see there is a test for successful observation, which only checks if there is at least one successful corner observation (see GridCalibrationTargetObservation::hasSuccessfulObservation).

But above at https://github.com/othlu/kalibr/blob/master/aslam_offline_calibration/kalibr/python/kalibr_rs_camera_calibration/RsCalibrator.py#L276 it was stated that the order of observations has to be preserved. As an observation is only a vector of corners (see GridCalibrationTargetObservation::getCornersImageFrame), if there is any missing corner it means the indexes of the following corners will be changed, in the worst case (first corner missing), all of them will be changed and all the errors for the frame will be setup wrong in the jacobian.

The confirmation of this can be seen at https://github.com/ethz-asl/kalibr/blob/master/aslam_cv/aslam_cameras/src/GridCalibrationTargetObservation.cpp#L75-L78

I think the fix is either:

• the test should discard the frame if any of the observations are missing;
• the observations should be passed in a hash so that their order is not impacted by missing detections at a certain frame.

That would make the dataset creation and camera calibration much more robust.

Do you think it makes sense?

[v0n0]

Another thing that I think is wrong (although not breaking anything) are the comments at https://github.com/ethz-asl/kalibr/blob/master/aslam_cv/aslam_cameras/include/aslam/cameras/GridCalibrationTargetObservation.hpp#L47 and https://github.com/ethz-asl/kalibr/blob/master/aslam_cv/aslam_cameras/include/aslam/cameras/GridCalibrationTargetObservation.hpp#L50

(same in the respective implementation file)

[rehderj]

Without diving deeper into the code, I would like to offer my view on that issue (though it might be incorrect, since I only briefly skimmed through the code): There are already checks in place that will invalidate the detection of a target where missed identifiers cause incorrect associations (please see here and here).
Accordingly, these lines should not be an issue, but they would be of value to determine that targets like April tags returned at least a single valid observation, right?

[v0n0]

Yes, I probably should have mentioned I am using April tags. With them a (valid) detection can be a partial detection, so a full detection is important to maintain the order of the corners in the array. Is that what you were asking?

[rehderj]

Ok, maybe I am missing the point here, but the April tag detector uses unique visual identifiers to determine the association between observations and world points. Please see this section: It appears that the data structure holds world points in the same order as the observations and with correct associations, or am I missing something here?

[v0n0]

Yes, the corners are stored properly in the internal data structure, but when you want to export them, they are lumped together into a plain vector. Unless I misunderstood the code.

[rehderj]

True, but where does this become an issue? The corresponding world points are lumped together in the exact same way, which should preserve correct associations, right?

[rehderj]

Ok, you are right, in the RS code, the method getCornersTargetFrame() is not called for each observation, but once in the beginning. I agree that this can be an issue even for the April tag target.
Would you be interested in fixing it? These lines add the corners as design variables to the problem. This should only be necessary in cases where the target geometry should be refined or for implementation reasons, when certain interfaces are not defined or exposed to python. Neither seems to be the case.

In other sections of the code, the target corners are queried correctly: https://github.com/ethz-asl/kalibr/blob/master/aslam_offline_calibration/kalibr/python/kalibr_imu_camera_calibration/IccSensors.py#L357-L359.
Since in most cases it will not make sense to refine the target geometry, could you add something analogously to here maybe around these lines https://github.com/othlu/kalibr/blob/master/aslam_offline_calibration/kalibr/python/kalibr_rs_camera_calibration/RsCalibrator.py#L322?

[v0n0]

Would you mind expanding/linking to the concept of design variables? I'm not sure I understand it fully. Would the change keep these design variables?

With this new change, how would you use the targetCornerPoints, after querying for them? Just want to make sure I fully understand your intent.

[rehderj]

In this context, design variables are entities that are potentially being adjusted during optimization to bring down the value of the objective function. As an example, camera intrinsics and poses are design variables as well, while in the camera/IMU calibration, target corners are not. They could be, if we believed that for example printing wasn't perfect or the target was warped significantly and we had hopes that our observations were sufficient to improve our understanding of their true positions.
I personally believe that in most cases we do not benefit from estimating their positions and accordingly, they do not need to be design variables.
The change necessary are just a few lines of code copied from the camera/IMU use-case. I can take care of that as soon as I find some time. Otherwise is all boils down to these lines:
https://github.com/ethz-asl/kalibr/blob/master/aslam_offline_calibration/kalibr/python/kalibr_imu_camera_calibration/IccSensors.py#L380
vs. https://github.com/othlu/kalibr/blob/master/aslam_offline_calibration/kalibr/python/kalibr_rs_camera_calibration/RsCalibrator.py#L338.
In the first case, the projection is an expression involving a constant, in the second case a design variable. However, it should be possible to just change it to being a constant.

[v0n0]

Ah OK, they are the equivalent of a variable passed to the Ceres function that automatically calculates the Jacobian. I'm looking into making the change.

[mhkabir]

Hi @v0n0,
Did you make any progress in rolling shutter - IMU calibration?

[JzHuai0108]

Two and half years later, I think we have done rolling shutter camera - IMU calibration on top of Kalibr with a report. We will make a pull request soon.

This feature is also relevant to my antique issue, issue #101, and issue #111.

[Enes1097]

Hey @JzHuai0108, I just tried out your package in ROS1 Kinetic with Ubuntu 16.04 and it worked very well.
The normal ROS package didn't work with my RS Camera, so I'm really glad that you guys provided your package :)

I have a question tho:
Does your package also work in ROS1 Noetic, because I have been using a VM with Ubuntu 16.04, but for my project I need to use ROS1 Noetic.

Thanks!

[JzHuai0108]

Unfortunately, I have not tested kalibr with ROS1 noetic. That probably will require an overhaul from opencv 2 to opencv 3. It is feasible but takes some time.
I see you are trying to make kalibr work with ROS1 noetic at issue.
I will try to find some time to triage and even carry out the upgrade.

[Enes1097]

@JzHuai0108 yes, that''s what I'm trying to do. I don't know how to implement that overhaul :/

I would be really glad, if you could carry out that upgrade :) Thank you so much!

[JzHuai0108]

Hi @Enes1097,

I will do that eventually. But I am busy making a living at the moment, so be on your guard that it may take a while for me to do so. Let's me know your timeline, i.e., what time you expect the upgrade.

Best wishes,
Josh Huai

[Enes1097]

Hey,

That's unfortunate, because I would need it till the end of the september which is very soon and I think that's a bit too early for you.

Still, thank you so much and Best Wishes!

[JzHuai0108]

I will get some free time after Sep 10 and look into the upgrade by then.

[Enes1097]

Oh, thank you so much!!! I'm looking forward to it :)

[JzHuai0108]

Actually I found that the ori-drs fork worked almost perfectly on ubuntu 20 with ros1 noetic.
See my comment on the issue #396.
The left job now is to merge ori-drs and my branch. I think this should be easy because the two features are almost perpendicular.

[JzHuai0108]

Hi @Enes1097, I have merged ori-drs noetic fork into my branch which now supports rolling shutter camera-IMU calibration and noise identification while working in ros1 noetic.

I have retouched several places in ori-drs. Notably, a through search and revision of "print ", making the python scripts say kalibr_calibrate_imu_camera visible even without "rosrun kalibr " prefix, and a few attempts to fix the warnings in compilation.

I will test the noetic codebase in ros1 melodic, to see if the codebase is indeed compatible to melodic as suggested by @wxmerkt in #396.

[Enes1097]

Actually I found that the ori-drs fork worked almost perfectly on ubuntu 20 with ros1 noetic.
See my comment on the issue #396.
The left job now is to merge ori-drs and my branch. I think this should be easy because the two features are almost perpendicular.

Oh okay. Weirdly enough, it didn't work for me.
Sadly couldn't fix it either.

Hi @Enes1097, I have merged ori-drs noetic fork into my branch which now supports rolling shutter camera-IMU calibration and noise identification while working in ros1 noetic.

I have retouched several places in ori-drs. Notably, a through search and revision of "print ", making the python scripts say kalibr_calibrate_imu_camera visible even without "rosrun kalibr " prefix, and a few attempts to fix the warnings in compilation.

I will test the noetic codebase in ros1 melodic, to see if the codebase is indeed compatible to melodic as suggested by @wxmerkt in #396.

Hey,
It's nice hearing back from you :)
I'm going to try out your package in the next few days then. Hopefully I can get back to you with some feedback

Thank you again for your efforts ✌️