Skip to content

[RSDSO-21316] FW-version-gated accel scale for the new 10 micro-g HID unit - #15638

Merged
Nir-Az merged 2 commits into
realsenseai:developmentfrom
AlonHirshberg:RSDSO-21316-accel-resolution
Sep 15, 2026
Merged

Nir-Az merged 2 commits into
realsenseai:developmentfrom
AlonHirshberg:RSDSO-21316-accel-resolution

Conversation

@AlonHirshberg

@AlonHirshberg AlonHirshberg commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Overview: D400 firmware 5.17.4.24 reports the accelerometer in 10 µg units instead of 1 mg (HID exponent −5 instead of −3). librealsense hardcodes the accel scale, so this adds a FW-version-gated scale that keeps old firmware unchanged and reads new firmware correctly, the same way the gyro's scale is gated at FW 5.16.

Tracked on [RSDSO-21316]

Why

The firmware converted accel with integer math, (raw * 1000 * range) / 32767. One raw LSB moved the numerator by 4000 to 6000 against a 32767 divisor, so it truncated to zero and every D400 IMU delivered 1 mg steps regardless of the part's real 122–183 µg LSB. The firmware fix (RealSenseAI-Internal/perc_hw_fw-submodule_hal#1, RealSenseAI-Internal/perc_hw_fw-rs400#262, both merged) uses float coefficients and a 100× finer unit, and updates the HID descriptor exponent to match.

librealsense does not read the descriptor. It hardcodes 0.001 * gravity in the transform and 1000.0 in the Windows backend. Against the new firmware an unchanged SDK reads 100× too large on Linux and rsusb: a D455 on firmware 5.17.4.24 measures 980.14 m/s² through the stock SDK instead of 9.8.

Summary

  • backend-device.h: get_accel_default_scale() returns the legacy 0.001, beside get_gyro_default_scale().
  • d400-motion.{h,cpp}: override returns 0.00001 for FW ≥ 5.17.4.24, the first version that can only carry the new unit. One gate; the Windows factor is derived from it as 1.0 / get_accel_default_scale() so the two cannot disagree.
  • d500-motion.cpp: passes the base value through. D500 firmware is unchanged, behavior bit-identical.
  • motion-transform.{h,cpp}, ds-motion-common.cpp: the hardcoded constant becomes a constructor parameter at both accel sites (USB-HID and MIPI).
  • Windows MF (platform/hid-device.h, mf-hid.{h,cpp}, hid-sensor.{h,cpp}, no-ops in linux/backend-hid.h, hid/hid-device.h, multi-pins): the Sensor API hands the backend values already in G, so mf-hid.cpp multiplies back up into FW counts. With the old factor it still produced the right value but truncated to the old 1 mg resolution, so these files are what deliver the fix on Windows. Same shape as the gyro's _gyro_scale_factor plumbing.
  • unit-tests/live/d400/pytest-accel-gravity-norm.py (new): streams accel on every D400 with an IMU and asserts |a| is within 20 % of standard gravity. A FW/SDK scale mismatch reads 100× off, so the window is loose on purpose. Replaces the C++ static test per review; note this moves the check to the live rig, no CI job runs unit-tests/live.
  • Unchanged on purpose: hid_sensor::get_imu_sensitivity_values() still returns 0.001 for accel. That is the change-sensitivity threshold in the HID feature report, a different unit domain the FW fix does not touch.

Behavior by combination, at a true 1 g:

FW SDK Linux / rsusb Windows MF
old old correct correct
new old 100× too large right value, old 1 mg resolution (measured)
new this PR correct, 0.001 m/s² step correct, 0.001 m/s² step
old this PR unchanged unchanged

The last row is what makes it safe to merge ahead of a firmware release.

