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

Handle dynamic number of intrinsic tables for USB2 support #7094

Conversation

Nir-Az
Copy link
Collaborator

@Nir-Az Nir-Az commented Aug 11, 2020

On L515 device we get 2 depth resolutions on USB3 and 1 on USB2.
This PR replace sharing the FW raw data vector with a physical preallocated full size buffer (Support up to 5 resolutions)

Same change apply for depth and color intrinsic tables handling.

src/l500/l500-color.cpp Show resolved Hide resolved
* sizeof( pinhole_camera_model ) );

// Validate table size
if (expected_size > response_vec.size())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also check for expected < actual, or just use !=
I think that since the table size is const it can all be replaced with check_calib<> above

Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment above: we can only use the bytes covered by the number of resolutions, but we're not guaranteed that there will be no extra information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The table has the reserved space for up to 5 tables, but only some of them will be active.
Yet the raw table size is predefined by FW spec, thus non-modifiable, is it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like mentioned above, FW send extra bytes for debugging purpose. and it can change on each FW version.

  • we get different size of output for USB2/USB3

src/l500/l500-color.cpp Show resolved Hide resolved
src/l500/l500-depth.cpp Show resolved Hide resolved
src/l500/l500-private.h Show resolved Hide resolved
src/l500/l500-color.cpp Outdated Show resolved Hide resolved
src/l500/l500-color.cpp Outdated Show resolved Hide resolved

auto resolutions_rgb_table_ptr = reinterpret_cast<const ivcam2::intrinsic_rgb *>(response_vec.data());

// Get full maximum size of the resolution array and deduct the unused resolutions size from it
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest you comment what the "agreement" is between FW and us: that they guarantee only as many bytes as required for the given number of resolutions, and that they may send more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

src/l500/l500-color.cpp Outdated Show resolved Hide resolved
src/l500/l500-depth.cpp Outdated Show resolved Hide resolved
// Set a new memory allocated intrinsics struct (Full size 5 resolutions)
// Copy the relevant data from the dynamic resolution received from the FW
ivcam2::intrinsic_depth resolutions_depth_table_output;
resolutions_depth_table_output.orient = resolutions_depth_table_ptr->orient;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as for color

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

src/l500/l500-depth.cpp Outdated Show resolved Hide resolved
@@ -221,7 +222,7 @@ namespace librealsense
ivcam2::intrinsic_depth get_intrinsic() const override
{
using namespace ivcam2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need this using statement

Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

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

The calib. table protocol must be established and replace the ad-hoc solution

@maloel maloel merged commit db450ab into IntelRealSense:development Aug 13, 2020
@Nir-Az Nir-Az deleted the Dev-fix_get_intrinsics_access_violation branch September 1, 2020 07:48
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