Detect the new products in the SDK and Viewer: D435f, D435if, D455f, D456 by Tamir91 · Pull Request #11399 · realsenseai/librealsense

Tamir91 · 2023-02-05T15:52:10Z

Tracked on [LRS-657]

Tamir91 · 2023-02-07T14:58:28Z

            fisheye_sensor_hb = 113,
            imu_acc_chip_id = 124,
+            ip65_sealed_offset = 161,
+            filter_sensor_offset = 164,


Maybe it is not the correct name "filter_sensor_offset"? Actually, a filter is not a sensor.

I agree, please align to your previous name: ir_filter_exist

Sorry, what do you mean by previous name? Do you mean the variable ir_filter_mask?
In the struct, we have offsets of a memory.

Like discussed

Nir-Az · 2023-02-08T10:29:09Z

            }

+            auto const CUR_GVD_VERSION = gvd_buff[gvd_version_offset];
+            auto const MIN_GVD_VERSION = 0x4;


Lets create a function for that, maybe inside ds5-private.h

This function will check once that MIN_GVD_VERSION <= CUR_GVD_VERSION
You also need to add comments for it.

Please inside the function add comments specificaly say how we change the name e.g. from D435i to D435IF
So when we search the code in the future for D435IF we will find it

Lets create a function for that, maybe inside ds5-private.h

This function will check once that MIN_GVD_VERSION <= CUR_GVD_VERSION You also need to add comments for it.

Due to multiple definitions, the project failed on the build with my function in *.h file.
What about creating a function in ds5-device.cpp?

Resolved together

Nir-Az · 2023-02-08T10:32:51Z

We need to search all of the old device names, see if someone is the code used the specific camera name to add some logic.
If so we need to add the new name as well
C++ and python wrapper

Nir-Az · 2023-02-08T10:38:18Z

+                val_in_range(_pid, {RS435_RGB_PID, RS435I_PID, RS455_PID}) &&
+                (_device_capabilities & ir_filter_mask) == ir_filter_mask)
+            {
+                device_name += "F";


Please search a way to replace it with a regex substitute, maybe it will look better :)

A regex use was added.
But I can find optimal logic for changing the name.
How much include <regex> increase our .cpp file?

Tamir91 · 2023-02-09T17:06:07Z

We need to search all of the old device names, see if someone is the code used the specific camera name to add some logic. If so we need to add the new name as well C++ and python wrapper

Meanwhile, I didn't find any used device by name only using a device by a pid.
I will look for it later.

Nir-Az · 2023-02-09T17:08:28Z

Please rebase and solve conflicts

This branch has conflicts that must be resolved

Nir-Az · 2023-02-09T17:12:15Z

I don't see the new changes, please push

Nir-Az · 2023-02-09T17:15:03Z

