Fix/stereo default config - #1959
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📜 Recent review details🧰 Additional context used🪛 Cppcheck (2.21.0)tests/src/ondevice_tests/stereo_depth_node_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) 🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe change adds a shared-device ChangesStereoDepth preset initialization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change fixes StereoDepth DEFAULT configuration when build() has not been called and adds a validating test; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Application
participant StereoDepth
participant setDefaultProfilePreset
Application->>StereoDepth: Construct with shared Device
StereoDepth->>setDefaultProfilePreset: Apply PresetMode::DEFAULT
setDefaultProfilePreset->>StereoDepth: Initialize profile configuration
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
aljazkonec1
left a comment
aljazkonec1
left a comment
There was a problem hiding this comment.
using DEFAULT preset will completely change the behavior of stereo depth when the node is initialized without the build() function. To me the presets that are default constructed in initialConfig look better than when using the DEFAULT preset. Are we sure we want to do that?
| initialConfig(std::make_shared<decltype(properties.initialConfig)>(properties.initialConfig)) {} | ||
|
|
||
| StereoDepth::StereoDepth(const std::shared_ptr<Device>& device) : DeviceNodeCRTP<DeviceNode, StereoDepth, StereoDepthProperties>(device) { | ||
| setDefaultProfilePreset(PresetMode::DEFAULT); |
There was a problem hiding this comment.
This makes "constructor only stereo" use DEFAULT presets instead of making the DEFAULT presets equal to the constructor version of the presets. This will completely change behavior in all stereo examples. It also sets DEFAULT preset for RVC2. Is that the same as not setting any preset?
There was a problem hiding this comment.
This unifies setting DEFAULT as default everywhere, seems intended to me.
There was a problem hiding this comment.
|
|
||
| void StereoDepth::setRvc2ProfilePreset(PresetMode mode) { | ||
| *initialConfig = StereoDepthConfig{}; | ||
| setPostProcessingHardwareResources(Properties::AUTO, Properties::AUTO); |
There was a problem hiding this comment.
Why is this needed? Is this PR not ment to adjust only RVC4?
There was a problem hiding this comment.
The config does not need to set all the parameters so we "reset" the parameters at first
2 participants
Purpose
On RVC4, in StereoDepth, the DEFAULT mode was not set if build() had not been called.
Tests
New test added validating the fix
Summary by CodeRabbit
New Features
Bug Fixes