Skip to content

Fix/ptp test change test cases #1773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sb-lxn wants to merge 2 commits into
base: develop
Choose a base branch
from
+16 −7
Open

Fix/ptp test change test cases #1773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42715a2
Remove 13 FPS and add 15 FPS
sb-lxn Apr 28, 2026
31d6ff5
Fix logic error in ptp tests, when using multiple sensors per camera …
sb-lxn Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions tests/include/fsync_ptp_test_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ void setUpCameraSocket(std::shared_ptr<dai::Pipeline>& pipeline,
std::optional<dai::ExternalFrameSyncRole> role,
std::optional<std::map<std::string, dai::Node::Output*>>& masterNode,
std::map<std::string, std::map<std::string, std::shared_ptr<dai::MessageQueue>>>& slaveQueues,
std::vector<std::string>& camSockets);
std::vector<std::string>& camSockets,
std::string &ptpMasterDeviceName);

void setUpIrLeds(std::shared_ptr<dai::Device> device);

Expand All @@ -68,6 +69,7 @@ void setupDevice(dai::DeviceInfo& deviceInfo,
std::map<std::string, std::map<std::string, std::shared_ptr<dai::MessageQueue>>>& slaveQueues,
std::vector<std::string>& camSockets,
float targetFps,
SyncType syncType);
SyncType syncType,
std::string &ptpMasterDeviceName);

int testFsync(float targetFps, struct FsyncTestParameters parameters);
2 changes: 1 addition & 1 deletion tests/src/onhost_tests/multi_device_ptp_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
TEST_CASE("Test Multi-device PTP frame sync with different FPS values", "[ptp]") {
// auto fps = GENERATE(10.0f, 13.0f, 18.5f, 30.0f, 60.0f, 120.0f, 240.0f, 300.0f, 600.0f);
// 60 FPS does not work as of 1.30.1
auto fps = GENERATE(10.0f, 13.0f, 18.5f, 30.0f, 45.0f);
auto fps = GENERATE(10.0f, 15.0f, 18.5f, 30.0f, 45.0f);
CAPTURE(fps);
struct FsyncTestParameters parameters {};
parameters.syncThresholdSec = 1 / (2 * fps); // lower this limit when we have better accuracy for timestamps
Expand Down
15 changes: 11 additions & 4 deletions tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ void setUpCameraSocket(std::shared_ptr<dai::Pipeline>& pipeline,
std::optional<dai::ExternalFrameSyncRole> role,
std::optional<std::map<std::string, dai::Node::Output*>>& masterNode,
std::map<std::string, std::map<std::string, std::shared_ptr<dai::MessageQueue>>>& slaveQueues,
std::vector<std::string>& camSockets) {
std::vector<std::string>& camSockets,
std::string &ptpMasterDeviceName) {
auto outNode = createPipeline(pipeline, socket, targetFps, syncType, role);

if(syncType == SyncType::EXTERNAL) {
Expand All @@ -207,6 +208,9 @@ void setUpCameraSocket(std::shared_ptr<dai::Pipeline>& pipeline,
// Actual PTP master might be different, but it doesn't matter for this test.
if(!masterNode.has_value()) {
masterNode.emplace();
ptpMasterDeviceName = name;
}
if (ptpMasterDeviceName == name) {
masterNode.value().emplace(dai::toString(socket), outNode);
Comment on lines +211 to 214

@coderabbitai coderabbitai Bot Apr 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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:

🏁 Script executed:

# Get the context around lines 211-214
sed -n '200,225p' tests/src/onhost_tests/utility/fsync_ptp_test_utils.cpp

Repository: 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 2

Repository: 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 -30

Repository: 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.cpp

Repository: 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 -20

Repository: 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).

} else {
if(slaveQueues.find(name) == slaveQueues.end()) {
Expand Down Expand Up @@ -255,7 +259,8 @@ void setupDevice(dai::DeviceInfo& deviceInfo,
std::map<std::string, std::map<std::string, std::shared_ptr<dai::MessageQueue>>>& slaveQueues,
std::vector<std::string>& camSockets,
float targetFps,
SyncType syncType) {
SyncType syncType,
std::string &ptpMasterDeviceName) {
auto pipeline = std::make_shared<dai::Pipeline>(std::make_shared<dai::Device>(deviceInfo));
auto device = pipeline->getDefaultDevice();

Expand All @@ -274,7 +279,7 @@ void setupDevice(dai::DeviceInfo& deviceInfo,
std::cout << " Num of cameras: " << device->getConnectedCameras().size() << std::endl;

for(auto socket : device->getConnectedCameras()) {
setUpCameraSocket(pipeline, socket, name, targetFps, syncType, role, masterNode, slaveQueues, camSockets);
setUpCameraSocket(pipeline, socket, name, targetFps, syncType, role, masterNode, slaveQueues, camSockets, ptpMasterDeviceName);
}

setUpIrLeds(device);
Expand Down Expand Up @@ -330,8 +335,10 @@ int testFsync(float targetFps, struct FsyncTestParameters parameters) {
std::vector<std::string> outputNames;
std::vector<std::string> camSockets;

std::string ptpMasterDeviceName = "";

for(auto deviceInfo : deviceInfos) {
setupDevice(deviceInfo, masterPipeline, masterNode, masterName, slavePipelines, slaveQueues, camSockets, targetFps, parameters.syncType);
setupDevice(deviceInfo, masterPipeline, masterNode, masterName, slavePipelines, slaveQueues, camSockets, targetFps, parameters.syncType, ptpMasterDeviceName);
}

if(masterPipeline == nullptr || !masterNode.has_value() || !masterName.has_value()) {
Expand Down
Loading