Stable timestamp sync test - #14311
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the stability of timestamp synchronization tests by replacing the original test-timestamps.py with an enhanced test-sync.py that includes frame drop detection, re-synchronization logic, and runs only on nightly builds with increased tolerance thresholds.
- Implements frame drop detection using hardware frame counters to identify when synchronization may be compromised
- Adds re-synchronization mechanism that skips frames after detecting drops to allow sensors to stabilize
- Increases test duration from 50 to 100 frames and adjusts tolerance from 1ms to 1.5ms to handle observed peak variations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| unit-tests/live/metadata/test-timestamps.py | Removed original test file |
| unit-tests/live/metadata/test-sync.py | Added enhanced test with frame drop detection, re-sync logic, and nightly-only execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Tolerance for gaps between frames | ||
| TS_TOLERANCE_MS = 1.5 # in ms | ||
| TS_TOLERANCE_MICROSEC = TS_TOLERANCE_MS * 1000 # in microseconds - 1 ms |
There was a problem hiding this comment.
Comment is misleading - it states '1 ms' but the actual value is 1.5ms (TS_TOLERANCE_MS * 1000). Should be updated to reflect the correct value.
| TS_TOLERANCE_MICROSEC = TS_TOLERANCE_MS * 1000 # in microseconds - 1 ms | |
| TS_TOLERANCE_MICROSEC = TS_TOLERANCE_MS * 1000 # in microseconds - 1.5 ms (1500 microseconds) |
| # Check for frame drops | ||
| if prev_frame_counters[sensor_name] is not None: | ||
| expected_counter = prev_frame_counters[sensor_name] + 1 | ||
| if current_counter != expected_counter: |
There was a problem hiding this comment.
I think we can remove this condition to simplify the function - if they are equal, dropped_frames will be zero and no drop message will show
| if frame_drop_detected: | ||
| frames_to_skip = SKIP_FRAMES_AFTER_DROP | ||
| log.w(f"Frame drop detected at frame {frame_count}, will skip next {frames_to_skip} frames for re-sync") | ||
| log.w(f"Drop details: {'; '.join(drop_info)}") |
There was a problem hiding this comment.
Do we need to log again? we already log at detect_frame_drops if we have a drop
If we don't need to log again, we don't need drop_info here or in the function
|
|
||
| log.d(f"Frame TS - Depth:{frame_timestamps['depth']}, IR1:{frame_timestamps['ir1']}, IR2:{frame_timestamps['ir2']}, Color:{frame_timestamps['color']}") | ||
|
|
||
| test.check_approx_abs(frame_timestamps['depth'], frame_timestamps['ir1'], TS_TOLERANCE_MICROSEC) |
There was a problem hiding this comment.
Let's add a comment to distinguish between frame ts and global ts above
AviaAv
left a comment
There was a problem hiding this comment.
LGTM, let's get green libci with the test before merging
Skip dropped frames
Resync when needed
Increase run time
Run only on nightly
Increase threshold as we saw a 1.2 ms peeks occasionally