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.

Sign up for GitHub

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

Fix: Should not set HDR when it is already enabled #12511

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Fix: Should not set HDR when it is already enabled #12511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/ds/d400/d400-device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,6 @@ namespace librealsense

synthetic_sensor::open(requests);

// needed in order to restore the HDR sub-preset when streaming is turned off and on
if (_hdr_cfg && _hdr_cfg->is_enabled())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This piece of code is invoked every time when the streams are opened. Since, FW will throw an error if we try to set it when it is already enabled, removing this code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure that is what we want, if the user explicitly turned on the hdr , if he closed the stream he expect it to stay on, even if we will block him from changing the exp/gain right?
This will even cause a confusion in the UI IMO, the checkbox will be marked but it will be off in FW.

according to this code Alex answer was wrong and streaming stop turn off HDR preset, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quoting Alex's response:
"SubPreset mechanism stays active in FW (even during start and stop streaming commands) till SETSUBPRESET 0 call."

So, my understanding from Alex's response is:

  • Stream CLOSE doesn't affect HDR.
  • So, when the stream is reopened, no need to enable HDR again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this was not the case when this was implemented originally.
Can we verify this behavior and verify he is right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified the following scenario from RS Viewer:

  1. HW Reset
  2. Set Exp.1 = 32000, Exp.2 = 1
  3. Enable HDR
  4. Open Depth stream
  5. From frame Metadata, verified that the Actual Exposure values to be 32000 & 1
  6. Close and Reopen Depth stream
  7. From frame Metadata, verified that the Actual Exposure values are still 32000 & 1

Do you want me to verify any other scenarios?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks , that was the required scenario to test

get_option(RS2_OPTION_HDR_ENABLED).set(1.f);

// Activate Thermal Compensation tracking
if (supports_option(RS2_OPTION_THERMAL_COMPENSATION))
_owner->_thermal_monitor->update(true);
Expand Down
4 changes: 0 additions & 4 deletions src/ds/d500/d500-device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,6 @@ namespace librealsense
set_frame_metadata_modifier([&](frame_additional_data& data) {data.depth_units = _depth_units.load(); });

synthetic_sensor::open(requests);

// needed in order to restore the HDR sub-preset when streaming is turned off and on
if (_hdr_cfg && _hdr_cfg->is_enabled())
get_option(RS2_OPTION_HDR_ENABLED).set(1.f);
}); //group_multiple_fw_calls
}

Expand Down
5 changes: 3 additions & 2 deletions src/ds/ds-options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,8 +629,9 @@ namespace librealsense
else
{
if (_hdr_cfg->is_enabled())
LOG_WARNING("The control - " << _uvc_option->get_description()
<< " - is locked while HDR mode is active.\n");
throw wrong_api_call_sequence_exception(
rsutils::string::from() << "The control - " << _uvc_option->get_description()
<< " - is locked while HDR mode is active.");
else
_uvc_option->set(value);
}
Expand Down
34 changes: 21 additions & 13 deletions src/hdr-config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ namespace librealsense
_sensor(depth_ep),
_is_enabled(false),
_is_config_in_process(false),
_has_config_changed(false),
_current_hdr_sequence_index(DEFAULT_CURRENT_HDR_SEQUENCE_INDEX),
_auto_exposure_to_be_restored(false),
_emitter_on_off_to_be_restored(false),
Expand Down Expand Up @@ -215,12 +214,6 @@ namespace librealsense
default:
throw invalid_value_exception("option is not an HDR option");
}

// subpreset configuration change is immediately sent to firmware if HDR is already running
if (_is_enabled && _has_config_changed)
{
send_sub_preset_to_fw();
}
}

bool hdr_config::is_config_in_process() const
Expand Down Expand Up @@ -264,7 +257,7 @@ namespace librealsense
if (!_is_enabled)
{
// saving status of options that are not compatible with hdr,
// so that they could be reenabled after hdr disable
// so that they could be reenabled after hdr disable
set_options_to_be_restored_after_disable();

if (_use_workaround)
Expand All @@ -281,7 +274,14 @@ namespace librealsense
}

_is_enabled = send_sub_preset_to_fw();
_has_config_changed = false;
if (!_is_enabled)
{
LOG_WARNING("Couldn't enable HDR." );
}
}
else
{
LOG_WARNING("HDR is already enabled. Skipping the request." );
}
}
else
Expand Down Expand Up @@ -504,14 +504,22 @@ namespace librealsense

void hdr_config::set_exposure(float value)
{
_hdr_sequence_params[_current_hdr_sequence_index]._exposure = value;
_has_config_changed = true;
if (!_is_enabled)
_hdr_sequence_params[_current_hdr_sequence_index]._exposure = value;
else
throw wrong_api_call_sequence_exception(rsutils::string::from()
<< "Cannot update HDR config (exposure) while HDR mode is active." );
}

void hdr_config::set_gain(float value)
{
_hdr_sequence_params[_current_hdr_sequence_index]._gain = value;
_has_config_changed = true;
if (!_is_enabled)
{
_hdr_sequence_params[_current_hdr_sequence_index]._gain = value;
}
else
throw wrong_api_call_sequence_exception(rsutils::string::from()
<< "Cannot update HDR config (gain) while HDR mode is active." );
}


Expand Down
1 change: 0 additions & 1 deletion src/hdr-config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ namespace librealsense
int _current_hdr_sequence_index;
mutable bool _is_enabled;
bool _is_config_in_process;
bool _has_config_changed;
bool _auto_exposure_to_be_restored;
bool _emitter_on_off_to_be_restored;
hw_monitor& _hwm;
Expand Down
Loading