udp receive logic redesign #1271

rex-schilasky · 2023-11-26T10:33:42Z

Description

UDP logic redesigned:

separate message udp receiver for:
- rcv_sample.cpp: udp payload and registration (eCAL::pb::Sample)
- rcv_logging.cpp: udp logging messages (eCAL::pb::LogMessage)
UDP defragmentation logic separated to rcv_sample_slot.cpp
all receive threads now spawned inside udp receiver implementation
eCAL::CThread fully replaced by std::thread
shm monitoring logic integration into registration receiver simplified

Related issues

Preparation for solving #1236
Fixes #1276

Cherry-pick to

none

CReceiveSlot logic in separate unit rcv_sample_slot.cpp shm monitoring registration simplified

buggy thread usage removed

…th a 'C' :-)).

KerstinKeller

@rex-schilasky I don't think I can review this just by looking at it, I guess it needs to be reviewed in a 1:1 session.

KerstinKeller · 2023-12-01T10:39:58Z

ecal/core/include/ecal/ecal_monitoring.h

@@ -107,7 +107,7 @@ namespace eCAL
     *
     * @return Zero if succeeded.
    **/
-    ECAL_DEPRECATE_SINCE_5_12("use GetMonitoring and publish yourself")
+    ECAL_DEPRECATE_SINCE_5_12("No more implemented")


This comment is missleading. How about "The function PubMonitoring is no longer implemented. Instead use GetMonitoring and publish yourself"?

new subfolder for higher level udp protobuf message handling io/udp/protobuf

cleanup naming add IO::UDP namespace for internal (IO) UDP logic

github-actions

clang-tidy made some suggestions

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

github-actions · 2023-12-07T13:38:49Z

app/rec/rec_tests/rec_rpc_tests/src/external_ecal_rec.cpp

+  {
+    std::this_thread::sleep_for(std::chrono::milliseconds(500));
+
+    eCAL::pb::rec_server::RecServerConfig config_pb;


warning: variable 'config_pb' is not initialized [cppcoreguidelines-init-variables]

Suggested change

eCAL::pb::rec_server::RecServerConfig config_pb;

eCAL::pb::rec_server::RecServerConfig config_pb = 0;

github-actions · 2023-12-07T13:38:49Z

ecal/core/src/ecal_def.h

+#define CMN_UDP_RECEIVE_THREAD_CYCLE_TIME_MS           1000



warning: replace macro with enum [modernize-macro-to-enum]

Suggested change

#define CMN_UDP_RECEIVE_THREAD_CYCLE_TIME_MS 1000

enum {

CMN_UDP_RECEIVE_THREAD_CYCLE_TIME_MS = 1000};

github-actions · 2023-12-07T13:38:50Z

ecal/core/src/ecal_def.h

-/* cylce time udp paylaod receive thread in ms */
-#define CMN_PAYLOAD_RECEIVE_THREAD_CYCLE_TIME_MS       1000
+/* cylce time udp receive threads in ms */
+#define CMN_UDP_RECEIVE_THREAD_CYCLE_TIME_MS           1000


warning: macro 'CMN_UDP_RECEIVE_THREAD_CYCLE_TIME_MS' defines an integral constant; prefer an enum instead [modernize-macro-to-enum]

#define CMN_UDP_RECEIVE_THREAD_CYCLE_TIME_MS 1000 ^

github-actions · 2023-12-07T13:38:50Z

ecal/core/src/io/udp/ecal_udp_logging_receiver.h

+{
+  namespace UDP
+  {
+    class CLoggingReceiver


warning: class 'CLoggingReceiver' defines a non-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]

class CLoggingReceiver ^

github-actions · 2023-12-07T13:38:50Z

ecal/core/src/io/udp/ecal_udp_sample_receiver.cpp

+      if (m_sample_receiver == nullptr) return(0);
+
+      // read sample_name size
+      const unsigned short sample_name_size = ((unsigned short*)(msg_buffer_.data()))[0];


warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

const unsigned short sample_name_size = ((unsigned short*)(msg_buffer_.data()))[0]; ^

github-actions · 2023-12-07T13:38:51Z

ecal/core/src/io/udp/fragmentation/snd_fragments.cpp

+      size_t sent_sum(0);
+
+      int32_t total_packet_num = int32_t(buf_len_ / MSG_PAYLOAD_SIZE);
+      if (buf_len_ % MSG_PAYLOAD_SIZE) total_packet_num++;


warning: implicit conversion 'unsigned long' -> bool [readability-implicit-bool-conversion]

Suggested change

if (buf_len_ % MSG_PAYLOAD_SIZE) total_packet_num++;

if ((buf_len_ % MSG_PAYLOAD_SIZE) != 0u) total_packet_num++;

github-actions · 2023-12-07T13:38:52Z

ecal/core/src/io/udp/sendreceive/linux/socket_os.h

+      getifaddrs(&ifap);
+
+      // create a list of network interfaces indexes
+      for (ifa = ifap; ifa; ifa = ifa->ifa_next)


warning: implicit conversion 'ifaddrs *' -> bool [readability-implicit-bool-conversion]

Suggested change

for (ifa = ifap; ifa; ifa = ifa->ifa_next)

for (ifa = ifap; ifa != nullptr; ifa = ifa->ifa_next)

github-actions · 2023-12-07T13:38:52Z

ecal/core/src/io/udp/sendreceive/linux/socket_os.h

+      // create a list of network interfaces indexes
+      for (ifa = ifap; ifa; ifa = ifa->ifa_next)
+      {
+        if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_PACKET)


warning: implicit conversion 'struct sockaddr *' -> bool [readability-implicit-bool-conversion]

Suggested change

if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_PACKET)

if ((ifa->ifa_addr != nullptr) && ifa->ifa_addr->sa_family == AF_PACKET)

github-actions · 2023-12-07T13:38:52Z

ecal/core/src/io/udp/sendreceive/linux/socket_os.h

+      {
+        if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_PACKET)
+        {
+          int index = if_nametoindex(ifa->ifa_name);


warning: variable 'index' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int index = if_nametoindex(ifa->ifa_name);

int const index = if_nametoindex(ifa->ifa_name);

github-actions · 2023-12-07T13:38:52Z

ecal/core/src/io/udp/sendreceive/linux/socket_os.h

+        if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_PACKET)
+        {
+          int index = if_nametoindex(ifa->ifa_name);
+          if (index)


warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]

Suggested change

if (index)

if (index != 0)

github-actions

clang-tidy made some suggestions

github-actions · 2023-12-07T15:20:00Z

ecal/core/src/io/udp/ecal_udp_sample_receiver.cpp

+    }
+
+    CSampleReceiver::CSampleReceiver(const IO::UDP::SReceiverAttr& attr_, HasSampleCallbackT has_sample_callback_, ApplySampleCallbackT apply_sample_callback_) :
+      m_has_sample_callback(has_sample_callback_), m_apply_sample_callback(apply_sample_callback_)


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

