Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for configuring external synchronization via a new depth XU control, with Linux V4L2 backend mapping to the corresponding MIPI CID.
Changes:
- Introduce new XU/control identifiers for external sync (0x12) and a new V4L2 CID mapping (
RS_CAMERA_CID_SYNC_MODE). - Detect availability of the external-sync XU control on D400 devices and register it as
RS2_OPTION_INTER_CAM_SYNC_MODEwhen present. - Add a new device capability flag (
CAP_EXTERNAL_SYNC_XU) to represent support for external sync via XU.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/linux/backend-v4l2.cpp |
Adds new CID and control ID mappings so MIPI/V4L2 path can access external sync via control 0x12. |
src/ds/ds-private.h |
Defines the new DS5 XU control ID and capability bit for external sync support. |
src/ds/d400/d400-device.cpp |
Probes the new XU control and registers RS2_OPTION_INTER_CAM_SYNC_MODE via XU when supported. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ds/d400/d400-device.cpp
Outdated
| DS5_EXTERNAL_SYNC, "External sync"); | ||
| try | ||
| { | ||
| auto external_sync_mode = external_sync_xu_control->query(); |
There was a problem hiding this comment.
external_sync_mode is assigned but never used; this introduces dead code and can trigger compiler warnings in some configurations. Consider removing the local variable and just calling external_sync_xu_control->query() (or explicitly casting the result to void) when probing for control availability.
| auto external_sync_mode = external_sync_xu_control->query(); | |
| (void) external_sync_xu_control->query(); |
| const uint8_t RS_ENABLE_AUTO_EXPOSURE = 0xB; | ||
| const uint8_t RS_LED_PWR = 0xE; | ||
| const uint8_t RS_EMITTER_FREQUENCY = 0x10; // Match to DS5_EMITTER_FREQUENCY | ||
| const uint8_t RS_DEPTH_AUTO_EXPOSURE_MODE = 0x11; |
There was a problem hiding this comment.
RS_DEPTH_AUTO_EXPOSURE_MODE is introduced but not referenced anywhere in this file (and has no mapping in xu_to_cid). If it’s not wired up yet, consider removing it until it’s used, or add the missing handling so it can’t silently drift out of sync with supported controls.
| const uint8_t RS_DEPTH_AUTO_EXPOSURE_MODE = 0x11; |
| CAP_IP65 = (1u << 9), | ||
| CAP_IR_FILTER = (1u << 10), | ||
| CAP_BMI_088 = (1u << 11), | ||
| CAP_EXTERNAL_SYNC_XU = ( 1u << 12 ), // Support external synchronization via XU |
There was a problem hiding this comment.
Minor style inconsistency: the bitmask expression has extra spaces ( 1u << 12 ) while the surrounding enum entries use (1u << N). Aligning formatting improves readability and keeps the enum consistent.
| CAP_EXTERNAL_SYNC_XU = ( 1u << 12 ), // Support external synchronization via XU | |
| CAP_EXTERNAL_SYNC_XU = (1u << 12), // Support external synchronization via XU |
remibettan
force-pushed
the
ext_sync_new_xu
branch
from
5e7a248 to
f04cd57
Compare
remibettan
force-pushed
the
ext_sync_new_xu
branch
from
f04cd57 to
3ecb1b2
Compare
remibettan
force-pushed
the
ext_sync_new_xu
branch
from
3ecb1b2 to
6bfe6c0
Compare
Tracked by: RSDEV-5861