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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 6, 2024
Merged

service protocol 0 removed #1846

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7719e02
service protocol v0 implementation removed (not the configuration)
rex-schilasky Dec 6, 2024
611d901
service test adapted
rex-schilasky Dec 6, 2024
618f715
Merge branch 'master' into feature/remove-service-protocol-0
rex-schilasky Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ecal/core/src/service/ecal_service_client_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,9 +423,9 @@ namespace eCAL
// I have no idea why, but for some reason the event callbacks of the actual connetions are not even used. The connect / disconnect callbacks are executed whenever a new connection is found, and not when the client has actually connected or disconnected. I am preserving the previous behavior.
};

// Only connect via V0 protocol / V0 port, if V1 port is not available
const auto protocol_version = (service_.tcp_port_v1 != 0 ? service_.version : 0);
const auto port_to_use = (protocol_version == 0 ? service_.tcp_port_v0 : service_.tcp_port_v1);
// use protocol version 1
const auto protocol_version = 1;
const auto port_to_use = service_.tcp_port_v1;

// Create the client and add it to the map
const std::vector<std::pair<std::string, uint16_t>> endpoint_list
Expand Down
68 changes: 14 additions & 54 deletions ecal/core/src/service/ecal_service_server_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
* @brief eCAL service server implementation
**/

#include <ecal/ecal_config.h>

#include "registration/ecal_registration_provider.h"
#include "ecal_global_accessors.h"
#include "ecal_service_server_impl.h"
Expand Down Expand Up @@ -113,17 +111,8 @@ namespace eCAL
return -1;
};

// start service protocol version 0
if (Config::IsServiceProtocolV0Enabled())
{
m_tcp_server_v0 = server_manager->create_server(0, 0, service_callback, true, event_callback);
}

// start service protocol version 1
if (Config::IsServiceProtocolV1Enabled())
{
m_tcp_server_v1 = server_manager->create_server(1, 0, service_callback, true, event_callback);
}
m_tcp_server = server_manager->create_server(1, 0, service_callback, true, event_callback);

// mark as created
m_created = true;
Expand All @@ -147,11 +136,8 @@ namespace eCAL
m_event_callback_map.clear();
}

if (m_tcp_server_v0)
m_tcp_server_v0->stop();

if (m_tcp_server_v1)
m_tcp_server_v1->stop();
if (m_tcp_server)
m_tcp_server->stop();

// mark as no more created
m_created = false;
Expand All @@ -165,8 +151,7 @@ namespace eCAL

{
const std::lock_guard<std::mutex> connected_lock(m_connected_mutex);
m_connected_v0 = false;
m_connected_v1 = false;
m_connected = false;
}

return(true);
Expand Down Expand Up @@ -290,8 +275,7 @@ namespace eCAL
{
if (!m_created) return false;

return (m_tcp_server_v0 && m_tcp_server_v0->is_connected())
|| (m_tcp_server_v1 && m_tcp_server_v1->is_connected());
return (m_tcp_server && m_tcp_server->is_connected());
}

// called by the eCAL::CServiceGate to register a client
Expand All @@ -314,11 +298,8 @@ namespace eCAL
ecal_reg_sample.cmd_type = bct_reg_service;

// might be zero in contruction phase
unsigned short const server_tcp_port_v0(m_tcp_server_v0 ? m_tcp_server_v0->get_port() : 0);
if ((Config::IsServiceProtocolV0Enabled()) && (server_tcp_port_v0 == 0)) return ecal_reg_sample;

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;
Copy link
Contributor

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"


auto& identifier = ecal_reg_sample.identifier;
identifier.entity_id = m_service_id;
Expand All @@ -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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

service.tcp_port_v1 = server_tcp_port;

