Conversation
added 30 commits
April 22, 2026 22:43
added 10 commits
May 19, 2026 13:22
Contributor
📝 WalkthroughWalkthroughImplements a telemetry subsystem with on-disk queuing and background uploader, UUIDv7/session IDs and OS helpers, integrates telemetry into Device and Pipeline lifecycles, exposes Python bindings/config, adds model‑zoo telemetry hooks, and provides end‑to‑end on‑device tests and README documentation. ChangesTelemetry Feature Implementation
Sequence Diagram(s): sequenceDiagram
participant Child as TelemetryChild
participant Device as DeviceBase
participant Telemetry as TelemetrySharedState
participant Pipeline as PipelineImpl
participant Server as LocalTelemetryServer
Child->>Device: open devices / start pipelines
Device->>Telemetry: emit events (constructor, node_created, pipeline_start)
Pipeline->>Telemetry: emit pipeline_start/stop events
Telemetry->>Server: HTTP POST uploader
Server-->>Child: captured requests
Device->>Telemetry: emit pipeline_stop / destructor
Telemetry->>Server: final POSTs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@bindings/python/src/pipeline/PipelineBindings.cpp`:
- Line 204: Duplicate bindings for Pipeline::getDefaultDevice exist in the
Python bindings; remove one of the .def("getDefaultDevice",
static_cast<std::shared_ptr<Device>
(Pipeline::*)()>(&Pipeline::getDefaultDevice), DOC(dai, Pipeline,
getDefaultDevice)) entries so the method is bound only once (keep the existing
correct binding with DOC and remove the duplicate occurrence).
In `@README.md`:
- Around line 140-141: Add a blank line immediately after the "### Telemetry"
heading to satisfy Markdown lint MD022; locate the "### Telemetry" heading (and
the paragraph that begins "We collect some anonymized telemetry...") and insert
one empty line between them so the heading is followed by a blank line while
keeping the existing text and the DEPTHAI_TELEMETRY reference unchanged.
In `@src/device/DeviceBase.cpp`:
- Around line 636-648: Wrap the entire telemetry property construction and
emission in a try/catch so any exceptions do not propagate to init2(): move the
block that builds properties (calls to lowercase(getProductName()),
lowercase(getPlatformAsString()), telemetryProtocolName(deviceInfo.protocol),
telemetryProtocolSpeed(deviceInfo.protocol, getUsbSpeed()),
isLoopbackDeviceName(deviceInfo.name)) and the subsequent getOSVersion()
assignment and dai::utility::Telemetry::getInstance().event(*this,
"depthai_device_constructor", std::move(properties)) call into a single try
block, and on catch use pimpl->logger.debug(...) to log the exception (including
ex.what()) and continue; ensure no exceptions escape this scope so telemetry is
strictly best-effort and cannot abort construction in init2().
In `@src/modelzoo/Zoo.cpp`:
- Around line 437-439: The cpr::Get call in ZooManager::fetchModelDetails lacks
a timeout and can block; read the DEPTHAI_ZOO_INTERNET_CHECK_TIMEOUT env var
(fallback 1000 ms), convert to an integer/long millisecond value, and pass it as
a cpr::Timeout (e.g., cpr::Timeout{timeout_ms}) alongside
cpr::Url{getModelDetailsEndpoint(modelId)} and makeHubHeaders(apiKey) so the
network call is bounded; update fetchModelDetails to use this timeout when
calling cpr::Get.
In `@src/utility/Telemetry.cpp`:
- Around line 467-473: The call to getTemporaryTelemetryHostId() inside
TelemetrySharedState::event can throw and must be guarded so telemetry never
propagates exceptions into product flows; wrap the host-id resolution in a
try/catch in TelemetrySharedState::event, on any exception fall back to a safe
default (e.g. empty string or "unknown") and continue building the payload, and
ensure the catch swallows or logs the error locally without rethrowing so the
payload creation and send path cannot throw due to host-id resolution.
- Around line 657-667: The retryable calculation in Telemetry.cpp incorrectly
excludes HTTP 429 and 5xx responses; update the logic that sets retryable (used
alongside response/error and shouldRetry) to treat statusCode == 429 and any 5xx
(statusCode >= 500 && statusCode <= 599) as retryable; adjust the condition
around the existing const bool retryable = ... (and any subsequent branches that
set shouldRetry or break) so transient server throttling/errors cause
shouldRetry to be true and the event is retried with backoff, while keeping the
existing handling for client errors and non-retryable statuses.
In `@tests/CMakeLists.txt`:
- Around line 541-557: Both telemetry_test and telemetry_multi_device_test bind
to the same localhost port and must be run serially; update the CMake test
registration to mark these tests as serial by setting the CTest property for
those test targets (telemetry_test and telemetry_multi_device_test) to run
serially (e.g., use set_tests_properties with PROPERTIES RUN_SERIAL TRUE for
telemetry_test and telemetry_multi_device_test) so they won't run in parallel
and race on port 8955.
In `@tests/src/ondevice_tests/telemetry_multi_device_test.cpp`:
- Around line 3-14: The file is missing the <condition_variable> header required
for std::condition_variable; add the include directive for <condition_variable>
near the other standard includes at the top of the file so the use of
std::condition_variable (and any std::unique_lock/std::mutex synchronization) in
telemetry_multi_device_test.cpp compiles correctly.
- Around line 38-45: ScopedTempDir currently ignores the constructor argument
and uses the global temp directory then removes it in the destructor; modify the
constructor of ScopedTempDir to create and own a unique subdirectory (e.g., join
dai::platform::getTempPath() with the provided name and create_directory or
create_directories) and store that full subpath in the member variable path, and
change the destructor to only remove that owned subdirectory
(std::filesystem::remove_all(path, ec)); ensure error handling for directory
creation and that the provided name is used to form the subdirectory path.
In `@tests/src/ondevice_tests/telemetry_test.cpp`:
- Around line 40-47: ScopedTempDir's constructor ignores the provided directory
name and assigns path to the global temp root, causing the destructor to remove
the shared temp root; change the constructor (ScopedTempDir::ScopedTempDir) to
join the provided name with dai::platform::getTempPath() (e.g., tempRoot /
dirName), create that test-specific directory
(std::filesystem::create_directories) and store it in the member path, and keep
the destructor (~ScopedTempDir) removing only this test-specific path via
std::filesystem::remove_all(path, ec).
🪄 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: 2c7cf6c4-942e-4235-9a2f-ad54dad0dd96
📒 Files selected for processing (43)
CMakeLists.txtREADME.mdbindings/python/src/DeviceBindings.cppbindings/python/src/pipeline/PipelineBindings.cppbindings/python/src/pipeline/node/HostNodeBindings.cppbindings/python/src/py_bindings.cppcmake/Depthai/DepthaiDeviceRVC4Config.cmakecmake/Depthai/DepthaiDeviceSideConfig.cmakeexamples/cpp/CMakeLists.txtexamples/cpp/RVC4/GPUStereo/check_device_gpu.cppexamples/cpp/Remapping/single_cam_remapping.cppinclude/depthai/device/DeviceBase.hppinclude/depthai/pipeline/DeviceNode.hppinclude/depthai/pipeline/DeviceNodeGroup.hppinclude/depthai/pipeline/InputQueue.hppinclude/depthai/pipeline/Node.hppinclude/depthai/pipeline/NodeGroup.hppinclude/depthai/pipeline/NodeObjInfo.hppinclude/depthai/pipeline/Pipeline.hppinclude/depthai/pipeline/ThreadedHostNode.hppinclude/depthai/pipeline/node/host/HostNode.hppinclude/depthai/pipeline/node/internal/PipelineStateMerge.hppinclude/depthai/xlink/XLinkConstants.hppinclude/depthai/xlink/XLinkStream.hppscripts/generate_depthai_nodes_names.pysrc/device/DeviceBase.cppsrc/modelzoo/Zoo.cppsrc/pipeline/Pipeline.cppsrc/utility/Initialization.cppsrc/utility/Platform.cppsrc/utility/Platform.hppsrc/utility/Telemetry.cppsrc/utility/Telemetry.hppsrc/utility/Uuid.cppsrc/utility/Uuid.hppsrc/utility/depthai_nodes_names.hppsrc/xlink/XLinkStream.cpptests/CMakeLists.txttests/src/ondevice_tests/gpu_availability_test.cpptests/src/ondevice_tests/telemetry_multi_device_test.cpptests/src/ondevice_tests/telemetry_multi_device_test_child.cpptests/src/ondevice_tests/telemetry_test.cpptests/src/ondevice_tests/telemetry_test_child.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-24T22:39:04.364Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:705-705
Timestamp: 2026-03-24T22:39:04.364Z
Learning: Do not flag the `!= ""` part of the auto-calibration condition as redundant when it appears in `PipelineImpl::build()` (or closely related pipeline build logic). If the code uses `utility::getEnvAs<std::string>(..., default)` with a default such as `"ON_START"`, the explicit empty-string guard may still be intentional to treat an explicitly empty env var as “OFF/disabled” (or to avoid special-casing elsewhere). Only consider removing `!= ""` if the codebase has an explicit, enforceable guarantee that `DEPTHAI_AUTOCALIBRATION` can never be set to an empty string (e.g., via validated parsing/CI checks); otherwise, keep the guard.
Applied to files:
src/pipeline/Pipeline.cpp
🪛 Cppcheck (2.20.0)
tests/src/ondevice_tests/telemetry_test_child.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)
tests/src/ondevice_tests/telemetry_multi_device_test_child.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)
src/utility/Uuid.cpp
[style] 13-13: The function 'generateUuidV7' is never used.
(unusedFunction)
tests/src/ondevice_tests/telemetry_test.cpp
[style] 135-135: The function 'getThreadLock' is never used.
(unusedFunction)
src/utility/Platform.cpp
[style] 166-166: The function 'getOSVersion' is never used.
(unusedFunction)
tests/src/ondevice_tests/telemetry_multi_device_test.cpp
[style] 135-135: The function 'getThreadLock' is never used.
(unusedFunction)
bindings/python/src/pipeline/node/HostNodeBindings.cpp
[error] 78-78: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
src/utility/Telemetry.cpp
[error] 78-78: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 140-140: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 Ruff (0.15.13)
scripts/generate_depthai_nodes_names.py
[warning] 83-83: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (40)
include/depthai/pipeline/Node.hpp (1)
727-732: LGTM!Also applies to: 755-766
include/depthai/pipeline/DeviceNode.hpp (1)
59-72: LGTM!include/depthai/pipeline/NodeGroup.hpp (1)
16-18: LGTM!include/depthai/pipeline/DeviceNodeGroup.hpp (1)
15-17: LGTM!include/depthai/pipeline/InputQueue.hpp (1)
38-40: LGTM!include/depthai/pipeline/NodeObjInfo.hpp (1)
20-20: LGTM!Also applies to: 33-33
include/depthai/pipeline/Pipeline.hpp (1)
5-5: LGTM!Also applies to: 89-89, 171-172, 597-603
include/depthai/pipeline/ThreadedHostNode.hpp (1)
35-35: LGTM!include/depthai/pipeline/node/host/HostNode.hpp (1)
64-65: LGTM!include/depthai/pipeline/node/internal/PipelineStateMerge.hpp (1)
11-11: LGTM!src/pipeline/Pipeline.cpp (2)
23-25: LGTM!Also applies to: 98-160, 199-201, 399-399, 1188-1218
1279-1289: LGTM!include/depthai/xlink/XLinkConstants.hpp (1)
15-15: LGTM!include/depthai/xlink/XLinkStream.hpp (1)
103-103: LGTM!src/xlink/XLinkStream.cpp (1)
51-60: LGTM!bindings/python/src/pipeline/node/HostNodeBindings.cpp (2)
18-29: LGTM!Also applies to: 72-78, 111-111
49-51: 💤 Low valueThe initialization is correct.
HostNodedoes not define its ownNAMEconstant and only inherits fromThreadedHostNode, which definesNAME = "HostNode". BothPyThreadedHostNodeandPyHostNodecorrectly initializenodeNamewithThreadedHostNode::NAME, and this is the only NAME available through inheritance. There is no copy-paste issue here.> Likely an incorrect or invalid review comment.bindings/python/src/pipeline/PipelineBindings.cpp (1)
294-296: LGTM!bindings/python/src/py_bindings.cpp (1)
42-60: LGTM!tests/CMakeLists.txt (1)
401-408: LGTM!tests/src/ondevice_tests/telemetry_test_child.cpp (1)
21-62: LGTM!tests/src/ondevice_tests/telemetry_multi_device_test_child.cpp (1)
48-136: LGTM!src/utility/Telemetry.hpp (1)
15-49: LGTM!src/utility/Uuid.hpp (1)
8-8: LGTM!src/utility/Uuid.cpp (1)
13-43: LGTM!src/utility/Platform.hpp (1)
33-33: LGTM!src/utility/Platform.cpp (1)
166-224: LGTM!src/utility/Initialization.cpp (1)
14-14: LGTM!Also applies to: 160-160
scripts/generate_depthai_nodes_names.py (1)
10-92: LGTM!src/utility/depthai_nodes_names.hpp (1)
1-54: LGTM!CMakeLists.txt (1)
409-410: LGTM!README.md (1)
225-227: LGTM!cmake/Depthai/DepthaiDeviceRVC4Config.cmake (1)
6-6: LGTM!cmake/Depthai/DepthaiDeviceSideConfig.cmake (1)
5-5: LGTM!examples/cpp/CMakeLists.txt (1)
152-152: LGTM!examples/cpp/RVC4/GPUStereo/check_device_gpu.cpp (1)
1-1: LGTM!Also applies to: 10-10
examples/cpp/Remapping/single_cam_remapping.cpp (1)
48-48: LGTM!tests/src/ondevice_tests/gpu_availability_test.cpp (1)
2-2: LGTM!Also applies to: 7-7, 14-17
include/depthai/device/DeviceBase.hpp (1)
10-10: LGTM!Also applies to: 290-295, 454-455, 1208-1211, 1279-1291
bindings/python/src/DeviceBindings.cpp (1)
345-351: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/utility/Telemetry.cpp`:
- Around line 106-127: generateUuidV4() currently uses a shared static
std::mt19937_64 generator and distribution without synchronization; wrap access
to the generator/distribution with a static std::mutex (e.g., static std::mutex
uuidMutex) and acquire a std::lock_guard<std::mutex> before generating bytes so
concurrent calls cannot mutate generator state; ensure <mutex> is included and
only the random-byte generation (the loop that fills bytes using
distribution(generator)) is performed while holding the lock, leaving formatting
and string construction outside the critical section.
🪄 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: 2bf3485d-1108-44f3-b51f-63b47fb88a2f
📒 Files selected for processing (3)
src/device/DeviceBase.cppsrc/modelzoo/Zoo.cppsrc/utility/Telemetry.cpp
📜 Review details
🧰 Additional context used
🪛 Cppcheck (2.20.0)
src/utility/Telemetry.cpp
[error] 78-78: There is an unknown macro here somewhere. Configuration is required. If DEPTHAI_NLOHMANN_DEFINE_TYPE_INTRUSIVE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (25)
src/device/DeviceBase.cpp (10)
94-140: LGTM!
540-555: LGTM!
557-603: LGTM!
605-619: LGTM!
621-654: LGTM!
656-678: LGTM!
899-902: LGTM!
1169-1172: LGTM!
1383-1383: LGTM!
1526-1538: LGTM!Also applies to: 1748-1751
src/modelzoo/Zoo.cpp (7)
289-297: LGTM!
299-311: LGTM!
313-324: LGTM!
423-423: LGTM!
437-449: LGTM!
451-470: LGTM!
707-707: LGTM!Also applies to: 722-724, 743-743, 783-783
src/utility/Telemetry.cpp (8)
40-55: LGTM!
156-350: LGTM!
353-428: LGTM!
430-443: LGTM!
445-521: LGTM!
543-709: LGTM!
711-739: LGTM!
741-851: LGTM!
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/CMakeLists.txt (1)
541-557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize telemetry tests that share a fixed port.
Line 542 and Line 550 register two telemetry tests, but there is still no CTest lock/serialization property; these can race under parallel execution when both bind the same localhost port.
Suggested CMake fix
dai_set_test_labels(telemetry_test ondevice rvc2_all rvc4 rvc4rgb ci) +set_tests_properties(telemetry_test PROPERTIES RESOURCE_LOCK telemetry_http_port_8955) @@ dai_set_test_labels(telemetry_multi_device_test ondevice rvc2_all rvc4 rvc4rgb) +set_tests_properties(telemetry_multi_device_test PROPERTIES RESOURCE_LOCK telemetry_http_port_8955)#!/bin/bash # Verify both telemetry tests are registered and whether a shared lock is set. rg -n -C2 'dai_add_test\(telemetry_test|dai_add_test\(telemetry_multi_device_test|RESOURCE_LOCK|RUN_SERIAL' tests/CMakeLists.txt # Verify both test sources reference the same hardcoded port (expected: 8955 in both files). rg -n -C2 '8955|localhost' tests/src/ondevice_tests/telemetry_test.cpp tests/src/ondevice_tests/telemetry_multi_device_test.cpp🤖 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 `@tests/CMakeLists.txt` around lines 541 - 557, Both telemetry_test and telemetry_multi_device_test bind the same fixed localhost port and can race under parallel CTest runs; locate the test targets telemetry_test and telemetry_multi_device_test and add a CTest serialization/resource lock so they cannot run concurrently (e.g., set_tests_properties or set_property with a shared RESOURCE_LOCK name or set RUN_SERIAL ON for both). Update the CMake entries that register telemetry_test and telemetry_multi_device_test to attach the same lock identifier (or RUN_SERIAL) so the two tests are serialized during CTest execution.
🤖 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.
Duplicate comments:
In `@tests/CMakeLists.txt`:
- Around line 541-557: Both telemetry_test and telemetry_multi_device_test bind
the same fixed localhost port and can race under parallel CTest runs; locate the
test targets telemetry_test and telemetry_multi_device_test and add a CTest
serialization/resource lock so they cannot run concurrently (e.g.,
set_tests_properties or set_property with a shared RESOURCE_LOCK name or set
RUN_SERIAL ON for both). Update the CMake entries that register telemetry_test
and telemetry_multi_device_test to attach the same lock identifier (or
RUN_SERIAL) so the two tests are serialized during CTest execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76928084-89f0-46be-9572-cf036e4cea13
📒 Files selected for processing (1)
tests/CMakeLists.txt
📜 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). (5)
- GitHub Check: run_vanilla_mac_tests / mac_rvc4_test_usb (1.27.1)
- GitHub Check: run_tsan_tests / build_docker_container
- GitHub Check: run_vanilla_tests / build_docker_container
- GitHub Check: run_windows_tests / build_windows_tests
- GitHub Check: run_vanilla_mac_tests / mac_rvc4_test (1.27.1)
🔇 Additional comments (1)
tests/CMakeLists.txt (1)
401-408: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/utility/Telemetry.cpp`:
- Around line 425-428: The libcurl global init in Telemetry.cpp uses
curl_global_init(CURL_GLOBAL_DEFAULT) guarded by `#ifdef` DEPTHAI_ENABLE_CURL but
never calls curl_global_cleanup(), leaving global state unbalanced; modify the
Telemetry class to add a boolean (e.g., curlInitialized or curlInitResultOk) set
true only when curl_global_init returns CURLE_OK, and in the Telemetry
destructor (~Telemetry) under the same `#ifdef` DEPTHAI_ENABLE_CURL check call
curl_global_cleanup() if that flag is true; ensure you reference
curl_global_init, curlInitResult (or your chosen flag), curl_global_cleanup, and
the Telemetry destructor so the init/cleanup lifecycle is balanced.
🪄 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: 44c64613-d117-4ea1-a691-60d88b3a5344
📒 Files selected for processing (2)
src/pipeline/Pipeline.cppsrc/utility/Telemetry.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-24T22:39:04.364Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:705-705
Timestamp: 2026-03-24T22:39:04.364Z
Learning: Do not flag the `!= ""` part of the auto-calibration condition as redundant when it appears in `PipelineImpl::build()` (or closely related pipeline build logic). If the code uses `utility::getEnvAs<std::string>(..., default)` with a default such as `"ON_START"`, the explicit empty-string guard may still be intentional to treat an explicitly empty env var as “OFF/disabled” (or to avoid special-casing elsewhere). Only consider removing `!= ""` if the codebase has an explicit, enforceable guarantee that `DEPTHAI_AUTOCALIBRATION` can never be set to an empty string (e.g., via validated parsing/CI checks); otherwise, keep the guard.
Applied to files:
src/pipeline/Pipeline.cpp
🔇 Additional comments (2)
src/pipeline/Pipeline.cpp (1)
1282-1297: LGTM!src/utility/Telemetry.cpp (1)
35-35: LGTM!
Comment on lines
+425
to
+428
| const auto curlInitResult = curl_global_init(CURL_GLOBAL_DEFAULT); | ||
| if(curlInitResult != CURLE_OK) { | ||
| logger::debug("Telemetry curl global init failed: {}", curl_easy_strerror(curlInitResult)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'Telemetry.cpp' --exec rg -n 'curl_global_init|curl_global_cleanup|TelemetrySharedState::~TelemetrySharedState'Repository: luxonis/depthai-core
Length of output: 191
🏁 Script executed:
cat -n src/utility/Telemetry.cpp | sed -n '420,450p'Repository: luxonis/depthai-core
Length of output: 1048
🏁 Script executed:
rg 'curl_global_cleanup' --type cppRepository: luxonis/depthai-core
Length of output: 46
🏁 Script executed:
cat -n src/utility/Telemetry.cpp | sed -n '410,435p'Repository: luxonis/depthai-core
Length of output: 1052
🏁 Script executed:
rg -n 'struct TelemetrySharedState' src/utility/Telemetry.cpp -A 20Repository: luxonis/depthai-core
Length of output: 785
🏁 Script executed:
rg -n 'struct TelemetrySharedState' src/utility/Telemetry.cpp -A 50 | head -60Repository: luxonis/depthai-core
Length of output: 1865
Balance libcurl global lifecycle calls.
Line 425 initializes libcurl globally with curl_global_init(CURL_GLOBAL_DEFAULT) within the #ifdef DEPTHAI_ENABLE_CURL guard, but the destructor (line 436) does not call curl_global_cleanup(). This leaves the global libcurl state unbalanced.
Add a flag to track successful initialization and call curl_global_cleanup() in the destructor within the same conditional guard:
Suggested fix
struct TelemetrySharedState {
bool enabled{false};
@@
std::optional<Clock::time_point> pausedUntil;
+#ifdef DEPTHAI_ENABLE_CURL
+ bool curlGlobalInitialized{false};
+#endif
@@
TelemetrySharedState::TelemetrySharedState() {
@@
- const auto curlInitResult = curl_global_init(CURL_GLOBAL_DEFAULT);
- if(curlInitResult != CURLE_OK) {
+ const auto curlInitResult = curl_global_init(CURL_GLOBAL_DEFAULT);
+ curlGlobalInitialized = (curlInitResult == CURLE_OK);
+ if(!curlGlobalInitialized) {
logger::debug("Telemetry curl global init failed: {}", curl_easy_strerror(curlInitResult));
}
@@
TelemetrySharedState::~TelemetrySharedState() {
@@
if(worker.joinable()) {
worker.join();
}
+
+#ifdef DEPTHAI_ENABLE_CURL
+ if(curlGlobalInitialized) {
+ curl_global_cleanup();
+ }
+#endif
cleanupRunDirectory();
}🤖 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 `@src/utility/Telemetry.cpp` around lines 425 - 428, The libcurl global init in
Telemetry.cpp uses curl_global_init(CURL_GLOBAL_DEFAULT) guarded by `#ifdef`
DEPTHAI_ENABLE_CURL but never calls curl_global_cleanup(), leaving global state
unbalanced; modify the Telemetry class to add a boolean (e.g., curlInitialized
or curlInitResultOk) set true only when curl_global_init returns CURLE_OK, and
in the Telemetry destructor (~Telemetry) under the same `#ifdef`
DEPTHAI_ENABLE_CURL check call curl_global_cleanup() if that flag is true;
ensure you reference curl_global_init, curlInitResult (or your chosen flag),
curl_global_cleanup, and the Telemetry destructor so the init/cleanup lifecycle
is balanced.
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
1 participant
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.
Purpose
Add anonymous usage telemetry so we can improve the library
Specification
Added telemetry events from depthai-core and device FW's
Dependencies & Potential Impact
no breaking changes or risks
Deployment Plan
None / not applicable
Testing & Validation
added 2 tests + manually tested on both rvc2 and rvc4 with 1 and 2 devices. Also tested inside oak app. Tested both python and C++ usage.
AI Usage
Assisted-by: Codex:gpt-5.5
Submitted code was reviewed by a human: YES
The author is taking the responsibility for the contribution: YES
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores