[core] Make STopicId available in subscriber callbacks #1719

KerstinKeller · 2024-08-23T07:04:31Z

Add new Callback Type which includes STopicId (ecal_callback.h)
Remove topic_name and topic_id from memfile_pool
Subgate applies Payload::Topic

TODO: take care of PID for TCP and UDP layer.

Description

We don't yet have the information in callbacks about who sent a message, we only receive the topic name. This PR fixes this in a way that callbacks receive an STopicID containing both topic name and additional information (unique sender id).

github-actions

clang-tidy made some suggestions

ecal/core/src/readwrite/shm/ecal_reader_shm.cpp

KerstinKeller · 2024-08-27T07:04:32Z

PID is currently not transferred correctly in UDP und TCP layer.
This information can be passed via CUDPReaderLayer, CTCTReaderLayer.

Otherwise things look good. Might consider some renaming for the future, but all to be discussed.

github-actions

clang-tidy made some suggestions

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp

github-actions

clang-tidy made some suggestions

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp

github-actions

clang-tidy made some suggestions

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp

ecal/tests/cpp/pubsub_test/src/pubsub_receive_test.cpp

rex-schilasky · 2024-09-03T07:04:51Z

PID is currently not transferred correctly in UDP und TCP layer. This information can be passed via CUDPReaderLayer, CTCTReaderLayer.

Otherwise things look good. Might consider some renaming for the future, but all to be discussed.

Callback function type renamed to ReceiveIDCallbackT
UDP and TCP layer transport host name and process id as well now

github-actions

clang-tidy made some suggestions

github-actions · 2024-09-03T08:02:09Z

ecal/core/src/readwrite/shm/ecal_reader_shm.cpp

      }
    }
  }

-  size_t CSHMReaderLayer::OnNewShmFileContent(const std::string& topic_name_, const std::string& topic_id_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_)
+  size_t CSHMReaderLayer::OnNewShmFileContent(const Payload::TopicInfo& topic_id_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_)


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

ecal/core/src/readwrite/shm/ecal_reader_shm.h:51:

- size_t OnNewShmFileContent(const Payload::TopicInfo& topic_id_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_); + static size_t OnNewShmFileContent(const Payload::TopicInfo& topic_id_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_);

github-actions

clang-tidy made some suggestions

github-actions · 2024-09-03T09:37:54Z

ecal/core/src/readwrite/shm/ecal_reader_shm.cpp

      }
    }
  }

-  size_t CSHMReaderLayer::OnNewShmFileContent(const std::string& topic_name_, const std::string& topic_id_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_)
+  size_t CSHMReaderLayer::OnNewShmFileContent(const Payload::TopicInfo& topic_info_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_)


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

ecal/core/src/readwrite/shm/ecal_reader_shm.h:51:

- size_t OnNewShmFileContent(const Payload::TopicInfo& topic_info_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_); + static size_t OnNewShmFileContent(const Payload::TopicInfo& topic_info_, const char* buf_, size_t len_, long long id_, long long clock_, long long time_, size_t hash_);

github-actions · 2024-09-03T09:37:54Z

ecal/tests/cpp/pubsub_test/src/pubsub_callback_topicid.cpp

+
+#include <atomic>
+#include <thread>
+#include <gtest/gtest.h>


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

#include <gtest/gtest.h> ^

github-actions · 2024-09-03T09:37:54Z

ecal/tests/cpp/pubsub_test/src/pubsub_callback_topicid.cpp

+    eCAL::Initialize(config, "callback_topic_id");
+  }
+
+  void TearDown() override


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

Suggested change

void TearDown() override

static void TearDown() override

github-actions · 2024-09-03T09:37:55Z

ecal/tests/cpp/pubsub_test/src/pubsub_callback_topicid.cpp

+  }
+};
+
+TEST_P(TestFixture, OnePubSub)


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

Suggested change

TEST_P(TestFixture, OnePubSub)

TEST_P(TestFixture /*unused*/, OnePubSub /*unused*/)

github-actions · 2024-09-03T09:37:55Z

ecal/tests/cpp/pubsub_test/src/pubsub_callback_topicid.cpp

+  EXPECT_EQ(callback_topic_id, pub_id);
+}
+
+TEST_P(TestFixture, MultiplePubSub)


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

Suggested change

TEST_P(TestFixture, MultiplePubSub)

TEST_P(TestFixture /*unused*/, MultiplePubSub /*unused*/)

github-actions · 2024-09-03T09:37:55Z

ecal/tests/cpp/pubsub_test/src/pubsub_callback_topicid.cpp

