derive context settings from configuration file #12432

maloel · 2023-11-20T12:38:21Z

Continuation (2/3) of previous PR for persistent DDS settings.

Move the filename for our config file (the one the Viewer uses) into rs.h as RS2_CONFIG_FILENAME
The configuration file (JSON) implementation in common/rs-config now can read non-string JSON values
Contexts now take their settings from:
- The global configuration file context key, merged with:
- Application-specific context key, merged with:
- Context-specific settings as handed to the context
Context settings can disable the above with "inherit":false

E.g.:

{
  "context": {
    "dds": {
      "enabled": false,
      "domain": 5
    }
  },
  "realsense-viewer": {
    "context": {
      "dds": {
        "enabled": true,
        "device": {
          "metadata": { "reliability": "best-effort" }
        }
      }
    }
  },
  ...
}

would disable DDS by default, except in the viewer
would default to domain 5 when enabled

Tracked on [LRS-963]

OhadMeir · 2023-11-23T13:40:50Z

common/rs-config.cpp

-    auto it = _values.find(key);
-    return it != _values.end();
+    auto it = _j.find(key);
+    return it != _j.end() && it->is_string();


Why check for string? Don't we support other values?

Because, at least in rs-config, and in this PR, I didn't want to change any existing functionality: tools will keep behaving the same.

My main goal was to make rs-config compatible with raw json -- before, it couldn't even load -- and able to keep non-string content intact.

So in the next PR this is about to change?

No, I'm not touching it again without a good reason.

OhadMeir · 2023-11-23T13:42:59Z

common/rs-config.h

        std::map<std::string, std::string> _defaults;
        std::string _filename;
+        nlohmann::json _j;


_j does not imply the member meaning, just the type. _config?

It's the underlying json object. I can name it _json, but I don't know if that adds much...

The name should not change if we decide to switch from JSON to XML or other.
The meaning of this variable is to hold the configuration in memory.

_data? _map? _key_value? Anything you pick is meaningless. :)
Let's be realistic. It's a file with an extension of .json.

OhadMeir · 2023-11-23T13:43:12Z

common/rs-config.h

        std::map<std::string, std::string> _defaults;
        std::string _filename;
+        nlohmann::json _j;
    };
 }


Newline needed?

OhadMeir · 2023-11-23T16:50:32Z

src/context.cpp

+namespace librealsense {
+
+
+    static nlohmann::json load_config()


I think it might be useful to have a load/save_json( filename ) utility. I know that Noa has a task concerning saving and parsing json content.

Not quite sure why, it's very straightforward as you see...

OhadMeir · 2023-11-23T16:58:24Z

src/context.cpp

+        // Take the global 'context' settings out of the configuration
+        nlohmann::json settings;
+        if( auto global_context = rsutils::json::nested( config, "context" ) )
+            merge_settings( settings, global_context, "global config-file/context" );


I thought that the parameter order should be switched.
As I understand specified context_settings should override executable settings that in turn override global setting

That's exactly what it does
Here it take empty settings and "merges" the global, so we get global.

OhadMeir · 2023-11-23T17:13:25Z

tools/realsense-viewer/realsense-viewer.cpp

    if( only_sw_arg.getValue() )
+    {
+        settings["dds"]["enabled"] = true;  // override global dds:false or dds/enabled:false, if any


I think this should be true only if BUILD_WITH_DDS is set

Doesn't hurt, but OK.
Do you think it makes sense to enable DDS automatically if we see "sw-only" device-mask (i.e., override enabled=false if device-mask is sw-only)?

No, could be playback or future type devices. If settings set to disable then leave disabled

OhadMeir · 2023-11-23T17:17:07Z

tools/dds/dds-adapter/rs-dds-adapter.cpp

@@ -58,10 +60,67 @@ topics::device_info rs2_device_to_info( rs2::device const & dev )
 }


+static nlohmann::json load_config()


This makes my point about a utility stronger :-)

The 3 static functions should probably all be in rsutils

Two. I thought the same but I don't think it belongs in rsutils.
Load config shouldn't know about RS2_CONFIG_FILENAME. It can get the filename. But then what're we adding? The exception handling? The error would be different, I could make it "failed to load json file: ..." but then I just don't feel the function adds much.
Same with merge_settings: it's just a wrapper around merge_patch with error handling.
In the end I decided to just keep as is. You think I should make rsutils::json::load_file() and rsutils::json::merge?

load_file in rsutils makes less and less sense to me (no filename generation; if would have to return bool to denote if the file was there, so you'd have to handle it in the caller). I prefer to just embed directly in load_settings.

merge_patch is a weird name and I can see introducing rsutils::json::merge() or json::patch with maybe an optional name for error handling.

Anyway:

added rsutils::json::patch()

added rsutils::json::load_settings that does the basic global+app merge

OhadMeir

LGTM

maloel requested a review from OhadMeir November 20, 2023 12:38

OhadMeir reviewed Nov 23, 2023

View reviewed changes

OhadMeir approved these changes Nov 26, 2023

View reviewed changes

maloel added 8 commits November 26, 2023 10:12

change rs2::config_file (in common/) to use actual json

1c6cd31

define RS2_CONFIG_FILENAME

55ee86e

context settings now merged onto the configuration 'context' settings

e2741af

participant domain-id of -1 means get from settings

5d6d09e

add dds/enabled to control dds

ca82dce

dds-adapter reads librealsense settings for defaults

f2a9e91

tools --sw-only overrides 'dds'

6cec1da

central rsutils::json::load_[app_]settings() and ::patch()

2361208

maloel force-pushed the config-file branch from 25ad20b to 2361208 Compare November 26, 2023 08:12

maloel merged commit e9541bb into IntelRealSense:development Nov 26, 2023
16 of 17 checks passed

maloel deleted the config-file branch November 26, 2023 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

derive context settings from configuration file #12432

derive context settings from configuration file #12432

maloel commented Nov 20, 2023 •

edited

Loading

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

maloel Nov 23, 2023

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

OhadMeir Nov 23, 2023

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

OhadMeir Nov 26, 2023

OhadMeir Nov 23, 2023

maloel Nov 23, 2023

maloel Nov 23, 2023

maloel Nov 24, 2023

OhadMeir left a comment

		@@ -58,10 +60,67 @@ topics::device_info rs2_device_to_info( rs2::device const & dev )
		}


		static nlohmann::json load_config()

derive context settings from configuration file #12432

derive context settings from configuration file #12432

Conversation

maloel commented Nov 20, 2023 • 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

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

OhadMeir left a comment

Choose a reason for hiding this comment

maloel commented Nov 20, 2023 •

edited

Loading