[ymodlin]

Make test_hdr_start_stop_recover_manual_exposure_and_gain measure the FW, not the SDK

Host-side counterpart to FW PR # 222 (FW lost the pre-HDR manual exposure/gain restore on sub-preset exit in 5.14).

As @Nir-Az pointed out, the test cannot prove that FW fix today: hdr_config::_use_workaround is hardcoded true, so on HDR enable the SDK overwrites the manual exposure with PRE_ENABLE_HDR_EXPOSURE = 30000 and on disable re-writes the saved value itself. The test measures that re-write.

1. Gate the workaround on FW version— kept, not deleted

hdr_config ctor takes bool use_workaround, threaded through d400_depth_sensor::init_hdr_config(). In d400-device.cpp, next to the existing hdr_firmware_version:

firmware_version hdr_exposure_restore_firmware_version("5.17.4.13");
...
bool use_exposure_restore_workaround = _fw_version < hdr_exposure_restore_firmware_version;

A single LOG_INFO states which side performs the restore, so a CI log shows the mode actually in effect (it is per-device — a rig with mixed FW shows both).

Only the exposure save/re-write is gated. The set_sequence_index(0.f) option-routing reset is not — see §3.

The constant is 5.17.4.13, resolved against the FW repo rather than predicted.

tag	commit	contains the fix?
v5.17.4.11	c4086b04	no
v5.17.4.12	b9e9f94e	no
v5.17.4.13	3654c669	yes

The version bot bumps uvc_version.h ahead of the work rather than once per merge, so .11 and .12 were both tagged before the fix landed. 5.17.4.13 is the first released FW that carries it. (An earlier revision of this PR had 5.17.4.11, which would have gated the workaround off on real .11/.12 cameras that have no FW fix.)

2. Also assert the queried exposure/gain (adaabe42)

Gating alone is not sufficient — verified on hardware. With the workaround off on broken FW the test still passes, because broken FW does restore what reaches the sensor:

--- HDR OFF at iteration 50 ---
iter 55  META exp=5000  gain=50  seq=0 | GET_CUR exp=7500.0   <- metadata restored, query stuck
iter 69  META exp=5000  gain=50  seq=0 | GET_CUR exp=7500.0

