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

Cam IMU with RS calibration #72

Open
v0n0 opened this issue Aug 27, 2016 · 22 comments
Open

Cam IMU with RS calibration #72

v0n0 opened this issue Aug 27, 2016 · 22 comments
Labels
enhancement New feature or request question Theory or implementation question

Comments

@v0n0
Copy link

v0n0 commented Aug 27, 2016

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
Copy link
Author

v0n0 commented Aug 27, 2016

@rehderj
Copy link
Contributor

rehderj commented Aug 29, 2016

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
Copy link
Author

v0n0 commented Aug 29, 2016

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
Copy link
Contributor

rehderj commented Aug 29, 2016

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
Copy link
Author

v0n0 commented Aug 29, 2016

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
Copy link
Contributor

rehderj commented Aug 29, 2016

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
Copy link
Contributor

rehderj commented Aug 29, 2016

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
Copy link
Author

v0n0 commented Aug 29, 2016

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
Copy link
Contributor

rehderj commented Aug 31, 2016

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
Copy link
Author

v0n0 commented Sep 2, 2016

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
Copy link

mhkabir commented Feb 24, 2017

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

@JzHuai0108
Copy link

JzHuai0108 commented Aug 17, 2021

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
Copy link

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
Copy link

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
Copy link

@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
Copy link

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
Copy link

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
Copy link

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

@Enes1097
Copy link

Enes1097 commented Sep 3, 2021

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

@JzHuai0108
Copy link

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
Copy link

JzHuai0108 commented Sep 20, 2021

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
Copy link

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 ✌️

@goldbattle goldbattle added enhancement New feature or request question Theory or implementation question labels Apr 22, 2022
@goldbattle goldbattle pinned this issue Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Theory or implementation question
Projects
None yet
Development

No branches or pull requests

6 participants