+{
+  const int num_publishers = 4;
+
+  std::vector<eCAL::CPublisher> publishers;


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

Suggested change

std::vector<eCAL::CPublisher> publishers;

std::vector<eCAL::CPublisher> publishers = 0;

KerstinKeller · 2024-09-03T12:43:12Z

ecal/core/src/io/shm/ecal_memfile_pool.cpp


 #ifndef NDEBUG
    // log it
-    Logging::Log(log_level_debug2, std::string("CMemFileObserver started (" + topic_name_ + ", " + topic_id_ + ")"));
+    Logging::Log(log_level_debug2, std::string("CMemFileObserver started."));
 #endif


Should we maybe log the memfile name (currently it's not, but we could make it a member variable)

KerstinKeller

Thanks for making the adaptions.

I'm a bit undecided. The changes that you made will work, but it will introduce an incompatibility, if different eCAL versions now communicate. The callbackID will be "wrong" for anyone receiving messages send by an older eCAL version.
And what is even worse, if you will then want to look up the corresponding descriptors to this ID from the descgate, you will get missmatches, or no info.

I wonder if we can make adjustments based on what we know from registration only.
E.g. on registration layer we already receive the info... tid (our unique identifier) - relates to pid X, topic_name Y, host_name Z. ...
Thus the ecal_reader layers can keep track of the information, and based on the incoming tid fill the rest of the information. Then you do not have to send it over the layers together with the actual data.

KerstinKeller · 2024-09-03T12:47:12Z

ecal/core/src/readwrite/tcp/ecal_writer_tcp.cpp

    proto_header_topic.tname = m_topic_name;
    proto_header_topic.tid   = m_topic_id;
+    proto_header_topic.pid   = static_cast<int32_t>(Process::GetProcessID());



Doesn't this "break compatibility" with older ecal versions? we're now filling the sample differently?

KerstinKeller · 2024-09-03T12:54:06Z

ecal/tests/cpp/pubsub_test/src/pubsub_callback_topicid.cpp

+{
+  return std::make_tuple(false, false, true);
+}
+


hmhm I am not convinced. Let's pass Configs instead of tuples to the tests. We want to test based on different configs, not based on different tuples.
You can still create functions that return different Config objects.

Maybe this shows, that we might want to create some helper functions, like "DisableAllLayers", "EnableSHM", "EnableUDP", ... which take reference to a Config object.

- Add new Callback Type which includes STopicId (ecal_callback.h) - Remove topic_name and topic_id from memfile_pool - Subgate applies Payload::TopicInfo - Provide new GetId function for publishers and subscribers (should add also for client / servers in different PR) - Add testcases to check correct IDs are received in callbacks. - Do not modify info being sent over the wire (for compatibility reasons). Instead modify == and < operators for STopicId.

rex-schilasky

Great

github-actions bot reviewed Aug 23, 2024

View reviewed changes

ecal/core/src/readwrite/shm/ecal_reader_shm.cpp Outdated Show resolved Hide resolved

KerstinKeller force-pushed the feature/topicinfo-in-callback branch from 13f952b to e2d035a Compare August 23, 2024 13:22

KerstinKeller added the cherry-pick-to-NONE Don't cherry-pick these changes label Aug 26, 2024

github-actions bot reviewed Aug 27, 2024

View reviewed changes

github-actions bot reviewed Sep 2, 2024

View reviewed changes

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp Outdated Show resolved Hide resolved

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp Outdated Show resolved Hide resolved

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Sep 2, 2024

View reviewed changes

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp Outdated Show resolved Hide resolved

ecal/tests/cpp/pubsub_test/src/callback_topicid_test.cpp Outdated Show resolved Hide resolved

ecal/tests/cpp/pubsub_test/src/pubsub_receive_test.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Sep 3, 2024

View reviewed changes

KerstinKeller commented Sep 3, 2024

View reviewed changes

KerstinKeller force-pushed the feature/topicinfo-in-callback branch from 2caf792 to ab4f097 Compare September 12, 2024 15:20

rex-schilasky approved these changes Sep 12, 2024

View reviewed changes

(de)serialize topic id.

a1d8520

KerstinKeller merged commit 1f1a151 into master Sep 13, 2024
19 of 20 checks passed

KerstinKeller deleted the feature/topicinfo-in-callback branch September 26, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Make STopicId available in subscriber callbacks #1719

[core] Make STopicId available in subscriber callbacks #1719

KerstinKeller commented Aug 23, 2024

github-actions bot left a comment

KerstinKeller commented Aug 27, 2024

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

rex-schilasky commented Sep 3, 2024

github-actions bot left a comment

github-actions bot Sep 3, 2024

github-actions bot left a comment

github-actions bot Sep 3, 2024

github-actions bot Sep 3, 2024

github-actions bot Sep 3, 2024

github-actions bot Sep 3, 2024

github-actions bot Sep 3, 2024

github-actions bot Sep 3, 2024

KerstinKeller Sep 3, 2024

KerstinKeller left a comment •

edited

Loading

KerstinKeller Sep 3, 2024

KerstinKeller Sep 3, 2024

KerstinKeller Sep 3, 2024

rex-schilasky left a comment

	TEST_P(TestFixture, OnePubSub)
	TEST_P(TestFixture /unused/, OnePubSub /unused/)

	TEST_P(TestFixture, MultiplePubSub)
	TEST_P(TestFixture /unused/, MultiplePubSub /unused/)

	std::vector<eCAL::CPublisher> publishers;
	std::vector<eCAL::CPublisher> publishers = 0;

[core] Make STopicId available in subscriber callbacks #1719

[core] Make STopicId available in subscriber callbacks #1719

Conversation

KerstinKeller commented Aug 23, 2024

Description

github-actions bot left a comment

Choose a reason for hiding this comment

KerstinKeller commented Aug 27, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

rex-schilasky commented Sep 3, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 3, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 3, 2024

Choose a reason for hiding this comment

github-actions bot Sep 3, 2024

Choose a reason for hiding this comment

github-actions bot Sep 3, 2024

Choose a reason for hiding this comment

github-actions bot Sep 3, 2024

Choose a reason for hiding this comment

github-actions bot Sep 3, 2024

Choose a reason for hiding this comment

github-actions bot Sep 3, 2024

Choose a reason for hiding this comment

KerstinKeller Sep 3, 2024

Choose a reason for hiding this comment

KerstinKeller left a comment • edited Loading

Choose a reason for hiding this comment

KerstinKeller Sep 3, 2024

Choose a reason for hiding this comment

KerstinKeller Sep 3, 2024

Choose a reason for hiding this comment

KerstinKeller Sep 3, 2024

Choose a reason for hiding this comment

rex-schilasky left a comment

Choose a reason for hiding this comment

KerstinKeller left a comment •

edited

Loading