Auto Exp/Gain Limit feature addition to D435i and D455 by noacoohen · Pull Request #12582 · realsenseai/librealsense

noacoohen · 2024-01-17T14:58:28Z

FW version < 5.12.11.0 No exp/gain limit support
FW version >= 5.12.11.0 AND FW version < 5.13.0.200 using AUTO_CALIB command
FW Version >= 5.13.0.200 using GETAELIMITS, SETAELIMITS commands
reactivated test-auto-limits.cpp

Tracked by RSDSO-19409

Nir-Az · 2024-01-17T18:44:11Z

-//#test:donotrun

 #include "../../catch.h"
 #include "../../unit-tests-common.h"


Please change
//#test:device D400* !D457
to specific D435i && D455 and remove the are_limit_options_supported logic.

This test will pass even if you forgot to register this option
Let's make sure this test actually run

Nir-Az · 2024-01-17T18:50:27Z

    public:
-        auto_exposure_limit_option(hw_monitor& hwm, sensor_base* depth_ep, option_range range, std::shared_ptr<limits_option> exposure_limit_enable);
+        auto_exposure_limit_option( hw_monitor & hwm,
+                                    sensor_base * depth_ep,


@OhadMeir do we want a weak_ptr here?

I think we can remove this parameter completely, it is not used.
In other places we used a weak_ptr to check the sensor before sending HMC, but looking now I see that it is checked by locked_transfer so no need here.

We still need to do some order in all this, command_transfer_over_xu might need a weak_ptr and there are still options using direct reference to the sensor. In another PR.

Nir-Az · 2024-01-17T18:50:52Z

    public:
-        auto_gain_limit_option(hw_monitor& hwm, sensor_base* depth_ep, option_range range, std::shared_ptr<limits_option> gain_limit_enable);
+        auto_gain_limit_option( hw_monitor & hwm,
+                                sensor_base * depth_ep,


same, weak_ptr?

Nir-Az · 2024-01-17T18:51:17Z

@@ -100,52 +109,101 @@ namespace librealsense
        hw_monitor& _hwm;
        sensor_base* _sensor;


Nir-Az · 2024-01-17T18:53:55Z

                  dev_info, d400_device::_hw_monitor, get_firmware_logs_command(), get_flash_logs_command() )
        {
            check_and_restore_rgb_stream_extrinsic();
+            firmware_version fw_ver = firmware_version( get_info( RS2_CAMERA_INFO_FIRMWARE_VERSION ) );


I think we already have _fw_version set no?

Nir-Az · 2024-01-17T18:54:34Z

                  dev_info, d400_device::_hw_monitor, get_firmware_logs_command(), get_flash_logs_command() )
            , d400_thermal_tracking( d400_device::_thermal_monitor )
        {
+            firmware_version fw_ver = firmware_version( get_info( RS2_CAMERA_INFO_FIRMWARE_VERSION ) );


Nir-Az · 2024-01-17T18:56:08Z


-    auto_exposure_limit_option::auto_exposure_limit_option(hw_monitor& hwm, sensor_base* ep, option_range range, std::shared_ptr<limits_option> exposure_limit_enable)
-        : option_base(range), _hwm(hwm), _sensor(ep), _exposure_limit_toggle(exposure_limit_enable)
+    auto_exposure_limit_option::auto_exposure_limit_option( hw_monitor & hwm,


Can we place in the feature file?
Or does it really complicate things?

We can place non generic options in the feature file, but we need to consider the right place for the feature as well.
This is a d400 option, is the feature d400 only? Is it relevant to d500? If not then ds/features is not the right place for it.

Nir-Az · 2024-01-17T18:56:23Z

-    auto_exposure_limit_option::auto_exposure_limit_option(hw_monitor& hwm, sensor_base* ep, option_range range, std::shared_ptr<limits_option> exposure_limit_enable)
-        : option_base(range), _hwm(hwm), _sensor(ep), _exposure_limit_toggle(exposure_limit_enable)
+    auto_exposure_limit_option::auto_exposure_limit_option( hw_monitor & hwm,
+                                                            sensor_base * ep,


Nir-Az · 2024-01-17T18:57:31Z

-    auto_gain_limit_option::auto_gain_limit_option(hw_monitor& hwm, sensor_base* ep, option_range range, std::shared_ptr <limits_option> gain_limit_enable)
-        : option_base(range), _hwm(hwm), _sensor(ep), _gain_limit_toggle(gain_limit_enable)
+    auto_gain_limit_option::auto_gain_limit_option( hw_monitor & hwm,
+                                                    sensor_base * ep,


Nir-Az · 2024-01-17T18:58:15Z

-        command cmd(ds::AUTO_CALIB);
-        cmd.param1 = 5;
+        float ret;
+        if( _ae_gain_limits_new_opcode )


Maybe we need 2 features?
instead of using if/else everywhere?

Nir-Az · 2024-01-17T18:59:06Z

-            cmd.param2 = *(reinterpret_cast<uint32_t*>(ret.data()));
-            cmd.param3 = static_cast<int>(set_limit);
-            if (_option == RS2_OPTION_AUTO_EXPOSURE_LIMIT_TOGGLE)
+            if( _ae_gain_limits_new_opcode )


2 features?

I think we need 2 features (see another comment) but here it is splitting to 2 options.
At least using helper functions.

if( _new_opcode ) set_using_new_opcode() else set_using_old_opcode()

OhadMeir · 2024-01-18T12:13:54Z

+    auto exposure_range = sensor.get_option( RS2_OPTION_EXPOSURE ).get_range();
+    auto gain_range = sensor.get_option( RS2_OPTION_GAIN ).get_range();
+
+    bool auto_exposure_new_opcode = false;


Why initialize a value you are about to change? Declare and initialize in the same expression.

OhadMeir · 2024-01-18T12:16:10Z

+
+    bool auto_exposure_new_opcode = false;
+    auto fw_ver = firmware_version(sensor.get_info( RS2_CAMERA_INFO_FIRMWARE_VERSION ));
+    auto_exposure_new_opcode = (fw_ver >= firmware_version( 5, 13, 0, 200 )) ? true : false;


Trinary expression ( ?: ) redundant.
Use bool opcode = fw_ver >= firmware_version( 5, 13, 0, 200 )

OhadMeir · 2024-01-18T12:40:31Z

        hw_monitor& _hwm;
        sensor_base* _sensor;
        std::weak_ptr<limits_option> _exposure_limit_toggle;
+        bool _ae_gain_limits_new_opcode;


No need to mention gain in AE option, rename _new_opcode.
Same goes for other occurences of this name.

OhadMeir · 2024-01-18T12:45:25Z

+            std::vector< uint8_t > res;
+            if( _ae_gain_limits_new_opcode )
+            {
+               offset = 8;


Indentation

OhadMeir · 2024-01-18T12:47:45Z


        virtual void set(float value) override
        {
            auto set_limit = _cached_limit;


Move implementation to cpp file

OhadMeir · 2024-01-18T12:50:02Z

+               offset = 8;
+                if( _option == RS2_OPTION_AUTO_GAIN_LIMIT_TOGGLE )


It might be better to have ae_limits_option and gain_limits_option inheriting limits_option, each calling a shared function passing a different offset value.

OhadMeir · 2024-01-18T13:22:44Z

-        _hwm.send(cmd);
-        _record_action(*this);
+        if( _ae_gain_limits_new_opcode )
+        {


Use helper function for readability. Same for other places in these options.

if( _new_opcode ) use_ae_limits_opcode(); else use_auto_calib_opcode();

OhadMeir · 2024-01-18T13:25:30Z

+class synthetic_sensor;
+class hw_monitor;
+
+class auto_exposure_gain_limit_feature : public feature_interface


I think we need to seperate it to a range_limit_featrue that is inherited by auto_exposure_limit_feature and gain_limit_feature.
Maybe we will need later for other ranges as well - brightness, hue, contrast etc...

OhadMeir · 2024-01-18T13:27:03Z

-// auto-limits option is deprecated currently [LRS-487]
-//#test:donotrun

 #include "../../catch.h"


Won't it be better to change to a python test?

OhadMeir · 2024-01-22T14:05:04Z

+class synthetic_sensor;
+class hw_monitor;
+
+class range_limit_feature : public feature_interface


Not exactly what I meant. As it is now, there is no need for range_limit_feature, it serves no purpose.

option 1 - Having 2 completely separate features for AE and gain.
Delete range_limit_feature and split auto_exposure_limit_feature and gain_limit_feature into separate files.

option 2 - Using range_limit_feature for common implementation.
Delete the public interface, we won't have objects of this type.
For instance you can have protected API with functions like:
option_range get_range( rs2_option opt );
std::shared_ptr< limits_option > create_limits_option( rs2_option opt, const char * description );
...
Or each inheriting feature will set members that a common function will use.

Your call which option you prefer

OhadMeir · 2024-01-22T18:34:28Z

+        : option_base( range )
+        , _hwm( hwm )
+        , _sensor( ep )
+        , _exposure_limit_toggle( exposure_limit_enable )


You can use exposure_limit_enable in line 85 without the need to lock.

OhadMeir · 2024-01-22T18:37:31Z

-        if (ret< get_range().min || ret > get_range().max)
+        float ret;
+        if( _new_opcode )
+        {


The current coding convention is not to add braces for one line if/else expressions

OhadMeir · 2024-01-22T18:43:34Z

-
-        auto ret = static_cast<float>(*(reinterpret_cast<uint32_t*>(res.data())));
-        if (ret< get_range().min || ret > get_range().max)
+        float ret;


Make a habit of initializing all declared variables.
float ret = get_range().def;
or
float ret = _new_opcode ? query_using_new_opcode() : query_using_old_opcode();

OhadMeir · 2024-01-22T18:50:26Z

+        if( ret.empty() )
+            throw invalid_value_exception( "auto_exposure_limit_option::query result is empty!" );
+
+        static const int offset = 12;


Use more verbose names. Is this max_ae_offset?

OhadMeir · 2024-01-28T10:04:46Z

+public:
+    static const feature_id ID;
+
+    gain_limit_feature( synthetic_sensor & depth_sensor, std::shared_ptr< hw_monitor > hw_monitor );


Pass const std::shared_ptr< hw_monitor > & as parameter.
If you pass by value there are unneeded calls to increase and decrease the ref count.

OhadMeir · 2024-01-28T10:05:27Z

+
+class synthetic_sensor;
+class hw_monitor;
+class limits_option;


Not needed here

OhadMeir · 2024-01-28T10:05:56Z

+#include <src/feature-interface.h>
+
+#include <memory>
+#include <librealsense2/hpp/rs_options.hpp>


Can it be included only in the cpp?

OhadMeir · 2024-01-30T10:00:29Z

+from rspy import test
+
+#############################################################################################
+with test.closure("Gain/Exposure auto limits"):


Split into separate tests - gain toggle one device, gain toggle 2 devices, ae toggle one device, ae toggle 2 devices.
Also are these all the use cases? Test trying to set value when toggle is off etc...

…ases

noacoohen requested a review from Nir-Az January 17, 2024 14:58

Nir-Az reviewed Jan 17, 2024

View reviewed changes

Comment thread src/ds/features/auto-exposure-gain-limit-feature.h

Nir-Az reviewed Jan 17, 2024

View reviewed changes

OhadMeir reviewed Jan 18, 2024

View reviewed changes

OhadMeir reviewed Jan 22, 2024

View reviewed changes

OhadMeir reviewed Jan 28, 2024

View reviewed changes

Tamir91 requested review from Nir-Az and removed request for Nir-Az January 29, 2024 11:39

OhadMeir reviewed Jan 30, 2024

View reviewed changes

noacoohen added 13 commits January 31, 2024 14:51

add auto exposure/gain limit feature

1c147d8

reactivated test-auto-limits.cpp

e514678

use space instead of tabs

b4f0909

fixing PR comments

9b4e92f

fix compilation warning

dcc8b55

seperate the 2 features to different files, fix PR comments

e7765d0

replace sensor_base pointer to weak_ptr in options

1835850

fix PR comments

126b11e

converted test-auto-limts to python

ce0b73e

disable cpp test from running

83c785e

remove the cpp test and seperate the python test to 4 seperate test c…

e2d919a

…ases

remove loop

0da7ca0

remove device for loop and picked_sensor dict

8f78079

noacoohen force-pushed the auto_exp_and_gain_limit_missing branch from 5550864 to 8f78079 Compare January 31, 2024 12:53

Disable the test from running with D435i

38363af

OhadMeir approved these changes Feb 1, 2024

View reviewed changes

OhadMeir merged commit 52fc021 into realsenseai:development Feb 1, 2024

MartyG-RealSense mentioned this pull request Mar 7, 2024

RS2_OPTION_AUTO_EXPOSURE_LIMIT for camera D455 not supported realsenseai/realsense-ros#3037

Closed

		@@ -100,52 +109,101 @@ namespace librealsense
		hw_monitor& _hwm;
		sensor_base* _sensor;

		offset = 8;
		if( _option == RS2_OPTION_AUTO_GAIN_LIMIT_TOGGLE )

Uh oh!

Conversation

noacoohen commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants