add dds config tool by Noy-Zini · Pull Request #13628 · realsenseai/librealsense

Noy-Zini · 2024-12-25T09:40:20Z

Tracked on : [RSDEV-2884]

Nir-Az · 2024-12-29T13:42:09Z

@@ -0,0 +1,332 @@
+#include "dds-model.h"
+// License: Apache 2.0. See LICENSE file in root directory.


Misplaced, should be on top

Nir-Az · 2024-12-29T13:44:32Z

    LIST_DIRECTORIES false
    RELATIVE ${PROJECT_SOURCE_DIR}
    "${CMAKE_CURRENT_LIST_DIR}/*"
+    "${PROJECT_SOURCE_DIR}/../../../third-party/rsutils/include/rsutils/type/*"


This path seems weird..
relative to the source dir we only need third-party/rsutils/include/rsutils/type/* no?

The source dir for the dds tool is librealsense\tools\dds\dds-config so only using part of the path won't work but I can set the THIRD_PARTY_DIR so it looks more readable:
set(THIRD_PARTY_DIR "${PROJECT_SOURCE_DIR}/../../../third-party")
"${THIRD_PARTY_DIR}/rsutils/include/rsutils/type/*"

Nir-Az · 2024-12-29T13:45:17Z

        }

        _calib_model.update(window, error_message);
+        _dds_model.render_dds_config_window(window , error_message);


Will this work id we build with DDS off?

I'll add a check to see if the device supports DDS before using this function.
But in general, yes the functionality works with DDS off because the function already checks whether the window should be open. If the device doesn't support DDS, the window is closed by default.

Nir-Az · 2024-12-29T13:45:55Z

                        }
                    }
                }
+                ImGuiSelectableFlags is_streaming_flag = (is_streaming) ? ImGuiSelectableFlags_Disabled : 0;


What is the difference between 0 and ImGuiSelectableFlags_Disabled?

0 is equal to ImGuiSelectableFlags_None that will show the "DDS Configuration" option an enabled
and using the ImGuiSelectableFlags_Disabled will show this option as disabled (faded and not allowing clicks on it)

So why not replace 0 with ImGuiSelectableFlags_None?

Nir-Az · 2024-12-29T13:47:33Z

+#include <set>
+#include <rsutils/type/ip-address.h>
+#include <rsutils/string/hexdump.h>
+#include "../third-party/rsutils/include/rsutils/type/eth-config.h"


Can we also use <>?
<party/rsutils/include/rsutils/type/eth-config.h>?

Nir-Az · 2024-12-29T13:48:06Z

+        eth_config _current_config;
+        eth_config _changed_config;
+
+        bool _window_open;


Not initialized but other is?

initialized _window_open to false as well

initialized in the constructor initialization list

Nir-Az · 2024-12-29T13:50:31Z

+    if (ImGui::InputText(label_name.c_str(), buffer, sizeof(buffer))) {
+        std::string new_ip_str(buffer);
+        if (rsutils::type::ip_address(new_ip_str).is_valid()) {
+            ip = rsutils::type::ip_address(new_ip_str);


Why calling it twice?
Maybe call it once and use it on both cases?

Nir-Az · 2024-12-29T13:50:57Z

+        if (rsutils::type::ip_address(new_ip_str).is_valid()) {
+            ip = rsutils::type::ip_address(new_ip_str);
+        }
+        else {


new line please for the {

Nir-Az · 2024-12-29T13:52:48Z

+            ImGui::Text("DHCP Timeout (seconds)");
+            ImGui::SameLine();
+            int tempTimeout = static_cast<int>(_changed_config.dhcp.timeout);
+            if (ImGui::InputInt("##DHCP Timeout (seconds)", &tempTimeout)) {


Please place { on a new line for all uses here.
I will send you a mail about using clang format

Nir-Az · 2024-12-29T13:55:43Z

@Noy-Zini can you please build a version for us to play with and check?

Nir-Az · 2024-12-31T13:17:19Z

+
+bool rs2::dds_model::supports_DDS()
+{
+    return _dds_suported;


supported

OhadMeir · 2025-01-08T15:01:47Z

    return configured.ip == other.configured.ip && configured.netmask == other.configured.netmask
        && configured.gateway == other.configured.gateway && dds.domain_id == other.dds.domain_id
-        && dhcp.on == other.dhcp.on && link.priority == other.link.priority && link.timeout == other.link.timeout;
+        && dhcp.on == other.dhcp.on && link.priority == other.link.priority && link.timeout == other.link.timeout && dhcp.timeout != other.dhcp.timeout;


Should be ==?

yes thank I fixed it 👍

OhadMeir · 2025-01-09T07:51:02Z

+                }
+                current = &( *current )[token];  // getting to the next level in the JSON structure
+            }
+            return current->get< T >();


get might throw if value is of wrong type.
You can use get_ex to get value and success indication.

OhadMeir · 2025-01-09T07:51:48Z


+        // Retrieves a value from a nested JSON structure using dot notation
+        template< typename T >
+        T get_nested( const std::string & path ) const


Should we return T or config_value like get?

OhadMeir · 2025-01-09T07:53:58Z

+                {
+                    ( *current )[keys[i]] = rsutils::json::object();
+                }
+                current = &( *current )[keys[i]];


I think that working with a reference, not a pointer, will be easier and the syntax clearer

OhadMeir · 2025-01-09T08:53:37Z

+                std::stringstream val_ss;
+                val_ss << default_val;
+                _defaults[path] = val_ss.str();
+                save();


Why are we saving to file here? User did not select apply

OhadMeir · 2025-01-09T09:05:06Z

+#include <set>
+#include <rsutils/type/ip-address.h>
+#include <rsutils/string/hexdump.h>
+#include <../third-party/rsutils/include/rsutils/type/eth-config.h>


Can shorten #include <rsutils/type/eth-config.h> like the other includes

OhadMeir · 2025-01-09T09:07:36Z

+bool const ACTUAL_VALUES = 0;
+bool const DEFULT_VALUES = 1;
+
+enum priority {


Define under the class namespace. Looks like it can be private

OhadMeir · 2025-01-09T09:08:32Z

+#include <rsutils/json.h>
+#include <rsutils/json-config.h>
+
+uint32_t const GET_ETH_CONFIG = 0xBB;


Constsants should be under a namespace at minimum. Better yet, move to CPP as these are internal details that don't need to be included by users

OhadMeir · 2025-01-09T09:09:49Z

+#include <realsense_imgui.h>
+#include <set>
+#include <rsutils/type/ip-address.h>
+#include <rsutils/string/hexdump.h>


Are all these includes needed here? Move to cpp what's used only by implementation and doesn't need to be declared here

OhadMeir · 2025-01-09T09:19:48Z

+{
+    if( check_DDS_support() )
+    {
+        _defult_config = get_eth_config( _device, DEFULT_VALUES );


You implicitly convert int to bool and later check the bool and convert back to int. Better just pass int as parameter and use it int curr_or_default.

OhadMeir · 2025-01-09T09:47:27Z

+    if( check_DDS_support() )
+    {
+        _defult_config = get_eth_config( _device, DEFULT_VALUES );
+        _current_config = get_eth_config( _device, ACTUAL_VALUES );


We don't need the first parameter, it is always _device and can be used internally.

OhadMeir · 2025-01-09T09:55:42Z

+        }
+        else
+        {
+            std::snprintf( buffer, sizeof( buffer ), "%s", ip.to_string().c_str() );


Why is this needed?

OhadMeir · 2025-01-09T10:05:32Z

+                break;
+            case DYNAMIC:
+                _changed_config.link.priority
+                    = _current_config.link.speed ? link_priority::dynamic_eth_first : link_priority::dynamic_usb_first;


Logic not clear. Add a comment "If link speed is not 0 than we are connected by Ethernet"

OhadMeir · 2025-01-09T10:08:53Z

+        }
+        ImGui::Checkbox( "No Reset after changes", &_no_reset );
+
+        if( ImGui::Checkbox( "Load to defult values", &_set_defult ) )


"Load default values", "to" is redundant

OhadMeir · 2025-01-09T11:20:39Z

Need to add scroll bars to the settings when minimized. In another PR.

OhadMeir

Great PR, helpful capability.

As discussed offline, we will merge now, open comments will be handled in a follow-up PR

Noy-Zini force-pushed the add-dds-config-tool-to-viewer branch 2 times, most recently from 4c15cb3 to 6234882 Compare December 25, 2024 12:13

add dds config tool

7e2db73

Noy-Zini force-pushed the add-dds-config-tool-to-viewer branch from 6234882 to 7e2db73 Compare December 25, 2024 13:24

adjustments to the UI

ed2c607

Noy-Zini requested a review from Nir-Az December 25, 2024 14:55

Nir-Az reviewed Dec 29, 2024

View reviewed changes

Noy-Zini force-pushed the add-dds-config-tool-to-viewer branch from 4bead95 to 16d7803 Compare December 31, 2024 12:03

Noy-Zini requested a review from Nir-Az December 31, 2024 12:54

Nir-Az reviewed Dec 31, 2024

View reviewed changes

adjustments

6cb2c25

Noy-Zini force-pushed the add-dds-config-tool-to-viewer branch from 16d7803 to 6cb2c25 Compare December 31, 2024 13:41

add dds enable to viewer settings

b03f190

Noy-Zini requested a review from OhadMeir January 8, 2025 10:16

OhadMeir reviewed Jan 8, 2025

View reviewed changes

add dds json struct to config file

81fa5a9

Noy-Zini force-pushed the add-dds-config-tool-to-viewer branch from 896a47d to 81fa5a9 Compare January 9, 2025 06:39

OhadMeir reviewed Jan 9, 2025

View reviewed changes

OhadMeir approved these changes Jan 9, 2025

View reviewed changes

OhadMeir merged commit 273a8f7 into realsenseai:development Jan 9, 2025

Nir-Az mentioned this pull request Jan 12, 2025

Fix playback device #13675

Merged

Noy-Zini mentioned this pull request Jan 14, 2025

dds config tool repair #13681

Merged

		@@ -0,0 +1,332 @@
		#include "dds-model.h"
		// License: Apache 2.0. See LICENSE file in root directory.

Uh oh!

Conversation

Noy-Zini commented Dec 25, 2024

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!

Nir-Az commented Dec 29, 2024

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 commented Jan 9, 2025

Uh oh!

OhadMeir left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!