Non-SDK users are not affected: the HID exponent is updated in the same firmware change, so any reader that honours the descriptor self-corrects (measured: the Linux kernel's in_accel_scale moved to 0.000098066 with no driver change and reads 9.66 m/s²). Only readers that hardcode a milli-g scale need updating, which is worth one line in the firmware release notes.

Test plan

Measured on rs-nuc01, two D455s, camera stationary, 600 samples per leg (pytest uses 400). Standard gravity 9.80665 m/s^2.

  • New FW through this branch — D455 213622301050 flashed to 5.17.4.24: |a| 9.8007 m/s^2 (-0.06 %), smallest step 0.001177 m/s^2. Same unit on pre-fix 5.17.4.19 read 9.7963 through the same SDK, so before/after on one camera is +0.04 %.
  • The hazard, on real shipping-version FW — same camera on 5.17.4.24 through the stock SDK: 980.14 m/s^2, a 100x error. This is what the gate prevents.
  • Old FW is untouched — D455 013322250138 on pre-fix 5.17.4.19: 8.6121 through this branch vs 8.6234 through stock, i.e. the same path. (That unit reads ~12 % low on every SDK build; a pre-existing fault on the camera, not this change.)
  • pytest-accel-gravity-norm.py passes on both D455s and skips the D405/D415, which have no IMU. Gated with context("nightly") per review, so it does not collect on PR runs; test body measures 0.83-1.22 s.
  • Linux gcc build on the rebased development base; Windows MSVC build including mf-hid.cpp, no warnings in changed files.
  • Windows, on hardware, both arms of the gate. Branch built with MSVC and run against a D430i flashed between firmware generations on one host: pre-fix 5.17.0.214 gives 9.6020 m/s^2 at a 0.009807 m/s^2 step (gate silent, behaviour unchanged); 5.17.4.24 gives 9.6159 m/s^2 at a 0.001667 m/s^2 step (gate fires, value correct and resolution 5.9x finer). The stock SDK also reads correctly on both, confirming the Windows Sensor API honours the descriptor exponent.
  • MIPI: the MIPI converter takes the same get_accel_default_scale(), not run on a D457; the d4xx driver's own IMU scaling is unverified.

🤖 Generated by AI

@sysrsbuild-gh-agentic sysrsbuild-gh-agentic 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.

Auto-generated review by rs-agentic bot. Found one bug worth flagging before the follow-up version commit.

Comment thread src/ds/d400/d400-motion.cpp Resolved
Comment thread src/ds/d400/d400-motion.cpp Resolved
@AlonHirshberg AlonHirshberg changed the title D400: FW-version-gated accel scale for the new 10 micro-g HID unit [RSDSO-21316] FW-version-gated accel scale for the new 10 micro-g HID unit Sep 3, 2026
Comment thread src/ds/d400/d400-motion.cpp Resolved

@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.

please also make sure and post that old FW and new FW show expected accel values.

@sysrsbuild-gh-agentic sysrsbuild-gh-agentic 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.

Reviewed new commit (FW gate finalised to 5.17.4.24). Previous dual-gate and guard concerns are resolved. One new finding below.

import time
log = logging.getLogger(__name__)

pytestmark = [pytest.mark.device_each("D400*")]

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.

please add nightly context definition no need to run on every PR

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.

✅ Fixed in 1016499ca. Added pytest.mark.context("nightly") alongside the existing device_each.

Verified on a rig with four D400s: without the flag all four are skipped with "Requires --context nightly"; with --context nightly the two D455s pass and the D405/D415 skip for having no IMU. So it no longer collects on PR runs at all.

🤖 Generated by AI

Signed-off-by: AlonHirshberg <alonhberg@gmail.com>
@AlonHirshberg
AlonHirshberg force-pushed the RSDSO-21316-accel-resolution branch from ab9c9a6 to 9b6e12c Compare September 15, 2026 12:45
@AlonHirshberg

Copy link
Copy Markdown
Contributor Author

Rebased onto development and squashed to one commit — the conflict was in d500-motion.cpp, where the GMSL gyro-sensitivity work restructured the same processing-block lambda. That logic is kept as-is and the accel scale is threaded through it.

Old and new FW, both measured through this branch on rs-nuc01 (two D455s, stationary, 600 samples per leg):

FW SDK ||a|| m/s² smallest step
5.17.4.24 this branch 9.7987 (−0.08 %) 0.001177
5.17.4.24 stock (no fix) 980.14 0.1177
5.17.4.19 (pre-fix) this branch 9.7963 0.0098

The same camera was flashed from 5.17.4.19 to 5.17.4.24 and read 9.7963 → 9.7987 across the flash, so the value is unchanged and only the resolution improves, by 8.3×. The middle row is the hazard the gate prevents, measured on the shipping firmware version rather than a dev build.

The new live test passes on both D455s and skips the D405/D415, which have no IMU. All of the above was re-run on the rebased commit, not just the pre-rebase tree.

One caveat worth stating: D455 013322250138 reads about 12 % low on every SDK build, stock included, so that is a pre-existing fault on that camera and not related to this change.

🤖 Generated by AI

@AlonHirshberg
AlonHirshberg marked this pull request as ready for review September 15, 2026 12:52
@AlonHirshberg

Copy link
Copy Markdown
Contributor Author

Windows is now validated on hardware, both arms of the gate, so the earlier "compiles but untested" caveat is gone from the description.

Branch built with MSVC and run against one D430i, flashed between firmware generations on the same host with nothing else changed:

FW gate ||a|| smallest step
5.17.0.214 (pre-fix) silent 9.6020 m/s² 0.009807 m/s²
5.17.4.24 fires 9.6159 m/s² 0.001667 m/s²

The second row is the one that had never been exercised: set_accel_scale_factor( 100000 ) in the Media Foundation backend against 0.00001 in the transform. They cancel, the value stays at gravity, and the step drops by a factor of 5.9. The first row confirms the gate stays silent on older firmware, so Windows behaviour there is untouched.

I also ran the stock SDK on the same camera on both firmware generations and it read correctly each time (0.984 G and 0.9946 G). That is worth stating because it means Windows honours the HID unit exponent, which is why an unchanged SDK on Windows does not hit the 100× error that Linux does.

One unrelated observation while testing, raised separately because it is not caused by this change: a raw Windows Sensor API consumer reads about 2× the true value on this camera. It does so identically on pre-fix firmware, so it predates this ticket. Worth its own issue rather than holding up this PR.

🤖 Generated by AI

…ightly

Signed-off-by: AlonHirshberg <alonhberg@gmail.com>
@AlonHirshberg

AlonHirshberg commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Round 2 of review fixes is in 1016499ca.

Nir's two points on the live test. It is now gated with pytest.mark.context("nightly"), so it no longer collects on PR runs. Verified on a four-camera rig: without the flag all four skip; with --context nightly the two D455s pass and the D405/D415 skip for having no IMU. Sampling is down to 200 samples with an 8 s ceiling, and --durations puts the test body at 0.83 s and 1.22 s. The 20 s was a ceiling rather than a wait, and the 9 s per device you may see is framework device setup.

The Aikido finding on d400-motion.cpp. Fixed by guarding the call with if (hid_ep); the bot has since re-scanned and cleared it. Two notes worth keeping, since its comment is now gone:

The mechanism was not a null dereference. _motion_module_device_idx is an optional_value whose value() raises bad optional access (src/types.h:134) when unset, which the surrounding try/catch turns into _has_motion_module_failed. The guard is still right, because get_raw_motion_sensor() returns a dynamic_pointer_cast that can legitimately be null.

The real defect was behavioural and introduced by this PR. That line previously carried its own version check, which I removed after an earlier bot suggestion to stop duplicating the gate in get_accel_default_scale(). Removing it was correct, but the check had been incidentally skipping the call on devices below the threshold. Without it, a device whose HID enumeration fails on FW < 5.16 went from constructing fine to raising. The guard restores exactly that case.

I deliberately did not guard the gyro call beside it, which the bot also suggested. That line has shipped since January. Guarding it would change existing behaviour: a device with FW >= 5.16 and no HID endpoint currently fails construction and would afterwards construct silently with an unscaled gyro. Worth looking at separately, not in a PR about the accelerometer scale.

Unrelated CI note. Minimal CMake version shows a failure here, but it fails identically on development (the four most recent static_analysis.yaml runs). It dies while cloning nlohmann/cmake_min_version in its own setup step, before reading this tree. Every other check is green.

🤖 Generated by AI

@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, lets see CI pass and pend your test on MIPI

@Nir-Az
Nir-Az merged commit de6abf5 into realsenseai:development Sep 15, 2026
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants