Refactor backend-v4l#15464
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
sysrsbuild-gh-agentic
left a comment
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
[rs-agentic bot] Automated review of PR #15464 (add_d585_gmsl). Comments below are auto-generated.
| if (dfu_ver.find("recovery") == std::string::npos) | ||
| continue; | ||
| mipi_device_info info{}; | ||
| info.pid = 0xbbcd; |
There was a problem hiding this comment.
@AikidoSec ignore: old code that will be refactored. Not added by this PR, only line number shifted due to deletions in the file.
| const uint16_t D415_GMSL_PID = 0xABCF; | ||
| const uint16_t D401_GMSL_PID = 0xABCC; | ||
|
|
||
| const uint16_t D585_GMSL_PID = 0xBAAA; |
There was a problem hiding this comment.
lets add a TODO to replace with real PID
Nir-Az
left a comment
Nir-Az
left a comment
There was a problem hiding this comment.
Refactor looks solid — clean layering and removing the s_mipi_camera_pid global is a real win. A few inline notes.
🤖 Generated by AI
| } | ||
| else | ||
| { | ||
| throw backend_exception( "Unsupported connection type" ); |
There was a problem hiding this comment.
This throw is a behavioral change. The old d500 path used if (usb_modality) { register... } and silently skipped connection-type registration when the USB spec was undefined, letting init continue — the removed comment noted this happens on RS3 builds. A genuine USB d500 whose backend doesn't report a descriptor would now fail init entirely. Suggest warn + skip instead of throw to preserve the old resilience.
Also: this throw plus the new GMSL/MIPI-driver registration are functional changes, so the "no functional change" claim in the description isn't accurate — worth correcting.
🤖 Generated by AI
| { | ||
| auto opcode = gvd[0]; | ||
| if( opcode != GVD_VALID_OPCODE ) | ||
| LOG_WARNING( "Wrong opcode when pulling GVD: gvd[0] returned as: " << opcode ); |
There was a problem hiding this comment.
opcode is uint8_t, so this logs a raw byte as a character, not a number. Use static_cast<int>(opcode) (as already done for product_pid in camera-identifier-v4l.cpp).
🤖 Generated by AI
| // - video6 for IMU (accel or gyro TBD) | ||
| // next several lines permit to use D457 even if a usb device has already "taken" the video0,1,2 (for example) | ||
| // further development is needed to permit use of several mipi devices | ||
| static int first_video_index = video_index; |
There was a problem hiding this comment.
static int first_video_index is process-lifetime state that never resets across enumerations — a sibling of the s_mipi_camera_pid global this PR just removed. Not a regression (pre-existing), but worth folding into the follow-up GVD-identification step rather than re-hiding it here.
🤖 Generated by AI
| ### 7. Comments | ||
| * **Comments should be sparse.** Keep to 1–2 lines unless the code is large and genuinely complex. | ||
| * **Intention** Don't restate what the code already says, comment should convey the reason for the code. | ||
| * **Locallity** The comment should refer to the local code. e.g. class description should not mention inheritors or where other logic live. |
There was a problem hiding this comment.
Typos in the new section: Locallity → Locality, and Lenght → Length (next line). Also missing colons after the bold labels (**Intention**, **Locality**, **Length**, **Generality**).
🤖 Generated by AI
3 participants
Tracked on [RSDEV-9251]
Refactoring as preparation for next step camera identification using GVD.
Splits the monolithic V4L2 enumeration code in backend-v4l2 into layered units with single responsibilities.
Layering (dependencies point down):
camera_identifier (src/platform/camera-identifier.h) — backend-agnostic interface supplying a device's VID/PID.
camera_identifier_v4l_{mipi,usb} (src/linux/camera-identifier-v4l.) — identification policy: resolve() reads a device's GVD (MIPI) or modalias (USB) and derives PID/VID.
v4l_mipi_logic / v4l_usb_logic (src/linux/v4l-{mipi,usb}-logic.) — pure low-level V4L2 primitives (raw GVD read, sysfs/modalias reads, node-naming conventions). No identifier dependency.
backend-v4l2 — orchestration only: drives enumeration and assembles uvc_device_info, calling the identifiers for VID/PID and the logic units for primitives.
No functional change — pure reorganization. Behavior of USB/MIPI/GMSL device enumeration is preserved.