Tm2 calibration write api #4650
Conversation
There was a problem hiding this comment.
Very nicely implemented @honpong
One quick point - communicating success / failure via bool return code goes somewhat against librealsense convention. I would recommend throwing from tm2_sensor level and changing all return types to void. The exception would propagate through the hourglass layer and will be rethrown in C++.
honpong
force-pushed
the
tm2-calibration-write-api
branch
from
6e20010
to
c1e0a7d
Compare
|
@dorodnic I changed all return to void after force-push |
|
@cchen6, @harville, @schmidtp1, I checked-in a C++ example to test write/read/reset the OEM calibration. It needs custom FW from @cchen6, please get it from our Slack channel. It works for me in Windows. @schmidtp1, @bfulkers-i I haven't yet tested in python. If you have time, please go ahead without me, while I will try on my end soon. |
include/librealsense2/h/rs_device.h
Outdated
| * \param[out] error If non-null, receives any error that occurs during this call, otherwise, errors are ignored | ||
| * \return 0 if the operation is completed successfully. | ||
| */ | ||
| void rs2_set_tm2_intrinsics(const rs2_device* device, int sensor_id, const rs2_intrinsics* intr, rs2_error** error); |
There was a problem hiding this comment.
The functions shouldn't include "tm2" as there is nothing tm2-specific in the signature.
Additionally, these functions shall later be reused for other devices, so the APIs shall remain generic
There was a problem hiding this comment.
@ev-mp, I can't be sure these set functions signature are good for non-tm2. I'll wait for your internal discussion.
There was a problem hiding this comment.
Now all removed and move to use rs2_sensor* and rs2_stream_profile*, more aligned with the existing get interfaces
include/librealsense2/h/rs_device.h
Outdated
| void rs2_set_tm2_intrinsics(const rs2_device* device, int sensor_id, const rs2_intrinsics* intr, rs2_error** error); | ||
|
|
||
| /** | ||
| * Set tm2 camera extrinsics |
There was a problem hiding this comment.
All instances of "tm2" to be omitted int the public API, see above
include/librealsense2/h/rs_device.h
Outdated
| * \param[out] error If non-null, receives any error that occurs during this call, otherwise, errors are ignored | ||
| * \return 0 if the operation is completed successfully. | ||
| */ | ||
| void rs2_reset_tm2_to_factory_calibration(const rs2_device* device, rs2_error** e); |
There was a problem hiding this comment.
PLs consider renaming to rs2_reset_device_to factory_calibration or similar
include/librealsense2/h/rs_device.h
Outdated
| * \param[out] error If non-null, receives any error that occurs during this call, otherwise, errors are ignored | ||
| * \return 0 if the operation is completed successfully. | ||
| */ | ||
| void rs2_set_tm2_extrinsics(const rs2_device* device, rs2_stream stream_type, int stream_index, const rs2_extrinsics* extr, rs2_error** error); |
There was a problem hiding this comment.
What are the constrains of the API - can it be applied at any time or during idle state only?
Is it a persistent change or will it reset to internally-stored values after device power cycle?
There was a problem hiding this comment.
@ev-mp as far as I know, those set functions will modify the virtual table of the device. So it won't affect run time result until the device being reset.
Even said that, we don't recommend the change those parameters during run-time.
@cchen6 @bfulkers-i, any comment?
There was a problem hiding this comment.
They will take effect once you start device/play which will load calibration. It doesn't make sense to call these functions during play.
|
@ev-mp @dorodnic let me know if you still want me remove the |
honpong
force-pushed
the
tm2-calibration-write-api
branch
from
e218e00
to
bec5666
Compare
honpong
force-pushed
the
tm2-calibration-write-api
branch
from
1294eb4
to
7f2930c
Compare
…n write API defined motion.h
honpong
force-pushed
the
tm2-calibration-write-api
branch
from
7f2930c
to
15f6d20
Compare
| * \param[in] from_stream only support RS2_STREAM_FISHEYE | ||
| * \param[in] from_id only support left fisheye = 1 | ||
| * \param[in] to_stream only support RS2_STREAM_FISHEYE | ||
| * \param[in] to_id only support left fisheye = 2 |
There was a problem hiding this comment.
we only support set_extrinsics(fe1, fe2, extr) in the first merge, aren't we?
There was a problem hiding this comment.
@honpong, I think @schmidtp1 is saying that the documentation should say right not left
There was a problem hiding this comment.
@bfulkers-i thanks for the catch. I will fix it in next PR.
src/tm2/tm-device.cpp
Outdated
| rs2_extrinsics dst; | ||
| auto dst_rotation = dst.rotation; | ||
| for (auto i : { 0,3,6,1,4,7,2,5,8 }) { *dst_rotation++ = dst.rotation[i]; } | ||
| for (auto i : { 0,1,2 }) { dst.translation[i] = -src.translation[i]; } |
There was a problem hiding this comment.
this is missing rotation (by R^T)
| auto H_fe1_pose = get_extrinsics(from_profile, reference_sensor_id); | ||
| //H_fe2_pose = H_fe2_fe1 * H_fe1_pose | ||
| auto H_fe2_pose = mult(inv(H_fe1_fe2), H_fe1_pose); | ||
| set_extrinsics_to_ref(RS2_STREAM_FISHEYE, 2, H_fe2_pose); |
There was a problem hiding this comment.
i think this could be more generic:
you could get the transformation of any sensor w.r.t. the pose (frame).
There was a problem hiding this comment.
let's focus on getting the basic set_extrinsic(fe1,fe2,extr) version merged first, we can improve in future PR.
Adding 5 libRS API in
rs_device.hppfor writing T265 calibration:tm2::set_intrinsics(...)
tm2::set_extrinsics(...)
tm2::set_motion_device_intrinsics(...)
tm2::reset_to_factory_calibration(...)
tm2::write_calibration(...)
And a C++ example
rs-tm2-calibration-writecan be found in another branch:https://github.com/honpong/librealsense/tree/tm2-calibration-write-example