Skip to content

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

Merged
Nir-Az merged 1 commit into
realsenseai:developmentfrom
Arun-Prasad-V:dynamic_hdr_subpreset
Dec 15, 2023
Merged

Fix: Should not set HDR when it is already enabled#12511
Nir-Az merged 1 commit into
realsenseai:developmentfrom
Arun-Prasad-V:dynamic_hdr_subpreset

Conversation

@Arun-Prasad-V

Copy link
Copy Markdown
Contributor

Tracked on LRS-962

From FW version 5.14.x.x, the FW doesn't support updating the HDR presets when HDR is already enabled.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@Arun-Prasad-V
Arun-Prasad-V force-pushed the dynamic_hdr_subpreset branch 2 times, most recently from f9886e3 to ad95819 Compare December 12, 2023 17:52
Comment thread src/hdr-config.cpp Outdated
@Arun-Prasad-V
Arun-Prasad-V requested a review from Nir-Az December 12, 2023 17:56
@Arun-Prasad-V
Arun-Prasad-V force-pushed the dynamic_hdr_subpreset branch 2 times, most recently from dc14073 to 30c3f90 Compare December 13, 2023 07:02
@Arun-Prasad-V Arun-Prasad-V reopened this Dec 13, 2023
@Arun-Prasad-V
Arun-Prasad-V force-pushed the dynamic_hdr_subpreset branch 3 times, most recently from 437ac2a to 7876aa9 Compare December 13, 2023 13:54
@Arun-Prasad-V
Arun-Prasad-V marked this pull request as ready for review December 13, 2023 13:54
@Arun-Prasad-V Arun-Prasad-V reopened this Dec 14, 2023
@Arun-Prasad-V Arun-Prasad-V reopened this Dec 14, 2023

@Nir-Az Nir-Az left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Nir-Az
Nir-Az merged commit 861db20 into realsenseai:development Dec 15, 2023
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.

2 participants