new publisher config API first draft #1561
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ecal/samples/cpp/pubsub/binary/binary_zero_copy_snd/src/binary_zero_copy_snd.cpp
Show resolved
Hide resolved
| m_connected(false), | ||
| m_id(0), | ||
| m_clock(0), | ||
| m_frequency_calculator(3.0f), | ||
| m_frequency_calculator(0.0f), |
There was a problem hiding this comment.
is this change on purpose?
|
|
||
| // tcp config | ||
| tcp.send_mode = eCAL::Config::GetPublisherTcpMode(); | ||
| } |
There was a problem hiding this comment.
This introduces a tight coupling between the publisher config, and eCAL::Config.
I'd prefer the constructor to have hardcoded values, but then an eCAL::Config::GetPublisherConfig which returns then config object, filled with the default values from eCAL::Config.
| size_t m_buffering_shm; | ||
| bool m_zero_copy; | ||
| long long m_acknowledge_timeout_ms; | ||
| CPublisher::Config m_config; |
There was a problem hiding this comment.
I am a bit unsure about this here. Suddenly, ecal_writer needs to know about CPublisher (you had to add an include), and then your low level implementations depends on the high level implementation.
If Publisher knows Writer, and Writer knows publisher, this leads to circular includes, which is a bad thing.
So there are two alternative options:
- he Configuration needs to be extracted from the publisher, into it's own header file.
- Actually the cleanest would be a
WriterConfiguration. and then a function where thePublisherConfigurationis mapped to theWriterConfiguration, even if in our case they are exactly the same.
| memory_file_attr.reserve = m_config.memfile_reserve_percent; | ||
| memory_file_attr.timeout_open_ms = PUB_MEMFILE_OPEN_TO; | ||
| memory_file_attr.timeout_ack_ms = m_config.acknowledge_timeout_ms; | ||
|
|
There was a problem hiding this comment.
Maybe extract this into its own function?
| { | ||
| SetBufferCount(attr_.buffering); | ||
|
|
||
| // store new buffer count and flag change | ||
| m_buffer_count = attr_.buffering; | ||
| m_config.memfile_buffer_count = attr_.buffering; |
There was a problem hiding this comment.
it feels a bit wrong if you change the config internally? the config is set from outside, maybe here you need a variable "actual_buffer_count" or something?
SHMConfig -> Publisher::SHM::Configuration UDPConfig -> Publisher::UDP::Configuration TCPConfig -> Publisher::TCP::Configuration
| **/ | ||
| CPublisher(const std::string& topic_name_) | ||
| : eCAL::CPublisher(topic_name_, GetDataTypeInformation()) | ||
| CPublisher(const std::string& topic_name_, const eCAL::Publisher::Configuration& config_ = {}) |
There was a problem hiding this comment.
warning: constructor does not initialize these fields: builder [cppcoreguidelines-pro-type-member-init]
CPublisher(const std::string& topic_name_, const eCAL::Publisher::Configuration& config_ = {})
^| **/ | ||
| CMsgPublisher(const std::string& topic_name_, const struct SDataTypeInformation& topic_info_) : CPublisher(topic_name_, topic_info_) | ||
| CMsgPublisher(const std::string& topic_name_, const struct SDataTypeInformation& data_type_info_, const Publisher::Configuration& config_ = {}) : CPublisher(topic_name_, data_type_info_, config_) |
There was a problem hiding this comment.
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_ = {}) : CPublisher(topic_name_, data_type_info_, config_)
^| **/ | ||
| explicit CMsgPublisher(const std::string& topic_name_) : CMsgPublisher(topic_name_, GetDataTypeInformation()) | ||
| explicit CMsgPublisher(const std::string& topic_name_, const Publisher::Configuration& config_ = {}) : CMsgPublisher(topic_name_, GetDataTypeInformation(), config_) |
There was a problem hiding this comment.
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_ = {}) : CMsgPublisher(topic_name_, GetDataTypeInformation(), config_)
^
| @@ -1190,29 +863,23 @@ namespace eCAL | |||
| return is_internal_only; | |||
| } | |||
|
|
|||
| void CDataWriter::LogSendMode(TLayer::eSendMode smode_, const std::string& base_msg_) | |||
| void CDataWriter::LogLayerState(bool state_, const std::string& base_msg_) | |||
There was a problem hiding this comment.
warning: method 'LogLayerState' can be made static [readability-convert-member-functions-to-static]
ecal/core/src/readwrite/ecal_writer.h:125:
- void LogLayerState(bool state_, const std::string& base_msg_);
+ static void LogLayerState(bool state_, const std::string& base_msg_);
| @@ -1190,33 +845,27 @@ namespace eCAL | |||
| return is_internal_only; | |||
| } | |||
|
|
|||
| void CDataWriter::LogSendMode(TLayer::eSendMode smode_, const std::string& base_msg_) | |||
| void CDataWriter::LogLayerState(bool state_, const std::string& base_msg_) | |||
There was a problem hiding this comment.
warning: method 'LogLayerState' can be made static [readability-convert-member-functions-to-static]
ecal/core/src/readwrite/ecal_writer.h:133:
- void LogLayerState(bool state_, const std::string& base_msg_);
+ static void LogLayerState(bool state_, const std::string& base_msg_);| @@ -53,83 +52,67 @@ | |||
| #include <string> | |||
| #include <atomic> | |||
| #include <map> | |||
| #include <tuple> | |||
There was a problem hiding this comment.
warning: duplicate include [readability-duplicate-include]
ecal/core/src/readwrite/ecal_writer.h:53:
- #include <map>
- #include <tuple>
+ #include <map>reader Create/Destroy replaced by Constructor/Destructor
|
|
||
| // allow to share topic type | ||
| m_use_ttype = Config::IsTopicTypeSharingEnabled(); | ||
| m_share_ttype = Config::IsTopicTypeSharingEnabled(); |
There was a problem hiding this comment.
warning: 'm_share_ttype' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
ecal/core/src/readwrite/ecal_reader.cpp:73:
- m_created(false)
+ m_share_ttype(Config::IsTopicTypeSharingEnabled()), m_created(false)| m_share_ttype = Config::IsTopicTypeSharingEnabled(); | |
|
|
||
| // allow to share topic description | ||
| m_use_tdesc = Config::IsTopicDescriptionSharingEnabled(); | ||
| m_share_tdesc = Config::IsTopicDescriptionSharingEnabled(); |
There was a problem hiding this comment.
warning: 'm_share_tdesc' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
ecal/core/src/readwrite/ecal_reader.cpp:73:
- m_created(false)
+ m_share_tdesc(Config::IsTopicDescriptionSharingEnabled()), m_created(false)| m_share_tdesc = Config::IsTopicDescriptionSharingEnabled(); | |
| @@ -1190,33 +847,27 @@ namespace eCAL | |||
| return is_internal_only; | |||
| } | |||
|
|
|||
| void CDataWriter::LogSendMode(TLayer::eSendMode smode_, const std::string& base_msg_) | |||
| void CDataWriter::LogLayerState(bool state_, const std::string& base_msg_) | |||
There was a problem hiding this comment.
warning: method 'LogLayerState' can be made static [readability-convert-member-functions-to-static]
ecal/core/src/readwrite/ecal_writer.h:131:
- void LogLayerState(bool state_, const std::string& base_msg_);
+ static void LogLayerState(bool state_, const std::string& base_msg_);…scriber, client, server) Create/Destroy -> Start/Stop
| } | ||
|
|
||
| void CSubGate::Create() | ||
| void CSubGate::Start() |
There was a problem hiding this comment.
warning: method 'Start' can be made static [readability-convert-member-functions-to-static]
ecal/core/src/pubsub/ecal_subgate.h:42:
- void Start();
+ static void Start();| } | ||
|
|
||
| void CClientGate::Create() | ||
| void CClientGate::Start() |
There was a problem hiding this comment.
warning: method 'Start' can be made static [readability-convert-member-functions-to-static]
ecal/core/src/service/ecal_clientgate.h:47:
- void Start();
+ static void Start();| bool Create(const std::string& service_name_, const ServiceMethodInformationMapT& method_information_map_); | ||
|
|
||
| bool Destroy(); | ||
| bool Start(const std::string& service_name_, const ServiceMethodInformationMapT& method_information_map_); |
There was a problem hiding this comment.
warning: function 'eCAL::CServiceClientImpl::Start' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
bool Start(const std::string& service_name_, const ServiceMethodInformationMapT& method_information_map_);
^Additional context
ecal/core/src/service/ecal_service_client_impl.cpp:68: the definition seen here
bool CServiceClientImpl::Start(const std::string& service_name_, const ServiceMethodInformationMapT& methods_information_map_)
^ecal/core/src/service/ecal_service_client_impl.h:51: differing parameters are named here: ('method_information_map_'), in definition: ('methods_information_map_')
bool Start(const std::string& service_name_, const ServiceMethodInformationMapT& method_information_map_);
^| } | ||
|
|
||
| void CServiceGate::Create() | ||
| void CServiceGate::Start() |
There was a problem hiding this comment.
warning: method 'Start' can be made static [readability-convert-member-functions-to-static]
ecal/core/src/service/ecal_servicegate.h:41:
- void Start();
+ static void Start();
Description
Draft implementation of a publisher configuration API in preparation of the new global eCAL configuration API.