iox-#13 add config file parser and some refactorings #38

elBoberido · 2020-01-29T00:26:11Z

Signed-off-by: Kraus Mathias (CC-AD/ESW1) mathias.kraus2@de.bosch.com

Karsten1987

oh boy, this PR is pretty hardcore to review.

Just tried to browse through it and noted some style things.

Karsten1987 · 2020-01-29T00:30:10Z

.gitignore

+/CMakeLists.txt
+.vscode


Karsten1987 · 2020-01-29T00:35:08Z

iceoryx_examples/iceperf/iceperf_laurel.cpp

@@ -30,7 +30,7 @@ double measureLatency(iox::popo::Publisher& publisher, iox::popo::Subscriber& su
 {
    auto start = std::chrono::high_resolution_clock::now();
    // run the performance test
-    for (uint64_t i = 0; i < NUMBER_OF_ROUNDTRIPS; ++i)
+    for (int64_t i = 0; i < NUMBER_OF_ROUNDTRIPS; ++i)


Karsten1987 · 2020-01-29T00:36:12Z

iceoryx_posh/CMakeLists.txt

+find_package(cpptoml REQUIRED)
+message("-- Build with TOML config file support.")


consider indenting

Karsten1987 · 2020-01-29T00:36:32Z

iceoryx_posh/CMakeLists.txt

@@ -72,6 +77,7 @@ add_library(iceoryx_posh
    source/runtime/message_queue_interface.cpp
    source/runtime/message_queue_message.cpp
    source/runtime/posh_runtime.cpp
+    source/runtime/service_discovery_notifier.cpp


alphabetical sort

Karsten1987 · 2020-01-29T00:37:14Z

iceoryx_posh/cmake/iceoryxversions.cmake

+	DESTINATION share/doc/iceoryx
+	COMPONENT dev)


wrong indentation?

Karsten1987 · 2020-01-29T00:43:28Z

iceoryx_posh/test/integrationtests/service_discovery/test_roudi_startfindservice.cpp

@@ -0,0 +1,685 @@
+// Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.


this was actually done in a branch in 2019, it just happens that it got ready this year

Karsten1987 · 2020-01-29T00:43:48Z

iceoryx_posh/test/integrationtests/test_shm_sigbus_handler.cpp

+    // /dev/shm
+    iox::runtime::SharedMemoryCreator<iox::roudi::MiddlewareShm> sut(goodconfig);
+}
+} // namespace


Karsten1987 · 2020-01-29T00:44:19Z

iceoryx_utils/include/iceoryx_utils/cxx/string.hpp

@@ -0,0 +1,287 @@
+// Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.


this was actually done in a branch in 2019, it just happens that it got ready this year

Karsten1987 · 2020-01-29T00:44:43Z

iceoryx_utils/include/iceoryx_utils/internal/cxx/string.inl

@@ -0,0 +1,243 @@
+// Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.


this was actually done in a branch in 2019, it just happens that it got ready this year

Karsten1987 · 2020-01-29T00:44:55Z

iceoryx_utils/include/iceoryx_utils/internal/cxx/string.inl

+    return std::string(c_str());
+}
+} // namespace cxx
+} // namespace iox


Signed-off-by: Kraus Mathias (CC-AD/ESW1) <mathias.kraus2@de.bosch.com>

README.md

mossmaurice · 2020-01-29T11:54:11Z

README.md

