derive context settings from configuration file by maloel · Pull Request #12432 · realsenseai/librealsense

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

-    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

-        std::map<std::string, std::string> _values;
        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

        std::string _filename;
+        nlohmann::json _j;
    };
 }


Newline needed?

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

+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

+        // 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

+    nlohmann::json settings = nlohmann::json::object();
    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

 }


+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 realsenseai:development Nov 26, 2023

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

Uh oh!

Conversation

maloel commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

OhadMeir left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maloel commented Nov 20, 2023 •

edited

Loading