ds: split fw_cmd per family + add AE_ACCEL_PARAMS = 0x95 opcode by remibettan · Pull Request #15382 · realsenseai/librealsense

remibettan · 2026-07-08T14:47:30Z

Tracked by: RSDSO-21571

Overview

Part 2 of RSDSO-21571 (part 1: #15381 for the viewer-side Depth AE mode toggle). This PR sets up the LibRS-side plumbing for the new AE_ACCEL_PARAMS = 0x95 Hardware-Monitor command described in the ticket, so the FW-side implementation (RSDSO-21570) can be exercised end-to-end from terminal/Python. No new RS2_OPTION is added — the tuning parameters are engineering-only and must not appear in the viewer UI.

Why the enum split

Ticket calls for AE_ACCEL_PARAMS = 0x95. That opcode was already SAFETY_PRESET_WRITE in the shared ds::fw_cmd enum (src/ds/ds-private.h). Since the two families are diverging and the shared enum was already accumulating single-family opcodes, this PR does the split cleanly rather than picking a workaround opcode number.

ds::fw_cmd (ds/ds-private.h) — shrunk to genuinely-shared opcodes (flash ops, GVD, DFU/HWRST, GTEMP, advanced-mode, ROI, cam-sync, pwm, subpreset, LASERONCONST, AUTO_CALIB, MMER, RECPARAMSGET, CAL_RESTORE_DFLT).
ds::d400_fw_cmd (ds/d400/d400-private.h) — d400-only: GLD, GETINTCAL, SETINTCAL, CALIBRECALC, SETINTCALNEW, ASIC_TEMP_MIPI, GETAELIMITS, SETAELIMITS, plus the new AE_ACCEL_PARAMS = 0x95.
ds::d500_fw_cmd (ds/d500/d500-private.h) — d500-only: HKR_THERMAL_COMPENSATION, SAFETY_PRESET_READ, SAFETY_PRESET_WRITE, APM_STROBE_SET/GET, SET/GET_HKR_CONFIG_TABLE, RGB_TNR, GET_FW_LOGS, SET_CALIB_MODE, GET_CALIB_STATUS.

Because all three enums live in namespace ds and are unscoped, existing ds::GLD / ds::SAFETY_PRESET_WRITE / etc. call sites resolve unchanged. Only a handful of type-qualified fw_cmd::X references needed updating (d400-device.cpp, d500-device.cpp, d500-options.cpp, hw_monitor_extended_buffers.cpp).

emitter_always_on_option was retyped from ds::fw_cmd to uint8_t — its callers now pass a mix of shared-family and d500-family opcodes, and enum-to-enum doesn't cross-convert; uint8_t matches what command::cmd actually stores anyway.

Commits

9e526154f — split enum
329f1d60c — add AE_ACCEL_PARAMS = 0x95
e880ec261 — build fix (missing include + one local-var retype)
58de51839 — wrappers/python/examples/ae_accel_params.py GET/SET utility

Python utility

wrappers/python/examples/ae_accel_params.py — terminal-only helper that calls debug_protocol.build_command + send_and_receive_raw_data for the 7 float fields. Ranges from the ticket table are validated client-side.

Wire convention assumed (per ticket, needs RSDSO-21570 confirmation):

GET: param1 = 0, no payload → response has 7 IEEE-754 floats after the 4-byte opcode echo.
SET: param1 = 1, payload = 5 IEEE-754 floats (score_setpoint, score_deadband, saturation_weight, saturation_value, stability_factor). score_low_th / score_high_th are recomputed by FW.

Test plan

Verify realsense2 and pyrealsense2 compile clean (done locally on Win/x64/Release).
Once RSDSO-21570 FW lands, run ae_accel_params.py on D405/D435 to GET+SET.
Confirm SET is NACK'd while depth is streaming (per FW spec).
Confirm no new option appears in realsense-viewer for D400 SKUs.
Confirm existing D500 auto-cal / safety-preset / HKR / RGB-TNR paths still work (enum split touched their opcode namespace).

sysrsbuild-gh-agentic

Auto-generated review by rs-agentic bot

Overall the enum-split design is clean and the rationale is well-documented. The C++ refactor looks correct. Unscoped enums in namespace ds mean existing call sites (ds::GLD, ds::SAFETY_PRESET_WRITE, etc.) continue to resolve without changes. A few bugs and enhancement notes are called out inline below.

Summary of findings:

BUG (critical): device.as_debug_protocol() can return None - no null-check before use.
BUG: send_and_receive_raw_data exceptions are unhandled in get_params / set_params.
Concern: CALIBRESTOREEPROM = 0xA8 left in shared ds::fw_cmd needs confirmation it is genuinely dual-family.
Enhancement: uint8_t opcode erasure in emitter_always_on_option.
Enhancement: Missing FW-version guard and D415/product-line validation in Python utility.
Enhancement: d400_fw_cmd2str / d500_fw_cmd2str appear unused.
Tests: No unit or integration tests added for the enum split or the new AE_ACCEL_PARAMS command.

The shared enum in ds-private.h had grown to include opcodes only ever used by one family (SAFETY_PRESET_*, HKR_*, APM_STROBE_*, RGB_TNR, GET_FW_LOGS, SET/GET_CALIB_MODE/STATUS on d500; GLD, GET/SETINTCAL*, CALIBRECALC, ASIC_TEMP_MIPI, GET/SETAELIMITS on d400), and the two families are starting to compete for opcode numbers. Move d400-only opcodes to ds::d400_fw_cmd in d400-private.h and d500-only opcodes to ds::d500_fw_cmd in d500-private.h. Keep genuinely-shared opcodes in ds::fw_cmd. Because these are unscoped enums all living in the ds namespace, existing `ds::GLD`, `ds::SAFETY_PRESET_WRITE`, etc. references resolve unchanged; only a handful of type-qualified `fw_cmd::X` call sites needed updating. emitter_always_on_option was taking ds::fw_cmd, but its call sites now pass opcodes from either the shared or the d500 enum. Switched its ctor + fields to uint8_t (the actual HWM byte), which composes with any of the three enums via implicit underlying-type conversion. Frees opcode 0x95 on the d400 side (currently d500's SAFETY_PRESET_WRITE), which is the prerequisite for AE_ACCEL_PARAMS = 0x95 in a follow-up commit. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Register the AE_ACCEL_PARAMS opcode from RSDSO-21571 in ds::d400_fw_cmd so terminal HWM commands can address it. The FW-side handler (get/set of the 7 float tuning parameters, per-SKU defaults, setpoint/deadband clamping, stream-guard NACK, IEEE-754 encoding) lives in RSDSO-21570 and is not part of this change. No SDK-visible RS2_OPTION is added: the tuning parameters are intended for terminal + engineering use only, and must not appear in the viewer UI. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Two follow-ups to the previous enum split commit: - d500-options.cpp used ds::RGB_TNR and ds::HKR_THERMAL_COMPENSATION but only included d500-options.h, which didn't transitively include the new d500-private.h where those enumerators now live. Added the include. - d400-auto-calibration.cpp had a local `fw_cmd cmd{}` that then gets assigned SETINTCAL / SETINTCALNEW — both are now d400_fw_cmd, and unscoped enum types don't cross-convert. Retyped the local as d400_fw_cmd; it composes with `command(cmd, ...)` unchanged since the command constructor takes uint8_t. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Terminal-only GET/SET helper for AE_ACCEL_PARAMS (opcode 0x95). Handles the 7-float IEEE-754 payload documented in RSDSO-21571 and the writable- vs-read-only field distinction (5 R/W + 2 R-only). Ranges from the ticket table are validated client-side before dispatch. The GET/SET wire protocol (param1=0 for GET, param1=1 with 5-float body for SET) is inferred from the ticket and needs confirmation once FW-side RSDSO-21570 lands. The script prints raw response bytes on failure so mismatches surface quickly. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Two follow-ups from PR review: - CALIBRESTOREEPROM (0xA8, "HKR Store EEPROM Calibration") was left in the shared enum but has no call sites and its name is HKR/d500-specific. Moving it to d500_fw_cmd completes the split cleanly. - The uint8_t retype on emitter_always_on_option's opcode fields loses compile-time enum typing. Added a comment explaining why it's necessary: the class is instantiated with opcodes from more than one family enum (ds::LASERONCONST vs d500_fw_cmd::APM_STROBE_*), and unscoped enums don't cross-convert. Raw byte is what the HWM actually stores. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…nd errors Addresses PR review comments: - D400 product-line guard: on D500 opcode 0x95 is SAFETY_PRESET_WRITE, so running this against a D500 could be destructive. Refuse to proceed unless product_line == "D400". - FW-version guard: bail out below 5.17.3.20 with a clear message pointing at RSDSO-21358 instead of letting the FW NACK. - as_debug_protocol() null-check: prints a human-readable message instead of AttributeError. - GET/SET try/except: convert raw C++ tracebacks into a single sys.exit line. SET's failure path now short-circuits before the re-GET so the "After SET" table can no longer echo the pre-SET values on a NACK. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Live-device test covering the RSDSO-21571 acceptance criteria: - GET returns 7 IEEE-754 floats, each within the ticket-documented range. - SET applies the 5 writable fields and leaves score_low_th / score_high_th recomputed by FW as setpoint +/- deadband/2. - Setpoint / deadband clamping (ported from D457) keeps score_low_th >= 0.08 and score_high_th <= 0.45 even when the input would push them out. - SET while depth is streaming is NACK'd and values do not move. - Sanity check that no RS2_OPTION_* was accidentally registered (this is a terminal-only HWM per the ticket, no viewer UI). Marked pytest.mark.skip until RSDSO-21570 FW ships, following the pattern already used by pytest-depth-ae-toggle.py / pytest-depth-ae-metadata.py. D415 family is skipped in-fixture since Accelerated AE is unsupported there. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…ofile Follow-up to PR review: the streaming test grabbed the first depth-stream profile via a bare `next(...)` generator, which raises StopIteration (surfaced by pytest as ERROR rather than SKIP) on any D400* variant whose first depth sensor exposes only IR or confidence streams. Use `next(..., None)` with a `pytest.skip(...)` fallback so unusual sensor configurations get a clean skip instead of a red-error. Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Ticket IDs belong in commit messages and PR descriptions, not in inline source comments (they rot as tickets get closed or the codebase evolves). Removed the ticket markers from three files touched by this PR: - src/ds/d400/d400-private.h (AE_ACCEL_PARAMS enum comment) - wrappers/python/examples/ae_accel_params.py (header + FW-gate message + argparse description) - unit-tests/live/d400/pytest-ae-accel-params.py (header + MIN_FW comment + skip reason) Tracked by: RSDSO-21571 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…-gating

Nir-Az · 2026-07-21T12:44:43Z

        register_info(RS2_CAMERA_INFO_FIRMWARE_VERSION, _fw_version);
        register_info(RS2_CAMERA_INFO_PHYSICAL_PORT, group.uvc_devices.front().device_path);
-        register_info(RS2_CAMERA_INFO_DEBUG_OP_CODE, std::to_string(static_cast<int>(fw_cmd::GLD)));
+        register_info(RS2_CAMERA_INFO_DEBUG_OP_CODE, std::to_string(static_cast<int>(ds::GLD)));


What is DEBUG_OP_CODE and why we want to change the enum to a global one?

DEBUG_OP_CODE is the HWM opcode we report via RS2_CAMERA_INFO_DEBUG_OP_CODE so external tools (rs-terminal / rs-fw-logger) can pull FW flash logs from the device. On D400 that means GLD (0x0f) - legacy flash-logs command. On D500 the equivalent is GET_FW_LOGS (0xB4).

The enum was not made global - the opposite: GLD moved into the new per-family d400_fw_cmd enum (D400-only opcode). I've now scoped the reference explicitly as d400_fw_cmd::GLD instead of the bare ds::GLD in c7a4b58, matching your other comments in this PR.

Nir-Az · 2026-07-21T12:48:39Z

        register_info(RS2_CAMERA_INFO_FIRMWARE_VERSION, gvd_parsed_fields.fw_version);        
        register_info(RS2_CAMERA_INFO_PHYSICAL_PORT, group.uvc_devices.front().device_path);
-        register_info(RS2_CAMERA_INFO_DEBUG_OP_CODE, std::to_string(static_cast<int>(fw_cmd::GET_FW_LOGS)));
+        register_info(RS2_CAMERA_INFO_DEBUG_OP_CODE, std::to_string(static_cast<int>(ds::GET_FW_LOGS)));


lets try to scope it under a dedicated common enum and not right after ds

Nir-Az · 2026-07-21T12:49:29Z

        switch( cmd.cmd)
        {
-        case ds::fw_cmd::GET_HKR_CONFIG_TABLE:
+        case ds::GET_HKR_CONFIG_TABLE:


same all over, lets scope it

Nir-Az · 2026-07-21T12:54:29Z

@@ -0,0 +1,169 @@
+# License: Apache 2.0. See LICENSE file in root directory.


lets place example and test in the Jira ticket for Algo to run and remove from the PR

Nir-Az · 2026-07-21T12:54:36Z

@@ -0,0 +1,177 @@
+# License: Apache 2.0. See LICENSE file in root directory.


sysrsbuild-gh-agentic

Auto-generated review by rs-agentic bot — commit `c7a4b58d14e`

Reviewed the new push: "ds: scope HWM opcode call sites under their per-family enum" (18 lines, 5 files).

This commit adds explicit per-family enum scoping to all HWM opcode call sites in response to @Nir-Az's review:

File	Change
`d400-device.cpp`	`fw_cmd::GLD` → `d400_fw_cmd::GLD` ✅
`d500-active.cpp`	`APM_STROBE_GET/SET` → `d500_fw_cmd::APM_STROBE_GET/SET`, `LASERONCONST` → `fw_cmd::LASERONCONST` ✅
`d500-device.cpp`	`fw_cmd::GET_FW_LOGS` → `d500_fw_cmd::GET_FW_LOGS` ✅
`d500-options.cpp`	`ds::fw_cmd::HKR_THERMAL_COMPENSATION` → `ds::d500_fw_cmd::HKR_THERMAL_COMPENSATION` ✅
`hw_monitor_extended_buffers.cpp`	`ds::fw_cmd::GET_HKR_CONFIG_TABLE/SET_HKR_CONFIG_TABLE` → `ds::d500_fw_cmd::*` ✅

No bugs found in the new commit. The scoping changes are consistent and correct across all affected call sites. All previously flagged issues from prior reviews remain resolved.

⚠️ Note: @Nir-Az's open request to move the Python example (ae_accel_params.py) and pytest file to the Jira ticket and remove them from this PR is still unaddressed.

sysrsbuild-gh-agentic reviewed Jul 8, 2026

View reviewed changes

sysrsbuild-gh-agentic reviewed Jul 9, 2026

View reviewed changes

Comment thread unit-tests/live/d400/pytest-ae-accel-params.py Outdated

sysrsbuild-gh-agentic reviewed Jul 12, 2026

View reviewed changes

Comment thread unit-tests/live/d400/pytest-ae-accel-params.py Outdated

remibettan force-pushed the rsdso-21571-part2-ae-accel-params-hwmc branch from 8c1556e to e7c7cf1 Compare July 12, 2026 11:25

remibettan marked this pull request as ready for review July 12, 2026 12:28

remibettan force-pushed the rsdso-21571-part2-ae-accel-params-hwmc branch 2 times, most recently from ae8cfc5 to 81e078d Compare July 15, 2026 12:53

remibettan and others added 12 commits July 19, 2026 12:11

d500-active: retype emitter opcode locals to uint8_t after fw_cmd split

abec3b2

d400-auto-calibration: brace nested if/else to silence -Wdangling-else

3bc8151

unit-tests/d400: GET after stop() in streaming NACK test to avoid GET…

ffe5b2c

…-gating

remibettan force-pushed the rsdso-21571-part2-ae-accel-params-hwmc branch from 81e078d to ffe5b2c Compare July 19, 2026 09:13

remibettan requested a review from Nir-Az July 19, 2026 11:52

Nir-Az reviewed Jul 21, 2026

View reviewed changes

remibettan added 2 commits July 21, 2026 16:02

drop AE_ACCEL_PARAMS example and pytest from PR (moved to Jira for Algo)

b7666e9

ds: scope HWM opcode call sites under their per-family enum

c7a4b58

sysrsbuild-gh-agentic reviewed Jul 21, 2026

View reviewed changes

Nir-Az approved these changes Jul 21, 2026

View reviewed changes

sysrsbuild-gh added the libci-triggered label Jul 21, 2026

		@@ -0,0 +1,169 @@
		# License: Apache 2.0. See LICENSE file in root directory.

		@@ -0,0 +1,177 @@
		# License: Apache 2.0. See LICENSE file in root directory.

Uh oh!

Conversation

remibettan commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Why the enum split

Commits

Python utility

Test plan

Uh oh!

sysrsbuild-gh-agentic left a comment

Choose a reason for hiding this comment

Auto-generated review by rs-agentic bot

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sysrsbuild-gh-agentic left a comment

Choose a reason for hiding this comment

Auto-generated review by rs-agentic bot — commit c7a4b58d14e

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

remibettan commented Jul 8, 2026 •

edited

Loading

Auto-generated review by rs-agentic bot — commit `c7a4b58d14e`