ecal/core/src/io/udp/ecal_udp_sample_receiver.cpp:27:

+ + #include <utility>

Suggested change

m_has_sample_callback(has_sample_callback_), m_apply_sample_callback(apply_sample_callback_)

m_has_sample_callback(std::move(has_sample_callback_)), m_apply_sample_callback(apply_sample_callback_)

github-actions · 2023-12-07T15:20:01Z

ecal/core/src/io/udp/sendreceive/linux/socket_os.h

+    inline static bool set_socket_mcast_group_option(int socket, const char* ipaddr_, int option)
+    {
+      // set the multicast socket option on all interfaces
+      for (int iface : get_interface_index_list())


warning: variable 'iface' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

for (int iface : get_interface_index_list())

for (int const iface : get_interface_index_list())

github-actions · 2023-12-07T15:20:01Z

ecal/core/src/io/udp/sendreceive/linux/socket_os.h

+        group->sin_addr.s_addr = inet_addr(ipaddr_);
+        group->sin_port = 0;
+
+        int rc = setsockopt(socket, IPPROTO_IP, option, &group_req, sizeof(group_source_req));


warning: variable 'rc' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int rc = setsockopt(socket, IPPROTO_IP, option, &group_req, sizeof(group_source_req));

int const rc = setsockopt(socket, IPPROTO_IP, option, &group_req, sizeof(group_source_req));

github-actions · 2023-12-07T15:20:01Z

ecal/core/src/io/udp/sendreceive/udp_receiver_asio.cpp

+      AddMultiCastGroup(attr_.address.c_str());
+
+      // state successful creation
+      m_created = true;


warning: 'm_created' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]

ecal/core/src/io/udp/sendreceive/udp_receiver_asio.cpp:39:

- m_created(false), + m_created(true),

Suggested change

m_created = true;

github-actions · 2023-12-07T15:20:01Z

