[WIP] Convert RealSense2 options to MetriCam2 parameter #47

jangernert · 2017-12-21T12:12:37Z

WORK IN PROGRESS!

This pull request was opened to get some early feedback while working on the changes.

@f00f:
As you can see each option can be set for each sensor in the camera. This doesn't mean the option is supported by all sensors, just that the API allows it. Currently there are 2 sensors in the camera samples we got (named RGB and Stereo). Should we hide this implementation detail in the camera implementation?

My idea is to check which option is supported by which sensor and if it makes sense to have it for that sensor. If just one of the sensor support it have just one parameter and implicitly set the desired sensor. If the option works with both sensors implement two parameter named something like Option27Color and Option27Stereo.

Another thing is: the API uses float values for all options. IMO it is nicer to have types that make sense for the specific option. For example an option that can be either turned off or on should be boolean and internally we use the float values 1 and 0.

jangernert · 2017-12-21T14:22:43Z

@f00f
Librealsense2 provides the settings EXPOSURE, ENABLE_AUTO_EXPOSURE and AUTO_EXPOSURE_MODE. So you can enable auto exposure and on top of that specify one of three auto exposure modes (Static, Anti-Flicker and Hybrid). So the question is now how to expose the property.
My first idea was to have an AutoExposure(Color/Depth) property that takes an enum with the specified mode and enables auto exposure as well. But it turns out the camera I'm using to test everything on (D435) does not support AUTO_EXPOSURE_MODE but does support ENABLE_AUTO_EXPOSURE.
So currently I only provide a property to enable/disable auto exposure and ignore the mode setting that could work with other cameras (don't know). What do you think would be the best solution here?

edit: Simon says (pun intended) the other intel cameras also don't support the AUTO_EXPOSURE_MODE and there aren't any switches in the intel tool for it either. So I'm just gonna omit it.

jangernert · 2017-12-21T17:00:16Z

All compatible options of the D435 have been turned into properties.
Still needs some testing and a review by @f00f

TODO for 2018:

Test all Properties with D435
Test all properties with D415
Remove methods to mess with realsense options directly
Don't expose RealSense2API anymore
Write summary comments for tool tips

jangernert · 2017-12-22T14:59:12Z

@f00f :
I noticed parameter with a RangeParamDesc accept values even outside the specified range just fine. Should the camera implementation itself check if the value is valid or is there a layer of MetriCam2 that should catch the mistake?

…exception

…s out of bounds

…t to 5000ms according to examples (500ms sometimes caused problems)

jangernert · 2018-01-10T15:15:47Z

@f00f review please =)

f00f · 2018-01-12T13:43:49Z

I'll try to address your questions/remarks in the order of appearance

Multiple sensors which might support the same parameter:
I agree that our interface should use typed parameters
AUTO_EXPOSURE_MODE: agree, ditch it. We can always care for it later, if needed.
RangeParamDesc: we have talked about that. So far no checks are done (in MC2 in general) if the property is set directly. You addressed that [Proposal] Rework Camera Properties #49

Now, I'll have a look at the code...

…same time. This seems to be a limitation of old libRealSense2 versions that no longer are valid

…them directly from the sensor first

…es as well

f00f · 2018-01-16T13:00:47Z

Just a note: the last three commits are off-topic.
They further delay this review which is already taking way too long.

jangernert · 2018-01-16T13:05:38Z

@mischastik needs the stereo calibration tool to work with realsense2 devices. These changes were necessary to make the application work. I just wanted to squeeze them in as fast as possible :D

f00f

Many of my comments apply to multiple code sections, in most cases I have not repeatet them.

In addition to the comments I'd prefer it if more specific Exception types were used.

f00f · 2018-01-12T15:42:12Z

BetaCameras/RealSense2/RealSense2API.cs

+                        HandleError(error);
+                        return height;
+                    }
+                    catch (Exception)


What's the use of this try-catch-throw construct? Should be the same without it.

f00f · 2018-01-12T15:42:56Z

BetaCameras/RealSense2/RealSense2API.cs

@@ -651,6 +731,21 @@ public RS2Frame(IntPtr p)
            public bool IsValid() => (null != Handle);
        }

