-
Notifications
You must be signed in to change notification settings - Fork 5k
UCAL enhancements and refactoring #9336
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
Conversation
3c7738c to
9c5d9d5
Compare
2. Taget based focal length calibration. 3. UVMapping calibration.
…perators, floating point conversion
…Minor inconsistencies
…fication adjustments
Change-Id: I441f7208edd6de25f433b53fc3f216d3fe8fd2b0
Change-Id: Iaa92be84b7b77e9dbac126535dd2e320f534a7c0
Change-Id: I35043b064960fb8509085898329f6c9e8d15d2be
Change-Id: I6d75b823d3a58b49c832cc25601e130453853337
…gly. Verify that the cali table is valid before calling FW flash API.Remove comments Change-Id: Iefb5b37a871af9ce4cdd8185b7c8d36e9d814879
Change-Id: I1750b5e1f2e23fed537be228147610b7de2af33e
Change-Id: Ibb4f45c86a88d693214dc9d46b08da8edc9efb62
Change-Id: I5d5a0bdca32e1be13452e81c6d3a7c7aa692fb9f
Change-Id: I9ba2ae9fbe1a6f268cb79d8651e39049b5f25dc0
Change-Id: Ic0ddf6b1a0935367ab0237e971820d089c982cbd
Change-Id: I7cf8a2bc2335f9f543c17a217be59365b7a78c98
Change-Id: Idc41413272ae5236976fc65b68302f702613823f
remibettan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor changes - Impressive work!
| namespace helpers | ||
| { | ||
| // Calculate CRC code for arbitrary characters buffer | ||
| uint32_t calc_crc32(const uint8_t* buf, size_t bufsize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the implementation of this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calibration-model.cpp
common/on-chip-calib.cpp
Outdated
| : process_manager("On-Chip Calibration"), _model(model), _dev(dev), _sub(sub), _viewer(viewer), _sub_color(sub_color), py_px_only(!uvmapping_calib_full) | ||
| { | ||
| auto dev_name = dev.get_info(RS2_CAMERA_INFO_NAME); | ||
| if (!strcmp(dev_name, "Intel RealSense D415")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make it characteristics oriented instead of specific camera name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec is SKU-specific. I will change the code for robustness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| { | ||
| if (_sub) | ||
| { | ||
| _sub->show_algo_roi = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like same code instead of the pointer, both for on, and off - please consider to refactor with some helper function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but lower priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| { | ||
| // Stop viewer UI | ||
| _sub->stop(_viewer.not_model); | ||
| if (_sub_color.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why there is a need to check _sub_color pointer and not _sub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_sub stands for depth, and _sub_color - RGB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| { | ||
| auto profiles = _sub->get_selected_profiles(); | ||
| _sub->stop(_viewer.not_model); | ||
| if (_sub_color.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| for (int i = 0; i < _sub_color->res_values.size(); i++) | ||
| { | ||
| auto kvp = _sub_color->res_values[i]; | ||
| if (kvp.first == 1280 && kvp.second == 720) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| int counter = 0; | ||
| int frm_idx = 0; | ||
| int limit = 50; // input frames required to calculate the target | ||
| float step = 50.f / limit; // frames gathering is 50% of the process, the rest is the internal data extraction and algo processing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used by GUI marker, don't think that it is necessary
| ImGui::SetCursorScreenPos({ float(x + 135), tmp_y }); | ||
| if (ImGui::RadioButton("OCC Extended", (int *)&(get_manager().action), 0)) | ||
| get_manager().action = on_chip_calib_manager::RS2_CALIB_ACTION_ON_CHIP_OB_CALIB; | ||
| // Deprecase OCC-Extended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments - consider to remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to keep it till the version is stabilized.
I'm not sure that this decision will hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| auto table = (librealsense::ds::coefficients_table*)calib_table.data(); | ||
|
|
||
| float ar[2] = { 0 }; | ||
| float tmp = left_rect_sides[2] + left_rect_sides[3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider to use helper function in order to remove repetition of code for left and right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithmic part implementation is according to a reference design. I'd rather not refactor it to keep the Matlab/C++ parity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| if (ar[0] > 0.0f) | ||
| align = ar[1] / ar[0] - 1.0f; | ||
|
|
||
| float ta[2] = { 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please comment these "ta" and "gt" so that their meaning would be clear (or change the names)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I knew it :) - it is the algo domain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-D
|
Approved |
Change-Id: Ib28fc35ebbd9fc783d25b8b992fedc5879e614d9
Change-Id: Ia86a5a76f2b0a11ae56deba3d0b426a909e84191
Fix python compilation for new calibration values. add comments Fix some warnings move calculation of sum of pixels in depth image to function: get_depth_frame_sum fix merge. retry on set_pu SEMAPHORE_TIMEOUT error CR updates add ticket number to comment changed inheritence to using added comment fix callback ptr from PR realsenseai#9336
Tracked on LRS-62