ecal/core/src/monitoring/ecal_monitoring_def.cpp


      log_ = logging.SerializeAsString();
      return((int)log_.size());
    }

-    int PubMonitoring(bool state_, std::string name_ /* = "ecal.monitoring"*/)
+    int PubMonitoring(bool /*state_*/, std::string /*name_*/)


warning: the parameter #2 is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change

int PubMonitoring(bool /*state_*/, std::string /*name_*/)

int PubMonitoring(bool /*state_*/, const std::string& /*name_*/)

ecal/core/include/ecal/ecal_monitoring.h:110:

- ECAL_API int PubMonitoring(bool state_, std::string name_ = "ecal.monitoring"); + ECAL_API int PubMonitoring(bool state_, const std::string& name_ = "ecal.monitoring");

github-actions · 2023-12-07T15:20:02Z

ecal/core/src/registration/ecal_registration_provider.h

-    bool                             m_reg_topics;
-    bool                             m_reg_services;
-    bool                             m_reg_process;
+    static std::atomic<bool>            m_created;


warning: variable 'm_created' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static std::atomic<bool> m_created; ^

github-actions · 2023-12-07T15:20:02Z

ecal/core/src/registration/ecal_registration_receiver.cpp

-          return_value &= ApplySample(sample);
+          for (const auto& sample : sample_list.samples())
+          {
+            if (g_registration_receiver()) g_registration_receiver()->ApplySample(sample);


warning: implicit conversion 'CRegistrationReceiver *' -> bool [readability-implicit-bool-conversion]

Suggested change

if (g_registration_receiver()) g_registration_receiver()->ApplySample(sample);

if (g_registration_receiver() != nullptr) g_registration_receiver()->ApplySample(sample);

github-actions · 2023-12-07T15:20:02Z

ecal/core/src/registration/ecal_registration_receiver.h

@@ -84,6 +80,7 @@ namespace eCAL
    void EnableLoopback(bool state_);
    bool LoopBackEnabled() const { return m_loopback; };

+    bool HasSample(const std::string& /*sample_name_*/) { return(true); };


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

Suggested change

bool HasSample(const std::string& /*sample_name_*/) { return(true); };

static bool HasSample(const std::string& /*sample_name_*/) { return(true); };

github-actions · 2023-12-07T15:20:02Z

ecal/core/src/registration/ecal_registration_receiver.h

-    static std::atomic<bool>         m_created;
-    bool                             m_network;
-    bool                             m_loopback;
+    static std::atomic<bool>              m_created;


warning: variable 'm_created' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static std::atomic<bool> m_created; ^

github-actions · 2023-12-07T15:20:03Z

ecal/core/src/util/ecal_thread.h

+     * @param callback A callback function to be executed in the CallbackThread thread.
+     */
+    CCallbackThread(std::function<void()> callback)
+      : callback_(callback) {}


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

ecal/core/src/util/ecal_thread.h:28:

+ #include <utility>

Suggested change

: callback_(callback) {}

: callback_(std::move(callback)) {}

JoinMultiCastGroupTwice feature finally removed

github-actions

clang-tidy made some suggestions

github-actions · 2023-12-14T09:06:17Z

ecal/core/src/io/udp/sendreceive/udp_receiver_asio.cpp

+      m_created = true;
+    }
+
+    CUDPReceiverAsio::~CUDPReceiverAsio()


warning: an exception may be thrown in function '~CUDPReceiverAsio' which should not throw exceptions [bugprone-exception-escape]

CUDPReceiverAsio::~CUDPReceiverAsio() ^

KerstinKeller

I believe that you know what you are doing 😄

rex-schilasky added 5 commits November 24, 2023 21:12

receive thread now inside CSampleReceiver

b3d89f8

one common cycle time for all udp receiver

e8011af

logging receiver moved to io/udp

7892840

eCAL::CThread replaced finally by std::thread

e6c2458

CReceiveSlot logic in separate unit rcv_sample_slot.cpp shm monitoring registration simplified

missing include fixed

f0baa8a

rex-schilasky marked this pull request as draft November 26, 2023 19:04

rex-schilasky added 7 commits November 26, 2023 20:17

missing header fixed

7486257

exclude rec_rpc_tests to run all other tests for now

a238c1c

ecal_thread back again :-) (now based on std::thread)

aa39d7c

udp ttl 1 for all platforms in local mode

8de0a9a

buggy thread usage removed

dynamic sleep time for rec rpc test

181506f

timoout for GetTopics test adapted