+When no config file is specified, this config will be used:
+```
+[general]
+version = 1
+
+[[segment]]
+
+[[segment.mempool]]
+size = 32
+count = 10000
+
+[[segment.mempool]]
+size = 128
+count = 10000
+
+[[segment.mempool]]
+size = 1024
+count = 2000
+
+[[segment.mempool]]
+size = 16384
+count = 500
+
+[[segment.mempool]]
+size = 131072
+count = 200
+
+[[segment.mempool]]
+size = 1048576
+count = 50
+
+[[segment.mempool]]
+size = 2097152
+count = 20
+
+[[segment.mempool]]
+size = 4194304
+count = 10
+
+[[segment.mempool]]
+size = 8388608
+count = 10
+
+[[segment.mempool]]
+size = 16777216
+count = 5
+
+[[segment.mempool]]
+size = 33554432
+count = 2


This could be moved to a separate TOML file.

we need a fallback, just in case there is no config file specified

mossmaurice · 2020-01-29T11:57:58Z

iceoryx_posh/CMakeLists.txt

@@ -1,10 +1,15 @@
 cmake_minimum_required(VERSION 3.5)
-set(iceoryx_posh_VERSION 0.16.0)
+set(iceoryx_posh_VERSION 0.16.0.1)


@michael-poehnl After this pull request has been merged, IMHO we can directly create the new release.

mossmaurice · 2020-01-29T11:59:03Z

iceoryx_posh/include/iceoryx_posh/capro/service_description.hpp

@@ -37,7 +37,7 @@ static constexpr char AnyEventString[]{"65535"};
 static constexpr int32_t MAX_NUMBER_OF_CHARS = 64;

 /// @brief Scope of a service description
-enum class Scope : unsigned int
+enum class Scope : uint16_t


Good catch!

mossmaurice · 2020-01-29T12:09:56Z

iceoryx_posh/include/iceoryx_posh/runtime/service_discovery_notifier.hpp

+    mutable std::mutex m_queueAccessMutex;
+
+    // used for starting the timer, first time StartFindService() is called.
+    /// @todo pbt2kor 3 : Introducing isRunning() method in Timer might be a better idea


Please rewrite this comment.

mossmaurice · 2020-01-29T12:23:25Z

iceoryx_posh/source/roudi/application/roudi_main.cpp

+#ifdef TOML_CONFIG_FILE
+#include "cpptoml.h"
+
+class TomlRouDiConfigFileParser : public iox::roudi::RouDiConfigFileParser


This class will be moved to a separate file as part of the next refactoring.

elBoberido · 2020-01-29T16:27:26Z

oh boy, this PR is pretty hardcore to review.

sorry for the big code drop. this is a temporary exception and will not become the rule. we need a little bit more time for converging our internal codebase with the public one. we are on track, tough and this should happen soon™. Then the PRs will be smaller and focused on the topics

jeising · 2020-01-29T17:10:13Z

iceoryx_posh/source/roudi/application/roudi_app.cpp

-            std::cout << "-b,                               Start address hint of the shared memory segments"
-                         "                                  e.g. 0x1f4000000"
+            std::cout << "-c, --config-file                 Path to the RouDi Config File."
+                         "                                  Have a look at the documentation for the format."


Maybe it would be nice to point to a website here instead of the generic term documentation :)

I like this idea. Maybe we should have a FAQ where the most common issues are listed and link it here?!

Actually from my point of view it would be fine to add a link to the markdown file in this repo, which describes how the config file works.

But of course: Providing a link to the general documentation in the help text would be nice.

Signed-off-by: Kraus Mathias (CC-AD/ESW1) <mathias.kraus2@de.bosch.com>

Karsten1987 reviewed Jan 29, 2020

View reviewed changes

iox-eclipse-iceoryx#13 add config file parser and some refactorings

9d68aca

Signed-off-by: Kraus Mathias (CC-AD/ESW1) <mathias.kraus2@de.bosch.com>

elBoberido force-pushed the iox-#13-config-file-and-refactorings branch from 0c13e33 to 9d68aca Compare January 29, 2020 00:53

mossmaurice added the enhancement New feature label Jan 29, 2020

mossmaurice requested review from budrus, mossmaurice, dkroenke and elfenpiff January 29, 2020 07:39

mossmaurice requested changes Jan 29, 2020

View reviewed changes

jeising reviewed Jan 29, 2020

View reviewed changes

iox-eclipse-iceoryx#13 fix review findings

48cd69e

Signed-off-by: Kraus Mathias (CC-AD/ESW1) <mathias.kraus2@de.bosch.com>

elBoberido force-pushed the iox-#13-config-file-and-refactorings branch from 7ce961e to 48cd69e Compare January 30, 2020 09:42

budrus approved these changes Jan 30, 2020

View reviewed changes

mossmaurice approved these changes Jan 30, 2020

View reviewed changes

elBoberido merged commit 9336525 into eclipse-iceoryx:master Jan 30, 2020

budrus mentioned this pull request Jan 30, 2020

Need for easy mempool configuration #13

Closed

elBoberido deleted the iox-#13-config-file-and-refactorings branch May 28, 2020 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#13 add config file parser and some refactorings #38

iox-#13 add config file parser and some refactorings #38

elBoberido commented Jan 29, 2020

Karsten1987 left a comment

Karsten1987 Jan 29, 2020

Karsten1987 Jan 29, 2020

Karsten1987 Jan 29, 2020

Karsten1987 Jan 29, 2020

Karsten1987 Jan 29, 2020

Karsten1987 Jan 29, 2020

elBoberido Jan 29, 2020

Karsten1987 Jan 29, 2020

Karsten1987 Jan 29, 2020

elBoberido Jan 29, 2020

Karsten1987 Jan 29, 2020

elBoberido Jan 29, 2020

Karsten1987 Jan 29, 2020

mossmaurice Jan 29, 2020

elBoberido Jan 29, 2020

mossmaurice Jan 29, 2020

mossmaurice Jan 29, 2020

mossmaurice Jan 29, 2020

mossmaurice Jan 29, 2020

elBoberido commented Jan 29, 2020

jeising Jan 29, 2020

elfenpiff Jan 29, 2020

jeising Jan 29, 2020 •

edited

Loading

		find_package(cpptoml REQUIRED)
		message("-- Build with TOML config file support.")

		@@ -0,0 +1,685 @@
		// Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.

		@@ -0,0 +1,287 @@
		// Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.

		@@ -0,0 +1,243 @@
		// Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.

		/CMakeLists.txt
		.vscode

iox-#13 add config file parser and some refactorings #38

iox-#13 add config file parser and some refactorings #38

Conversation

elBoberido commented Jan 29, 2020

Karsten1987 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

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

elBoberido commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeising Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

jeising Jan 29, 2020 •

edited

Loading