-
Notifications
You must be signed in to change notification settings - Fork 180
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
service protocol 0 removed #1846
Conversation
@@ -331,8 +312,8 @@ namespace eCAL | |||
service.uname = Process::GetUnitName(); | |||
service.sname = m_service_name; | |||
|
|||
service.tcp_port_v0 = server_tcp_port_v0; | |||
service.tcp_port_v1 = server_tcp_port_v1; | |||
service.tcp_port_v0 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the internal API? should it be removed there, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the eCAL service registration sample. We could remove this from the registration sample as well but we have to ensure that it is communicated as 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe a separate PR for that, or at least we add it to the TODO list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general very good that we can remove so much code!
Just see the two remarks that I made...
@@ -76,19 +75,13 @@ namespace eCAL | |||
} | |||
|
|||
ClientSession::ClientSession(const std::shared_ptr<asio::io_context>& io_context | |||
, std::uint8_t protocol_version | |||
, std::uint8_t /*protocol_version*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't it make sense to remove the variable from the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the service library. Not sure if one day this library will introduce a new protocol again. This "protocol_version" is exiting all over the library implementation ..
There was a problem hiding this 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
unsigned short const server_tcp_port_v1(m_tcp_server_v1 ? m_tcp_server_v1->get_port() : 0); | ||
if ((Config::IsServiceProtocolV1Enabled()) && (server_tcp_port_v1 == 0)) return ecal_reg_sample; | ||
unsigned short const server_tcp_port(m_tcp_server ? m_tcp_server->get_port() : 0); | ||
if ((Config::IsServiceProtocolV1Enabled()) && (server_tcp_port == 0)) return ecal_reg_sample; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "eCAL::Config::IsServiceProtocolV1Enabled" is directly included [misc-include-cleaner]
ecal/core/src/service/ecal_service_server_impl.cpp:23:
- #include "registration/ecal_registration_provider.h"
+ #include "ecal/ecal_config.h"
+ #include "registration/ecal_registration_provider.h"
@@ -57,7 +57,7 @@ eCAL::service::LoggerT critical_logger(const std::string& node_name) | |||
}; | |||
} | |||
|
|||
constexpr std::uint8_t min_protocol_version = 0; | |||
constexpr std::uint8_t min_protocol_version = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::uint8_t" is directly included [misc-include-cleaner]
ecal/service/test/src/ecal_tcp_service_test.cpp:19:
- #include <gtest/gtest.h>
+ #include <cstdint>
+ #include <gtest/gtest.h>
Description
Service protocol v0 implementation removed (client and server side).
PR may conflict with #1842