Fix/ptp test change test cases - #1773
Conversation
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
…with PTP Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
📝 WalkthroughWalkthroughFunction signatures for PTP test utilities are updated to thread a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp`:
- Around line 211-214: ptpMasterDeviceName and the keys for slaveQueues are
currently using the non-unique string from deviceInfo.getXLinkDeviceDesc().name
(the local variable name); change the identifier source to the device MXID by
replacing uses of name with deviceInfo.getDeviceId() (or a new variable like
deviceId = deviceInfo.getDeviceId()) so ptpMasterDeviceName, the comparison
(ptpMasterDeviceName == deviceId), masterNode.emplace(...) and the slaveQueues
map use the guaranteed-unique MXID consistently throughout this function
(including the location where name is originally set around the earlier code).
🪄 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: a4ad85f6-76b1-4237-8684-2c605556abda
📒 Files selected for processing (3)
tests/include/fsync_ptp_test_utils.hpptests/src/onhost_tests/multi_device_ptp_test.cpptests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run_vanilla_mac_tests / mac_rvc4_test (1.27.1)
- GitHub Check: run_vanilla_mac_tests / mac_rvc4_test_usb (1.27.1)
- GitHub Check: run_windows_tests / build_windows_tests
- GitHub Check: run_tsan_tests / build_docker_container
🔇 Additional comments (3)
tests/src/onhost_tests/multi_device_ptp_test.cpp (1)
9-9: FPS test vector update aligns with the PR objective.Line 9 correctly replaces the unstable 13 FPS case with 15 FPS for this test matrix.
tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp (1)
186-187: Parameter threading forptpMasterDeviceNameis wired correctly through the call chain.The new argument is consistently propagated from
testFsynctosetupDevicetosetUpCameraSocket, which fixes the multi-sensor state-sharing gap in this flow.Also applies to: 262-263, 282-282, 338-341
tests/include/fsync_ptp_test_utils.hpp (1)
59-60: Header declarations are consistent with the implementation changes.The added
std::string& ptpMasterDeviceNameparameter is correctly reflected in both public declarations.Also applies to: 72-73
| ptpMasterDeviceName = name; | ||
| } | ||
| if (ptpMasterDeviceName == name) { | ||
| masterNode.value().emplace(dai::toString(socket), outNode); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In DepthAI, is dai::DeviceInfo::getXLinkDeviceDesc().nameguaranteed to be unique across simultaneously connected devices? If not, which identifier is guaranteed unique for device identity in tests (for examplegetDeviceId() or MXID)?
💡 Result:
No, dai::DeviceInfo::getXLinkDeviceDesc.name is not guaranteed to be unique across simultaneously connected devices. The name field in DeviceInfo (populated from XLink's deviceDesc_t.name via the DeviceInfo constructor) represents connection-specific identifiers like USB port paths (e.g., '1.3') or IP addresses (e.g., '192.168.33.201'), which are not unique—multiple devices can share USB hubs/ports or IPs in different scenarios. Official examples show different devices with unique names but emphasize MxId uniqueness. Device::getDeviceName is also unpredictable per GitHub issues. The identifier guaranteed unique for device identity is the MXID (Myriad X ID), accessed via DeviceInfo::getMxId or info.mxid. Official docs explicitly call it the "Unique Mx (chip) identification code", used to distinguish devices in multi-device setups (e.g., XLinkConnection::getDeviceByMxId(mxId)). Examples list distinct MXIDs like '18443010D116631200' for different devices. getDeviceId appears as an alias for MXID in some docs.
Citations:
- 1: http://docs.ros.org/en/noetic/api/depthai/html/structdai_1_1DeviceInfo.html
- 2: https://docs.ros.org/en/api/depthai/html/structdai_1_1DeviceInfo.html
- 3: https://docs.ros.org/en/kilted/p/depthai/generated/structdai_1_1DeviceInfo.html
- 4: https://docs.luxonis.com/software-v3/depthai/examples/device_information/
- 5: https://docs.oakchina.cn/projects/api/samples/host_side/device_information.html
- 6: https://docs.luxonis.com/software/depthai/multi-device-setup/
- 7: http://docs.ros.org/en/noetic/api/depthai/html/XLinkConnection_8hpp_source.html
- 8: https://github.com/luxonis/depthai-core/blob/main/src/device/DeviceBase.cpp
- 9: https://github.com/luxonis/depthai-python/blob/0625251d/src/XLinkBindings.cpp
- 10: https://github.com/luxonis/depthai-core/blob/ba7a920a2568ea6eaaaebf3f92bbdb40924187ae/tests/src/ondevice_tests/xlink_test.cpp
- 11: https://github.com/luxonis/depthai-core/tree/main
- 12: http://docs.ros.org/en/noetic/api/depthai/html/device__information_8cpp_source.html
- 13: https://docs.luxonis.com/software/depthai-components/device
🏁 Script executed:
# Get the context around lines 211-214
sed -n '200,225p' tests/src/onhost_tests/utility/fsync_ptp_test_utils.cppRepository: luxonis/depthai-core
Length of output: 1160
🏁 Script executed:
# Check where ptpMasterDeviceName is defined and how it's used
rg 'ptpMasterDeviceName' tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp -B 2 -A 2Repository: luxonis/depthai-core
Length of output: 1415
🏁 Script executed:
# Check what methods are available on DeviceInfo object in the file
rg 'deviceInfo\.' tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp | head -30Repository: luxonis/depthai-core
Length of output: 204
🏁 Script executed:
# Look for getDeviceId or getMxId usage in the codebase
rg 'getMxId\(\)|getDeviceId\(\)' tests/src/onhost_tests/utility/fsync_ptp_test_utils.cppRepository: luxonis/depthai-core
Length of output: 370
🏁 Script executed:
# Check how slaveQueues map is organized and used with the name identifier
sed -n '185,295p' tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp | rg -A 2 -B 2 'slaveQueues'Repository: luxonis/depthai-core
Length of output: 1590
🏁 Script executed:
# Check the overall data flow - how masterNode and slaveQueues store devices
grep -n 'masterNode\|slaveQueues' tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp | head -20Repository: luxonis/depthai-core
Length of output: 1676
Change PTP master selection to use a guaranteed-unique device identifier.
At lines 211–214, the PTP master device is selected and compared using name, which is derived from deviceInfo.getXLinkDeviceDesc().name. This value is not guaranteed to be unique across simultaneously connected devices. The verified unique identifier for device identity in DepthAI is the MXID, accessible via deviceInfo.getDeviceId(). Multiple devices sharing the same name could be misclassified as the master, breaking PTP synchronization.
Additionally, the slaveQueues map is also keyed by the same non-unique name identifier. For a complete fix, consider updating the source where name is derived (around line 271) to use getDeviceId() instead, which would consistently fix both ptpMasterDeviceName and the slave queue organization throughout this function.
Suggested approach
Change the source identifier to the unique device ID:
- std::string name = deviceInfo.getXLinkDeviceDesc().name;
+ std::string name = deviceInfo.getDeviceId();This single change will ensure both PTP master selection and device queue organization use the guaranteed-unique identifier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp` around lines 211 -
214, ptpMasterDeviceName and the keys for slaveQueues are currently using the
non-unique string from deviceInfo.getXLinkDeviceDesc().name (the local variable
name); change the identifier source to the device MXID by replacing uses of name
with deviceInfo.getDeviceId() (or a new variable like deviceId =
deviceInfo.getDeviceId()) so ptpMasterDeviceName, the comparison
(ptpMasterDeviceName == deviceId), masterNode.emplace(...) and the slaveQueues
map use the guaranteed-unique MXID consistently throughout this function
(including the location where name is originally set around the earlier code).
2 participants
Remove 13 FPS due to consistent failing.
Add 15 FPS
Fix a logic error in PTP tests when using multiple sensors per device.
Summary by CodeRabbit