Repository navigation
Report device removal promptly and hold back a device that is still enumerating - #15631
Conversation
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
Auto-generated review by rs-agentic bot. Focused on potential bugs and guideline violations.
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
mf-backend.cpp.
instance_id_key(std::string const&) — unsafe narrow-to-wide conversion
static std::string instance_id_key( std::string const & device_path )
{
std::wstring wide( device_path.begin(), device_path.end() ); // byte-cast, not UTF-8 decode
return instance_id_key( wide.c_str() );
}std::wstring(str.begin(), str.end()) widens each char by value, treating each byte as a code point. For a UTF-8 encoded uvc.device_path with any multi-byte sequence (e.g. a vendor string with a non-ASCII character), the resulting wide string is garbled — and the instance_id_from_device_path call inside the LPCWSTR overload will then extract a corrupted instance-ID fragment, causing the lookup in removed_instance_ids to silently miss the match (removal not recognized, no callback fired).
In practice Windows USB device interface paths are ASCII-only (\\?\USB#VID_xxxx&PID_yyyy&MI_zz#...) so this doesn't trigger today, but using MultiByteToWideChar(CP_UTF8, ...) or the project's own wchar_from_utf8 helper would make it correct by construction.
Note: the open thread on info.unique_id = uvc.unique_id (line 285) remains unaddressed.
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
Auto-review of new commit (rs-agentic bot).
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
Reviewed new commits — one potential bug flagged below in the Linux incomplete-device check.
Cosmetic: `_warned_incomplete` was indented 8 spaces instead of 4, so it visually sat inside the preceding `_incomplete_since` map declaration. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Mirror the Windows guard: a composite already in `_devices_data` stays published even if `incomplete_devices()` flags it for a tick. Without this, a transient sysfs miss (e.g. `/dev/videoN` symlink not visible during an unrelated bind on the same composite) would surface as a spurious removal followed by a re-add. Also compute the wait as a `waited` local and only warn on subsequent sightings, so the "still incomplete after Xs" log cannot ever print 0s. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Compute the wait as a `waited` local and skip the warn branch on the first sighting of a composite. Behavior is unchanged at MAX_DEFERRAL=10s today; a future edit that lowered the budget could otherwise make the log line silently print 0s. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Overview: A camera that re-enumerates in place — after a hardware reset or a firmware update — is now reported as removed within ~100 ms of the OS notification on Windows, and on both Windows and Linux a device is held back until the interfaces it declares have surfaced, so it is not published with only part of its sensors.
Tracked on [RSDEV-14305]
Why
Both watchers decided what changed by diffing two full enumeration snapshots. Three failures came out of that.
1. On Windows the removal was reported late, or not at all. The snapshot is dominated by
query_hid_devices(), which walks every sensor the Windows Sensor API knows about. Measured on a developer laptop:A reset only takes the camera away for 3–6 s, so it was often back — on identical device paths — before the snapshot completed, and
list_changed()saw nothing. The application then kept a device whose handles were dead: the viewer left it in its panel, and FW error polling queried it once a second forever.2. A device was published before all its interfaces had surfaced. On Linux, a D585 came back from a hardware reset with
['Motion Module', 'Stereo Module']instead of its five sensors in 1 of 6 attempts — missing Depth Mapping Camera, Perception and RGB Camera. On Windows the same class of failure showed up during a firmware update as a depth-only device seconds ahead of the real one.PR #15058 added a Windows gate for this, but it deferred behind any HID-class composite, including unrelated webcams, and its budget was a single global 15 s rather than per device.
3. Snapshot diffing cannot see a device that leaves and returns on the same path — no ordering or timing fix can change that.
Why it shows on some machines and not others
The Windows trigger is how much the Sensor API has to enumerate, not the camera:
That is why this was found by hand rather than by CI, and why the CI baseline run below is green with the new assertion but without the fix.
What changed
Windows —
src/mf/mf-backend.cpp_last, with no enumeration at all — the notification names the interface and_lastsays which device owned it. Removal no longer waits on a multi-second Sensor API walk, and a camera that returns on the same paths is still correctly reported as having left.incomplete_composites(), replacinghid_binding_in_progress(). It waits only on HID instances carrying aSensor-class collection — found by searching the whole subtree under the HID interface, so a driver stack that inserts an intermediate node cannot hide it, and gives each composite its own budget. A plain HID interface — a vendor-defined control on an unrelated webcam — never appears in the Sensor API, and waiting on one deferred every device on the machine.Linux —
src/linux/udev-device-watcher.{cpp,h}No arrival gate existed here; this adds one, using the same principle with a sysfs probe.
bInterfaceClass 0einterface of subclass 01 (VideoControl) must have surfaced a/dev/videoNthe backend lists;uvcvideoregisters the node against the VideoControl interface and its VideoStreaming siblings never get one, so requiring a node for those would hold every camera back until its budget expired.03interface is only waited on when its HID child is bound tohid-sensor-hub— the IMU path, which surfaces as an IIO sensor. A button or vendor HID bindshid-genericand never reaches the HID backend at all.Both
Behaviour change
hardware_reset()and firmware updates now raise a removed + added pair where the released SDK sometimes raised nothing. That is the point — the old device's handles are stale after a reboot — but an application that cached a device across a reset will now see it replaced.Tests added
unit-tests/live/hw-reset/pytest-sanity.py— records the sensor set before the reset and asserts the device comes back with all of it, sampled inside the devices-changed callback, because a partial device is superseded by a repaired one a second or two later so polling afterwards would only ever see the good one. Also bounds Windows USB removal latency at 2.5 s.tools/realsense-viewer/tests/hw-reset/test-hw-reset.cpp— the GUI test already checked that the device disappears and returns; it now also latches the sensor set of the first device the viewer publishes and asserts it is complete.unit-tests/live/tools/pytest-realsense-viewer.py— the viewer GUI tests now run on D5xx as well as D4xx, viadevice_each.Test plan
Windows:
hardware_resetx10 — 10/10 complete, removal 0.78–1.04 s. Before: 7.5–15 s to report removal.hardware_resetx10 — 10/10 complete, 0.72–0.84 s; 5 instrumented cold plugs — 5/5 complete, zero gate deferrals.hardware_resetx10 and 3 DFU firmware flashes, all complete; the recovery device is not gated and appears 0.35 s after the removal.Linux (Ubuntu 22.04):
CI:
-r hw-reset— SUCCESS, 12 ran, 0 failed.developmentplus the new assertion only,-r hw-reset— also green. CI hardware does not reproduce the partial-device race, so the red-before/green-after evidence is the bench measurements above, not CI.🤖 Generated by AI