Support L535 options #8952

aangerma · 2021-05-04T06:12:38Z

Created l535_hw_options class for l535 options.
removed option RS2_OPTION_DIGITAL_GAIN not applicable for l535
added option RS2_OPTION_RX_SENSITIVITY
removed all preset values (except custom) for now - until we will have declaration of presets for L535
removed options RS2_OPTION_ENABLE_MAX_USABLE_RANGE and RS2_OPTION_ENABLE_IR_REFLECTIVITY -for now until we will have declaration of them for L535

maloel · 2021-05-04T09:57:48Z

src/l500/l535-options.h

+        l535_get_default = 4
+    };
+
+    typedef uvc_xu_option< int > super;


maloel · 2021-05-04T10:25:58Z

src/l500/l535-options.h

+
+    typedef uvc_xu_option< int > super;
+
+    class l535_hw_options : public option


Shouldn't be in this header

amc_option?

maloel · 2021-05-04T10:27:08Z

src/l500/l535-options.h

+
+namespace librealsense
+{
+    enum l535_control


Add l535 namespace inside ivcam2 namespace

Name something like amc_control

maloel · 2021-05-04T10:27:50Z

src/l500/l535-options.h

+        l535_alternate_ir = 8
+    };
+
+    enum l535_command


amc_command

maloel · 2021-05-04T10:28:29Z

src/l500/l535-options.h

+        std::string _description;
+    };
+
+    class l535_preset_option : public float_option_with_description< rs2_l500_visual_preset >


Shouldn't be in this header

src/l500/l535-options.cpp

maloel · 2021-05-04T10:37:39Z

src/l500/l535-options.cpp

+#include "l500-private.h"
+#include "l500-depth.h"
+
+const std::string MIN_CONTROLS_FW_VERSION("1.3.9.0");


These still applicable?

maloel · 2021-05-04T10:41:46Z

src/l500/l535-options.cpp

