Expose camera sensor temperature#1775
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new sensor temperature field is added across the codebase. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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. Review rate limit: 0/1 reviews remaining, refill in 53 minutes and 7 seconds.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/src/onhost_tests/pipeline/datatype/imgframe_test.cpp (1)
105-131:⚠️ Potential issue | 🟡 MinorAdd an explicit test for the “unavailable temperature” path.
Current additions validate set/copy behavior, but not the documented default (
NaNwhen unavailable). Please add one assertion for a freshImgFramewith no temperature set.✅ Suggested test addition
TEST_CASE("ImgFrame timestamps and camera settings", "[ImgFrame][Timestamps]") { dai::ImgFrame frame; + SECTION("Sensor temperature defaults to NaN when unavailable") { + REQUIRE(std::isnan(frame.getSensorTemperature())); + } auto now = std::chrono::steady_clock::now();Also applies to: 177-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/onhost_tests/pipeline/datatype/imgframe_test.cpp` around lines 105 - 131, A fresh ImgFrame should return NaN for unavailable sensor temperature—add an assertion that a default-constructed dai::ImgFrame (no temperature set) yields NaN from getSensorTemperature(); implement this by creating a new ImgFrame (or reusing one after reset) and asserting std::isnan(frame.getSensorTemperature()) (or using Catch's REQUIRE(std::isnan(...))) near the other temperature checks; do the same for the other similar test block that starts around the second occurrence mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protos/common.proto`:
- Line 27: Change the proto scalar field sensorTemperatureC in
protos/common.proto to be optional so presence can be tracked (replace the
non-optional float with an optional float); then update the deserialization
logic in ProtoSerialize.cpp that populates obj.cam.sensorTemperatureC to check
presence via imgFrame->cam().has_sensortemperaturec() and, when absent, set
obj.cam.sensorTemperatureC to std::numeric_limits<float>::quiet_NaN() (matching
the ImgFrame.hpp initialization) instead of using the default 0.0.
---
Outside diff comments:
In `@tests/src/onhost_tests/pipeline/datatype/imgframe_test.cpp`:
- Around line 105-131: A fresh ImgFrame should return NaN for unavailable sensor
temperature—add an assertion that a default-constructed dai::ImgFrame (no
temperature set) yields NaN from getSensorTemperature(); implement this by
creating a new ImgFrame (or reusing one after reset) and asserting
std::isnan(frame.getSensorTemperature()) (or using Catch's
REQUIRE(std::isnan(...))) near the other temperature checks; do the same for the
other similar test block that starts around the second occurrence mentioned.
🪄 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: 873ea51d-edd6-43a2-bf18-e3c08ca0e787
📒 Files selected for processing (10)
bindings/python/src/pipeline/datatype/EncodedFrameBindings.cppbindings/python/src/pipeline/datatype/ImgFrameBindings.cppexamples/cpp/Camera/camera_multiple_outputs.cppinclude/depthai/pipeline/datatype/EncodedFrame.hppinclude/depthai/pipeline/datatype/ImgFrame.hppprotos/common.protosrc/pipeline/datatype/EncodedFrame.cppsrc/pipeline/datatype/ImgFrame.cppsrc/utility/ProtoSerialize.cpptests/src/onhost_tests/pipeline/datatype/imgframe_test.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📚 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/datatype/EncodedFrame.cppsrc/pipeline/datatype/ImgFrame.cpp
📚 Learning: 2026-03-23T09:35:30.339Z
Learnt from: aljazkonec1
Repo: luxonis/depthai-core PR: 1728
File: protos/common.proto:20-25
Timestamp: 2026-03-23T09:35:30.339Z
Learning: In luxonis/depthai-core’s `protos/common.proto`, do not change existing enumerator values for the public `LengthUnit` and `CameraBoardSocket` enums, and do not prepend new zero-value entries like `UNSPECIFIED`. These enums are already released as public API and are used in serialized data; altering numeric values or changing the first/zero member will break backward compatibility with existing user code and stored/serialized representations.
Applied to files:
protos/common.proto
📚 Learning: 2026-03-23T09:35:34.824Z
Learnt from: aljazkonec1
Repo: luxonis/depthai-core PR: 1728
File: protos/common.proto:20-25
Timestamp: 2026-03-23T09:35:34.824Z
Learning: In the depthai-core repository (luxonis/depthai-core), the `LengthUnit` and `CameraBoardSocket` enums (defined in both C++ headers and `protos/common.proto`) are already released as public API and must not have their existing enumerator values changed or have new zero-value "UNSPECIFIED" entries prepended, as that would break existing user code and serialized data.
Applied to files:
src/utility/ProtoSerialize.cpp
📚 Learning: 2026-04-20T14:47:11.304Z
Learnt from: aljazkonec1
Repo: luxonis/depthai-core PR: 1760
File: tests/src/ondevice_tests/script_node_test.cpp:631-689
Timestamp: 2026-04-20T14:47:11.304Z
Learning: In `tests/src/ondevice_tests/script_node_test.cpp` (depthai-core), the `NNData roundtrip in Script node` test is intentionally a metadata-only test that validates `TensorInfo` fields (datatype, order, dims, strides, quantization params, etc.). Payload/element-wise value validation for NNData tensors is handled in a separate dedicated test. Do not flag the absence of payload assertions in this test case.
Applied to files:
tests/src/onhost_tests/pipeline/datatype/imgframe_test.cpp
🔇 Additional comments (8)
examples/cpp/Camera/camera_multiple_outputs.cpp (1)
151-151: Formatting-only change looks good.This is a safe newline-at-EOF normalization with no behavioral impact.
bindings/python/src/pipeline/datatype/ImgFrameBindings.cpp (1)
255-255: Binding addition is clean and consistent.
ImgFrame.getSensorTemperature()is exposed in the expected getter block with doc wiring.src/pipeline/datatype/EncodedFrame.cpp (1)
52-54: Getter implementation is straightforward and correct.Directly returning
cam.sensorTemperatureCmatches the new metadata field contract.src/pipeline/datatype/ImgFrame.cpp (1)
142-145: Getter implementation is consistent with the rest ofImgFrame.No issues in this addition.
include/depthai/pipeline/datatype/EncodedFrame.hpp (1)
95-99: Public API addition is clear and well-placed.Declaration and docstring are consistent with neighboring getters.
bindings/python/src/pipeline/datatype/EncodedFrameBindings.cpp (1)
91-91: Python binding extension looks good.
EncodedFrame.getSensorTemperature()is correctly exposed alongside related getters.include/depthai/pipeline/datatype/ImgFrame.hpp (1)
4-4: Header updates are cohesive and correctly wired.NaN default initialization plus serialization and getter declaration are aligned.
Also applies to: 213-217, 748-750
src/utility/ProtoSerialize.cpp (1)
646-646: ConsistentsensorTemperatureCround-trip wiring looks correct.The new field is mapped in both directions for
ImgFrameandEncodedFrame, including helper paths used byRGBDData, so sensor temperature metadata is preserved across protobuf serialization/deserialization.Also applies to: 707-707, 971-971, 1020-1020, 1109-1109
| CameraFsync fsync = 6; | ||
| int32 sensorMode = 7; | ||
| float fps = 8; | ||
| float sensorTemperatureC = 9; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Verify proto declaration"
rg -nP 'sensorTemperatureC\s*=\s*9' protos/common.proto
echo "2) Verify serialization/deserialization handling"
rg -n 'sensorTemperatureC|sensortemperaturec|has_sensortemperaturec' src/utility/ProtoSerialize.cpp
echo "Expected: deserialization path should gate assignment with has_sensortemperaturec(), otherwise missing field becomes 0.0."Repository: luxonis/depthai-core
Length of output: 665
🏁 Script executed:
# Check the context around deserialization lines to understand the current implementation
head -n 980 src/utility/ProtoSerialize.cpp | tail -n 20Repository: luxonis/depthai-core
Length of output: 1026
🏁 Script executed:
# Look for documentation about sensorTemperatureC contract
rg -i 'unavailable|nan|quiet_nan' src/utility/ProtoSerialize.cpp -B 2 -A 2Repository: luxonis/depthai-core
Length of output: 46
🏁 Script executed:
# Check if there are other optional fields in the proto to understand the pattern
rg -n 'optional' protos/common.protoRepository: luxonis/depthai-core
Length of output: 107
🏁 Script executed:
# Search for how other temperature or sensor fields handle unavailable values
rg -i 'temperature' protos/common.proto -B 1 -A 1Repository: luxonis/depthai-core
Length of output: 119
🏁 Script executed:
# Search for the cam struct definition and any documentation about sensorTemperatureC
rg -n 'sensorTemperatureC' --type cpp --type hpp --type h -B 3 -A 3Repository: luxonis/depthai-core
Length of output: 92
🏁 Script executed:
# Look for header file definitions of the camera metadata structure
fd -e hpp -e h | xargs rg -l 'sensorTemperatureC' 2>/dev/null || echo "No matches in header files"Repository: luxonis/depthai-core
Length of output: 111
🏁 Script executed:
# Search for any comments or documentation about unavailable temperature values
rg -i 'temperature.*unavailable|unavailable.*temperature|nan.*temperature|temperature.*nan' --type cpp --type hpp --type hRepository: luxonis/depthai-core
Length of output: 92
🏁 Script executed:
# Read the ImgFrame header to understand the sensorTemperatureC field
cat -n include/depthai/pipeline/datatype/ImgFrame.hpp | grep -A 10 -B 10 'sensorTemperatureC'Repository: luxonis/depthai-core
Length of output: 1057
🏁 Script executed:
# Also search for any documentation in comments about temperature or "unavailable"
rg -n 'unavailable|quiet_NaN|quiet_nan' include/depthai/pipeline/datatype/ImgFrame.hppRepository: luxonis/depthai-core
Length of output: 144
🏁 Script executed:
# Look at the proto definition more thoroughly to see if there are comments
cat -n protos/common.proto | grep -A 5 -B 5 'sensorTemperatureC'Repository: luxonis/depthai-core
Length of output: 403
🏁 Script executed:
# Verify the exact deserialization pattern and confirm no presence checks are used
cat -n src/utility/ProtoSerialize.cpp | sed -n '968,975p'Repository: luxonis/depthai-core
Length of output: 452
🏁 Script executed:
# Check other deserialization instances (lines 1020, 1109) to see if they have the same issue
cat -n src/utility/ProtoSerialize.cpp | sed -n '1017,1024p'Repository: luxonis/depthai-core
Length of output: 531
Make sensorTemperatureC optional and preserve NaN semantics in deserialization.
The C++ struct initializes sensorTemperatureC to quiet_NaN() (ImgFrame.hpp:748) to represent unavailable values. However, the proto3 field at line 27 is a non-optional scalar, which cannot distinguish between absent and zero. When deserialized, missing fields become 0.0, overwriting the NaN semantics.
The proto field must be declared as optional to enable presence tracking:
Proto change
- float sensorTemperatureC = 9;
+ optional float sensorTemperatureC = 9;Additionally, deserialization at ProtoSerialize.cpp lines 971 and 1020 must check presence and preserve the NaN default when the field is absent:
obj.cam.sensorTemperatureC = imgFrame->cam().has_sensortemperaturec()
? imgFrame->cam().sensortemperaturec()
: std::numeric_limits<float>::quiet_NaN();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@protos/common.proto` at line 27, Change the proto scalar field
sensorTemperatureC in protos/common.proto to be optional so presence can be
tracked (replace the non-optional float with an optional float); then update the
deserialization logic in ProtoSerialize.cpp that populates
obj.cam.sensorTemperatureC to check presence via
imgFrame->cam().has_sensortemperaturec() and, when absent, set
obj.cam.sensorTemperatureC to std::numeric_limits<float>::quiet_NaN() (matching
the ImgFrame.hpp initialization) instead of using the default 0.0.
1 participant
Summary by CodeRabbit
New Features
getSensorTemperature()method on both encoded and image frames. Temperature returns NaN when unavailable.Tests