Repository navigation
[RSDSO-21316] FW-version-gated accel scale for the new 10 micro-g HID unit - #15638
Conversation
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
Auto-generated review by rs-agentic bot. Found one bug worth flagging before the follow-up version commit.
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
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*")] |
There was a problem hiding this comment.
please add nightly context definition no need to run on every PR
There was a problem hiding this comment.
✅ 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>
ab9c9a6 to
9b6e12c
Compare
|
Rebased onto Old and new FW, both measured through this branch on rs-nuc01 (two D455s, stationary, 600 samples per leg):
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 🤖 Generated by AI |
|
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:
The second row is the one that had never been exercised: 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>
|
Round 2 of review fixes is in Nir's two points on the live test. It is now gated with The Aikido finding on The mechanism was not a null dereference. 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 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. 🤖 Generated by AI |
Nir-Az
left a comment
There was a problem hiding this comment.
LGTM, lets see CI pass and pend your test on MIPI
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 * gravityin the transform and1000.0in 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, besideget_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 as1.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).platform/hid-device.h,mf-hid.{h,cpp},hid-sensor.{h,cpp}, no-ops inlinux/backend-hid.h,hid/hid-device.h, multi-pins): the Sensor API hands the backend values already in G, somf-hid.cppmultiplies 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_factorplumbing.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 runsunit-tests/live.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:
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_scalemoved 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.
213622301050flashed 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 %.013322250138on 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.pypasses on both D455s and skips the D405/D415, which have no IMU. Gated withcontext("nightly")per review, so it does not collect on PR runs; test body measures 0.83-1.22 s.developmentbase; Windows MSVC build includingmf-hid.cpp, no warnings in changed files.get_accel_default_scale(), not run on a D457; the d4xx driver's own IMU scaling is unverified.🤖 Generated by AI