Reset Auto Exp/Gain Limit back to default value 0 by nohayassin · Pull Request #9692 · realsenseai/librealsense

nohayassin · 2021-08-26T12:42:01Z

Track on DSO-17393

ev-mp

I can't find the segment that shows how the manual control overrides the toggle

ev-mp · 2021-08-26T19:44:21Z

+        RS2_OPTION_ENABLE_EXPOSURE_LIMIT, /**< Enable / disable color image auto-exposure*/
+        RS2_OPTION_ENABLE_GAIN_LIMIT, /**< Enable / disable color image auto-gain*/


Options must be added at the end of enum for back-compat.
Rename the according to the main controls: auto_exposure_limit_on, auto_gain_limit_on

ev-mp · 2021-08-26T19:45:51Z


+    // Limits Enable/ Disable
+
+    class limits_option : public option


why a new class?

ev-mp · 2021-08-26T19:53:26Z

+        virtual ~limits_option() = default;
+        virtual void set(float value) override
+        {
+            _value = value; // 0: gain limit set by user is enabled, 1 : gain limit is disabled (all range 16-248 is valid)


0 should correspond to off, 1 -> On

ev-mp · 2021-08-26T20:00:34Z

+            if (value == 1) // disabled: save current limit
+                _cached_limit = _sensor->get_option(RS2_OPTION_AUTO_GAIN_LIMIT).query();


1.Shouldn't be in the base class
2. What happens in case polling the data returns 0?

ev-mp · 2021-08-26T20:03:15Z

+            case RS2_OPTION_ENABLE_GAIN_LIMIT: return "Disable gain limit";
+            case RS2_OPTION_ENABLE_EXPOSURE_LIMIT: return "Disable exposure limit";


Why is it required here?

ev-mp · 2021-08-31T06:16:34Z

        RS2_OPTION_VERTICAL_BINNING, /**< Enables vertical binning which increases the maximal sensed distance. */
        RS2_OPTION_RECEIVER_SENSITIVITY, /**< Control receiver sensitivity to incoming light, both projected and ambient (same as APD on L515). */
-        RS2_OPTION_COUNT /**< Number of enumeration values. Not a valid input: intended to be used in for-loops. */
+        RS2_OPTION_COUNT, /**< Number of enumeration values. Not a valid input: intended to be used in for-loops. */


Add those before count which must remain last

ev-mp · 2021-08-31T06:17:59Z

+            std::shared_ptr<auto_exposure_limit_option> auto_exposure_limit_enable_option = nullptr;
+            std::shared_ptr<auto_gain_limit_option> auto_gain_limit_enable_option = nullptr;


Combine the declaration and allocation into a single statement

ev-mp · 2021-08-31T06:19:18Z

+            auto_exposure_limit_enable_option = std::make_shared<auto_exposure_limit_option>(*_hw_monitor, &depth_sensor, exposure_range);
+            auto_gain_limit_enable_option = std::make_shared<auto_gain_limit_option>(*_hw_monitor, &depth_sensor, gain_range);
+
+            option_range enable_range = { 0.f /*min*/, 1.f /*max*/, 1.f /*step*/, 1.f /*default*/ };


Make it disabled by default

ev-mp · 2021-08-31T06:22:58Z

+
+            //GAIN Limit
+            std::shared_ptr<gain_limit_option> gain_limit_enable_option = nullptr;
+            gain_limit_enable_option = std::make_shared<gain_limit_option>(RS2_OPTION_AUTO_GAIN_LIMIT_ON, enable_range, auto_gain_limit_enable_option.get());


Pass the dependent option with via shared/weak_ptr according to the content

ev-mp · 2021-08-31T06:26:06Z

    void auto_exposure_limit_option::set(float value)
    {
-        if (!is_valid(value))
+        if (!is_valid(value) && value != 0)


is_valid shall handle all cases, why the extra clause?

ev-mp · 2021-08-31T06:28:23Z

+        if (value != 0)
+            _cached_limit = value;
+        else
+            value = get_range().max;


Injecting "0" is not a valid use-case for the control, - must have the same range as the corresponding UVC control

ev-mp · 2021-08-31T06:28:33Z

    void auto_gain_limit_option::set(float value)
    {
-        if (!is_valid(value))
+        if (!is_valid(value) && value != 0)


Same as above

ev-mp · 2021-08-31T11:42:31Z

+            case RS2_OPTION_AUTO_GAIN_LIMIT_ON: return "Enable gain limit";
+            case RS2_OPTION_AUTO_EXPOSURE_LIMIT_ON: return "Enable exposure limit";


Imo redundant here

It is required by design

ev-mp · 2021-08-31T11:44:38Z

+        virtual ~limits_option() = default;
+        virtual void set(float) override = 0;
+        virtual float query() const override { return _value; };
+        virtual option_range get_range() const override { return _toggle_range; };
+        virtual bool is_enabled() const override { return true; }
+        virtual const char* get_description() const override { return "Enable Limits Option"; };
+        virtual void enable_recording(std::function<void(const option&)> record_action) override { _record_action = record_action; }


most of the override re-implement the base-class functionality.
Needs refactoring

ev-mp · 2021-09-12T06:53:34Z

-            _cached_limit = value;
+
+        _exposure_limit_enable->set_cached_limit(value);
+        if (value != get_range().max && _exposure_limit_enable->query() == 0.f)


Why the max value is not allowed?

I wanted to keep the toggle off at the beginning but I can remove the max-value condition.
The second part of the condition is relevant to prevent redundant commands to FW.

ev-mp · 2021-09-12T06:54:19Z


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


As discussed, pass and store the option's handler via weak_ptr

ev-mp · 2021-09-12T06:56:44Z

+            std::shared_ptr<limits_option>  exposure_limit_enable_option = std::make_shared<limits_option>(RS2_OPTION_AUTO_EXPOSURE_LIMIT_ON, enable_range, "Enable Exposure auto-Limit", *_hw_monitor);
+            std::shared_ptr<auto_exposure_limit_option> auto_exposure_limit_enable_option = std::make_shared<auto_exposure_limit_option>(*_hw_monitor, &depth_sensor, exposure_range, exposure_limit_enable_option.get());
+            depth_sensor.register_option(RS2_OPTION_AUTO_EXPOSURE_LIMIT_ON, exposure_limit_enable_option);


The names are confusing, make it more distinct, like "ae_limit_toggle_control", "ae_limit_value_control"

ev-mp · 2021-09-12T07:00:02Z

            //GAIN Limit
-            std::shared_ptr<gain_limit_option> gain_limit_enable_option = std::make_shared<gain_limit_option>(RS2_OPTION_AUTO_GAIN_LIMIT_ON, enable_range, auto_gain_limit_enable_option.get());
-            depth_sensor.register_option(RS2_OPTION_AUTO_GAIN_LIMIT, auto_gain_limit_enable_option);
+            std::shared_ptr<limits_option>  gain_limit_enable_option = std::make_shared<limits_option>(RS2_OPTION_AUTO_GAIN_LIMIT_ON, enable_range, "Enable Gain auto-Limit", *_hw_monitor);


"Enable Gain auto-Limit" -> "Toggle Auto-Gain Limit"

ev-mp · 2021-09-12T07:01:38Z

            CASE(RECEIVER_SENSITIVITY)
-            case RS2_OPTION_AUTO_GAIN_LIMIT_ON: return "Enable gain limit";
-            case RS2_OPTION_AUTO_EXPOSURE_LIMIT_ON: return "Enable exposure limit";
+            CASE(AUTO_GAIN_LIMIT_ON)


Pls rename the options to RS2_OPTION_AUTO_XXX_LIMIT_TOGGLE

ev-mp · 2021-09-12T08:15:02Z


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


replace flat pointer with smart

ev-mp · 2021-09-12T08:17:37Z

            throw invalid_value_exception("set(enable_auto_gain) failed! Invalid Auto-Gain mode request " + std::to_string(value));

+        _gain_limit_enable->set_cached_limit(value);
+        if(value != get_range().max && _gain_limit_enable->query() == 0.f)


As long as the value is valid the toggle shall be set on.
At this point the valid is already verified to be within the range - I don't think the check is required

ev-mp · 2021-09-12T08:18:09Z

    {
    public:
-        auto_exposure_limit_option(hw_monitor& hwm, sensor_base* depth_ep, option_range range);
+        auto_exposure_limit_option(hw_monitor& hwm, sensor_base* depth_ep, option_range range, limits_option* exposure_limit_enable);


use smart_ptrs here and below

ev-mp · 2021-09-12T08:24:28Z

+        std::cout << "1. Check if initial values of limit control is set to max value" << std::endl;
+        auto init_limit = s.get_option(limit_control);
+        //auto enable = s.get_option(enable_limit);
+        REQUIRE(init_limit == range.max);


we can't use this assumption, the value is stored in FW only

ev-mp · 2021-09-13T05:52:08Z

+            // 1. Toggle : 
+            //        - if toggle is off check that control limit value is 0
+            //        - if toggle is on check that control value is in the correct range
+            auto limit = s.get_option(limits_value[i]);
+            auto toggle = s.get_option(limits_toggle[i]);
+            if (toggle == 0)
+                REQUIRE(limit == 0);


For the limit control zero is not in the valid range. It is only applicable to the toggle

ev-mp · 2021-09-13T05:58:24Z

-        s.set_option(enable_limit, 0.f);
-        auto post_disable = s.get_option(limit_control);
-        REQUIRE(post_disable == range.max);
+        for (auto i = 0; i < 2; i++) // exposure or gain


Also run the following check:

toggle on both dev1 and dev2 and set two distinct values for the auto-exposure /gain.

toggle both dev1 and dev2 off.

toggle dev1 on:

verify that the limit value is the value that was stored (cached) in dev1.

verify that for dev2 both the limit and the toggle values are similar to those of dev1

ev-mp · 2021-09-13T05:59:37Z

+            std::string val = s.get_info(RS2_CAMERA_INFO_NAME);
+            if (!s.supports(limits_value[i]))


to simplify the flow extract the depth sensor and check if the option is supported in

std::array < std::vector, 2> sensors = { dev1.query_sensors(), dev2.query_sensors() };

ev-mp · 2021-09-13T06:01:25Z

Note that there is a merge conflict with the dev branch - needs to be resolved

ev-mp

There are multiple unrequired files in src/types_XXX.cpp form to be removed

ev-mp · 2021-09-26T06:34:22Z

+        float query_limit_value() 
+        {
+            auto offset = 0;
+            if (_option == RS2_OPTION_AUTO_GAIN_LIMIT_TOGGLE)
+                offset = 4;
+            command cmd(ds::AUTO_CALIB);
+            cmd.param1 = 5;
+            auto res = _hwm.send(cmd);
+            if (res.empty())
+                throw invalid_value_exception("auto_exposure_limit_option::query result is empty!");
+
+            return static_cast<float>(*(reinterpret_cast<uint32_t*>(res.data() + offset)));
+        };


The call returns the actual value stored, rather than [0,1]. Is it intended?

This func is redundant - removed

ev-mp · 2021-09-26T06:38:56Z

Rebase the code - this segment has been moved to "ro-string.cpp"

ev-mp · 2021-09-26T06:59:57Z

-        float _value;
        option_range _toggle_range;
        const std::map<float, std::string> _description_per_value;
        float _cached_limit;


No need here - use the original "float _value;"

it is not used

Add maintainer's comment then

ev-mp · 2021-09-26T07:06:04Z

        std::shared_ptr<lazy<rs2_extrinsics>> _color_extrinsic;
        bool _is_locked = true;
+
+        std::shared_ptr<limits_option>  _gain_limit_toggle_control;


_gain_limit_toggle_control and _ae_limit_toggle_control are registered directly into the options container. No need for extra owner

ev-mp · 2021-09-26T07:06:56Z

        hw_monitor& _hwm;
        sensor_base* _sensor;
-        std::shared_ptr<limits_option> _exposure_limit_enable;
+        std::shared_ptr<limits_option> _exposure_limit_toggle;


Store a weak_ptr to the handle

It doesn't work with weak_ptr

ev-mp · 2021-09-26T07:07:02Z

        hw_monitor& _hwm;
        sensor_base* _sensor;
-        std::shared_ptr<limits_option> _gain_limit_enable;
+        std::shared_ptr<limits_option> _gain_limit_toggle;


ev-mp · 2021-09-26T07:11:17Z

        virtual bool is_enabled() const override { return true; }
        virtual const char* get_description() const override { return _description; };


Check if those already exist in the base class

Those are not implemented in base class

ev-mp · 2021-09-26T07:15:11Z

+        float query_limit_value() 
+        {
+            auto offset = 0;
+            if (_option == RS2_OPTION_AUTO_GAIN_LIMIT_TOGGLE)
+                offset = 4;
+            command cmd(ds::AUTO_CALIB);
+            cmd.param1 = 5;
+            auto res = _hwm.send(cmd);
+            if (res.empty())
+                throw invalid_value_exception("auto_exposure_limit_option::query result is empty!");
+
+            return static_cast<float>(*(reinterpret_cast<uint32_t*>(res.data() + offset)));
+        };


I can't see references in code - where is it used?

ev-mp

Minor changes required

ev-mp · 2021-10-10T06:54:40Z

            return range;
        };
-        _exposure_limit_toggle->set_cached_limit(range.max);
+        _exposure_limit_toggle.lock()->set_cached_limit(range.max);


Add check for weak->share ptr conversion is successful

ev-mp · 2021-10-10T06:54:51Z

-        _exposure_limit_toggle->set_cached_limit(value);
-        if (_exposure_limit_toggle->query() == 0.f)
-            _exposure_limit_toggle->set(1);
+        _exposure_limit_toggle.lock()->set_cached_limit(value);


Also here and below in 703, 720, 728-30, 759

ev-mp

Looks good

ev-mp suggested changes Aug 26, 2021

View reviewed changes

ev-mp suggested changes Aug 31, 2021

View reviewed changes

ev-mp suggested changes Sep 12, 2021

View reviewed changes

ev-mp reviewed Sep 13, 2021

View reviewed changes

ev-mp suggested changes Sep 26, 2021

View reviewed changes

nohayassin added 18 commits October 3, 2021 10:13

added gain toggle

770cee9

added gain toggle

202036d

Added exposure limit button

347643c

fix design

5313ef5

fix design

01a810c

fix design

9e63be5

design fix

6748aa1

fixes

b572029

Added unit test

8ca7927

p fixed design

ee75b52

fixes

f9524bf

fixes

8d8d7bf

fixes

579a5f8

fixes

9f4126b

test plan fix - wip

ba95acc

fixes

5da4b15

unit test fixes

0ee4f19

unit test fixes

6fee1ac

nohayassin force-pushed the dso_17393_fix branch from 63bfa6a to 6fee1ac Compare October 3, 2021 07:20

nohayassin added 5 commits October 3, 2021 10:33

removed extra files

15f06ed

types.cpp

9e7f422

types.cpp

22e33b9

fix

80c36f5

fix

1b5adc5

nohayassin added 2 commits October 4, 2021 15:46

fixes

a66f7b2

fixes

9f05733

ev-mp suggested changes Oct 10, 2021

View reviewed changes

nohayassin added 3 commits October 10, 2021 10:53

fixes

dd3aa6b

java

71692cd

fixes

0a4b922

ev-mp approved these changes Oct 12, 2021

View reviewed changes

ev-mp merged commit dd48c6d into realsenseai:development Oct 12, 2021

		RS2_OPTION_ENABLE_EXPOSURE_LIMIT, /*< Enable / disable color image auto-exposure/
		RS2_OPTION_ENABLE_GAIN_LIMIT, /*< Enable / disable color image auto-gain/


		// Limits Enable/ Disable

		class limits_option : public option

		if (value == 1) // disabled: save current limit
		_cached_limit = _sensor->get_option(RS2_OPTION_AUTO_GAIN_LIMIT).query();

		case RS2_OPTION_ENABLE_GAIN_LIMIT: return "Disable gain limit";
		case RS2_OPTION_ENABLE_EXPOSURE_LIMIT: return "Disable exposure limit";

		std::shared_ptr<auto_exposure_limit_option> auto_exposure_limit_enable_option = nullptr;
		std::shared_ptr<auto_gain_limit_option> auto_gain_limit_enable_option = nullptr;

		case RS2_OPTION_AUTO_GAIN_LIMIT_ON: return "Enable gain limit";
		case RS2_OPTION_AUTO_EXPOSURE_LIMIT_ON: return "Enable exposure limit";

		std::string val = s.get_info(RS2_CAMERA_INFO_NAME);
		if (!s.supports(limits_value[i]))

		virtual bool is_enabled() const override { return true; }
		virtual const char* get_description() const override { return _description; };

Uh oh!

Conversation

nohayassin commented Aug 26, 2021

Uh oh!

ev-mp left a comment

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!

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!

ev-mp commented Sep 13, 2021

Uh oh!

ev-mp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nohayassin Oct 3, 2021 • 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

nohayassin Oct 3, 2021 •

edited

Loading