Repository navigation
D401 gmsl fixes - #14582
D401 gmsl fixes#14582
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| }; | ||
|
|
||
| class rs401_gmsl_device : //public d400_color, | ||
| class rs401_gmsl_device : public d400_color, |
There was a problem hiding this comment.
Class now inherits d400_color; ensure _color_stream is initialized before use or guard its dereference.
Details
✨ AI Reasoning
The class now publicly inherits and initializes the color subsystem but the device may not actually create or register the color stream in all codepaths. Later the code dereferences _color_stream (calling _color_stream.get()) when building matcher streams. If _color_stream is null (color not initialized for this SKU), dereferencing will cause a segmentation fault. This risk was introduced by enabling d400_color inheritance/initialization and then adding _color_stream to the matcher list without null checks.
🔧 How do I fix it?
Add null checks before dereferencing pointers, validate array bounds before access, avoid using pointers after free/delete, don't write to string literals, and prefer smart pointers in modern C++.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| std::shared_ptr<matcher> rs401_gmsl_device::create_matcher(const frame_holder& frame) const | ||
| { | ||
| std::vector<stream_interface*> streams = { _depth_stream.get() , _left_ir_stream.get() , _right_ir_stream.get() }; | ||
| std::vector<stream_interface*> streams = { _depth_stream.get() , _left_ir_stream.get() , _right_ir_stream.get(), _color_stream.get() }; |
There was a problem hiding this comment.
Added _color_stream.get() to matcher streams without null-check; guard against null _color_stream before dereferencing.
Details
✨ AI Reasoning
The create_matcher implementation was changed to include _color_stream.get() in the streams vector. This dereferences the shared pointer without checking whether the color stream was created/registered. If _color_stream is empty (nullptr), calling get() yields nullptr and passing that into matcher_factory can cause segmentation faults later when accessed. This is a direct regression compared to prior code that did not include the color stream here.
🔧 How do I fix it?
Add null checks before dereferencing pointers, validate array bounds before access, avoid using pointers after free/delete, don't write to string literals, and prefer smart pointers in modern C++.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
Pull request overview
This PR implements fixes for D401 GMSL device support, addressing platform detection, color sensor initialization, and video node enumeration.
Changes:
- Enhanced platform detection to properly identify Jetson platforms in multi-pin UVC devices
- Enabled color sensor support for RS401 GMSL devices by uncommenting inheritance and initialization code
- Fixed device enumeration script to correctly parse device IDs and match video nodes
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/uvc-device.h | Implements proper Jetson platform detection by iterating through device elements |
| src/linux/backend-v4l2.cpp | Adds blank line for code formatting |
| src/ds/d400/d400-factory.cpp | Enables color sensor inheritance and stream matching for RS401 GMSL |
| src/ds/d400/d400-color.cpp | Adds RS401_GMSL_PID exception to MIPI device color endpoint logic |
| scripts/rs-enum.sh | Fixes device ID parsing and video node matching patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for( auto & elem : _dev ) | ||
| { | ||
| return elem->is_platform_jetson(); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
The loop returns on the first iteration regardless of the result, making it functionally equivalent to checking only the first device. If all devices should be Jetson for this to return true, use a proper all-of check. If any device being Jetson is sufficient, the current logic works but the loop is misleading.
| for( auto & elem : _dev ) | |
| { | |
| return elem->is_platform_jetson(); | |
| } | |
| return false; | |
| if( _dev.empty() ) | |
| return false; | |
| for( auto & elem : _dev ) | |
| { | |
| if( ! elem->is_platform_jetson() ) | |
| return false; | |
| } | |
| return true; |
| @@ -65,7 +65,7 @@ namespace librealsense | |||
| // except for D405, in which the color is part of the depth unit | |||
| mi = 3; // metadata node indicator | ||
| else if (ind == 6) | ||
| mi = 4; // IMU node indicator | ||
|
|
There was a problem hiding this comment.
Can you revert the changes here?
|
|
||
| # get cameras physical location | ||
| dfuid=($(ls -1 /sys/class/d4xx-class/ | awk -F'-' '{print $3"-"$4}')) | ||
| dfuid=($(ls -1 /sys/class/d4xx-class/ | awk -F'-' '{print $4}')) |
There was a problem hiding this comment.
Lets investigate why it did not work as discussed
| { | ||
| for( auto & elem : _dev ) | ||
| { | ||
| return elem->is_platform_jetson(); |
There was a problem hiding this comment.
Let's change to check if we have the first device and return the function call from it or false
Tracked by: RSDSO-20657