Dynamic calibration - multi sensor calibration#1839
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtends dynamic calibration from a fixed stereo (A/B) model to a flexible multi-sensor list. Adds a ChangesDynamic Calibration Multi-Sensor Refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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 |
MaticTonin
left a comment
MaticTonin
left a comment
There was a problem hiding this comment.
Overall a good PR, I like it and most of API stays almost the same it seems to me.
JakubFara
force-pushed
the
feature/dynamicCalibration_multiSensorCalibration
branch
2 times, most recently
from
eba0159 to
6ab1f10
Compare
|
Later I will separate this draft into 2 PRs:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/depthai/pipeline/node/DynamicCalibrationNode.hpp (1)
21-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo in documentation comment.
The comment starts with
/*uinstead of/**for Doxygen documentation.-/*u +/** * `@brief` Dynamic calibration node. Performs calibration check and dynamically calibrates the device */🤖 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 `@include/depthai/pipeline/node/DynamicCalibrationNode.hpp` around lines 21 - 23, Update the Doxygen comment opener in DynamicCalibrationNode.hpp so it starts with the proper /** sequence (not "/*u"); locate the block comment above the DynamicCalibrationNode declaration (the comment that begins with "/*u" and reads "`@brief` Dynamic calibration node...") and change it to a standard Doxygen doc comment /** to ensure the documentation is picked up correctly.
🤖 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 `@include/depthai/pipeline/datatype/DynamicCalibrationResults.hpp`:
- Around line 43-50: Update the deprecation comments for the legacy fields
coveragePerCellA and coveragePerCellB to use the Doxygen-style format consistent
with rotationChange (e.g., /** `@deprecated` Use coveragePerCell keyed by
CameraBoardSocket instead. */) so documentation generators pick them up
uniformly; locate the declarations of coveragePerCellA and coveragePerCellB in
DynamicCalibrationResults.hpp and replace the // `@deprecated` lines with the /**
`@deprecated` ... */ style while keeping the same message.
In `@src/pipeline/node/DynamicCalibrationNode.cpp`:
- Around line 646-650: The handler for DC::Commands::CalibrationQuality in
DynamicCalibrationNode.cpp currently only logs and returns
ErrorCode::INVALID_COMMAND; implement proper handling in the function that
processes cmd so it computes or retrieves the calibration quality and publishes
a DatatypeEnum::CalibrationQuality message to the node's qualityOutput (or, if
there is existing helper logic, call the node's computeCalibrationQuality /
evaluateCalibrationQuality routine). Use the existing command variant
(std::get<DC::Commands::CalibrationQuality>) named c, preserve any fields such
as c.force when deciding to recompute, and replace the
ErrorCode::INVALID_COMMAND return with appropriate success/failure ErrorCode
after publishing; alternatively, if this command is no longer valid,
remove/deprecate the DC::Commands::CalibrationQuality handling and the exposed
qualityOutput port to keep behavior consistent.
In `@tests/src/ondevice_tests/dynamic_calibration_test.cpp`:
- Line 485: Calls to stereoImageToMessageGroup are missing the required
CalibrationHandler parameter; update every call (including the one that
constructs 'group' using makeFilename("data/LeftCam_", 0, helper) /
makeFilename("data/RightCam_", 0, helper)) to pass the existing calibration
object (the variable named calibration) as the third argument so
stereoImageToMessageGroup(leftPath, rightPath, calibration) can use
setFrameTransformation() to set intrinsics/distortion/transform; alternatively,
if omission was intentional, add an overload or default parameter to
stereoImageToMessageGroup that accepts no handler and applies the desired
default behavior.
---
Outside diff comments:
In `@include/depthai/pipeline/node/DynamicCalibrationNode.hpp`:
- Around line 21-23: Update the Doxygen comment opener in
DynamicCalibrationNode.hpp so it starts with the proper /** sequence (not
"/*u"); locate the block comment above the DynamicCalibrationNode declaration
(the comment that begins with "/*u" and reads "`@brief` Dynamic calibration
node...") and change it to a standard Doxygen doc comment /** to ensure the
documentation is picked up correctly.
🪄 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: 39dc4bd4-a2b4-4235-9db5-6dabba8ed233
📒 Files selected for processing (19)
bindings/python/src/pipeline/datatype/DynamicCalibrationControlBindings.cppbindings/python/src/pipeline/datatype/DynamicCalibrationResultsBindings.cppbindings/python/src/pipeline/node/DynamicCalibrationBindings.cppbindings/python/tests/dynamic_calibration_test.pyexamples/cpp/DynamicCalibration/README.mdexamples/python/DynamicCalibration/README.mdexamples/python/DynamicCalibration/calibration_dynamic.pyinclude/depthai/device/CalibrationHandler.hppinclude/depthai/pipeline/datatype/DynamicCalibrationControl.hppinclude/depthai/pipeline/datatype/DynamicCalibrationResults.hppinclude/depthai/pipeline/node/DynamicCalibrationNode.hppinclude/depthai/utility/matrixOps.hppsrc/device/CalibrationHandler.cppsrc/pipeline/node/DynamicCalibrationNode.cppsrc/pipeline/node/DynamicCalibrationUtils.hppsrc/pipeline/node/Sync.cppsrc/utility/matrixOps.cpptests/src/ondevice_tests/dynamic_calibration_test.cpptests/src/onhost_tests/dynamic_calibration_test.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 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/node/Sync.cppsrc/pipeline/node/DynamicCalibrationNode.cpp
📚 Learning: 2026-05-28T13:36:26.383Z
Learnt from: moratom
Repo: luxonis/depthai-core PR: 1812
File: examples/cpp/ImageManip/image_manip_remap.cpp:51-52
Timestamp: 2026-05-28T13:36:26.383Z
Learning: In depthai-core example code, do not set `ImageManip::Backend::GPU` unconditionally. The GPU backend is only available on RVC4 (not RVC2). Prefer leaving the backend as the default, or comment out the GPU backend selection and add a clear note explaining it is RVC4-only support (so the example won’t fail or mislead on RVC2).
Applied to files:
examples/python/DynamicCalibration/calibration_dynamic.py
🪛 Cppcheck (2.20.0)
tests/src/onhost_tests/dynamic_calibration_test.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/pipeline/node/DynamicCalibrationNode.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 (60)
include/depthai/pipeline/datatype/DynamicCalibrationControl.hpp (4)
54-59: LGTM!
86-95: LGTM!
171-173: LGTM!
206-210: LGTM!bindings/python/src/pipeline/datatype/DynamicCalibrationControlBindings.cpp (4)
40-43: LGTM!
53-60: LGTM!
90-95: LGTM!
106-112: LGTM!bindings/python/tests/dynamic_calibration_test.py (5)
43-56: LGTM!
73-89: LGTM!
143-149: LGTM!
159-165: LGTM!
212-235: LGTM!tests/src/onhost_tests/dynamic_calibration_test.cpp (2)
8-19: LGTM!
28-39: LGTM!examples/python/DynamicCalibration/calibration_dynamic.py (2)
52-56: LGTM!
112-119: LGTM!include/depthai/utility/matrixOps.hpp (1)
9-9: LGTM!Also applies to: 39-39, 51-52, 57-58
src/utility/matrixOps.cpp (4)
269-274: LGTM!
436-450: LGTM!
452-464: LGTM!
466-490: LGTM!include/depthai/device/CalibrationHandler.hpp (3)
30-32: LGTM!Also applies to: 778-778
653-656: LGTM!
768-773: LGTM!src/device/CalibrationHandler.cpp (3)
612-686: LGTM!
1122-1145: LGTM!
1183-1200: LGTM!tests/src/ondevice_tests/dynamic_calibration_test.cpp (5)
554-554: Same handler omission pattern as non-housing test.Lines 554 and 560 also call
stereoImageToMessageGroupwithout the calibration handler. If intentional for testing purposes, consider adding a brief comment explaining why transformation data is omitted for these frames.Also applies to: 560-560
160-203: LGTM!
519-531: LGTM!
581-584: 💤 Low valueVery tight rotation threshold may cause flaky tests.
The housing-base test uses
thresholdRotation = 1e-7f(line 581), which is 10,000× tighter than the non-housing test's1e-3f(line 514). While synthetic data may achieve this precision, numerical differences across platforms or library versions could cause intermittent failures. Consider whether this strictness is intentional or if a slightly relaxed threshold (e.g.,1e-5f) would be more robust while still validating correctness.
601-637: LGTM!include/depthai/pipeline/datatype/DynamicCalibrationResults.hpp (2)
86-104: LGTM!
68-68: Ensurestd::hash<dai::CameraBoardSocket>(or a custom hasher) exists forunordered_mapkeys
CameraBoardSocketis a scopedenum class(include/depthai/common/CameraBoardSocket.hpp), andstd::unordered_map<CameraBoardSocket, ...>(e.g.,coveragePerCellinDynamicCalibrationResults.hpp) relies onstd::hash<CameraBoardSocket>by default. Make sure the project provides astd::hash<dai::CameraBoardSocket>specialization or uses anunordered_maphasher explicitly for portability across standard libraries.bindings/python/src/pipeline/datatype/DynamicCalibrationResultsBindings.cpp (1)
24-24: LGTM!include/depthai/pipeline/node/DynamicCalibrationNode.hpp (3)
137-149: LGTM!
105-120: LGTM!
131-131: LGTM!Also applies to: 173-173, 187-188, 205-213
src/pipeline/node/DynamicCalibrationUtils.hpp (1)
13-27: LGTM!examples/cpp/DynamicCalibration/README.md (1)
22-23: LGTM!examples/python/DynamicCalibration/README.md (1)
22-23: LGTM!src/pipeline/node/Sync.cpp (1)
52-53: LGTM!src/pipeline/node/DynamicCalibrationNode.cpp (16)
24-100: LGTM!
115-123: LGTM!
142-154: LGTM!
156-175: LGTM!
292-311: LGTM!
313-334: LGTM!
395-411: LGTM!
413-429: LGTM!
432-476: LGTM!
478-517: LGTM!
519-596: LGTM!
599-626: LGTM!
639-644: LGTM!Also applies to: 652-657, 670-682, 686-692, 699-704
711-754: LGTM!
756-780: LGTM!
348-376: RecheckkeepCameraCentersvskeptBaselineEdgessemantics (possible inversion)
DynamicCalibrationControl.hppdocumentskeepCameraCentersas “Keep the position of the cameras”, butDynamicCalibrationNode.cppbuildskeptBaselineEdgesonly whenkeepCameraCenters == false(otherwise it passesstd::nullopttodynCalibImpl.findNewCalibration). This appears counterintuitive unless the DCL API defineskeptBaselineEdgesin the opposite sense.Please confirm in the DCL
findNewCalibration(..., keptBaselineEdges)contract whether:
- passing
std::nulloptkeeps camera centers (translations) fixed, and passing a vector instead allows shifting while only constraining baseline lengths- or, if not, align the condition/parameter naming (and/or add a comment) so
keepCameraCentersmatches actual behavior.bindings/python/src/pipeline/node/DynamicCalibrationBindings.cpp (1)
34-37: LGTM!
JakubFara
force-pushed
the
feature/dynamicCalibration_multiSensorCalibration
branch
4 times, most recently
from
eb5c75c to
3355008
Compare
|
Testing observations:
so empty Mean Sampson error current and empty Rotation Difference. |
JakubFara
force-pushed
the
feature/dynamicCalibration_multiSensorCalibration
branch
8 times, most recently
from
436ca31 to
73b3759
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmake/Depthai/DepthaiDynamicCalibrationConfig.cmake`:
- Line 2: Before merging this change to DEPTHAI_DYNAMIC_CALIBRATION_VERSION with
the new hash 848ba66de1b880ce6cbcdf54232323f20743cc1a, verify that the
corresponding artifact has been published and is accessible. Confirm the
artifact is available on the artifact server for all required platform
architectures (x86_64-ubuntu-20.04, x86_64-macos, armv7l, etc.) and that builds
with DEPTHAI_DYNAMIC_CALIBRATION_SUPPORT enabled can successfully download it.
If the artifact is not yet available publicly, ensure that CI/deployment uses
the DEPTHAI_DYNAMIC_CALIBRATION_PATH override with a local archive, or wait
until the artifact is published before merging this version update.
In `@tests/src/ondevice_tests/dynamic_calibration_test.cpp`:
- Around line 333-350: Add TODO comments or issue tracking references before the
commented-out test blocks for CalibrationQuality commands (both with force=true
and force=false variations). Place these tracking comments above the
commented-out code sections to clearly document why they are disabled and
indicate that they should be re-enabled once the corresponding command handler
implementation is complete. This will help future developers understand the
intent and when these tests should be activated.
🪄 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: 6ea8ec88-9e10-45a7-bdfc-1d7edaa6247e
📒 Files selected for processing (18)
bindings/python/src/pipeline/datatype/DynamicCalibrationControlBindings.cppbindings/python/src/pipeline/datatype/DynamicCalibrationResultsBindings.cppbindings/python/src/pipeline/node/DynamicCalibrationBindings.cppbindings/python/tests/dynamic_calibration_test.pycmake/Depthai/DepthaiDynamicCalibrationConfig.cmakeexamples/cpp/DynamicCalibration/README.mdexamples/python/DynamicCalibration/README.mdexamples/python/DynamicCalibration/calibration_dynamic.pyinclude/depthai/pipeline/datatype/DynamicCalibrationControl.hppinclude/depthai/pipeline/datatype/DynamicCalibrationResults.hppinclude/depthai/pipeline/node/DynamicCalibrationNode.hppinclude/depthai/utility/matrixOps.hppsrc/pipeline/node/DynamicCalibrationNode.cppsrc/pipeline/node/DynamicCalibrationUtils.hppsrc/pipeline/node/Sync.cppsrc/utility/matrixOps.cpptests/src/ondevice_tests/dynamic_calibration_test.cpptests/src/onhost_tests/dynamic_calibration_test.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 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/node/Sync.cppsrc/pipeline/node/DynamicCalibrationNode.cpp
📚 Learning: 2026-05-28T13:36:26.383Z
Learnt from: moratom
Repo: luxonis/depthai-core PR: 1812
File: examples/cpp/ImageManip/image_manip_remap.cpp:51-52
Timestamp: 2026-05-28T13:36:26.383Z
Learning: In depthai-core example code, do not set `ImageManip::Backend::GPU` unconditionally. The GPU backend is only available on RVC4 (not RVC2). Prefer leaving the backend as the default, or comment out the GPU backend selection and add a clear note explaining it is RVC4-only support (so the example won’t fail or mislead on RVC2).
Applied to files:
examples/python/DynamicCalibration/calibration_dynamic.py
🪛 Cppcheck (2.21.0)
tests/src/onhost_tests/dynamic_calibration_test.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/pipeline/node/DynamicCalibrationNode.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 (28)
include/depthai/utility/matrixOps.hpp (1)
9-9: LGTM!Also applies to: 39-39, 51-59
src/utility/matrixOps.cpp (1)
138-147: LGTM!Also applies to: 280-285, 447-475, 477-501
bindings/python/src/pipeline/node/DynamicCalibrationBindings.cpp (1)
34-37: LGTM!examples/python/DynamicCalibration/calibration_dynamic.py (1)
54-54: LGTM!Also applies to: 112-119
include/depthai/pipeline/datatype/DynamicCalibrationControl.hpp (1)
55-59: LGTM!Also applies to: 87-95, 171-173, 206-210
bindings/python/src/pipeline/datatype/DynamicCalibrationControlBindings.cpp (1)
41-43: LGTM!Also applies to: 54-60, 94-95, 111-112
bindings/python/tests/dynamic_calibration_test.py (1)
45-55: LGTM!Also applies to: 75-88, 146-147, 162-163, 218-231, 245-253, 304-312
tests/src/onhost_tests/dynamic_calibration_test.cpp (1)
9-19: LGTM!Also applies to: 34-39
examples/cpp/DynamicCalibration/README.md (1)
22-23: LGTM!examples/python/DynamicCalibration/README.md (1)
22-23: LGTM!src/pipeline/node/DynamicCalibrationNode.cpp (8)
662-665:CalibrationQualitycommand handler remains unimplemented.This handler logs and returns
INVALID_COMMANDwhile the node exposes aqualityOutputport. This was flagged in a previous review and appears to still be pending implementation.
23-98: LGTM!
100-168: LGTM!
285-446: LGTM!
448-534: LGTM!
536-642: LGTM!
644-661: LGTM!Also applies to: 666-725
727-796: LGTM!include/depthai/pipeline/datatype/DynamicCalibrationResults.hpp (2)
46-71: LGTM!
90-110: LGTM!bindings/python/src/pipeline/datatype/DynamicCalibrationResultsBindings.cpp (1)
24-24: LGTM!Also applies to: 35-36, 56-57
include/depthai/pipeline/node/DynamicCalibrationNode.hpp (1)
105-120: LGTM!Also applies to: 131-149, 173-173, 204-212
src/pipeline/node/DynamicCalibrationUtils.hpp (1)
13-27: LGTM!src/pipeline/node/Sync.cpp (1)
51-54: LGTM!tests/src/ondevice_tests/dynamic_calibration_test.cpp (4)
160-203: LGTM!
480-537: LGTM!
539-601: LGTM!
603-641: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/depthai/pipeline/node/DynamicCalibrationNode.hpp (1)
21-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo in doxygen comment opening.
The comment starts with
/*uinstead of/**, which breaks doxygen documentation parsing.📝 Proposed fix
-/*u +/** * `@brief` Dynamic calibration node. Performs calibration check and dynamically calibrates the device */🤖 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 `@include/depthai/pipeline/node/DynamicCalibrationNode.hpp` around lines 21 - 23, The Doxygen comment opening for the DynamicCalibrationNode class contains a typo with `/*u` instead of the correct `/**` syntax required for Doxygen documentation parsing. Fix this by changing the opening from `/*u` to `/**` to ensure the documentation comment is properly recognized and parsed by Doxygen.src/pipeline/node/DynamicCalibrationNode.cpp (2)
508-510:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent error handling: exception thrown instead of returning error code.
Other error conditions in this class (e.g.,
runLoadImage,runCalibration,initializePipeline) returnErrorCodevalues, allowing the caller to handle failures gracefully. Throwing an exception here could crash the node instead of allowing recovery.🛠️ Proposed fix
if(!resultCoverage.passed()) { - throw std::runtime_error("Coverage check failed!"); + logger->error("Coverage check failed: {}", resultCoverage.errorMessage()); + return DynamicCalibration::ErrorCode::CALIBRATION_FAILED; }🤖 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/pipeline/node/DynamicCalibrationNode.cpp` around lines 508 - 510, The error handling in the coverage check block (where resultCoverage.passed() is checked) is inconsistent with other error handling in the DynamicCalibrationNode class. Instead of throwing a std::runtime_error exception, return an appropriate ErrorCode value like the runLoadImage, runCalibration, and initializePipeline methods do. Replace the throw statement with a return of an error code that indicates the coverage check failure, allowing callers to handle the error gracefully rather than experiencing an exception-based crash.
741-768:⚠️ Potential issue | 🟡 Minor
calibrationPeriodis only used for initial delay, not for ongoing calibration frequency.The
StartCalibrationcommand includes bothloadImagePeriodandcalibrationPeriod, documented as "Seconds between calibration cycles." However,calibrationPeriodis used only at initialization (line 782) to set the first time point:previousLoadingTimeFloat = std::chrono::steady_clock::now() + std::chrono::duration<float>(calibrationPeriod). After that initial delay,loadImagePeriod(line 748) controls both image loading and calibration frequency, notcalibrationPeriod.This means
calibrationPerioddoes not achieve its documented purpose of controlling how often calibration cycles run independently. If separate control of calibration frequency was intended, the code should checkcalibrationPeriodin the loop (similar to line 748), not just useloadImagePeriod.Additionally, after the first successful calibration (lines 765–766),
startCalibrationCommandis reset, stopping the entire process. The parameter's intended purpose of periodic calibration is not realized by the current implementation.🤖 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/pipeline/node/DynamicCalibrationNode.cpp` around lines 741 - 768, The calibrationPeriod parameter is only used at initialization for the initial delay but is not applied during the periodic calibration loop, where loadImagePeriod controls both image loading and calibration frequency. To fix this, introduce a separate timing variable for calibration (similar to previousLoadingAndCalibrationTime) and add an independent elapsed time check against calibrationPeriod before calling runCalibration. Additionally, the startCalibrationCommand.reset() call after successful calibration stops the periodic process prematurely; modify the logic to not reset the command so that calibration continues to run periodically based on calibrationPeriod until explicitly stopped by external request.
🤖 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.
Outside diff comments:
In `@include/depthai/pipeline/node/DynamicCalibrationNode.hpp`:
- Around line 21-23: The Doxygen comment opening for the DynamicCalibrationNode
class contains a typo with `/*u` instead of the correct `/**` syntax required
for Doxygen documentation parsing. Fix this by changing the opening from `/*u`
to `/**` to ensure the documentation comment is properly recognized and parsed
by Doxygen.
In `@src/pipeline/node/DynamicCalibrationNode.cpp`:
- Around line 508-510: The error handling in the coverage check block (where
resultCoverage.passed() is checked) is inconsistent with other error handling in
the DynamicCalibrationNode class. Instead of throwing a std::runtime_error
exception, return an appropriate ErrorCode value like the runLoadImage,
runCalibration, and initializePipeline methods do. Replace the throw statement
with a return of an error code that indicates the coverage check failure,
allowing callers to handle the error gracefully rather than experiencing an
exception-based crash.
- Around line 741-768: The calibrationPeriod parameter is only used at
initialization for the initial delay but is not applied during the periodic
calibration loop, where loadImagePeriod controls both image loading and
calibration frequency. To fix this, introduce a separate timing variable for
calibration (similar to previousLoadingAndCalibrationTime) and add an
independent elapsed time check against calibrationPeriod before calling
runCalibration. Additionally, the startCalibrationCommand.reset() call after
successful calibration stops the periodic process prematurely; modify the logic
to not reset the command so that calibration continues to run periodically based
on calibrationPeriod until explicitly stopped by external request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2a6a88c-4ad8-420e-8deb-142598f7efaa
📒 Files selected for processing (2)
include/depthai/pipeline/node/DynamicCalibrationNode.hppsrc/pipeline/node/DynamicCalibrationNode.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/node/DynamicCalibrationNode.cpp
🔇 Additional comments (15)
include/depthai/pipeline/node/DynamicCalibrationNode.hpp (5)
105-106: LGTM!Also applies to: 117-120
131-150: LGTM!
163-163: LGTM!
172-174: LGTM!
205-213: LGTM!src/pipeline/node/DynamicCalibrationNode.cpp (10)
23-98: LGTM!
113-168: LGTM!
285-327: LGTM!
329-446: LGTM!
449-493: LGTM!
536-642: LGTM!
644-673: LGTM!
697-709: LGTM!
711-726: LGTM!
773-797: LGTM!
JakubFara
force-pushed
the
feature/dynamicCalibration_multiSensorCalibration
branch
from
c41350f to
3d3cc57
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pipeline/node/DynamicCalibrationNode.cpp (2)
508-510:⚠️ Potential issue | 🟠 Major | ⚡ Quick winException thrown in function that returns
ErrorCodewill crash the node thread.The function signature returns
ErrorCode, but this path throws an exception instead of returning an error code. This inconsistency will crash the node thread when coverage computation fails, rather than allowing graceful error handling.🐛 Proposed fix to return error code consistently
if(!resultCoverage.passed()) { - throw std::runtime_error("Coverage check failed!"); + logger->error("Coverage check failed: {}", resultCoverage.errorMessage()); + return DynamicCalibration::ErrorCode::CALIBRATION_FAILED; }🤖 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/pipeline/node/DynamicCalibrationNode.cpp` around lines 508 - 510, The coverage check validation in DynamicCalibrationNode.cpp throws an exception when resultCoverage.passed() returns false, but the function returns ErrorCode and should handle errors consistently through return values instead of exceptions. Replace the throw std::runtime_error statement with a return statement that returns an appropriate ErrorCode value to allow graceful error handling by the caller instead of crashing the node thread.
680-682:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn value of
computeCoverage()is ignored.Once the throw in
computeCoverage()is replaced with an error return, the error will be silently dropped here. Consider propagating the error or at minimum logging if coverage computation fails after a successful image load.♻️ Suggested fix to propagate error
auto error = runLoadImage(true); - computeCoverage(); - return error; + if(error != ErrorCode::OK) { + return error; + } + return computeCoverage();🤖 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/pipeline/node/DynamicCalibrationNode.cpp` around lines 680 - 682, The return value from the computeCoverage() function call is being ignored after a successful runLoadImage(true) operation. Capture the error returned by computeCoverage() and either propagate it by returning it (potentially combined with the error from runLoadImage if needed) or log it to ensure that coverage computation failures are not silently dropped. This will ensure error handling consistency throughout the function flow in DynamicCalibrationNode.
🤖 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.
Outside diff comments:
In `@src/pipeline/node/DynamicCalibrationNode.cpp`:
- Around line 508-510: The coverage check validation in
DynamicCalibrationNode.cpp throws an exception when resultCoverage.passed()
returns false, but the function returns ErrorCode and should handle errors
consistently through return values instead of exceptions. Replace the throw
std::runtime_error statement with a return statement that returns an appropriate
ErrorCode value to allow graceful error handling by the caller instead of
crashing the node thread.
- Around line 680-682: The return value from the computeCoverage() function call
is being ignored after a successful runLoadImage(true) operation. Capture the
error returned by computeCoverage() and either propagate it by returning it
(potentially combined with the error from runLoadImage if needed) or log it to
ensure that coverage computation failures are not silently dropped. This will
ensure error handling consistency throughout the function flow in
DynamicCalibrationNode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5d32a37-c3f7-46dd-aec8-2ece7db079c7
📒 Files selected for processing (1)
src/pipeline/node/DynamicCalibrationNode.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/node/DynamicCalibrationNode.cpp
🔇 Additional comments (9)
src/pipeline/node/DynamicCalibrationNode.cpp (9)
23-98: LGTM!
100-147: LGTM!
149-247: LGTM!
285-327: LGTM!
329-446: LGTM!
449-493: LGTM!
512-534: LGTM!
536-642: LGTM!
665-667: LGTM!Also applies to: 774-798
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pipeline/node/DynamicCalibrationNode.cpp (1)
739-766:⚠️ Potential issue | 🟠 Major
calibrationPeriodis unused for rate limiting; calibration runs only once per command.The
StartCalibrationcommand acceptscalibrationPeriodas a parameter (documented as controlling "Seconds between calibration cycles"), but the implementation only usesloadImagePeriodfor rate limiting. IndoWork()at line 47, the elapsed time check compares only againstloadImagePeriod, notcalibrationPeriod. Additionally,startCalibrationCommandis cleared after the first successful calibration (line 65), which causes subsequentdoWork()calls to return early (lines 40–41) rather than continuing to run calibration cycles atcalibrationPeriodintervals.The
calibrationPeriodvalue is referenced only once during node initialization (line 80), to add an initial delay before the first calibration attempt, but it plays no role in the ongoing periodic loop. This behavior contradicts the API contract implied by the parameter documentation and struct design.🤖 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/pipeline/node/DynamicCalibrationNode.cpp` around lines 739 - 766, The `calibrationPeriod` parameter from the `startCalibrationCommand` is not being used for rate limiting calibration cycles. Fix this by separating the rate limiting logic for image loading and calibration: add a new timing variable to track calibration intervals independently (similar to how `previousLoadingAndCalibrationTime` is used), and modify the condition that checks `loadingAndCalibrationRequired` to separately check if `calibrationPeriod` has elapsed for the calibration step. Additionally, remove the `startCalibrationCommand.reset()` call after successful calibration so that the command persists and allows `runCalibration()` to execute repeatedly at intervals defined by `calibrationPeriod` until the command is explicitly stopped by external logic, rather than clearing it after the first run.
🤖 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.
Outside diff comments:
In `@src/pipeline/node/DynamicCalibrationNode.cpp`:
- Around line 739-766: The `calibrationPeriod` parameter from the
`startCalibrationCommand` is not being used for rate limiting calibration
cycles. Fix this by separating the rate limiting logic for image loading and
calibration: add a new timing variable to track calibration intervals
independently (similar to how `previousLoadingAndCalibrationTime` is used), and
modify the condition that checks `loadingAndCalibrationRequired` to separately
check if `calibrationPeriod` has elapsed for the calibration step. Additionally,
remove the `startCalibrationCommand.reset()` call after successful calibration
so that the command persists and allows `runCalibration()` to execute repeatedly
at intervals defined by `calibrationPeriod` until the command is explicitly
stopped by external logic, rather than clearing it after the first run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ebd9c0e-01e7-41bc-8d21-614aedb20540
📒 Files selected for processing (2)
src/pipeline/node/DynamicCalibrationNode.cppsrc/pipeline/node/DynamicCalibrationUtils.hpp
📜 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/node/DynamicCalibrationNode.cpp
🔇 Additional comments (12)
src/pipeline/node/DynamicCalibrationUtils.hpp (1)
13-42: LGTM!src/pipeline/node/DynamicCalibrationNode.cpp (11)
23-95: LGTM!
97-144: LGTM!
146-165: LGTM!
247-270: LGTM!
282-324: LGTM!
446-490: LGTM!
492-531: LGTM!
533-639: LGTM!
641-724: LGTM!
771-795: LGTM!
391-396: Unit conversion is correct.The translation vector is in meters (as shown by
LengthUnit::METERusage elsewhere in the codebase), andupdateCameraExtrinsicscorrectly expects centimeters per its API documentation. The 100x multiplier at lines 392-394 properly converts meters to centimeters.
JakubFara
force-pushed
the
feature/dynamicCalibration_multiSensorCalibration
branch
from
6e17291 to
cfca204
Compare
JakubFara
force-pushed
the
feature/dynamicCalibration_multiSensorCalibration
branch
from
a36fb0f to
328ecbd
Compare
aljazkonec1
added
testable
MaticTonin
added
testable
MaticTonin
force-pushed
the
feature/dynamicCalibration_multiSensorCalibration
branch
from
894ff29 to
8363fff
Compare
3 participants
Summary
This PR expands DynamicCalibration from a fixed stereo-oriented flow into a more flexible multi-input calibration path with improved host/device APIs,
broader calibration-frame support, and updated Python bindings/examples.
What Changed
DynamicCalibration node
Generalized DynamicCalibration to work with an arbitrary number of synced inputs instead of assuming only two fixed sensors.
Added support for an additional rgb input and exposed the internal sync/inputs interface for more flexible pipeline wiring, including Python.
Reworked initialization and calibration flow to track connected sensors explicitly, preserve socket ordering from calibration data, and support multi-
sensor baseline handling.
Added keepCameraCenters support to calibration commands so callers can choose whether sensor centers remain fixed during recalibration.
Improved runtime behavior when the sync node has no input by exiting cleanly instead of throwing.
Calibration handling
Added CalibrationHandler::overwriteCameraExtrinsics(...) for updating existing extrinsics in-place.
Extended housing-origin APIs:
Updated dynamic calibration internals to work with either camera-based or housing-based calibration frames.
Results and API updates
Added per-socket coverage output:
Marked older stereo-only result fields as deprecated:
Extended DynamicCalibrationControl commands:
Python bindings and examples
Summary by CodeRabbit
keepCameraCenters(defaulttrue) to dynamic calibration commands and execution flow.CoverageData.coveragePerCellandCalibrationQuality’spairwiseRotationDifference.DynamicCalibrationproperties:syncandinputs.coveragePerCellA/coveragePerCellBdeprecated in favor ofcoveragePerCell.CalibrationQuality.rotationChangemarked deprecated.coveragePerCellterminology.