+        public struct RS2StreamProfileList


Would name it RS2StreamProfilesList, like the underlying API function.

f00f · 2018-01-16T13:35:23Z

MetriCam2.Controls/CameraSettingsControl.cs

@@ -104,6 +105,9 @@ public Camera Camera
                //   use cam.Channels;
                // Update child controls.
                InitConfigurationParameters(cam);
+
+                cam.OnConnected += (camera) => { InitConfigurationParameters(camera); };


If you attach event handlers you have to detach them as well.
At the beginning of this setter check for cam != null and then detach the handlers.
Otherwise you could get UI updates of the old camera after assigning a new one (rare case, but costs a day to debug).

f00f · 2018-01-16T13:38:01Z

MetriCam2.Controls/CameraSettingsControl.cs

+                            string parameterName = scrollbarValue.Name.Replace(VALUE_SUFFIX, string.Empty);
+                            Dictionary<string, object> keyValues = new Dictionary<string, object>();
+                            keyValues.Add(parameterName, parameterValue);
+                            Camera.SetParameters(keyValues);


If you use Camera.SetParameter(paramDesc.Name, parameterValue) you don't need the Dictionary.

What happens if the parameter is not writable in the current connection state? Just curious.

f00f · 2018-01-16T13:38:54Z

MetriCam2.Controls/CameraSettingsControl.cs

+                            string parameterName = upDownValue.Name.Replace(VALUE_SUFFIX, string.Empty);
+                            Dictionary<string, object> keyValues = new Dictionary<string, object>();
+                            keyValues.Add(parameterName, parameterValue);
+                            Camera.SetParameters(keyValues);


Same as with slider

f00f · 2018-01-16T15:00:03Z

BetaCameras/RealSense2/RealSense2.cs

+            get
+            {
+                ParamDesc<bool> res = new ParamDesc<bool>();
+                res.Description = "Enable / disable trigger to be outputed from the camera to any external device on every depth frame";


"Enable / disable output trigger on every depth frame"

f00f · 2018-01-16T15:03:38Z

BetaCameras/RealSense2/RealSense2.cs

-            bool haveRight = false;
-
-            while (true)
+            while(_updatingPipeline)


Busy waiting feels dirty.
Wouldn't this be nicer with a ManualResetEvent? I don't know how fast it is to check that, though.

f00f · 2018-01-16T15:04:01Z

BetaCameras/RealSense2/RealSense2.cs

+                        break;
+                }
+            }
+            catch (Exception)


Why catch/throw?

f00f · 2018-01-16T15:16:06Z

BetaCameras/RealSense2/RealSense2.cs

+                Point2i resolution = RealSense2API.GetStreamProfileResolution(p);
+                if (resolution == refResolution)
+                {
+                    profile = p;


You could return p; here and get rid of the profile variable

f00f · 2018-01-16T15:18:20Z

BetaCameras/RealSense2/RealSense2.cs

+                if (adjusted)
+                    throw new Exception(string.Format("Value {0} for '{1}' is outside of the range between {2} and {3}", value, desc.Name, desc.Min, desc.Max));
+                else
+                    throw new Exception(string.Format("Value {0} (adjusted to {1} to match stepsize) for 'LaserPower' is outside of the range between {2} and {3}", value, adjustedValue, desc.Min, desc.Max));


Hardcoded string "LaserPower" looks wrong, should use desc.Name

f00f

Open issues from last review:

In addition to the comments I'd prefer it if more specific Exception types were used.
What happens if someone sets ColorResolution while the pipeline is stopped?
resolution -> Point2i conversion method not used in Camera. I'd make the resolution -> Point2i conversion a static method of a helper class.

f00f · 2018-01-17T09:38:43Z

MetriCam2.Controls/CameraSettingsControl.cs

-                            Dictionary<string, object> keyValues = new Dictionary<string, object>();
-                            keyValues.Add(parameterName, parameterValue);
-                            Camera.SetParameters(keyValues);
+                            string parameterName = paramDesc.Name;


Unused variable parameterName

f00f · 2018-01-17T09:41:15Z

MetriCam2.Controls/CameraSettingsControl.cs

@@ -647,5 +644,11 @@ private TextBox CreateTextBox(Camera.ParamDesc paramDesc, int currentRow)
            return textBoxValue;
        }
        #endregion