+
+namespace librealsense
+{
+    using namespace ivcam2;


maloel · 2021-05-05T08:25:43Z

include/librealsense2/h/rs_option.h

@@ -109,6 +109,7 @@ extern "C" {
        RS2_OPTION_ENABLE_IR_REFLECTIVITY, /**< Enables data collection for calculating IR pixel reflectivity  */
        RS2_OPTION_AUTO_EXPOSURE_LIMIT, /**< Set and get auto exposure limit in microseconds. Default is 0 which means full exposure range. If the requested exposure limit is greater than frame time, it will be set to frame time at runtime. Setting will not take effect until next streaming session. */
        RS2_OPTION_AUTO_GAIN_LIMIT, /**< Set and get auto gain limits ranging from 16 to 248. Default is 0 which means full gain. If the requested gain limit is less than 16, it will be set to 16. If the requested gain limit is greater than 248, it will be set to 248. Setting will not take effect until next streaming session. */
+        RS2_OPTION_RX_SENSITIVITY,


Description?

I'm waiting for description from Aviad

maloel · 2021-05-05T08:28:07Z

src/l500/l535-options.cpp

+#include "l500-private.h"
+#include "l500-depth.h"
+
+namespace librealsense


I don't like the .cpp files indenting like this.
Won't a single using librealsense::ivcam2::l535::device_options; do?

maloel · 2021-05-05T08:28:57Z

src/l500/l535-options.h

+    {
+        namespace l535
+        {
+            class l535_options : public virtual l500_device


Rename device_options

maloel · 2021-05-05T08:29:33Z

src/l500/l535-preset-option.cpp

+#include "l500-private.h"
+#include "l500-depth.h"
+
+namespace librealsense


maloel · 2021-05-05T08:30:34Z

src/l500/l535-preset-option.cpp

+            void l535_preset_option::set(float value)
+            {
+                if (static_cast<rs2_l500_visual_preset>(int(value))
+                    == RS2_L500_VISUAL_PRESET_DEFAULT)


Can this happen?? We should throw for anything not custom...

src/l500/l535-preset-option.h

maloel · 2021-05-05T08:31:57Z

src/l500/l535-preset-option.h

+        {
+            class l535_preset_option : public float_option_with_description< rs2_l500_visual_preset >
+            {
+            public:


Use super because it repeats in the .cpp

maloel · 2021-05-05T08:32:07Z

src/l500/l535-preset-option.h

+            class l535_preset_option : public float_option_with_description< rs2_l500_visual_preset >
+            {
+            public:
+                l535_preset_option(option_range range, std::string description);


maloel · 2021-05-05T08:32:32Z

src/types.cpp

@@ -455,6 +455,7 @@ namespace librealsense
            case RS2_OPTION_ENABLE_IR_REFLECTIVITY: return "Enable IR Reflectivity";
            CASE(AUTO_EXPOSURE_LIMIT)
            CASE(AUTO_GAIN_LIMIT)
+            CASE(RX_SENSITIVITY)


We want this displayed as "Rx Sensitivity"?

maloel · 2021-05-05T08:33:43Z

wrappers/python/pybackend.cpp

@@ -187,6 +187,7 @@ PYBIND11_MODULE(NAME, m) {
        .value("enable_ir_reflectivity", RS2_OPTION_ENABLE_IR_REFLECTIVITY)
        .value("auto_exposure_limit", RS2_OPTION_AUTO_EXPOSURE_LIMIT)
        .value("auto_gain_limit", RS2_OPTION_AUTO_GAIN_LIMIT)
+        .value("rx_sensitivity", RS2_OPTION_RX_SENSITIVITY)


I think you're missing the C# and other wrappers?

will do when I will have the exact name of the control.

maloel · 2021-05-05T08:34:46Z

src/l500/l535-preset-option.h

+    {
+        namespace l535
+        {
+            class l535_preset_option : public float_option_with_description< rs2_l500_visual_preset >


Don't need the l535 in the name, do we?

maloel · 2021-05-05T08:37:21Z

src/l500/l535-options.cpp

+                                      RS2_L500_VISUAL_PRESET_CUSTOM,
+                                      1,
+                                      RS2_L500_VISUAL_PRESET_CUSTOM },
+                        "Preset to calibrate the camera to environment ambient, no ambient or low "


maloel · 2021-05-05T08:39:19Z

src/l500/l535-amc-option.h

+        namespace l535
+        {
+
+            enum amc_control


Shouldn't we list only the controls that are applicable to the L535 here?

its all of them + rx_sensitivity

maloel · 2021-05-05T08:40:47Z

src/l500/l535-amc-option.cpp

+#include "l500-private.h"
+#include "l500-depth.h"
+
+const std::string MIN_CONTROLS_FW_VERSION("1.3.9.0");


maloel · 2021-05-05T08:41:36Z

src/l500/l535-amc-option.cpp

+            void amc_option::enable_recording(std::function<void(const option&)> recording_action)
+            {}
+
+            float amc_option::query_current(rs2_sensor_mode mode) const //todo: remove sensor mode


removed query_current

maloel · 2021-05-05T08:42:13Z

src/l500/l535-amc-option.cpp

+                return float(val);
+            }
+
+            amc_option::amc_option(l500_device* l500_dev,


Put ctor first in file please

maloel · 2021-05-05T08:42:57Z

src/l500/l535-amc-option.h

+            private:
+                float query_default() const;
+
+                amc_control _type;


Rename _control

maloel · 2021-05-05T08:43:34Z

src/l500/l535-amc-option.h

+                float query_default() const;
+
+                amc_control _type;
+                l500_device* _l500_dev;


Remove l500 from the name, _device

maloel

Looking much better already!

maloel · 2021-05-06T07:38:11Z

Please rebase

maloel · 2021-05-06T14:09:24Z

src/l500/CMakeLists.txt

@@ -21,6 +24,9 @@ target_sources(${LRS_TARGET}
        "${CMAKE_CURRENT_LIST_DIR}/l500-fw-update-device.h"
        "${CMAKE_CURRENT_LIST_DIR}/l500-serializable.h"
        "${CMAKE_CURRENT_LIST_DIR}/l500-options.h"
+        "${CMAKE_CURRENT_LIST_DIR}/l535-options.h"


shouldn't this be l535-device-options.h?

maloel · 2021-05-06T14:11:10Z

src/l500/l535-amc-option.cpp

+
+
+using namespace librealsense;
+using namespace librealsense::ivcam2::l535;


Why not l535::amc_option?

maloel · 2021-05-06T14:13:04Z

src/l500/l535-amc-option.cpp

+
+float amc_option::query() const
+{
+    auto res = _hw_monitor->send( command{ AMCGET, _control, get_current, RS2_SENSOR_MODE_VGA } );


Please comment why we're sending the sensor mode when it shouldn't be needed

maloel · 2021-05-06T14:13:44Z

src/l500/l535-amc-option.cpp

+float amc_option::query_default() const
+{
+    auto res = _hw_monitor->send(
+        command{ AMCGET, _control, l500_command::get_default, RS2_SENSOR_MODE_VGA }


Here, too....

maloel · 2021-05-06T14:14:45Z

src/l500/l535-options.cpp

+#include "l500-depth.h"
+
+using namespace librealsense;
+using namespace librealsense::ivcam2::l535;


Why not l535::device_options?

maloel · 2021-05-06T14:16:24Z

src/l500/l535-options.cpp

+              { min_distance, "Minimal distance to the target (in mm)" } },
+            { RS2_OPTION_INVALIDATION_BYPASS,
+              { invalidation_bypass, "Enable/disable pixel invalidation" } },
+            { RS2_OPTION_INVALIDATION_BYPASS,


Two invalidation bypass?

maloel · 2021-05-06T14:18:41Z

src/l500/l535-preset-option.cpp

+#include "l500-depth.h"
+
+using namespace librealsense;
+using namespace librealsense::ivcam2::l535;


Why not l535::preset_option?

and added to wrappers.

maloel reviewed May 4, 2021

View reviewed changes

src/l500/l535-options.cpp Outdated Show resolved Hide resolved

maloel reviewed May 4, 2021

View reviewed changes

maloel reviewed May 5, 2021

View reviewed changes

src/l500/l535-preset-option.h Outdated Show resolved Hide resolved

maloel reviewed May 5, 2021

View reviewed changes

aangerma added 2 commits May 6, 2021 12:40

Support L535 options

804f5f4

Added rx_sensitivity (auto gain) option

00d349f

aangerma added 3 commits May 6, 2021 12:40

CR fixes

c1be1d2

CR fixes

33f7025

update l500_serializable

1ce3ae7

aangerma force-pushed the L353-options branch from c771a82 to 1ce3ae7 Compare May 6, 2021 09:46

Support for RS2_OPTION_RX_SENSITIVITY on nodejs

a4b6adf

aangerma force-pushed the L353-options branch from 47ed3ea to a4b6adf Compare May 6, 2021 12:40

maloel reviewed May 6, 2021

View reviewed changes

aangerma added 2 commits May 9, 2021 12:08

CR fixes

8307356

change RX_SENSITIVITY to AUTO_RX_SENSITIVITY,

4a74d6a

and added to wrappers.

maloel approved these changes May 10, 2021

View reviewed changes

Merge branch 'development' into L353-options

9c412cc

maloel merged commit b8ec3b5 into IntelRealSense:development May 10, 2021


		typedef uvc_xu_option< int > super;

		class l535_hw_options : public option



		using namespace librealsense;
		using namespace librealsense::ivcam2::l535;

Support L535 options #8952

Support L535 options #8952

Conversation

aangerma commented May 4, 2021 • edited by maloel Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maloel left a comment

Choose a reason for hiding this comment

maloel commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aangerma commented May 4, 2021 •

edited by maloel

Loading