[core] ported CMsgPublisher/Subscriber to v6 #1944

rex-schilasky · 2025-01-24T11:52:03Z

Description

CMsgPub/Sub porting to v6.

…callback

github-actions

clang-tidy made some suggestions

app/mon/mon_tui/src/main.cpp

ecal/core/include/ecal/msg/subscriber.h

ecal/core/src/pubsub/ecal_subscriber.cpp

serialization/protobuf/samples/pubsub/person_events_rec/src/person_events_rec.cpp

github-actions · 2025-01-24T13:40:54Z

serialization/protobuf/samples/pubsub/person_events_rec/src/person_events_rec.cpp

-  sub.AddEventCallback(eCAL::eSubscriberEvent::connected,         evt_callback);
-  sub.AddEventCallback(eCAL::eSubscriberEvent::disconnected,      evt_callback);
-  sub.AddEventCallback(eCAL::eSubscriberEvent::dropped,           evt_callback);
+  eCAL::protobuf::CSubscriber<pb::People::Person> sub("person", OnEvent);


warning: variable 'sub' of type 'eCAL::protobuf::CSubscriberpb::People::Person' (aka 'CMessageSubscriber<pb::People::Person, internal::ProtobufDeserializer>') can be declared 'const' [misc-const-correctness]

Suggested change

eCAL::protobuf::CSubscriber<pb::People::Person> sub("person", OnEvent);

eCAL::protobuf::CSubscriber<pb::People::Person> const sub("person", OnEvent);

serialization/protobuf/samples/pubsub/person_rec/src/person_rec.cpp

serialization/string/samples/pubsub/minimal_rec/src/minimal_rec.cpp

msg/capnproto/publisher.h -> v6 play/play_core/src/measurement_container -> v6

github-actions

clang-tidy made some suggestions

serialization/capnproto/capnproto/include/ecal/msg/capnproto/publisher.h

serialization/protobuf/protobuf/include/ecal/msg/protobuf/publisher.h

serialization/protobuf/samples/pubsub/person_events_snd/src/person_events_snd.cpp

- no default constructor - 2 constructor types with and without event callback - no Create function

github-actions

clang-tidy made some suggestions

github-actions · 2025-01-27T08:52:58Z

serialization/capnproto/capnproto/include/ecal/msg/capnproto/publisher.h

      **/
-      CPublisher()
-        : eCAL::v5::CPublisher()
+      explicit CPublisher(const std::string& topic_name_, const eCAL::Publisher::Configuration& config_ = GetPublisherConfiguration()) : eCAL::CPublisher(topic_name_, GetDataTypeInformation(), config_)


warning: constructor does not initialize these fields: builder [cppcoreguidelines-pro-type-member-init]

serialization/capnproto/capnproto/include/ecal/msg/capnproto/publisher.h:172:

- std::unique_ptr<capnp::MallocMessageBuilder> builder; + std::unique_ptr<capnp::MallocMessageBuilder> builder{};

github-actions · 2025-01-27T08:52:58Z

serialization/capnproto/capnproto/include/ecal/msg/capnproto/publisher.h

      **/
-      CPublisher(const std::string& topic_name_, const eCAL::Publisher::Configuration& config_ = {})
-        : eCAL::v5::CPublisher(topic_name_, GetDataTypeInformation(), config_)
+      explicit CPublisher(const std::string& topic_name_, const eCAL::PubEventCallbackT& event_callback_, const eCAL::Publisher::Configuration& config_ = GetPublisherConfiguration()) : eCAL::CPublisher(topic_name_, GetDataTypeInformation(), event_callback_, config_)


warning: constructor does not initialize these fields: builder [cppcoreguidelines-pro-type-member-init]

explicit CPublisher(const std::string& topic_name_, const eCAL::PubEventCallbackT& event_callback_, const eCAL::Publisher::Configuration& config_ = GetPublisherConfiguration()) : eCAL::CPublisher(topic_name_, GetDataTypeInformation(), event_callback_, config_) ^

KerstinKeller · 2025-01-27T11:54:24Z

app/play/play_core/src/measurement_container.cpp

