Skip to content

[core] protobuf service client to v6#1981

Merged
rex-schilasky merged 26 commits intomasterfrom
core/msg-protobuf-client-v6
Feb 13, 2025
Merged

[core] protobuf service client to v6#1981
rex-schilasky merged 26 commits intomasterfrom
core/msg-protobuf-client-v6

Conversation

@rex-schilasky
Copy link
Contributor

@rex-schilasky rex-schilasky commented Jan 30, 2025

Refactor Protobuf Client API

This pull request introduces a major update and refactoring of the Protobuf-based service client API. The old API for CServiceClient has been replaced with a more robust, flexible, and type-safe design that separates untyped and typed interfaces.


Key Changes

  • New API Structure:

    • Untyped Interface (CServiceClient):
      • Provides templated methods for synchronous and asynchronous calls.
      • Returns untyped responses (SServiceResponse).
      • Methods:
        • CallWithResponse<RequestT>()
        • CallWithCallback<RequestT>()
        • CallWithCallbackAsync<RequestT>()
    • Typed Interface (CServiceClientTyped):
      • Offers templated methods for calls where both request and response types are specified.
      • Automatically parses raw responses into strongly typed objects.
      • Methods:
        • CallWithResponse<RequestT, ResponseT>()
        • CallWithCallback<RequestT, ResponseT>()
        • CallWithCallbackAsync<RequestT, ResponseT>()
  • New Client Instance Wrappers:

    • CClientInstance: Handles untyped service calls.
    • CClientInstanceTyped: Handles typed service calls and includes conversion of raw responses into the specified protobuf type.
  • Common Base Class (CServiceClientBase):

    • Consolidates shared functionality (e.g., retrieving client instances and processing responses) between the untyped and typed clients.
    • Parameterized on the service description type and the corresponding client instance type.
  • Improved Code Organization:

    • Enhanced inline documentation for all classes and methods to aid developers in usage and future maintenance.

Impact & Migration

  • Enhanced Maintainability & Flexibility:

    • The refactored design allows for clearer semantics and better type safety.
    • Provides a cleaner separation of concerns between untyped and typed service calls.
  • Breaking Changes:

    • The old CServiceClient interface is replaced. Users should migrate to:
      • eCAL::protobuf::CServiceClient for untyped calls.
      • eCAL::protobuf::CServiceClientTyped for calls that require typed responses.
    • Update any existing call invocations to use the new templated methods (CallWithResponse, CallWithCallback, and CallWithCallbackAsync).
  • Migration Steps:

    1. Replace instances of the old CServiceClient with the new interfaces.
    2. Adjust service call invocations to match the new method signatures.
    3. For typed responses, ensure that the correct request and response types are provided as template parameters.

Code Snippet Old and New API

1. Client Construction & Callback Registration

Old API:

// Create math service client using the old API
eCAL::protobuf::CServiceClient<MathService> math_client;

// Register response callback (using untyped responses)
math_client.AddResponseCallback(OnMathResponse);

// Register event callbacks (using v5 callback signature)
math_client.AddEventCallback(eCAL::eClientEvent::connected,    std::bind(OnClientState, std::placeholders::_2));
math_client.AddEventCallback(eCAL::eClientEvent::disconnected, std::bind(OnClientState, std::placeholders::_2));
math_client.AddEventCallback(eCAL::eClientEvent::timeout,      std::bind(OnClientState, std::placeholders::_2));

New API:

// Create a typed math service client with an inline client state callback
const eCAL::protobuf::CServiceClientTyped<MathService> math_client(OnClientState);

// (Client state callback now receives both the service ID and event data)

2. Service Call Invocation

Old API (Untyped calls):

// Prepare the request message
SFloatTuple math_request;
math_request.set_inp1(inp1);
math_request.set_inp2(inp2);

// Asynchronous call (callback variant)
if (!math_client.Call("Add", math_request))
{
  std::cout << "MathService::Add method call failed." << std::endl;
}

// Synchronous (blocking) call
if (!math_client.Call("Multiply", math_request))
{
  std::cout << "MathService::Multiply method call failed." << std::endl;
}

New API (Typed calls):

// Prepare the request message
SFloatTuple math_request;
math_request.set_inp1(counter);
math_request.set_inp2(counter + 1);

// Asynchronous call (callback variant) with typed response:
if (!math_client.CallWithCallback<SFloatTuple, SFloat>("Add", math_request, OnMathResponse))
{
  std::cout << "MathService::Add call (callback) failed." << std::endl;
}

// Synchronous (blocking) call with typed response:
auto multiply_response = math_client.CallWithResponse<SFloatTuple, SFloat>("Multiply", math_request);
if (multiply_response.first)
{
  for (const auto& resp : multiply_response.second)
  {
    std::cout << "Blocking: Received Multiply response: " << resp.response.out() << std::endl;
  }
}
else
{
  std::cout << "Blocking: MathService::Multiply call failed!" << std::endl;
}

3. Iterating Over Client Instances

Old API:

// Directly call the service method on the client instance
math_client.Call("Divide", math_request);

New API:

// Retrieve individual client instances and call methods on each
auto instances = math_client.GetClientInstances();
for (auto& instance : instances)
{
  if (!instance.CallWithCallback<SFloatTuple, SFloat>("Divide", math_request, OnMathResponse))
  {
    std::cout << "MathService::Divide call on an instance failed." << std::endl;
  }
}

4. Callback Function Signatures

Old API (Untyped Response):

void OnMathResponse(const eCAL::v5::SServiceResponse& service_response)
{
  // Parse the raw response manually
  SFloat response;
  response.ParseFromString(service_response.response);
  std::cout << "Received response: " << response.out() << std::endl;
}

New API (Typed Response):

void OnMathResponse(const eCAL::protobuf::SMsgServiceResponse<SFloat>& service_response)
{
  // Directly access the parsed, typed response
  if (service_response.call_state == eCAL::eCallState::executed)
  {
    std::cout << "Callback: Received response: " << service_response.response.out() << std::endl;
  }
  else
  {
    std::cout << "Callback: Error: " << service_response.error_msg << std::endl;
  }
}

5. Client State Callback Signature

Old API:

void OnClientState(const eCAL::v5::SClientEventCallbackData* data)
{
  // Handle events using the old callback signature.
}

New API:

void OnClientState(const eCAL::SServiceId& service_id, const eCAL::SClientEventCallbackData& data)
{
  // Now the callback provides both the service identity and event data.
  std::cout << "Connected to service: " << service_id.service_name << std::endl;
}

These snippets illustrate the key changes:

  • Typed Interface: The new API uses CServiceClientTyped with templated methods for request/response types, eliminating manual parsing.
  • Improved Call Methods: New methods like CallWithCallback and CallWithResponse enforce type safety.
  • Enhanced Callbacks: Client state and response callbacks now deliver additional context (e.g., service ID) and typed responses.
  • Instance-Level Access: The new API enables per-instance service calls via GetClientInstances().

@rex-schilasky rex-schilasky added the cherry-pick-to-NONE Don't cherry-pick these changes label Jan 30, 2025
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

There were too many comments to post at once. Showing the first 20 out of 21. Check the log or trigger a new build to see more.

{
public:
// Constructors
CClientInstance(eCAL::CClientInstance&& base_instance_) noexcept
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: rvalue reference parameter 'base_instance_' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]

      CClientInstance(eCAL::CClientInstance&& base_instance_) noexcept
                                              ^

@FlorianReimold FlorianReimold added this to the eCAL 6 milestone Feb 6, 2025
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

@hannemn hannemn linked an issue Feb 6, 2025 that may be closed by this pull request
6 tasks
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

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

@rex-schilasky rex-schilasky marked this pull request as ready for review February 6, 2025 10:19
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


#pragma once

#include <ecal/service/client.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'ecal/service/client.h' file not found [clang-diagnostic-error]

#include <ecal/service/client.h>
         ^


ServiceMethodInformationSetT method_information_set;
CProtoDynDecoder dyn_decoder;
std::string error_s;
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 'error_s' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::string error_s;
std::string error_s = 0;

const std::string& request_type_name = method_descriptor->input_type()->name();
const std::string& response_type_name = method_descriptor->output_type()->name();

std::string request_type_descriptor;
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 'request_type_descriptor' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::string request_type_descriptor;
std::string request_type_descriptor = 0;

const std::string& response_type_name = method_descriptor->output_type()->name();

std::string request_type_descriptor;
std::string response_type_descriptor;
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 'response_type_descriptor' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::string response_type_descriptor;
std::string response_type_descriptor = 0;

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

static eCAL::rec::Error GetRemoteConfig(const std::string& hostname, const std::shared_ptr<eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::rec_server::EcalRecServerService>>& remote_rec_server_service, eCAL::rec_server::RecServerConfig& config_output);

static eCAL::rec::Error CallRemoteEcalrecService(const std::shared_ptr<eCAL::protobuf::CServiceClient<eCAL::pb::rec_server::EcalRecServerService>>& remote_ecalsys_service
static eCAL::rec::Error CallRemoteEcalrecService(const std::shared_ptr<eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::rec_server::EcalRecServerService>>& remote_ecalsys_service
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'eCAL::rec_cli::command::Command::CallRemoteEcalrecService' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

        static eCAL::rec::Error CallRemoteEcalrecService(const std::shared_ptr<eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::rec_server::EcalRecServerService>>& remote_ecalsys_service
                                ^
Additional context

app/rec/rec_server_cli/src/commands/command.cpp:75: the definition seen here

      eCAL::rec::Error Command::CallRemoteEcalrecService(const std::shared_ptr<eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::rec_server::EcalRecServerService>>& remote_ecalrec_service
                                ^

app/rec/rec_server_cli/src/commands/command.h:63: differing parameters are named here: ('remote_ecalsys_service'), in definition: ('remote_ecalrec_service')

        static eCAL::rec::Error CallRemoteEcalrecService(const std::shared_ptr<eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::rec_server::EcalRecServerService>>& remote_ecalsys_service
                                ^

// Rec Sever instance and rec_server_service. We will only use one of those, depending on the remote-control setting
std::shared_ptr<eCAL::rec_server::RecServer> rec_server_instance;
std::shared_ptr<eCAL::protobuf::CServiceClient<eCAL::pb::rec_server::EcalRecServerService>> remote_rec_server_service;
std::shared_ptr<eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::rec_server::EcalRecServerService>> remote_rec_server_service;
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 'remote_rec_server_service' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

std::shared_ptr<eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::rec_server::EcalRecServerService>> remote_rec_server_service;
                                                                                                           ^


eCAL::protobuf::CServiceClient<orchestrator::ComponentService> component1("component1");
eCAL::protobuf::CServiceClient<orchestrator::ComponentService> component2("component2");
eCAL::protobuf::CServiceClientTypedResponse<orchestrator::ComponentService> component1("component1");
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 'component1' of type 'eCAL::protobuf::CServiceClientTypedResponseorchestrator::ComponentService' can be declared 'const' [misc-const-correctness]

Suggested change
eCAL::protobuf::CServiceClientTypedResponse<orchestrator::ComponentService> component1("component1");
eCAL::protobuf::CServiceClientTypedResponse<orchestrator::ComponentService> const component1("component1");

eCAL::protobuf::CServiceClient<orchestrator::ComponentService> component1("component1");
eCAL::protobuf::CServiceClient<orchestrator::ComponentService> component2("component2");
eCAL::protobuf::CServiceClientTypedResponse<orchestrator::ComponentService> component1("component1");
eCAL::protobuf::CServiceClientTypedResponse<orchestrator::ComponentService> component2("component2");
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 'component2' of type 'eCAL::protobuf::CServiceClientTypedResponseorchestrator::ComponentService' can be declared 'const' [misc-const-correctness]

Suggested change
eCAL::protobuf::CServiceClientTypedResponse<orchestrator::ComponentService> component2("component2");
eCAL::protobuf::CServiceClientTypedResponse<orchestrator::ComponentService> const component2("component2");

// create player service client
eCAL::protobuf::CServiceClient<eCAL::pb::play::EcalPlayService> player_service;
player_service.AddResponseCallback(OnPlayerResponse);
eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::play::EcalPlayService> player_service;
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 'player_service' of type 'eCAL::protobuf::CServiceClientUntypedCallbackeCAL::pb::play::EcalPlayService' can be declared 'const' [misc-const-correctness]

Suggested change
eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::play::EcalPlayService> player_service;
eCAL::protobuf::CServiceClientUntypedCallback<eCAL::pb::play::EcalPlayService> const player_service;

{
public:
using CServiceClientBase<T>::CServiceClientBase;
virtual ~CServiceClientCallbackBase() override = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'virtual' is redundant since the function is already declared 'override' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
virtual ~CServiceClientCallbackBase() override = default;
;


#pragma once

#include <ecal/service/types.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'ecal/service/types.h' file not found [clang-diagnostic-error]

#include <ecal/service/types.h>
         ^

* @tparam ResponseT The expected protobuf response type.
*/
template <typename ResponseT>
struct TMsgServiceResponse
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: server_id, service_method_information, error_msg [cppcoreguidelines-pro-type-member-init]

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_protobuf_types.h:50:

-       SServiceId                         server_id;                     //!< Identifier of the server that executed the call
-       SServiceMethodInformation          service_method_information;    //!< Information about the called method
+       SServiceId                         server_id{};                     //!< Identifier of the server that executed the call
+       SServiceMethodInformation          service_method_information{};    //!< Information about the called method

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_protobuf_types.h:54:

-       std::string                        error_msg;                     //!< Error message if the call failed
+       std::string                        error_msg{};                     //!< Error message if the call failed

* This class will serve as the parent for the typed and untyped variants.
*/
template <typename T>
class CServiceClientResponseBase : public CServiceClientBase<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'CServiceClientResponseBase' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

>
            ^

{
public:
using CServiceClientBase<T>::CServiceClientBase; // Inherit constructors
virtual ~CServiceClientResponseBase() override = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'virtual' is redundant since the function is already declared 'override' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
virtual ~CServiceClientResponseBase() override = default;
s

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

{
public:
// Constructors
CClientInstanceTyped(eCAL::CClientInstance&& base_instance_) noexcept
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: rvalue reference parameter 'base_instance_' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]

      CClientInstanceTyped(eCAL::CClientInstance&& base_instance_) noexcept
                                                   ^

{
}

CClientInstanceTyped(const SEntityId& entity_id_, const std::shared_ptr<eCAL::CServiceClientImpl>& service_client_impl_)
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_instance [cppcoreguidelines-pro-type-member-init]

      CClientInstanceTyped(const SEntityId& entity_id_, const std::shared_ptr<eCAL::CServiceClientImpl>& service_client_impl_)
      ^

* @tparam ResponseT The expected protobuf response type.
*/
template <typename ResponseT>
struct TMsgServiceResponse
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: server_id, service_method_information, error_msg [cppcoreguidelines-pro-type-member-init]

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_protobuf_types.h:50:

-       SServiceId                  server_id;                     //!< Identifier of the server that executed the call
-       SServiceMethodInformation   service_method_information;    //!< Information about the called method
+       SServiceId                  server_id{};                     //!< Identifier of the server that executed the call
+       SServiceMethodInformation   service_method_information{};    //!< Information about the called method

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_protobuf_types.h:54:

-       std::string                 error_msg;                     //!< Error message if the call failed
+       std::string                 error_msg{};                     //!< Error message if the call failed

@Peguen
Copy link
Contributor

Peguen commented Feb 10, 2025

Apps and service samples tested in vm environment on windows and several ubuntu machines - working as expected.

@rex-schilasky
Copy link
Contributor Author

Danke 🫶

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

There were too many comments to post at once. Showing the first 20 out of 39. Check the log or trigger a new build to see more.

}

bool CPublisherImpl::SetAttribute(const std::string& /* attr_name_ */_, const std::string& /* attr_value_ */)
bool CPublisherImpl::SetAttribute(const std::string& /* attr_name_ */, const std::string& /* attr_value_ */)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'SetAttribute' can be made const [readability-make-member-function-const]

ecal/core/src/pubsub/ecal_publisher_impl.h:91:

-     bool SetAttribute(const std::string& attr_name_, const std::string& attr_value_);
+     bool SetAttribute(const std::string& attr_name_, const std::string& attr_value_) const;
Suggested change
bool CPublisherImpl::SetAttribute(const std::string& /* attr_name_ */, const std::string& /* attr_value_ */)
bool CPublisherImpl::SetAttribute(const std::string& /* attr_name_ */, const std::string& /* attr_value_ */) const

* ========================= eCAL LICENSE =================================
*/

#include <ecal/ecal.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'ecal/ecal.h' file not found [clang-diagnostic-error]

#include <ecal/ecal.h>
         ^


#include "math_service_impl.h"

enum {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum '(unnamed enum at /github/workspace/serialization/protobuf/tests/clientserver_proto_test/src/clientserver_instances_typed_test.cpp:36:1)' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]

enum {
^

//
// Iterative Client Instance Tests (Typed Math - Blocking)
//
TEST(core_cpp_clientserver_proto, IterativeClientInstances_Typed_Math)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_clientserver_proto, IterativeClientInstances_Typed_Math)
TEST(core_cpp_clientserver_proto /*unused*/, IterativeClientInstances_Typed_Math /*unused*/)

//
// Iterative Client Instance Tests (Typed Math - Callback)
//
TEST(core_cpp_clientserver_proto, IterativeClientInstances_Typed_Math_Callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_clientserver_proto, IterativeClientInstances_Typed_Math_Callback)
TEST(core_cpp_clientserver_proto /*unused*/, IterativeClientInstances_Typed_Math_Callback /*unused*/)

eCAL::Finalize();
}

TEST(core_cpp_clientserver_proto, TypedCallbackAsync_Math)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_clientserver_proto, TypedCallbackAsync_Math)
TEST(core_cpp_clientserver_proto /*unused*/, TypedCallbackAsync_Math /*unused*/)


auto start = std::chrono::steady_clock::now();

bool initiated = math_client.CallWithCallbackAsync<SFloat>("Divide", request, callback);
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 'initiated' is not initialized [cppcoreguidelines-init-variables]

Suggested change
bool initiated = math_client.CallWithCallbackAsync<SFloat>("Divide", request, callback);
bool initiated = false = math_client.CallWithCallbackAsync<SFloat>("Divide", request, callback);

//
// Typed Ping Service Tests
//
TEST(core_cpp_clientserver_proto, TypedBlocking_Ping)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_clientserver_proto, TypedBlocking_Ping)
TEST(core_cpp_clientserver_proto /*unused*/, TypedBlocking_Ping /*unused*/)

eCAL::Finalize();
}

TEST(core_cpp_clientserver_proto, TypedCallback_Ping)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_clientserver_proto, TypedCallback_Ping)
TEST(core_cpp_clientserver_proto /*unused*/, TypedCallback_Ping /*unused*/)

prom.set_value(resp.response->answer());
};

bool initiated = ping_client.CallWithCallback<PingResponse>("Ping", request, callback);
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 'initiated' is not initialized [cppcoreguidelines-init-variables]

Suggested change
bool initiated = ping_client.CallWithCallback<PingResponse>("Ping", request, callback);
bool initiated = false = ping_client.CallWithCallback<PingResponse>("Ping", request, callback);

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

There were too many comments to post at once. Showing the first 20 out of 25. Check the log or trigger a new build to see more.

return newValue;
}

T operator++(T)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
T operator++(T)
T operator++(T /*unused*/)

return newValue;
}

T operator--(T)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
T operator--(T)
T operator--(T /*unused*/)

* ========================= eCAL LICENSE =================================
*/

#include <ecal/ecal.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'ecal/ecal.h' file not found [clang-diagnostic-error]

#include <ecal/ecal.h>
         ^

#include "math_service_impl.h"
#include "ping_service_impl.h"

enum { CMN_REGISTRATION_REFRESH_MS = 1000 };
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum '(unnamed enum at /github/workspace/serialization/protobuf/tests/clientserver_proto_test/src/clientserver_event_test.cpp:37:1)' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]

enum { CMN_REGISTRATION_REFRESH_MS = 1000 };
^


enum { CMN_REGISTRATION_REFRESH_MS = 1000 };

TEST(core_cpp_clientserver_proto, ClientConnectEvent)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST(core_cpp_clientserver_proto, ClientConnectEvent)
TEST(core_cpp_clientserver_proto /*unused*/, ClientConnectEvent /*unused*/)

}

// Implements the "Multiply" RPC.
void Multiply(::google::protobuf::RpcController* /*controller*/,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'Multiply' can be made static [readability-convert-member-functions-to-static]

Suggested change
void Multiply(::google::protobuf::RpcController* /*controller*/,
static void Multiply(::google::protobuf::RpcController* /*controller*/,

// Implements the "Multiply" RPC.
void Multiply(::google::protobuf::RpcController* /*controller*/,
const SFloatTuple* request,
SFloat* response,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pointer parameter 'response' can be pointer to const [readability-non-const-parameter]

Suggested change
SFloat* response,
const SFloat* response,

void Multiply(::google::protobuf::RpcController* /*controller*/,
const SFloatTuple* request,
SFloat* response,
::google::protobuf::Closure* done) override
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pointer parameter 'done' can be pointer to const [readability-non-const-parameter]

Suggested change
::google::protobuf::Closure* done) override
const ::google::protobuf::Closure* done) override

}

// Implements the "Divide" RPC.
void Divide(::google::protobuf::RpcController* /*controller*/,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'Divide' can be made static [readability-convert-member-functions-to-static]

Suggested change
void Divide(::google::protobuf::RpcController* /*controller*/,
static void Divide(::google::protobuf::RpcController* /*controller*/,

// Implements the "Divide" RPC.
void Divide(::google::protobuf::RpcController* /*controller*/,
const SFloatTuple* request,
SFloat* response,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pointer parameter 'response' can be pointer to const [readability-non-const-parameter]

Suggested change
SFloat* response,
const SFloat* response,

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

I think that this is a very, very, very good first step in the right direction.

I am unsure about the typed vs. untyped client versions. I'd vote to keep only the typed ones, since in my opinion you are using protobuf in order to get the typing support in the first place!

The client instances should not use the dynamic messages to decode the responses.
And we need to take a look at the response Type, if the message should be stored in a shared_ptr or not.

I am a bit unsure. If we do it, we should also change it in the Message subscribers. It is maybe a bit connected to error handling in message subscribers. #1988

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

{
public:
// Constructors
CClientInstance(eCAL::CClientInstance&& base_instance_) noexcept
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_instance [cppcoreguidelines-pro-type-member-init]

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_instance.h:197:

-       eCAL::CClientInstance m_instance;
+       eCAL::CClientInstance m_instance{};

{
}

CClientInstance(const SEntityId& entity_id_,
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_instance [cppcoreguidelines-pro-type-member-init]

      CClientInstance(const SEntityId& entity_id_,
      ^

{
public:
// Constructors
CClientInstanceTyped(eCAL::CClientInstance&& base_instance_) noexcept
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_instance [cppcoreguidelines-pro-type-member-init]

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_instance.h:320:

-       eCAL::CClientInstance m_instance;
+       eCAL::CClientInstance m_instance{};

{
}

CClientInstanceTyped(const SEntityId& entity_id_,
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_instance [cppcoreguidelines-pro-type-member-init]

      CClientInstanceTyped(const SEntityId& entity_id_,
      ^

auto ret = m_instance.CallWithResponse(method_name_, serialized_request, timeout_ms_);
TMsgServiceResponse<ResponseT> msg_response = ConvertResponse<ResponseT>(ret.second);

bool success = ret.first && (msg_response.call_state == eCallState::executed);
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 'success' is not initialized [cppcoreguidelines-init-variables]

Suggested change
bool success = ret.first && (msg_response.call_state == eCallState::executed);
bool success = false = ret.first && (msg_response.call_state == eCallState::executed);

void Divide(::google::protobuf::RpcController* /*controller*/,
const SFloatTuple* request,
SFloat* response,
::google::protobuf::Closure* done) override
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pointer parameter 'done' can be pointer to const [readability-non-const-parameter]

Suggested change
::google::protobuf::Closure* done) override
const ::google::protobuf::Closure* done) override


#pragma once

#include "ping.pb.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'ping.pb.h' file not found [clang-diagnostic-error]

#include "ping.pb.h"
         ^

{
public:
// Implements the "Ping" RPC.
void Ping(::google::protobuf::RpcController* /*controller*/,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'Ping' can be made static [readability-convert-member-functions-to-static]

Suggested change
void Ping(::google::protobuf::RpcController* /*controller*/,
static void Ping(::google::protobuf::RpcController* /*controller*/,

// Implements the "Ping" RPC.
void Ping(::google::protobuf::RpcController* /*controller*/,
const PingRequest* /*request*/,
PingResponse* response,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pointer parameter 'response' can be pointer to const [readability-non-const-parameter]

Suggested change
PingResponse* response,
const PingResponse* response,

void Ping(::google::protobuf::RpcController* /*controller*/,
const PingRequest* /*request*/,
PingResponse* response,
::google::protobuf::Closure* done) override
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pointer parameter 'done' can be pointer to const [readability-non-const-parameter]

Suggested change
::google::protobuf::Closure* done) override
const ::google::protobuf::Closure* done) override

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

* @tparam ResponseT The expected protobuf response type.
*/
template <typename ResponseT>
struct SMsgServiceResponse
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: server_id, service_method_information, error_msg [cppcoreguidelines-pro-type-member-init]

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_protobuf_types.h:46:

-       SServiceId                 server_id;                     //!< Identifier of the server that executed the call
-       SServiceMethodInformation  service_method_information;    //!< Information about the called method
+       SServiceId                 server_id{};                     //!< Identifier of the server that executed the call
+       SServiceMethodInformation  service_method_information{};    //!< Information about the called method

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_protobuf_types.h:50:

-       std::string                error_msg;                     //!< Error message if the call failed
+       std::string                error_msg{};                     //!< Error message if the call failed

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

{
public:
// Constructors
CClientInstance(eCAL::CClientInstance&& base_instance_) noexcept
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_instance [cppcoreguidelines-pro-type-member-init]

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_instance.h:188:

-       eCAL::CClientInstance m_instance;
+       eCAL::CClientInstance m_instance{};

{
public:
// Constructors
CClientInstanceTyped(eCAL::CClientInstance&& base_instance_) noexcept
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_instance [cppcoreguidelines-pro-type-member-init]

serialization/protobuf/protobuf/include/ecal/msg/protobuf/client_instance.h:312:

-       eCAL::CClientInstance m_instance;
+       eCAL::CClientInstance m_instance{};

request.set_inp1(100.0);
request.set_inp2(50.0);

auto result = math_client.CallWithResponse<SFloatTuple, SFloat>("Multiply", request);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
auto result = math_client.CallWithResponse<SFloatTuple, SFloat>("Multiply", request);
auto result = math_client.CallWithResponse<SFloatTuple;
auto SFloat>("Multiply";("Multiply", request);

request.set_inp1(20.0);
request.set_inp2(22.0);

auto result = math_client.CallWithResponse<SFloatTuple, SFloat>("Add", request);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
auto result = math_client.CallWithResponse<SFloatTuple, SFloat>("Add", request);
auto result = math_client.CallWithResponse<SFloatTuple;
auto SFloat>("Add";("Add", request);

prom.set_value(resp.response.out());
};

bool initiated = math_client.CallWithCallback<SFloatTuple, SFloat>("Divide", request, callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
bool initiated = math_client.CallWithCallback<SFloatTuple, SFloat>("Divide", request, callback);
bool initiated = math_client.CallWithCallback<SFloatTuple;
bool SFloat>("Divide";("Divide", request, callback);


auto start = std::chrono::steady_clock::now();

bool initiated = math_client.CallWithCallbackAsync<SFloatTuple, SFloat>("Divide", request, callback);
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 'initiated' is not initialized [cppcoreguidelines-init-variables]

  bool initiated = math_client.CallWithCallbackAsync<SFloatTuple, SFloat>("Divide", request, callback);
       ^

this fix will not be applied because it overlaps with another fix

PingRequest request;
request.set_message("PING");

auto result = ping_client.CallWithResponse<PingRequest, PingResponse>("Ping", request);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
auto result = ping_client.CallWithResponse<PingRequest, PingResponse>("Ping", request);
auto result = ping_client.CallWithResponse<PingRequest;
auto PingResponse>("Ping";("Ping", request);

prom.set_value(resp.response.answer());
};

bool initiated = ping_client.CallWithCallback<PingRequest, PingResponse>("Ping", request, callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
bool initiated = ping_client.CallWithCallback<PingRequest, PingResponse>("Ping", request, callback);
bool initiated = ping_client.CallWithCallback<PingRequest;
bool PingResponse>("Ping";("Ping", request, callback);

prom.set_value(resp.response.answer());
};

bool initiated = ping_client.CallWithCallback<PingRequest, PingResponse>("Ping", request, callback);
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 'PingResponse' is not initialized [cppcoreguidelines-init-variables]

  bool initiated = ping_client.CallWithCallback<PingRequest, PingResponse>("Ping", request, callback);
                                                             ^

this fix will not be applied because it overlaps with another fix

prom.set_value(resp.response.answer());
};

bool initiated = ping_client.CallWithCallback<PingRequest, PingResponse>("Ping", request, callback);
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 'initiated' is not initialized [cppcoreguidelines-init-variables]

  bool initiated = ping_client.CallWithCallback<PingRequest, PingResponse>("Ping", request, callback);
       ^

this fix will not be applied because it overlaps with another fix

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

Let's merge and continue from there.

@rex-schilasky rex-schilasky merged commit db3f4f1 into master Feb 13, 2025
26 checks passed
@rex-schilasky rex-schilasky deleted the core/msg-protobuf-client-v6 branch February 13, 2025 14:38
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.

eCAL 6 Serialization / Msg API Review

4 participants