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

Add Reflectivity option restrictions #7706

Conversation

Nir-Az
Copy link
Collaborator

@Nir-Az Nir-Az commented Nov 3, 2020

Implement IR Reflectivity option restrictions according to [RS5-8358]

Note:

  • When changing resolution (sensor mode) during streaming and Enable IR Reflectivity on , IR reflectivity will be disabled (With no error pop up window)
  • ROI restriction is not addressed on this PR

@Nir-Az Nir-Az requested a review from maloel November 3, 2020 18:37
@@ -226,6 +226,8 @@ namespace librealsense

void l500_options::on_set_option(rs2_option opt, float value)
{
verify_max_usable_range_restrictions(value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

opt may not be VISUAL_PRESET... but inside the verify fn you compare the value to MAX_RANGE...? Am I misunderstanding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct, the logic was not good.

Changed.


if (ds.supports_option(RS2_OPTION_ENABLE_IR_REFLECTIVITY) && ds.get_option(RS2_OPTION_ENABLE_IR_REFLECTIVITY).query() == 1.0f)
{
ds.get_option(RS2_OPTION_ENABLE_IR_REFLECTIVITY).set(0.0f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do this only after the other test passes

Copy link
Collaborator Author

@Nir-Az Nir-Az Nov 5, 2020

Choose a reason for hiding this comment

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

We leave it as is as we discussed, we changed the throw to set MUR off

(ds.get_option(RS2_OPTION_ENABLE_MAX_USABLE_RANGE).query() == 1.0) &&
(value != rs2_sensor_mode::RS2_SENSOR_MODE_VGA))
{
throw wrong_api_call_sequence_exception("Max Usable Range support VGA resolution only");
Copy link
Collaborator

Choose a reason for hiding this comment

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

supports

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should turn it off, not throw

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

{
int sensor_mode = static_cast<int>(s->get_option(RS2_OPTION_SENSOR_MODE));
int width = 0, height = 0;
width_height_from_resolution(static_cast<rs2_sensor_mode>(sensor_mode), width, height);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add function resolution_id_from_sensor_mode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Nir-Az Nir-Az requested a review from maloel November 5, 2020 09:30
@@ -76,6 +76,27 @@ static void width_height_from_resolution(rs2_sensor_mode mode, int &width, int &
}
}

static int get_resolution_id_from_sensor_mode(int sensor_mode, const rs2::sensor &s, const std::vector<std::pair<int, int>> &res_values)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not pass in rs2_sensor_mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

{
int width = 0, height = 0;
width_height_from_resolution(static_cast<rs2_sensor_mode>(sensor_mode), width, height);
auto iter = std::find_if(res_values.begin(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

clang...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

if( ds.supports_option( RS2_OPTION_ENABLE_IR_REFLECTIVITY )
&& ds.get_option( RS2_OPTION_ENABLE_IR_REFLECTIVITY ).query() == 1.0f )
throw librealsense::wrong_api_call_sequence_exception(
"Enabling Alternate IR while IR Reflectivity is enabled is prohibited" );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternate IR cannot be enabled with IR Reflectivity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@maloel maloel merged commit 68bf008 into IntelRealSense:development Nov 5, 2020
@Nir-Az Nir-Az deleted the restriction_for_reflectivity_from_dev branch December 23, 2020 07:24
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.

None yet

2 participants