@@ -219,7 +215,8 @@ bool MeasurementContainer::PublishFrame(long long index)
        {
          timestamp_usecs = std::chrono::duration_cast<std::chrono::microseconds>(frame_table_[index].send_timestamp_.time_since_epoch()).count();
        }
-        frame_table_[index].publisher_info_->publisher_.SetID(frame_table_[index].send_id_);
+        // this is not supported by the eCAL v6 API
+        //frame_table_[index].publisher_info_->publisher_.SetID(frame_table_[index].send_id_);


Could this in any way be problematic?
So with eCAL 5, publishers will have the same ID they had when originally recording the measurement. In eCAL 6, they won't.

Its about the topic id for subscriber side filtering. This feature is rarely used and no way to support this in the future, it's removed on both sides (pub and sub). So in v5 I can filter out different "streams" of the the same topic on subscriber side in v6 the connection will just receive all messages and I need to implement an ID myself (for example as part of my protobuf description).

KerstinKeller · 2025-01-27T11:55:57Z

app/rec/rec_server_cli/src/ecal_rec_server_cli.cpp

@@ -144,6 +144,9 @@ std::unique_ptr<eCAL::rec_cli::command::Record> record_command;
 int main(int argc, char** argv)
 {
 #ifdef WIN32
+  (void)argc; // suppress unused warning


Why surpress it only for Windows? also maybe just comment in the function declaration int main(int /*argc*/, char** /*argv*/) (also this has nothing to do with this PR!!! 😏

For LINUX platform both arguments are used unfortunately and yes it's not really related but I tried to fix at least some warnings when I touched a file.

KerstinKeller · 2025-01-27T11:59:45Z

contrib/ecaltime/simtime/src/ecal_time_simtime.h

@@ -113,7 +113,7 @@ namespace eCAL
    bool first_message_received;                /**< Whether we received at least one Message (used for the status message)*/
    eCAL::pb::SimTime::eState play_state;         /**< Current state (used for the status message)*/

-    eCAL::protobuf::CSubscriber<eCAL::pb::SimTime> sim_time_subscriber; /**< Subscriber for getting simulation timestamps */
+    std::unique_ptr<eCAL::protobuf::CSubscriber<eCAL::pb::SimTime>> sim_time_subscriber; /**< Subscriber for getting simulation timestamps */


I kind of don't like that now we're "forcing" code to use pointers.
Do you now have to check the pointers in other locations?

Without a Create/Destroy interface and the removed empty default constructors I see no alternative here.

We need to think about error handling for all communication entities (pub/sub/server/client) in case construction fails.

Yes, but the thing is, construction is unlikely to fail. Failure is more likely to occur during the runtime. E.g. The connections (memory files, sockets, ...) are usually not created during construction, but when we receive monitoring information about other entities in the system.
So we have to find a way to deal with this effectively.
Maybe error/event callbacks would be interesting in that case, too.

KerstinKeller

I just have the few minor remarks... but I guess it looks good!

KerstinKeller · 2025-01-27T12:07:30Z

ecal/core/include/ecal/msg/publisher.h

    **/
-    explicit CMsgPublisher(const std::string& topic_name_, const Publisher::Configuration& config_ = GetPublisherConfiguration()) : CMsgPublisher(topic_name_, GetDataTypeInformation(), config_)
+    CMsgPublisher(const std::string& topic_name_, const struct SDataTypeInformation& data_type_info_, const PubEventCallbackT& event_callback_, const Publisher::Configuration& config_ = GetPublisherConfiguration()) : CPublisher(topic_name_, data_type_info_, event_callback_, config_)


Why did you remove the explicit keyword?

Also why the struct keywords? They are not necessary and I would consider them bad practice in this case. (I know we have them in various places, but I already remove them when I do see them).

Fixed by next commit.

KerstinKeller · 2025-01-27T12:12:40Z

ecal/core/include/ecal/pubsub/publisher.h

@@ -70,7 +70,7 @@ namespace eCAL
       * @param config_          Optional configuration parameters.
      **/
      ECAL_API_EXPORTED_MEMBER
-        CPublisher(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const PubEventCallbackT event_callback_, const Publisher::Configuration& config_ = GetPublisherConfiguration());
+        CPublisher(const std::string& topic_name_, const SDataTypeInformation& data_type_info_, const PubEventCallbackT& event_callback_, const Publisher::Configuration& config_ = GetPublisherConfiguration());


I came across this, too, and I do wonder if it was on purpose, to copy the callbacks... But wasn't sure.

struct SDataTypeInformation -> SDataTypeInformation

ported CMsgPublisher/Subscriber to v6

3a36e3f

rex-schilasky added the cherry-pick-to-NONE Don't cherry-pick these changes label Jan 24, 2025

rex-schilasky added this to the eCAL 6 milestone Jan 24, 2025

rex-schilasky marked this pull request as draft January 24, 2025 11:52

rex-schilasky added 2 commits January 24, 2025 14:20

some bug fixes

1172908

samples/pubsub/addressbook_rec/src/addressbook_rec variant now using …

d6164c6

…callback

github-actions bot reviewed Jan 24, 2025

View reviewed changes

addressbook_rec sample fixed for v6 API

0065213

rex-schilasky marked this pull request as ready for review January 25, 2025 14:09

rex-schilasky added 10 commits January 25, 2025 15:26

clang-tidy

e327753

Merge branch 'master' into core/v6-msg-pub-sub

e651e79

Registration::STopicId -> STopicId

8064e89

msg/dynamic.h -> v6

a45837d

test fixed

287c7e8

some more fixes (Add -> Set, Rem -> Remove)

52f64d3

addressbook_rec_dynamic sample fixed

ec5eec4

msg/protobuf/publisher.h -> v6

0c911d9

msg/capnproto/publisher.h -> v6 play/play_core/src/measurement_container -> v6

Create() removed

9ddae4c

Create() removed

bb5f00b

github-actions bot reviewed Jan 26, 2025

View reviewed changes

all publisher aligned:

380d1ea

- no default constructor - 2 constructor types with and without event callback - no Create function

rex-schilasky requested a review from KerstinKeller January 27, 2025 08:42

github-actions bot reviewed Jan 27, 2025

View reviewed changes

Merge branch 'master' into core/v6-msg-pub-sub

70805f3

KerstinKeller reviewed Jan 27, 2025

View reviewed changes

KerstinKeller approved these changes Jan 27, 2025

View reviewed changes

explicit constructors

7d0078b

struct SDataTypeInformation -> SDataTypeInformation

rex-schilasky merged commit fc0b30f into master Jan 27, 2025
21 checks passed

rex-schilasky deleted the core/v6-msg-pub-sub branch January 27, 2025 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] ported CMsgPublisher/Subscriber to v6 #1944

[core] ported CMsgPublisher/Subscriber to v6 #1944

rex-schilasky commented Jan 24, 2025

github-actions bot left a comment

github-actions bot Jan 24, 2025

github-actions bot left a comment

github-actions bot left a comment

github-actions bot Jan 27, 2025

github-actions bot Jan 27, 2025

KerstinKeller Jan 27, 2025

rex-schilasky Jan 27, 2025

KerstinKeller Jan 27, 2025

rex-schilasky Jan 27, 2025

KerstinKeller Jan 27, 2025

rex-schilasky Jan 27, 2025

rex-schilasky Jan 27, 2025

KerstinKeller Jan 27, 2025

KerstinKeller left a comment

KerstinKeller Jan 27, 2025

KerstinKeller Jan 27, 2025

rex-schilasky Jan 27, 2025

KerstinKeller Jan 27, 2025

	eCAL::protobuf::CSubscriber<pb::People::Person> sub("person", OnEvent);
	eCAL::protobuf::CSubscriber<pb::People::Person> const sub("person", OnEvent);

[core] ported CMsgPublisher/Subscriber to v6 #1944

[core] ported CMsgPublisher/Subscriber to v6 #1944

Conversation

rex-schilasky commented Jan 24, 2025

Description

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 24, 2025

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

github-actions bot Jan 27, 2025

Choose a reason for hiding this comment

github-actions bot Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KerstinKeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment