Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[config] Implementation of new configuration management #1505

Open
wants to merge 118 commits into
base: master
Choose a base branch
from

Conversation

Peguen
Copy link
Contributor

@Peguen Peguen commented Apr 4, 2024

Description

Introduced config management via structs which opens the possibilty for users to configure eCAL in code. In addition, evaluation of the inputs will be easily possible.

If assigned configuration values can be evaluated and are incorrect, the program will exit with an error.

Internally also the new config structs will be used.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/src/config/ecal_cmd_parser.cpp Outdated Show resolved Hide resolved
ecal/core/src/config/ecal_cmd_parser.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/config_test/src/config_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/config_test/src/config_test.cpp Outdated Show resolved Hide resolved
ecal/tests/cpp/config_test/src/config_test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

@@ -101,7 +101,7 @@ namespace eCAL
* @param topic_name_ Unique topic name.
* @param config_ Optional configuration parameters.
**/
CPublisher(const std::string& topic_name_, const eCAL::Publisher::Configuration& config_ = {})
CPublisher(const std::string& topic_name_, const eCAL::Publisher::Configuration& config_ = eCAL::GetCurrentConfig().publisher_options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: constructor does not initialize these fields: builder [cppcoreguidelines-pro-type-member-init]

      CPublisher(const std::string& topic_name_, const eCAL::Publisher::Configuration& config_ = eCAL::GetCurrentConfig().publisher_options)
      ^

@@ -63,7 +63,7 @@ namespace eCAL
* @param data_type_info_ Topic data type information (encoding, type, descriptor).
* @param config_ Optional configuration parameters.
**/
CMsgPublisher(const std::string& topic_name_, const struct SDataTypeInformation& data_type_info_, const Publisher::Configuration& config_ = {}) : CPublisher(topic_name_, data_type_info_, config_)
CMsgPublisher(const std::string& topic_name_, const struct SDataTypeInformation& data_type_info_, const Publisher::Configuration& config_ = eCAL::GetCurrentConfig().publisher_options) : CPublisher(topic_name_, data_type_info_, config_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: constructor does not initialize these fields: m_buffer [cppcoreguidelines-pro-type-member-init]

    CMsgPublisher(const std::string& topic_name_, const struct SDataTypeInformation& data_type_info_, const Publisher::Configuration& config_ = eCAL::GetCurrentConfig().publisher_options) : CPublisher(topic_name_, data_type_info_, config_)
    ^

@@ -74,7 +74,7 @@
* @param topic_name_ Unique topic name.
* @param config_ Optional configuration parameters.
**/
explicit CMsgPublisher(const std::string& topic_name_, const Publisher::Configuration& config_ = {}) : CMsgPublisher(topic_name_, GetDataTypeInformation(), config_)
explicit CMsgPublisher(const std::string& topic_name_, const Publisher::Configuration& config_ = eCAL::GetCurrentConfig().publisher_options) : CMsgPublisher(topic_name_, GetDataTypeInformation(), config_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: constructor does not initialize these fields: m_buffer [cppcoreguidelines-pro-type-member-init]

    explicit CMsgPublisher(const std::string& topic_name_, const Publisher::Configuration& config_ = eCAL::GetCurrentConfig().publisher_options) : CMsgPublisher(topic_name_, GetDataTypeInformation(), config_)
    ^


// publisher options
auto& publisherOptions = publisher_options;
publisherOptions.shm.enable = (iniConfig.get(PUBLISHER, "use_shm", static_cast<int>(PUB_USE_SHM)) == 0) ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr]

Suggested change
publisherOptions.shm.enable = (iniConfig.get(PUBLISHER, "use_shm", static_cast<int>(PUB_USE_SHM)) == 0) ? false : true;
publisherOptions.shm.enable = iniConfig.get(PUBLISHER, "use_shm", static_cast<int>(PUB_USE_SHM)) != 0;

// publisher options
auto& publisherOptions = publisher_options;
publisherOptions.shm.enable = (iniConfig.get(PUBLISHER, "use_shm", static_cast<int>(PUB_USE_SHM)) == 0) ? false : true;
publisherOptions.tcp.enable = (iniConfig.get(PUBLISHER, "use_tcp", static_cast<int>(PUB_USE_TCP)) == 0) ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr]

Suggested change
publisherOptions.tcp.enable = (iniConfig.get(PUBLISHER, "use_tcp", static_cast<int>(PUB_USE_TCP)) == 0) ? false : true;
publisherOptions.tcp.enable = iniConfig.get(PUBLISHER, "use_tcp", static_cast<int>(PUB_USE_TCP)) != 0;

auto& publisherOptions = publisher_options;
publisherOptions.shm.enable = (iniConfig.get(PUBLISHER, "use_shm", static_cast<int>(PUB_USE_SHM)) == 0) ? false : true;
publisherOptions.tcp.enable = (iniConfig.get(PUBLISHER, "use_tcp", static_cast<int>(PUB_USE_TCP)) == 0) ? false : true;
publisherOptions.udp.enable = (iniConfig.get(PUBLISHER, "use_udp_mc", static_cast<int>(PUB_USE_UDP_MC)) == 0) ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr]

Suggested change
publisherOptions.udp.enable = (iniConfig.get(PUBLISHER, "use_udp_mc", static_cast<int>(PUB_USE_UDP_MC)) == 0) ? false : true;
publisherOptions.udp.enable = iniConfig.get(PUBLISHER, "use_udp_mc", static_cast<int>(PUB_USE_UDP_MC)) != 0;

std::vector<const char*> argv(args_.size());
std::transform(args_.begin(), args_.end(), argv.begin(), [](std::string& s) {return s.c_str();});
return Initialize(static_cast<int>(argv.size()), const_cast<char**>(argv.data()), unit_name_, components_);
eCAL::Configuration config(args_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: parameter 'args_' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

ecal/core/src/ecal.cpp:28:

- #include <vector>
+ #include <utility>
+ #include <vector>
Suggested change
eCAL::Configuration config(args_);
eCAL::Configuration config(std::move(args_));


TEST(ConfigDeathTest, user_config_death_test)
{
eCAL::Configuration custom_config(0, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'custom_config' of type 'eCAL::Configuration' can be declared 'const' [misc-const-correctness]

Suggested change
eCAL::Configuration custom_config(0, nullptr);
eCAL::Configuration const custom_config(0, nullptr);

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/src/ecal_global_accessors.cpp Show resolved Hide resolved
ecal/core/src/ecal_global_accessors.h Show resolved Hide resolved
Configuration::Configuration() :
share_topic_type(eCAL::Config::IsTopicTypeSharingEnabled()),
share_topic_description(eCAL::Config::IsTopicDescriptionSharingEnabled())
Configuration::Configuration()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: constructor does not initialize these fields: tcp, share_topic_type, share_topic_description [cppcoreguidelines-pro-type-member-init]

ecal/core/include/ecal/config/ecal_publisher_config.h:137:

-       TCP::Configuration   tcp;
+       TCP::Configuration   tcp{};

ecal/core/include/ecal/config/ecal_publisher_config.h:139:

-       bool                 share_topic_type;                            //!< share topic type via registration
-       bool                 share_topic_description;                     //!< share topic description via registration
+       bool                 share_topic_type{};                            //!< share topic type via registration
+       bool                 share_topic_description{};                     //!< share topic description via registration

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Configuration::Configuration() :
share_topic_type(eCAL::Config::IsTopicTypeSharingEnabled()),
share_topic_description(eCAL::Config::IsTopicDescriptionSharingEnabled())
Configuration::Configuration()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: constructor does not initialize these fields: tcp, share_topic_type, share_topic_description [cppcoreguidelines-pro-type-member-init]

ecal/core/include/ecal/config/publisher.h:137:

-       TCP::Configuration   tcp;
+       TCP::Configuration   tcp{};

ecal/core/include/ecal/config/publisher.h:139:

-       bool                 share_topic_type;                            //!< share topic type via registration
-       bool                 share_topic_description;                     //!< share topic description via registration
+       bool                 share_topic_type{};                            //!< share topic type via registration
+       bool                 share_topic_description{};                     //!< share topic description via registration

@Peguen Peguen changed the title [DRAFT][config] Implementation of new configuration management [config] Implementation of new configuration management May 28, 2024
@Peguen Peguen marked this pull request as ready for review May 28, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants