Examples/minimal fsync and ptp examples#1797
Conversation
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
📝 WalkthroughWalkthroughMove PTP master-name assignment into per-socket setup ( ChangesMulti-Device Frame Synchronization
Sequence Diagram(s)sequenceDiagram
participant MasterCameraOutput
participant HostSync
participant SlaveForwarder
participant DisplayLoop
MasterCameraOutput->>HostSync: push master outputs to Sync inputs
SlaveForwarder->>HostSync: forward slave frames into Sync inputs (worker threads)
HostSync->>DisplayLoop: emit synchronized MessageGroup batches
DisplayLoop->>DisplayLoop: validate timestamps and render frames
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/python/Misc/MultiDevice/external_sync_frame_sync_minimal.py`:
- Around line 142-145: The inner loop binds an unused variable camOutputQueue;
change the loop over sockets in the section that starts threads so it iterates
only over socketName (e.g., for socketName in sockets: or for socketName in
sockets.keys():) because data_collector reads the queue from the global
slaveQueues itself; keep the thread creation/keys using
f"slave_{deviceName}_{socketName}" and args=(deviceName, socketName) unchanged.
In `@examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py`:
- Line 15: The validation threshold variable syncThresholdSec is hardcoded to
1e-3 (1ms) while the sync node uses a threshold computed from targetFps
(1000/(2*targetFps) ms), causing mismatches; change syncThresholdSec to use the
same expression (in seconds) as the sync node by setting syncThresholdSec = 1.0
/ (2 * targetFps) (or otherwise compute 1000/(2*targetFps)/1000) so both the
sync node grouping logic and the validation use the identical threshold tied to
targetFps.
- Around line 51-95: After building pipelines for all deviceInfos, validate that
a master camera was actually found by checking that masterNode is not None and
contains entries (e.g., len(masterNode) > 0); if the check fails, raise a clear
RuntimeError like "No valid cameras found (all devices filtered or no cameras
connected)" so the later iteration over masterNode.items() won't crash with a
confusing error. Place this check after the loop that sets
masterPipeline/masterNode (i.e., after the for deviceInfo in deviceInfos block)
and before any code that iterates masterNode.items(); update any message to
reference device discovery variables (deviceInfos, masterPipeline, masterName)
for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3eb01b4-0a73-4180-a36f-6d261b2312fa
📒 Files selected for processing (6)
examples/cpp/Misc/MultiDevice/multi_device_frame_sync.cppexamples/python/Misc/MultiDevice/external_sync_frame_sync_minimal.pyexamples/python/Misc/MultiDevice/multi_device_frame_sync.pyexamples/python/Misc/MultiDevice/ptp_frame_sync_minimal.pytests/include/fsync_ptp_test_utils.hpptests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.12)
examples/python/Misc/MultiDevice/external_sync_frame_sync_minimal.py
[warning] 3-3: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 18-18: Unused function argument: sig
(ARG001)
[warning] 18-18: Unused function argument: frame
(ARG001)
[warning] 19-19: Using the global statement to update running is discouraged
(PLW0603)
[warning] 25-25: Use sys.exit() instead of exit
Replace exit with sys.exit()
(PLR1722)
[warning] 85-85: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 94-94: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 97-97: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 117-117: When using only the keys of a dict use the keys() method
Replace .items() with .keys()
(PERF102)
[warning] 137-137: When using only the values of a dict use the values() method
Replace .items() with .values()
(PERF102)
[warning] 143-143: Loop control variable camOutputQueue not used within loop body
Rename unused camOutputQueue to _camOutputQueue
(B007)
[warning] 143-143: When using only the keys of a dict use the keys() method
Replace .items() with .keys()
(PERF102)
[warning] 177-177: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py
[warning] 3-3: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 18-18: Unused function argument: sig
(ARG001)
[warning] 18-18: Unused function argument: frame
(ARG001)
[warning] 19-19: Using the global statement to update running is discouraged
(PLW0603)
[warning] 25-25: Use sys.exit() instead of exit
Replace exit with sys.exit()
(PLR1722)
[warning] 66-66: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 114-114: When using only the keys of a dict use the keys() method
Replace .items() with .keys()
(PERF102)
[warning] 134-134: When using only the values of a dict use the values() method
Replace .items() with .values()
(PERF102)
[warning] 140-140: Loop control variable camOutputQueue not used within loop body
Rename unused camOutputQueue to _camOutputQueue
(B007)
[warning] 140-140: When using only the keys of a dict use the keys() method
Replace .items() with .keys()
(PERF102)
[warning] 174-174: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
examples/python/Misc/MultiDevice/multi_device_frame_sync.py
[warning] 111-111: Using the global statement to update masterNode is discouraged
(PLW0603)
[warning] 111-111: Using the global statement to update masterNode is discouraged
(PLW0603)
[warning] 111-111: Using global for slaveQueues but no assignment is done
(PLW0602)
[warning] 111-111: Using global for camSockets but no assignment is done
(PLW0602)
[warning] 111-111: Using global for syncType but no assignment is done
(PLW0602)
[warning] 111-111: Using the global statement to update masterName is discouraged
(PLW0603)
🔇 Additional comments (12)
tests/include/fsync_ptp_test_utils.hpp (1)
59-60: LGTM!tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp (1)
186-187: LGTM!Also applies to: 211-214, 281-281
examples/cpp/Misc/MultiDevice/multi_device_frame_sync.cpp (1)
139-140: LGTM!Also applies to: 166-168, 221-221
examples/python/Misc/MultiDevice/multi_device_frame_sync.py (1)
111-111: LGTM!Also applies to: 134-136
examples/python/Misc/MultiDevice/external_sync_frame_sync_minimal.py (4)
148-175: LGTM!
93-97: LGTM!
99-123: LGTM!
53-54: ThePipeline(Device(...))pattern is valid API usage, but this specific implementation is redundant.While
dai.Pipeline()can accept a Device object (as shown in other examples likeSegmentation/semantic_segmentation.py), the inline creation followed bygetDefaultDevice()is unusual. The standard pattern used elsewhere in the codebase is to create the device first, then pass it to the pipeline:device = dai.Device(deviceInfo) pipeline = dai.Pipeline(device)Instead of:
pipeline = dai.Pipeline(dai.Device(deviceInfo)) device = pipeline.getDefaultDevice()Simplify by adopting the conventional approach above to improve clarity.
examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py (4)
29-34: LGTM!
78-94: LGTM!
123-131: LGTM!
174-176: LGTM!
| for deviceName, sockets in slaveQueues.items(): | ||
| for socketName, camOutputQueue in sockets.items(): | ||
| threads[f"slave_{deviceName}_{socketName}"] = threading.Thread(target=data_collector, args=(deviceName, socketName)) | ||
| threads[f"slave_{deviceName}_{socketName}"].start() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove unused loop variable camOutputQueue.
The variable camOutputQueue is bound in the loop but never used. The worker function retrieves the queue from slaveQueues directly (line 128).
♻️ Simplify the loop
threads = {}
for deviceName, sockets in slaveQueues.items():
- for socketName, camOutputQueue in sockets.items():
+ for socketName in sockets.keys():
threads[f"slave_{deviceName}_{socketName}"] = threading.Thread(target=data_collector, args=(deviceName, socketName))
threads[f"slave_{deviceName}_{socketName}"].start()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for deviceName, sockets in slaveQueues.items(): | |
| for socketName, camOutputQueue in sockets.items(): | |
| threads[f"slave_{deviceName}_{socketName}"] = threading.Thread(target=data_collector, args=(deviceName, socketName)) | |
| threads[f"slave_{deviceName}_{socketName}"].start() | |
| for deviceName, sockets in slaveQueues.items(): | |
| for socketName in sockets.keys(): | |
| threads[f"slave_{deviceName}_{socketName}"] = threading.Thread(target=data_collector, args=(deviceName, socketName)) | |
| threads[f"slave_{deviceName}_{socketName}"].start() |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 143-143: Loop control variable camOutputQueue not used within loop body
Rename unused camOutputQueue to _camOutputQueue
(B007)
[warning] 143-143: When using only the keys of a dict use the keys() method
Replace .items() with .keys()
(PERF102)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/python/Misc/MultiDevice/external_sync_frame_sync_minimal.py` around
lines 142 - 145, The inner loop binds an unused variable camOutputQueue; change
the loop over sockets in the section that starts threads so it iterates only
over socketName (e.g., for socketName in sockets: or for socketName in
sockets.keys():) because data_collector reads the queue from the global
slaveQueues itself; keep the thread creation/keys using
f"slave_{deviceName}_{socketName}" and args=(deviceName, socketName) unchanged.
| deviceInfos = dai.Device.getAllAvailableDevices() | ||
| targetFps = 30 | ||
| resolution = (640, 480) | ||
| syncThresholdSec = 1e-3 # 1ms |
There was a problem hiding this comment.
Critical threshold mismatch between sync node and validation.
The sync node is configured with a threshold of 1000 / (2 * targetFps) milliseconds (~16.67ms for 30fps) at line 102, but the validation logic at line 157 uses syncThresholdSec = 1e-3 seconds (1ms). This means the sync node will group frames that are up to 16.67ms apart, but the validation will immediately reject them as "sync lost" if they differ by more than 1ms. Most synchronized frame groups will fail validation.
🐛 Proposed fix to align thresholds
Option 1: Use the same threshold for both (recommended):
-syncThresholdSec = 1e-3 # 1ms
+syncThresholdSec = (1000 / (2 * targetFps)) * 1e-3 # 1/2 frame period in secondsOption 2: Set both to 1ms if tighter sync is required:
-syncNode.setSyncThreshold(timedelta(milliseconds=1000 / (2 * targetFps)))
+syncNode.setSyncThreshold(timedelta(milliseconds=1))Also applies to: 102-102, 156-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py` at line 15, The
validation threshold variable syncThresholdSec is hardcoded to 1e-3 (1ms) while
the sync node uses a threshold computed from targetFps (1000/(2*targetFps) ms),
causing mismatches; change syncThresholdSec to use the same expression (in
seconds) as the sync node by setting syncThresholdSec = 1.0 / (2 * targetFps)
(or otherwise compute 1000/(2*targetFps)/1000) so both the sync node grouping
logic and the validation use the identical threshold tied to targetFps.
| for deviceInfo in deviceInfos: | ||
| # Create pipeline for each device | ||
| devicePipeline = stack.enter_context(dai.Pipeline(dai.Device(deviceInfo))) | ||
| device = devicePipeline.getDefaultDevice() | ||
| deviceName = getDeviceName(device) | ||
|
|
||
| for socket in device.getConnectedCameras(): | ||
| ######################################################################## | ||
| # TODO: remove this when OV9282 is supporter for PTP | ||
| sensorName = "" | ||
| for sckt, sName in device.getCameraSensorNames().items(): | ||
| if sckt == socket: | ||
| sensorName = sName | ||
| break | ||
| if sensorName == "": | ||
| raise RuntimeError(f"No sensor name found for {socket.name} on {deviceName}") | ||
| if sensorName == "OV9282": | ||
| continue | ||
| ######################################################################## | ||
|
|
||
| # create a queue for each camera on the device | ||
| cam = devicePipeline.create(dai.node.Camera).build(socket, sensorFps=targetFps) | ||
| outputNode = cam.requestOutput(resolution, dai.ImgFrame.Type.NV12, dai.ImgResizeMode.CROP) | ||
|
|
||
| # Set sync mode to PTP | ||
| cam.initialControl.setFrameSyncMode(dai.CameraControl.FrameSyncMode.TIME_PTP) | ||
|
|
||
| # Put the first camera in master | ||
| # Actual PTP master might be different, but it doesn't matter for this example | ||
| if masterNode is None: | ||
| masterNode = {} | ||
| masterName = deviceName | ||
|
|
||
| if masterName == deviceName: | ||
| masterNode[socket.name] = outputNode | ||
| else: | ||
| if slaveQueues.get(deviceName) is None: | ||
| slaveQueues[deviceName] = {} | ||
| slaveQueues[deviceName][socket.name] = outputNode.createOutputQueue() | ||
|
|
||
| if masterPipeline is None: | ||
| masterPipeline = devicePipeline | ||
| else: | ||
| slavePipelines[deviceName] = devicePipeline | ||
|
|
There was a problem hiding this comment.
Consider validating that at least one camera was found.
If all devices have no cameras or only OV9282 sensors (filtered out at line 68), masterNode will remain None, causing a confusing error at line 105 when iterating masterNode.items(). For a minimal example, adding a simple check would improve the user experience.
🛡️ Proposed validation
else:
slavePipelines[deviceName] = devicePipeline
+ if masterNode is None:
+ raise RuntimeError("No compatible cameras found on any device")
+
# Create sync node
syncNode = masterPipeline.create(dai.node.Sync)🧰 Tools
🪛 Ruff (0.15.12)
[warning] 66-66: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py` around lines 51 -
95, After building pipelines for all deviceInfos, validate that a master camera
was actually found by checking that masterNode is not None and contains entries
(e.g., len(masterNode) > 0); if the check fails, raise a clear RuntimeError like
"No valid cameras found (all devices filtered or no cameras connected)" so the
later iteration over masterNode.items() won't crash with a confusing error.
Place this check after the loop that sets masterPipeline/masterNode (i.e., after
the for deviceInfo in deviceInfos block) and before any code that iterates
masterNode.items(); update any message to reference device discovery variables
(deviceInfos, masterPipeline, masterName) for clarity.
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/cpp/Misc/MultiDevice/external_sync_frame_sync_minimal.cpp`:
- Line 25: The validation constant SYNC_THRESHOLD_SEC is 1e-3 but the sync node
uses a ~16.67ms threshold, causing false failures; fix by making both use the
same value — either set SYNC_THRESHOLD_SEC to 1.0/60.0 (≈0.016667) or replace
the literal threshold in the sync node setup with SYNC_THRESHOLD_SEC so the sync
node and the validation (SYNC_THRESHOLD_SEC) are identical.
- Around line 196-199: The loop over outputNames copies each string into the
loop variable; change the range-for to use a const reference (e.g., for(const
auto& name : outputNames)) in the block where tsValues is filled (the loop that
calls latestFrameGroup.value()->get<dai::ImgFrame>(name) and
tsValues.emplace(name, ...)) to avoid unnecessary string copies while preserving
usage of name as the map key.
In `@examples/cpp/Misc/MultiDevice/ptp_frame_sync_minimal.cpp`:
- Line 26: The validation uses SYNC_THRESHOLD_SEC (1e-3) while the sync node
groups frames using 0.5 / TARGET_FPS (~16.67ms at 30fps), causing mismatched
acceptance criteria; update either the sync grouping or the validation to use
the same threshold constant. Specifically, make the sync validation compare
timestamps against the same value used by the sync node (use 0.5 / TARGET_FPS
instead of SYNC_THRESHOLD_SEC*1e6 in the validation), or alternatively change
the sync node to use SYNC_THRESHOLD_SEC when grouping frames so both the sync
logic and the validation reference the same symbol (SYNC_THRESHOLD_SEC or the
expression 0.5 / TARGET_FPS). Ensure units match (seconds vs microseconds) when
replacing the value.
- Around line 191-194: The loop copies each string when iterating outputNames;
change the range-for to use a const reference (e.g., const auto& name) so you
avoid unnecessary string copies when calling
latestFrameGroup.value()->get<dai::ImgFrame>(name) and tsValues.emplace(name,
...). Update the iterator variable in the loop that populates tsValues and
ensure any subsequent uses in that scope continue to treat name as a const
reference.
- Around line 100-117: The loop can finish without selecting any master
(masterOutputs unset or empty) causing undefined behavior when dereferencing
masterOutputs/masterName or using masterPipeline; after the device-iteration
loop, add a validation that masterOutputs.has_value() and masterPipeline !=
nullptr (and that masterOutputs->size() > 0 and masterName has a value) and if
the check fails log a clear error and exit/throw (or return) to stop execution;
also add defensive checks before dereferencing masterName/masterOutputs (e.g.,
ensure masterName && masterOutputs.has_value()) where they are used (symbols:
masterOutputs, masterName, masterPipeline, slaveQueues, slavePipelines,
deviceName, socketName, output).
In `@examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py`:
- Around line 27-35: getSensorName currently references an undefined deviceName
and also never returns the found sensor string; update getSensorName to derive
the device identifier from the passed device object (e.g., use device.getMxId()
or str(device)) when composing the RuntimeError message instead of deviceName,
and ensure the function returns sensorName after the loop; locate references to
getSensorName, the device parameter, and device.getCameraSensorNames() to make
these edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36ae4f38-c9a7-4238-9bac-eef3b5677062
📒 Files selected for processing (4)
examples/cpp/Misc/MultiDevice/CMakeLists.txtexamples/cpp/Misc/MultiDevice/external_sync_frame_sync_minimal.cppexamples/cpp/Misc/MultiDevice/ptp_frame_sync_minimal.cppexamples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py
📜 Review details
🧰 Additional context used
🪛 Cppcheck (2.20.0)
examples/cpp/Misc/MultiDevice/external_sync_frame_sync_minimal.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
examples/cpp/Misc/MultiDevice/ptp_frame_sync_minimal.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.15.13)
examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py
[warning] 34-34: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py (3)
15-15: Threshold mismatch between sync node and validation (previously flagged).The sync node threshold at line 102 (~16.67ms) does not match the validation threshold
syncThresholdSecat line 15 (1ms). This issue was already raised in a previous review.Also applies to: 102-102, 159-159
80-95: Missing validation for no compatible cameras (previously flagged).If all devices have only OV9282 sensors,
masterNodewill remainNone, causing an error at line 105. This issue was already raised in a previous review.
146-171: LGTM!examples/cpp/Misc/MultiDevice/external_sync_frame_sync_minimal.cpp (2)
116-122: LGTM!
49-114: LGTM!Also applies to: 152-182, 184-232
examples/cpp/Misc/MultiDevice/CMakeLists.txt (1)
7-9: LGTM!
|
|
||
| constexpr float TARGET_FPS = 30.0f; | ||
| constexpr std::pair<uint32_t, uint32_t> RESOLUTION{640, 480}; | ||
| constexpr double SYNC_THRESHOLD_SEC = 1e-3; |
There was a problem hiding this comment.
Critical threshold mismatch between sync node and validation.
Same issue as ptp_frame_sync_minimal.cpp: the sync node threshold at line 129 (~16.67ms) does not match the validation threshold SYNC_THRESHOLD_SEC (1ms) used at line 208. Most synchronized frames will fail validation.
🐛 Proposed fix: align both thresholds
-constexpr double SYNC_THRESHOLD_SEC = 1e-3;
+constexpr double SYNC_THRESHOLD_SEC = 0.5 / TARGET_FPS;Also applies to: 129-129, 208-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/cpp/Misc/MultiDevice/external_sync_frame_sync_minimal.cpp` at line
25, The validation constant SYNC_THRESHOLD_SEC is 1e-3 but the sync node uses a
~16.67ms threshold, causing false failures; fix by making both use the same
value — either set SYNC_THRESHOLD_SEC to 1.0/60.0 (≈0.016667) or replace the
literal threshold in the sync node setup with SYNC_THRESHOLD_SEC so the sync
node and the validation (SYNC_THRESHOLD_SEC) are identical.
| for(auto name : outputNames) { | ||
| auto frame = latestFrameGroup.value()->get<dai::ImgFrame>(name); | ||
| tsValues.emplace(name, frame->getTimestamp(dai::CameraExposureOffset::END)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use const reference to avoid string copies.
Same issue as ptp_frame_sync_minimal.cpp - name is copied by value.
♻️ Proposed fix
- for(auto name : outputNames) {
+ for(const auto& name : outputNames) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for(auto name : outputNames) { | |
| auto frame = latestFrameGroup.value()->get<dai::ImgFrame>(name); | |
| tsValues.emplace(name, frame->getTimestamp(dai::CameraExposureOffset::END)); | |
| } | |
| for(const auto& name : outputNames) { | |
| auto frame = latestFrameGroup.value()->get<dai::ImgFrame>(name); | |
| tsValues.emplace(name, frame->getTimestamp(dai::CameraExposureOffset::END)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/cpp/Misc/MultiDevice/external_sync_frame_sync_minimal.cpp` around
lines 196 - 199, The loop over outputNames copies each string into the loop
variable; change the range-for to use a const reference (e.g., for(const auto&
name : outputNames)) in the block where tsValues is filled (the loop that calls
latestFrameGroup.value()->get<dai::ImgFrame>(name) and tsValues.emplace(name,
...)) to avoid unnecessary string copies while preserving usage of name as the
map key.
|
|
||
| constexpr float TARGET_FPS = 30.0f; | ||
| constexpr std::pair<uint32_t, uint32_t> RESOLUTION{640, 480}; | ||
| constexpr double SYNC_THRESHOLD_SEC = 1e-3; |
There was a problem hiding this comment.
Critical threshold mismatch between sync node and validation.
The sync node at line 124 uses a threshold of 0.5 / TARGET_FPS (~16.67ms at 30fps), but the validation at line 203 uses SYNC_THRESHOLD_SEC * 1e6 (1ms). The sync node will group frames up to 16.67ms apart, but validation will reject anything over 1ms as "sync lost", causing most synchronized frames to fail validation.
🐛 Proposed fix: align both thresholds
Option 1 - Use the same threshold for both:
-constexpr double SYNC_THRESHOLD_SEC = 1e-3;
+constexpr double SYNC_THRESHOLD_SEC = 0.5 / TARGET_FPS;Option 2 - Set both to 1ms if tighter sync is desired:
- sync->setSyncThreshold(std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::duration<double>(0.5 / TARGET_FPS)));
+ sync->setSyncThreshold(std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::duration<double>(SYNC_THRESHOLD_SEC)));Also applies to: 124-124, 203-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/cpp/Misc/MultiDevice/ptp_frame_sync_minimal.cpp` at line 26, The
validation uses SYNC_THRESHOLD_SEC (1e-3) while the sync node groups frames
using 0.5 / TARGET_FPS (~16.67ms at 30fps), causing mismatched acceptance
criteria; update either the sync grouping or the validation to use the same
threshold constant. Specifically, make the sync validation compare timestamps
against the same value used by the sync node (use 0.5 / TARGET_FPS instead of
SYNC_THRESHOLD_SEC*1e6 in the validation), or alternatively change the sync node
to use SYNC_THRESHOLD_SEC when grouping frames so both the sync logic and the
validation reference the same symbol (SYNC_THRESHOLD_SEC or the expression 0.5 /
TARGET_FPS). Ensure units match (seconds vs microseconds) when replacing the
value.
| if(!masterOutputs.has_value()) { | ||
| masterOutputs.emplace(); | ||
| masterName = deviceName; | ||
| } | ||
|
|
||
| if(*masterName == deviceName) { | ||
| (*masterOutputs)[socketName] = output; | ||
| } else { | ||
| slaveQueues[deviceName][socketName] = output->createOutputQueue(); | ||
| } | ||
| } | ||
|
|
||
| if(masterPipeline == nullptr) { | ||
| masterPipeline = pipeline; | ||
| } else { | ||
| slavePipelines[deviceName] = pipeline; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add validation for no compatible cameras found.
If all devices have only OV9282 sensors (filtered at line 86-88), masterOutputs will remain unset, and dereferencing it at line 127 will cause undefined behavior. Similarly, masterPipeline could be set but masterOutputs could be empty.
🛡️ Proposed validation
}
+ if(!masterOutputs.has_value() || masterOutputs->empty()) {
+ throw std::runtime_error("No compatible cameras found on any device");
+ }
+
// Create sync node
auto sync = masterPipeline->create<dai::node::Sync>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/cpp/Misc/MultiDevice/ptp_frame_sync_minimal.cpp` around lines 100 -
117, The loop can finish without selecting any master (masterOutputs unset or
empty) causing undefined behavior when dereferencing masterOutputs/masterName or
using masterPipeline; after the device-iteration loop, add a validation that
masterOutputs.has_value() and masterPipeline != nullptr (and that
masterOutputs->size() > 0 and masterName has a value) and if the check fails log
a clear error and exit/throw (or return) to stop execution; also add defensive
checks before dereferencing masterName/masterOutputs (e.g., ensure masterName &&
masterOutputs.has_value()) where they are used (symbols: masterOutputs,
masterName, masterPipeline, slaveQueues, slavePipelines, deviceName, socketName,
output).
| for(auto name : outputNames) { | ||
| auto frame = latestFrameGroup.value()->get<dai::ImgFrame>(name); | ||
| tsValues.emplace(name, frame->getTimestamp(dai::CameraExposureOffset::END)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use const reference to avoid string copies.
name is copied by value on each iteration. Use const auto& for efficiency.
♻️ Proposed fix
- for(auto name : outputNames) {
+ for(const auto& name : outputNames) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/cpp/Misc/MultiDevice/ptp_frame_sync_minimal.cpp` around lines 191 -
194, The loop copies each string when iterating outputNames; change the
range-for to use a const reference (e.g., const auto& name) so you avoid
unnecessary string copies when calling
latestFrameGroup.value()->get<dai::ImgFrame>(name) and tsValues.emplace(name,
...). Update the iterator variable in the loop that populates tsValues and
ensure any subsequent uses in that scope continue to treat name as a const
reference.
| def getSensorName(device: dai.Device, socket: dai.CameraBoardSocket) -> str: | ||
| sensorName = "" | ||
| for sckt, sName in device.getCameraSensorNames().items(): | ||
| if sckt == socket: | ||
| sensorName = sName | ||
| break | ||
| if sensorName == "": | ||
| raise RuntimeError(f"No sensor name found for {socket.name} on {deviceName}") | ||
|
|
There was a problem hiding this comment.
Bug: deviceName is undefined in function scope.
Line 34 references deviceName which is not defined within getSensorName. This will raise a NameError at runtime. The function should derive the device name from the device parameter.
🐛 Proposed fix
def getSensorName(device: dai.Device, socket: dai.CameraBoardSocket) -> str:
sensorName = ""
for sckt, sName in device.getCameraSensorNames().items():
if sckt == socket:
sensorName = sName
break
if sensorName == "":
- raise RuntimeError(f"No sensor name found for {socket.name} on {deviceName}")
+ raise RuntimeError(f"No sensor name found for {socket.name} on {getDeviceName(device)}")
+ return sensorNameNote: The function also has a missing return statement for the success case.
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 34-34: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/python/Misc/MultiDevice/ptp_frame_sync_minimal.py` around lines 27 -
35, getSensorName currently references an undefined deviceName and also never
returns the found sensor string; update getSensorName to derive the device
identifier from the passed device object (e.g., use device.getMxId() or
str(device)) when composing the RuntimeError message instead of deviceName, and
ensure the function returns sensorName after the loop; locate references to
getSensorName, the device parameter, and device.getCameraSensorNames() to make
these edits.
1 participant
Add minimal examples for ptp and fsync for docs
Summary by CodeRabbit
New Features
Improvements
Tests