// add methods
{
Expand Down Expand Up @@ -486,42 +467,21 @@ namespace eCAL
{
const std::lock_guard<std::mutex> connected_lock(m_connected_mutex);

// protocol version 0
if (m_connected_v0)
{
if (m_tcp_server_v0 && !m_tcp_server_v0->is_connected())
{
mode_changed = true;
m_connected_v0 = false;
Logging::Log(log_level_debug2, m_service_name + ": " + "client with protocol version 0 disconnected");
}
}
else
{
if (m_tcp_server_v0 && m_tcp_server_v0->is_connected())
{
mode_changed = true;
m_connected_v0 = true;
Logging::Log(log_level_debug2, m_service_name + ": " + "client with protocol version 0 connected");
}
}

// protocol version 1
if (m_connected_v1)
if (m_connected)
{
if (m_tcp_server_v1 && !m_tcp_server_v1->is_connected())
if (m_tcp_server && !m_tcp_server->is_connected())
{
mode_changed = true;
m_connected_v1 = false;
m_connected = false;
Logging::Log(log_level_debug2, m_service_name + ": " + "client with protocol version 1 disconnected");
}
}
else
{
if (m_tcp_server_v1 && m_tcp_server_v1->is_connected())
if (m_tcp_server && m_tcp_server->is_connected())
{
mode_changed = true;
m_connected_v1 = true;
m_connected = true;
Logging::Log(log_level_debug2, m_service_name + ": " + "client with protocol version 1 connected");
}
}
Expand Down
6 changes: 2 additions & 4 deletions ecal/core/src/service/ecal_service_server_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ namespace eCAL
int RequestCallback(const std::string& request_pb_, std::string& response_pb_);
void EventCallback(eCAL_Server_Event event_, const std::string& message_);

std::shared_ptr<eCAL::service::Server> m_tcp_server_v0;
std::shared_ptr<eCAL::service::Server> m_tcp_server_v1;
std::shared_ptr<eCAL::service::Server> m_tcp_server;

static constexpr int m_server_version = 1;

Expand All @@ -125,8 +124,7 @@ namespace eCAL
EventCallbackMapT m_event_callback_map;

mutable std::mutex m_connected_mutex; //!< mutex protecting the m_connected_v0 and m_connected_v1 variable, as those are modified by the event callbacks in another thread.
bool m_connected_v0 = false;
bool m_connected_v1 = false;
bool m_connected = false;

std::atomic<bool> m_created;
};
Expand Down
6 changes: 0 additions & 6 deletions ecal/service/ecal_service/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,19 @@ set(sources
src/client_manager.cpp
src/client_session.cpp
src/client_session_impl_base.h
src/client_session_impl_v0.cpp
src/client_session_impl_v0.h
src/client_session_impl_v1.cpp
src/client_session_impl_v1.h
src/condition_variable_signaler.h
src/log_defs.h
src/log_helpers.h
src/protocol_layout.h
src/protocol_v0.cpp
src/protocol_v0.h
src/protocol_v1.cpp
src/protocol_v1.h
src/server.cpp
src/server_impl.cpp
src/server_impl.h
src/server_manager.cpp
src/server_session_impl_base.h
src/server_session_impl_v0.cpp
src/server_session_impl_v0.h
src/server_session_impl_v1.cpp
src/server_session_impl_v1.h
)
Expand Down
15 changes: 4 additions & 11 deletions ecal/service/ecal_service/src/client_session.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* ========================= eCAL LICENSE =================================
*
* Copyright (C) 2016 - 2019 Continental Corporation
* Copyright (C) 2016 - 2024 Continental Corporation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -34,7 +34,6 @@
#include <ecal/service/state.h>

#include "client_session_impl_v1.h"
#include "client_session_impl_v0.h"
#include "condition_variable_signaler.h"

namespace eCAL
Expand Down Expand Up @@ -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*/
Copy link
Contributor

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?

Copy link
Contributor Author

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 ..

, const std::vector<std::pair<std::string, std::uint16_t>>& server_list
, const EventCallbackT& event_callback
, const LoggerT& logger)
{
if (protocol_version == 0)
{
impl_ = ClientSessionV0::create(io_context, server_list, event_callback, logger);
}
else
{
impl_ = ClientSessionV1::create(io_context, server_list, event_callback, logger);
}
// we support v1 protocol only
impl_ = ClientSessionV1::create(io_context, server_list, event_callback, logger);
}

ClientSession::~ClientSession()
Expand Down
Loading
Loading