Only the queried (UVC control-DB) value stays at the last HDR sub-preset exposure — which is exactly the field the FW fix writes back. Since the test asserted only on frame metadata, it could not fail on this bug regardless of the workaround. (The original 2022 30000 == 5000 signature was the SDK's own sentinel in frames in flight — a workaround artifact.)

So at the existing iteration_to_check_after_disable point the test now also checks get_option(exposure) / get_option(gain). The 5-frame check window is unchanged.

3. Review round 2 (9b6fc755d)

• set_sequence_index(0.f) hoisted out of both if (_use_workaround) blocks. It is the option-routing reset, not part of the exposure workaround: disable() only sends the empty sub-preset and resets nothing, so without it an app that configured HDR through sequence_id != 0 keeps querying and setting the cached slot value after HDR is off. The routing flow is now identical for every FW version — today's behavior, unconditionally. On the disable path it had also been nested inside the _pre_hdr_exposure in-range check, so it was already being skipped when the saved exposure fell out of range; that is fixed too.
• Gate constant corrected to 5.17.4.13 (§1).
• The queried check can no longer silently not run. It now fires on the first iteration at-or-past the threshold that reaches the block, and assert queried_values_checked after the try/finally fails the test if no post-disable frame ever carried sequence_id metadata. Switched to check.equal so failures print both values.
• use_workaround default dropped from the hdr_config ctor — one construction site, and it already passes the flag explicitly.
• Comment at the gate widened so it no longer implies the condition starts at 5.14.

Hardware validation — rs-nuc01, D455 013322250138

development @ 4d3ecc025 + commits c4586a5a/adaabe42. Every leg pinned with --device <serial> (--device D455 is not deterministic on a 2×D455 rig — it silently picked the un-flashed camera once).

FW on camera	Workaround	Assertion	Result	Shows
5.17.4.10 (no fix)	ON	metadata only	PASS	the masking described above
5.17.4.10 (no fix)	OFF	metadata only	PASS	gating alone does not expose the bug
5.17.4.10 (no fix)	ON (via gate)	+ queried	PASS	older FW stays green — no CI regression
5.17.4.10 (no fix)	OFF	+ queried	FAIL	test now measures the FW restore path
fixed FW (#222)	OFF	+ queried	PASS	the FW fix restores it by itself
fixed FW (#222)	OFF	full pytest-hdr-long.py	12/12 PASS	gating the workaround off breaks no other HDR test

The "fixed FW" image is FW 185fdc5b — the content that shipped as 5.17.4.13 — hand-stamped in uvc_version.h at the time to what was then assumed to be the post-merge version. The FW behavior those rows prove is unaffected by the constant correction; the workaround was forced off explicitly, not via the gate.

Build note: rosbag2 was off in this validation build (the rig has no route to github.com for the FastCDR fetch) — orthogonal to HDR.

Hardware validation of round 2 — rs-nuc01

Two D455s that sit on either side of the gate. Suites run as
python3 -m pytest -v --context nightly --device <serial> live/d400/pytest-hdr-sanity.py live/d400/pytest-hdr-long.py live/hdr/
(covers hdr-sanity, hdr-long, hdr-configurations, hdr-performance, hdr-preset).

Camera	FW	Gate resolves to	Suites
D455 013322250138	5.17.4.13 (genuine release)	workaround OFF	32/32 passed
D455 213622301050	5.17.3.25	workaround ON	32/32 passed

The first row exercises the corrected constant end-to-end rather than a forced flag.

The suites do not prove the routing fix — as @Nir-Az predicted, 32/32 also passes pre-fix. Direct
evidence comes from driving his flow (sequence_id = 1/2, configure slots, HDR on, HDR off) and
reading back the state after disable:

librealsense build	FW 5.17.4.13 (gate off)	FW 5.17.3.25 (gate on)
a3d75e88d (pre-fix)	FAIL — sequence_id=2, exposure=1200, gain=20	PASS
9b6fc755d (fixed)	PASS — sequence_id=0, exposure=5000, gain=50	PASS

Pre-fix on fixed FW the query returns HDR slot 2's values instead of the pre-HDR 5000/50, and only on
the FW-version side where the workaround gates off — the divergence the gate was not meant to
introduce. With the hoist both FW versions behave identically.

Not confirmed by measurement: that set_option(exposure) never reaches the camera pre-fix. The probe
sets 8000 and reads back 8000 in every cell, because set and get route to the same slot; separating
that needs a streamed frame-metadata check.

---

FW PR 222 has merged. The gate constant is resolved and no longer merge-order-dependent.

Tracked on [RSDSO-18682]

[sysrsbuild-gh-agentic]

🤖 Auto-generated by rs-agentic bot

Code review of PR #15508 — see inline comments below.

[Nir-Az]

Reviewed the gate and the test change. The direction is right - keeping the workaround and gating it beats deleting it, and the queried-value assertion is the part that actually makes this test measure the FW.

One blocker inline in src/hdr-config.cpp: set_sequence_index(0.f) inside the workaround block is not only part of the exposure save/restore, it is also the option-routing reset. Gating it off changes exposure get/set behavior after HDR disable on fixed FW.

Validation gap: the HW table covers pytest-hdr-long.py only. The blocker lives in the sequence_id flow, so unit-tests/live/d400/pytest-hdr-sanity.py and unit-tests/live/hdr/* also want a run on 5.17.4.11 with the workaround gated off. Note that neither of those would fail on the blocker as written - hdr-sanity never re-reads exposure after HDR off, and function_scoped_device builds a fresh hdr_config per test, so nothing leaks between tests. Worth a manual check rather than trusting a green suite.

Leaving this as a comment, not request-changes - the PR is already marked do-not-merge until FW #222 lands.

🤖 Generated by AI

[sysrsbuild-gh-agentic]

🤖 Auto-generated by rs-agentic bot

The new push () is a merge from development into the PR branch — no changes to the PR's HDR files (hdr-config.cpp, hdr-config.h, d400-device.cpp, pytest-hdr-long.py). No new inline comments needed.

The two open items from the previous review round remain unaddressed:

1. Blocker (thread on src/hdr-config.cpp): set_sequence_index(0.f) routing reset is inside the _use_workaround block — hoist it out so option-routing always resets on HDR disable regardless of FW version.
2. Bug (thread on pytest-hdr-long.py:518): the queried-exposure assertion can silently not run if the single target frame lacks metadata — needs a guard to ensure the check actually executes.

[ymodlin]

Pushed 9b6fc755d — all five threads addressed, replies inline.

Two things worth surfacing here rather than only in a thread:

1. The gate constant was wrong — now 5.17.4.13, not 5.17.4.11. Chasing down your merge-checklist comment turned up that v5.17.4.11 and v5.17.4.12 were both tagged before the FW fix commit 4631fe1b landed; v5.17.4.13 is the first tag whose ancestry contains it. Left as-is, the gate would have turned the workaround off on real 5.17.4.11/.12 cameras that have no FW fix — a false CI failure, and those cameras losing the host-side restore. Details and the ancestry check are in that thread.

2. Your blocker is fixed, and it also removes most of the validation gap you flagged. With set_sequence_index(0.f) hoisted out of both _use_workaround blocks, the sequence_id / option-routing flow is now identical for every FW version — it is exactly today's behavior, unconditionally. So pytest-hdr-sanity.py and unit-tests/live/hdr/* no longer have a gate-dependent path to diverge on. On the disable side it is strictly more reset than before: it had been nested inside the _pre_hdr_exposure in-range check too, so it was already being skipped when the saved exposure was out of range.

What I did and did not run this round. Built clean (realsense2, Linux/GCC, Release) — d400-device.cpp and hdr-config.cpp compile and the lib links; pytest-hdr-long.py byte-compiles. I did not do a new hardware run: no pytest-hdr-sanity.py / unit-tests/live/hdr/* on a camera, and no re-run of the table in the description against a genuine 5.17.4.13 image. Given the routing change is unconditional, the manual check you asked for is still the right gate before this goes ready — flagging it rather than claiming a green suite I did not produce.

🤖 Generated by AI

[ymodlin]

Hardware validation of 9b6fc755d — rs-nuc01

Ran the checks @Nir-Az asked for, plus an A/B control for the blocker. Both HDR suites and a targeted probe, on two cameras that happen to sit on either side of the gate.

Suites — --context nightly

cd unit-tests && python3 -m pytest -v --context nightly --device <serial> \n    live/d400/pytest-hdr-sanity.py live/d400/pytest-hdr-long.py live/hdr/

Covers pytest-hdr-sanity.py, pytest-hdr-long.py, live/hdr/pytest-hdr-configurations.py, pytest-hdr-performance.py, pytest-hdr-preset.py.

Camera	FW	Gate resolves to	Result
D455 013322250138	5.17.4.13	workaround OFF	32/32 passed (203s)
D455 213622301050	5.17.3.25	workaround ON	32/32 passed (202s)

The first row is a genuine 5.17.4.13 release image, not the hand-stamped build from the original table — so the corrected gate constant is exercised end-to-end (5.17.4.13 < 5.17.4.13 → false) rather than a forced flag. That closes the other open item from the description too.

The suites do not prove the blocker fix — you called this

You predicted neither suite would fail on the blocker as written, and that is what I see: 32/32 also passes on the pre-fix build. So the green runs above are a no-regression result, nothing more.

For direct evidence I drove the exact flow from your comment — sequence_id = 1/2, configure both slots, HDR on, HDR off — and read back the state you named. A/B across the two builds:

librealsense build	FW 5.17.4.13 (gate off)	FW 5.17.3.25 (gate on)
a3d75e88d (pre-fix)	FAIL — sequence_id=2, exposure=1200, gain=20	PASS
9b6fc755d (fixed)	PASS — sequence_id=0, exposure=5000, gain=50	PASS

Pre-fix on fixed FW, after HDR off, get_option(exposure)/get_option(gain) return 1200 / 20 — the values I had written into HDR slot 2, not the pre-HDR 5000 / 50. That is the cached-slot read you described, and it only happens on the FW-version side where the workaround gates off, which is the divergence the gate was not meant to introduce. With the hoist, both FW versions behave identically.

One part of your analysis I did not confirm

You also noted that set_option(exposure) reaches hdr_config::set_exposure and never gets to the camera. My probe cannot show that: it sets 8000 and reads back 8000 on every cell of the matrix, because the set and the get route to the same slot, so the readback is self-consistent whether or not it reached the sensor. Distinguishing that needs a streamed frame-metadata check. The routing/query half is confirmed; the write-never-lands half I am taking on your reading of hdr-config.cpp:513-514 rather than claiming I measured it.

Setup

Built both trees from source on the rig (BUILD_PYTHON_BINDINGS=ON, Release, GCC); pytest picked up the local pyrealsense2 — -I- Using pyrealsense2 from: ~/lrs-18682-hdr/build/Release/pyrealsense2.cpython-310-...so. Cameras returned to hdr_enabled=0, sequence_id=0, enable_auto_exposure=1 afterwards.

🤖 Generated by AI

[Nir-Az]

LGTM