+
+        private Point2i StringToPoint2i(string s)


I'd name it ResolutionToPoint2i to clarify that it's different from Point2i.Parse

Renamed. I'll move it to the Conversions helper class in the Attributes-branch. Moving all this code around seems off topic here.

OK. I just wanted to squeeze them in as fast as possible :D

f00f · 2018-01-17T09:45:17Z

BetaCameras/RealSense2/RealSense2.cs

-                    return true;
-
-                return false;
+                return RealSense2API.GetOption(_pipeline, RealSense2API.SensorName.COLOR, RealSense2API.Option.BACKLIGHT_COMPENSATION) == 1.0f ? true : false;


The ? true : false is not needed.

If you want to write it in one line I would reverse the order of the comparison to make clear what the return value is compared against. As it is the line is too long to be displayed in Github CR.

f00f · 2018-01-17T09:54:18Z

BetaCameras/RealSense2/RealSense2.cs

-                if (adjusted_value < min)
-                    adjusted_value += step;
-
+                float adjusted_value = AdjustValue(option.min, option.max, value, option.step);
                CheckRangeValid<int>(WhiteBalanceDesc, value, (int)adjusted_value, true);


You could use the option object in the CheckRangeValid method (since you have it already).
Using the ParamDesc here repeats the QueryOption call and adds no value, but only delay.

f00f · 2018-01-17T09:57:10Z

BetaCameras/RealSense2/RealSense2.cs

-                    out string desc);
-
-                    res = new RangeParamDesc<int>((int)min, (int)max);
+                    var option = QueryOption(RealSense2API.Option.FRAMES_QUEUE_SIZE, RealSense2API.SensorName.COLOR);


I would even write a method which contains these two lines, QueryOption and new RangeParamDesc, just because I wouldn't want to write it over and over again (but it's there already, so you may as well keep it).

…nse2_options_to_metricam_parameter

…le already busy

…:Metrilus/MetriCam2 into realsense2_options_to_metricam_parameter

jangernert · 2018-01-17T12:14:39Z

@f00f I think everything is addressed

sisiplac · 2018-02-08T08:05:16Z

There was one bug introduced by this pull request. Setting the LaserMode accidentally sets laser power instead of "EMITTER_ENABLED".
I will fix that...

had been introduced by #47

Convert first option to a metricam parameter

6deab46

jangernert added the enhancement label Dec 21, 2017

jangernert self-assigned this Dec 21, 2017

jangernert requested a review from f00f December 21, 2017 12:12

jangernert added 4 commits December 21, 2017 13:34

Add brightness option for the color sensor

378f7ce

Add contrast option for the color sensor

8c70bae

Add exposure options for color and stereo sensor

dcc3ecb

Add gain options for color and stereo sensor

089f594

jangernert added 6 commits December 21, 2017 15:33

Add auto exposure priority and gain options for color and stereo sensor

bc6f9a9

add hue, saturation and sharpness properties for color sensor

bb8bbe7

whitebalance and laser options to property

a5344d9

more

e56a72a

more

55fcdfa

checked all options for compatibility with D435

19124f6

jangernert added 3 commits December 22, 2017 15:28

add summary comments to enable tool tips

6842ed8

remove direct access to realsense option APIs and enums

35cca8b

adjust PowerLineMode enum names

796a701

jangernert added 9 commits January 8, 2018 16:29

check if value is valid in setter of ranged parameter

97d56da

refactor checks for every property

dc515f7

Only query parameter info when camera is actually connected to avoid …

416131b

…exception

Fix value adjustment for settings with steps and ensure value never i…

038da03

…s out of bounds

change power frequency mode to listparamdesc and increase frame timou…

184888c

…t to 5000ms according to examples (500ms sometimes caused problems)

gather available resolutions and fps once connected

f4e53a0

implement live adjustments of resolution and fps

9aabec5

Check configuration before starting the pipeline

fc6c9d5

