Ds5 config retry fix - #377
Closed
Nikolai-L wants to merge 4 commits into
Closed
Conversation
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Contributor
Author
|
Duplicated |
There was a problem hiding this comment.
Pull request overview
This PR updates the RealSense D4XX kernel driver stream start/stop path to better handle configuration retries (avoiding repeated invalid DT/config states) and adds a standalone Python utility script for streaming frames via direct V4L2 ioctls, plus a small .gitignore update.
Changes:
- Add a bounded retry mechanism for DS5 config rejections and avoid rewriting stream DT registers unnecessarily in
ds5_configure()/ds5_mux_s_stream(). - Introduce
scripts/stream_frames.pyto exercise V4L2 streaming across one or two devices and across resolution profiles. - Ignore Python
__pycache__artifacts via.gitignore.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
kernel/realsense/d4xx.c |
Adds config retry limiting and DT caching behavior to reduce DS5 config failures; adjusts logging and stream start/stop sequencing. |
scripts/stream_frames.py |
New V4L2 ioctl-based streaming script for frame capture across profiles and devices. |
.gitignore |
Adds an ignore rule intended to exclude Python bytecode cache directories. |
Comments suppressed due to low confidence (1)
kernel/realsense/d4xx.c:4528
- The pre-toggle state verification treats the stream as being in the expected state even if all ds5_read() attempts fail:
statusis initialized (0 / DS5_STATUS_STREAMING) and the post-loopif (on == !(status & DS5_STATUS_STREAMING))check ignoresret. This can cause start/stop to proceed (or return -EBUSY) based on a syntheticstatusvalue rather than an actual device read. Track whether a read succeeded (e.g.,ret == 0or a separateread_okflag) and only usestatusafter a successful read; otherwise return the last I2C error (or -EIO) instead of assuming state.
if (on) {
stream_cmd = (DS5_STREAM_START | stream_id);
expected_streaming_state = DS5_STREAM_STREAMING;
status = 0;
} else {
stream_cmd = (DS5_STREAM_STOP | stream_id);
expected_streaming_state = DS5_STREAM_IDLE;
status = DS5_STATUS_STREAMING;
}
/* Verify stream is in the expected state before issuing command */
ts = jiffies;
for (timeout = ts + msecs_to_jiffies(DS5_START_MAX_TIME), i = 0;
time_before(jiffies, timeout); i++, msleep_range(i*DS5_START_POLL_TIME))
{
ret = ds5_read(state, config_status_base, &status);
if ((ret >= 0) && (on == !(status & DS5_STATUS_STREAMING))) {
break;
}
}
if (on == !(status & DS5_STATUS_STREAMING))
{
dev_dbg(&state->client->dev,
"stream %d in expected state, toggling to %d (status: 0x%04x) %dms\n",
stream_id, on, status, jiffies_to_msecs(jiffies - ts));
} else {
dev_warn(&state->client->dev,
"stream %d in %d state already (status: 0x%04x) %dms, return busy\n",
stream_id, on, status, jiffies_to_msecs(jiffies - ts));
return -EBUSY;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+427
to
+439
| for i, (width, height, fps, name) in enumerate(PROFILES): | ||
| ok = stream_frames(device, width, height, fps, frames_per_profile, name) | ||
| if not ok and stop_on_first_failure: | ||
| print(f"[{timestamp()}] Stopping on first failure as requested") | ||
| return False | ||
| # Add delay between profiles (not after the last one) | ||
| if i < len(PROFILES) - 1 and delay > 0: | ||
| print(f"[{timestamp()}] Waiting {delay}s before next stream...") | ||
| time.sleep(delay) | ||
|
|
||
| print(f"[{timestamp()}] === Repetition {rep} completed ===") | ||
| print() | ||
|
|
There was a problem hiding this comment.
run_profiles_single() never returns a success value when it completes, so main() receives None (and will exit 0 even if some profiles failed when stop_on_first_failure is false). Track whether any stream failed and return False if so, otherwise return True at the end of the function.
Comment on lines
+454
to
+482
| for i, (width, height, fps, name) in enumerate(PROFILES): | ||
| results = {} | ||
|
|
||
| def worker(dev, key): | ||
| results[key] = stream_frames(dev, width, height, fps, frames_per_profile, name, lock) | ||
|
|
||
| t1 = threading.Thread(target=worker, args=(device1, "d1")) | ||
| t2 = threading.Thread(target=worker, args=(device2, "d2")) | ||
|
|
||
| t1.start() | ||
| t2.start() | ||
|
|
||
| t1.join() | ||
| t2.join() | ||
| print() | ||
|
|
||
| # If either thread failed and stop_on_first_failure is set, stop. | ||
| if stop_on_first_failure and (not results.get("d1", True) or not results.get("d2", True)): | ||
| print(f"[{timestamp()}] Stopping on first failure as requested") | ||
| return False | ||
|
|
||
| # Add delay between profiles (not after the last one) | ||
| if i < len(PROFILES) - 1 and delay > 0: | ||
| print(f"[{timestamp()}] Waiting {delay}s before next stream...") | ||
| time.sleep(delay) | ||
|
|
||
| print(f"[{timestamp()}] === Repetition {rep} completed ===") | ||
| print() | ||
|
|
There was a problem hiding this comment.
run_profiles_dual() also falls off the end without returning success/failure. For consistent behavior with main() (and to avoid silently succeeding), return True when all profiles/repetitions complete and False if any stream failed (even when continuing past failures).
Comment on lines
+494
to
+499
| Profiles streamed (each with {DEFAULT_FRAMES_PER_PROFILE} frames by default): | ||
| - HD (720p): 1280x720 @ 30fps | ||
| - VGA: 640x480 @ 30fps | ||
| - 848x480: 848x480 @ 30fps | ||
| - 640x360: 640x360 @ 30fps | ||
| - 480x270: 480x270 @ 30fps |
There was a problem hiding this comment.
The help epilog lists several profiles (848x480, 640x360, 480x270) that are not present in PROFILES. This makes the CLI help misleading; either add these profiles to PROFILES or update the epilog text to match what the script will actually run.
Suggested change
| Profiles streamed (each with {DEFAULT_FRAMES_PER_PROFILE} frames by default): | |
| - HD (720p): 1280x720 @ 30fps | |
| - VGA: 640x480 @ 30fps | |
| - 848x480: 848x480 @ 30fps | |
| - 640x360: 640x360 @ 30fps | |
| - 480x270: 480x270 @ 30fps | |
| Profiles streamed (each with {DEFAULT_FRAMES_PER_PROFILE} frames by default) | |
| are taken from the PROFILES constant defined in this script. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.