Support for setting external strobe exposure master camera - #1859
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:
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 (1)
📜 Recent review details🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds a ChangessetExternalStrobeEnable CameraBoardSocket Overload
RVC4 Package Version Update
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to The PR changes the RVC4 firmware package selected by the build configuration. It is mergeable with owner awareness that package 0.0.1+d86afc8b66571dd5f7a0ff8daa3c5fb4bfc7502f must exist and match the intended build to avoid firmware download or version-reporting issues. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PythonBinding
participant DeviceBase
participant DeviceRPC
PythonBinding->>DeviceBase: setExternalStrobeEnable(exposureMasterSocket)
DeviceBase->>DeviceRPC: setExternalStrobeEnableExposureMaster(socket)
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@bindings/python/src/DeviceBindings.cpp`:
- Around line 1003-1010: The setExternalStrobeEnable method binding with the
dai::CameraBoardSocket parameter is using a shared DOC macro that does not
distinguish it from the bool overload, causing Python help text to display
incorrect parameter documentation. Replace the DOC(dai, DeviceBase,
setExternalStrobeEnable) macro with an overload-specific documentation key that
accurately describes the exposureMasterSocket parameter for this particular
variant, ensuring each overload has distinct documentation that matches its
actual parameters and semantics.
In `@include/depthai/device/DeviceBase.hpp`:
- Around line 1162-1166: The documentation comment for this external strobe
overload is misleading because it uses generic language about "whether the
external strobe should be enabled" which sounds like a boolean toggle, when this
particular overload actually accepts a CameraBoardSocket parameter to select
which camera board socket will control the external strobe exposure. Update the
documentation comment to clearly state that this overload specifically selects
or sets the exposure-master camera socket that will control the external strobe
output, distinguishing it from a boolean enable/disable operation and making the
purpose of the CameraBoardSocket parameter explicit.
🪄 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: fed29904-44fc-48c5-8fdf-e29c538bb52c
📒 Files selected for processing (3)
bindings/python/src/DeviceBindings.cppinclude/depthai/device/DeviceBase.hppsrc/device/DeviceBase.cpp
📜 Review details
🔇 Additional comments (1)
src/device/DeviceBase.cpp (1)
1684-1686: LGTM!
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 `@include/depthai/device/DeviceBase.hpp`:
- Line 1163: In the documentation comment for the external strobe exposure
control method around line 1163 in DeviceBase.hpp, fix the spelling error where
"Automaticaly" is misspelled. Change it to "Automatically" in the comment that
reads "Set which camera will control the external strobe exposure. Automaticaly
enables external strobe."
🪄 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: 2cf6ca50-d098-4595-9cb4-03eb5d7b1756
📒 Files selected for processing (1)
include/depthai/device/DeviceBase.hpp
| void setExternalStrobeEnable(bool enable); | ||
|
|
||
| /** | ||
| * Set which camera will control the external strobe exposure. Automaticaly enables external strobe. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix typo in documentation.
Line 1163 contains a typo: "Automaticaly" should be "Automatically".
📝 Proposed fix
- * Set which camera will control the external strobe exposure. Automaticaly enables external strobe.
+ * Set which camera will control the external strobe exposure. Automatically enables external strobe.Note: The previous review comment regarding clarifying the documentation has been addressed—the wording now clearly states this overload selects which camera controls the strobe and automatically enables it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Set which camera will control the external strobe exposure. Automaticaly enables external strobe. | |
| * Set which camera will control the external strobe exposure. Automatically enables external strobe. |
🤖 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/device/DeviceBase.hpp` at line 1163, In the documentation
comment for the external strobe exposure control method around line 1163 in
DeviceBase.hpp, fix the spelling error where "Automaticaly" is misspelled.
Change it to "Automatically" in the comment that reads "Set which camera will
control the external strobe exposure. Automaticaly enables external strobe."
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
b56ce3d to
65329d7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
71b332e to
78b455a
Compare
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
78b455a to
947885a
Compare
Signed-off-by: stas.bucik <stas.bucik@luxonis.com>
1 participant
This PR enables users to explicitly set which cameras exposure will be signaled on FSYNC/STROBE pin on M8 connector.
On normal devices, if exposure master is not set explicitly, the exposure master is determined automatically (keeping the same previous behavior)
On Devkit and FFC the user must explicitly set an exposure master, if he wants strobe to be outputed to M8. This is because the camera setup on those boards can be arbitrary.
Summary by CodeRabbit