+                (_device_capabilities & ip65_mask) == ip65_mask)
+            {
+                device_name = device_name.substr(0, device_name.size() - 1);
+                device_name += "6";


The "F" is OK since it's the last character,
Here we replace so I wish for something more elegant..
Look at std::string function or std::regex variations
And like I said we are missing comments, hard to understand what is done here

I added some comments and left commented lines of code with the purpose to understand what looks good for us.

Nir-Az · 2023-02-12T07:12:43Z

+		switch (cap)
+		{
+		    case ds::d400_caps::CAP_IR_FILTER:
+			    if (is_capability_supports(cap, cur_gvd_version))


No need to check it inside every case,
Lets take it out and only call update_device_name if it's true

The code is optimized.

Nir-Az · 2023-02-12T07:14:06Z

+                if (is_capability_supports(cap, cur_gvd_version))
+                {
+                    // device_name[device_name.size() - 1] = '6'; // Changing last digit name to "6" if device has IP65 standart.
+                    std::regex vowel_re("D455");


What does vowel_re means?
Maybe better to use a more meaningful name

The variable was renamed to old_name

Nir-Az · 2023-02-12T07:25:33Z

+                    std::regex vowel_re("D455");
+                    std::regex_replace(device_name, device_name.begin(), device_name.end(), vowel_re, "D456");
+                }
+                break;


Please add a default case (always)
Here you can throw and say the required capability is not supported for name update (with adding the cap value)

The default case was added.
I implemented throw in the default case but can't find it in IDE after throwing an exception.
As a result, the viewer continues work after throwing an exception and NOT showing the device name.
Please advise, if we can leave it as is or improve it.

Nir-Az · 2023-02-12T12:50:47Z

+                break;
+
+            default:
+                auto wrong_cap = ds::d400_capabilities_names.find(cap);


No need for all of this:

default: throw invalid_value_exception("capability '" + cap + "' is not supported for device name update");

cap variable is not a string, so I can't connect it to another string.

try this:

default: throw invalid_value_exception("capability '" + d400_capabilities_names.at( cap ) + "' is not supported for device name update");

Ok, thank you! I will move forward with your suggestion!

I will do next:
ds::d400_capabilities_names.find(cap)->second
Because d400_capabilities_names is a map.

ds::d400_capabilities_names.find(cap)->second this use can raise a null reference and crash the SW.
map api has .at() which will throw if the input is not inside the map, which is good for us.

Yes, you are right. The first time, IDE showed me a syntax error there.
Now it's OK, something flaky.

Nir-Az · 2023-02-12T12:51:29Z

+                break;
+
+		    case ds::d400_caps::CAP_IP65:
+                device_name = std::regex_replace(device_name, old_name_reg, "D456"); // Change device name from D455 to D456.


Notice you have alignment issues on several lines here.
Please use 'clang-format', (if you don't know how ask..)

May you send me a link to read about clang-format?
Because I found something but I think that is not was you meant.

Nir-Az · 2023-02-13T15:17:46Z

+            break;
+
+        case ds::d400_caps::CAP_IP65:
+            device_name = std::regex_replace(device_name, old_name_reg, "D456"); // Change device name from D455 to D456.


Can we remove line 1109 and place here:

device_name = std::regex_replace(device_name, std::regex("D455"), "D456"); // Change device name from D455 to D456.

?
The parameter at line 1109 is only relevant on this case

Yes, you are absolutely right. I tried your solution and VS marked it like an error, so full of pain I moved it out of "switch".
But now it worked. It happens the second time. Maybe delay in IDE.
Changed according to your suggestion.

Tamir91 requested a review from Nir-Az February 7, 2023 14:53

Tamir91 commented Feb 7, 2023

View reviewed changes

Nir-Az reviewed Feb 8, 2023

View reviewed changes

Nir-Az reviewed Feb 9, 2023

View reviewed changes

new devices added

6e50b41

Tamir91 force-pushed the detect_new_products branch from 3c9f58a to 1f7fb3c Compare February 12, 2023 07:01

Nir-Az reviewed Feb 12, 2023

View reviewed changes

Tamir91 marked this pull request as draft February 12, 2023 07:19

Nir-Az reviewed Feb 12, 2023

View reviewed changes

Tamir91 marked this pull request as ready for review February 12, 2023 11:23

new rs devices added

1f79659

Tamir91 force-pushed the detect_new_products branch from 3b3e0ef to 1f79659 Compare February 12, 2023 12:45

Nir-Az reviewed Feb 12, 2023

View reviewed changes

Tamir91 and others added 2 commits February 13, 2023 15:23

A tabs removed

b8b8ced

Unnecessary line removed

65c73cd

Nir-Az reviewed Feb 13, 2023

View reviewed changes

Unnecessary variable removed

928b5a0

Nir-Az merged commit 14669fd into realsenseai:development Feb 16, 2023

Tamir91 deleted the detect_new_products branch February 26, 2023 09:29

ycpss91255 mentioned this pull request Jul 3, 2026

Build librealsense from source (versioned) instead of the ROS-packaged SDK ycpss91255-docker/realsense_ros1#88

Closed

Uh oh!

Conversation

Tamir91 commented Feb 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tamir91 Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nir-Az commented Feb 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tamir91 commented Feb 9, 2023

Uh oh!

Nir-Az commented Feb 9, 2023

Uh oh!

Nir-Az commented Feb 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tamir91 Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tamir91 Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tamir91 Feb 8, 2023 •

edited

Loading

Tamir91 Feb 12, 2023 •

edited

Loading

Tamir91 Feb 12, 2023 •

edited

Loading