Fix MetriCam2 choking on Point2i properties

f75fcf1

jangernert added 5 commits January 12, 2018 14:51

rethrow exceptions from HandleErrors

3564460

Handle Point2i property and description in CameraSettingsControl

f258558

RealSense2: remove exception when certain channels are active at the …

f7878e8

…same time. This seems to be a limitation of old libRealSense2 versions that no longer are valid

Only use captured frames as a fallback to get intrinsics, try to get …

d28440a

…them directly from the sensor first

Try to get extrinsics from sensor first before checking recorded fram…

2f15f9b

…es as well

f00f suggested changes Jan 16, 2018

View reviewed changes

address almost all remarks of the review

3007383

f00f suggested changes Jan 17, 2018

View reviewed changes

Christian Schaller and others added 4 commits January 17, 2018 11:03

Merge remote-tracking branch 'refs/remotes/origin/master' into realse…

29b397f

…nse2_options_to_metricam_parameter

Rename StringToPoin2i and throw if pipeline needs to be restarted whi…

362b71e

…le already busy

Merge branch 'realsense2_options_to_metricam_parameter' of github.com…

944292d

…:Metrilus/MetriCam2 into realsense2_options_to_metricam_parameter

More specific exceptions

8089ff7

get name of argument for argumentexception instead of value

1224d3c

f00f approved these changes Jan 17, 2018

View reviewed changes

f00f merged commit e7756d9 into master Jan 17, 2018

f00f deleted the realsense2_options_to_metricam_parameter branch January 17, 2018 14:47

sisiplac added a commit that referenced this pull request Feb 8, 2018

[bugfix] Fixed buggy LaserMode settter in Realsense introduced by #47

80d8127

f00f pushed a commit that referenced this pull request Feb 8, 2018

[bugfix] Fixed buggy LaserMode settter in Realsense introduced by #47

0a7e550

f00f pushed a commit that referenced this pull request Feb 8, 2018

[bugfix] RealSense2: Fixed EmitterMode settter

b4b2c33

had been introduced by #47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Convert RealSense2 options to MetriCam2 parameter #47

[WIP] Convert RealSense2 options to MetriCam2 parameter #47

jangernert commented Dec 21, 2017 •

edited

Loading

jangernert commented Dec 21, 2017 •

edited

Loading

jangernert commented Dec 21, 2017 •

edited

Loading

jangernert commented Dec 22, 2017 •

edited

Loading

jangernert commented Jan 10, 2018

f00f commented Jan 12, 2018 •

edited by jangernert

Loading

f00f commented Jan 16, 2018

jangernert commented Jan 16, 2018

f00f left a comment

f00f Jan 12, 2018

f00f Jan 12, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f Jan 16, 2018

f00f left a comment •

edited

Loading

f00f Jan 17, 2018

f00f Jan 17, 2018

jangernert Jan 17, 2018

f00f Jan 17, 2018

f00f Jan 17, 2018

f00f Jan 17, 2018

f00f Jan 17, 2018

jangernert commented Jan 17, 2018

sisiplac commented Feb 8, 2018

[WIP] Convert RealSense2 options to MetriCam2 parameter #47

[WIP] Convert RealSense2 options to MetriCam2 parameter #47

Conversation

jangernert commented Dec 21, 2017 • edited Loading

jangernert commented Dec 21, 2017 • edited Loading

jangernert commented Dec 21, 2017 • edited Loading

jangernert commented Dec 22, 2017 • edited Loading

jangernert commented Jan 10, 2018

f00f commented Jan 12, 2018 • edited by jangernert Loading

f00f commented Jan 16, 2018

jangernert commented Jan 16, 2018

f00f left a 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

f00f left a comment • edited 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

jangernert commented Jan 17, 2018

sisiplac commented Feb 8, 2018

jangernert commented Dec 21, 2017 •

edited

Loading

jangernert commented Dec 21, 2017 •

edited

Loading

jangernert commented Dec 21, 2017 •

edited

Loading

jangernert commented Dec 22, 2017 •

edited

Loading

f00f commented Jan 12, 2018 •

edited by jangernert

Loading

f00f left a comment •

edited

Loading