e0a969e

CallbackThread renamed to CCallbackThread (all classes shall start wi…

f0061ca

…th a 'C' :-)).

rex-schilasky marked this pull request as ready for review November 30, 2023 10:15

KerstinKeller reviewed Dec 1, 2023

View reviewed changes

rex-schilasky mentioned this pull request Dec 1, 2023

ttl 0 is not a valid ttl for unicast #1276

Closed

rex-schilasky added 8 commits December 1, 2023 16:21

comment fixed

71c79a8

new subfolder for udp message (de)fragmentation io/udp/fragmentation

61455d5

new subfolder for higher level udp protobuf message handling io/udp/protobuf

some more cleanups in the udp functions file structure

82c4237

separate send module for log messages

94eca56

cleanup file structure

f406a3f

cleanup naming add IO::UDP namespace for internal (IO) UDP logic

linux fix

59dc218

clang-tidy

56a518e

removed some udp io dependencies from eCAL::Process and eCAL::Config

1771300

github-actions bot reviewed Dec 7, 2023

View reviewed changes

includes cleaned up

69da2b1

github-actions bot reviewed Dec 7, 2023

View reviewed changes

rex-schilasky added 3 commits December 12, 2023 20:40

minor alignments with ecal-core-udp

4a90d87

ecal_win_network.h removed

ac01c7f

bugfix in udp receiver destruction

36d8dc7

JoinMultiCastGroupTwice feature finally removed

github-actions bot reviewed Dec 14, 2023

View reviewed changes

protecting udp receive sockets by mutex (should fix #1212)

9073656

KerstinKeller approved these changes Dec 14, 2023

View reviewed changes

rex-schilasky merged commit 28ef1a1 into master Dec 14, 2023
14 checks passed

rex-schilasky deleted the feature/udp-receive-threading branch December 14, 2023 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

udp receive logic redesign #1271

udp receive logic redesign #1271

rex-schilasky commented Nov 26, 2023 •

edited

Loading

KerstinKeller left a comment

KerstinKeller Dec 1, 2023

github-actions bot left a comment

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot left a comment

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot Dec 7, 2023

github-actions bot left a comment

github-actions bot Dec 14, 2023

KerstinKeller left a comment

	eCAL::pb::rec_server::RecServerConfig config_pb;
	eCAL::pb::rec_server::RecServerConfig config_pb = 0;

	if (buf_len_ % MSG_PAYLOAD_SIZE) total_packet_num++;
	if ((buf_len_ % MSG_PAYLOAD_SIZE) != 0u) total_packet_num++;

	for (ifa = ifap; ifa; ifa = ifa->ifa_next)
	for (ifa = ifap; ifa != nullptr; ifa = ifa->ifa_next)

	if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_PACKET)
	if ((ifa->ifa_addr != nullptr) && ifa->ifa_addr->sa_family == AF_PACKET)

	int index = if_nametoindex(ifa->ifa_name);
	int const index = if_nametoindex(ifa->ifa_name);

	m_has_sample_callback(has_sample_callback_), m_apply_sample_callback(apply_sample_callback_)
	m_has_sample_callback(std::move(has_sample_callback_)), m_apply_sample_callback(apply_sample_callback_)

	for (int iface : get_interface_index_list())
	for (int const iface : get_interface_index_list())

	int rc = setsockopt(socket, IPPROTO_IP, option, &group_req, sizeof(group_source_req));
	int const rc = setsockopt(socket, IPPROTO_IP, option, &group_req, sizeof(group_source_req));

	int PubMonitoring(bool /state_/, std::string /name_/)
	int PubMonitoring(bool /state_/, const std::string& /name_/)

	if (g_registration_receiver()) g_registration_receiver()->ApplySample(sample);
	if (g_registration_receiver() != nullptr) g_registration_receiver()->ApplySample(sample);

	bool HasSample(const std::string& /sample_name_/) { return(true); };
	static bool HasSample(const std::string& /sample_name_/) { return(true); };

udp receive logic redesign #1271

udp receive logic redesign #1271

Conversation

rex-schilasky commented Nov 26, 2023 • edited Loading

Description

Related issues

Cherry-pick to

KerstinKeller left a comment

Choose a reason for hiding this comment

KerstinKeller Dec 1, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot Dec 7, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 14, 2023

Choose a reason for hiding this comment

KerstinKeller left a comment

Choose a reason for hiding this comment

rex-schilasky commented Nov 26, 2023 •

edited

Loading