Feature/ipm with rclcpp serialized messages #1931

DensoADAS · 2022-05-17T07:17:30Z

Enabling publishing rclcpp::SerializedMessage for IPM (rcl_serialized_message_t are not supported for IPM).

Generic publisher can be created now with rclcpp::Publisherrclcpp::SerializedMessage. Therefore GenericPublisher is not needed anymore.

uses changes from: #1928

alsora · 2022-06-10T08:18:17Z

rclcpp/include/rclcpp/experimental/intra_process_manager.hpp

+    const auto & sub_ids = publisher_it->second;
+
+    // check if (de)serialization is needed
+    if (sub_ids.take_subscriptions[Indicies::ownership_other].size() +


Should the correct spelling be Indices ?

alsora · 2022-06-10T08:21:12Z

rclcpp/include/rclcpp/experimental/intra_process_manager.hpp

@@ -199,41 +254,40 @@ class IntraProcessManager
        "Calling do_intra_process_publish for invalid or no longer existing publisher id");


Do we need this check also here? Should this function be private so that the user can only call do_intra_process_publish (which takes care of checking the publisher ID)

the method is needed from the callback, which needs public access

alsora · 2022-06-10T08:28:43Z

rclcpp/include/rclcpp/experimental/subscription_intra_process_buffer.hpp

+      return true;
+    } else {
+      // The method is only allowed to be called with valid data (serialized case).
+      throw std::runtime_error("nullptr provided for serialized inter-process communication");


nitpick: rather than having a huge if-else, I personally prefer to check the failure conditions at the top of the function such that the actual "body" is not within the if-else scope.

if (intraprocess_manager == nullptr || serialized_message == nullptr) { throw std::runtime_error("nullptr provided for serialized inter-process communication"); return false; } ..... everything else here ....

alsora · 2022-06-10T08:30:32Z

rclcpp/include/rclcpp/experimental/subscription_intra_process_buffer.hpp

+      }
+      SubscribedTypeUniquePtr message(ptr, subscribed_type_deleter_);
+
+      detail::do_intra_process_publish_same_type<SubscribedType, ROSMessageType, Alloc, Deleter>(


I'm sorry, but I have been out of the loop with the recent changes to the intra-process manager, so maybe this question may be unrelated related to this PR.
This class is a subscription buffer: why does it call a "publish" function?

from comment:

Helper function to expose method of IntraProcessManager

for publishing messages with same data type.

This is needed as the publisher of serialized message is not aware of the subscribed

data type. While the subscription has all needed information (MessageT, Allocator) to

cast and deserialize the message. The type information is forwarded by the helper function.

I think that this helper function should be modified.
It currently does two separate things:

create a message of a certain type

invoke intra-process publish

IMO the helper should just take care of creating the message.
Then it should return it as output such that intra-process publish is invoked elsewhere.

I've splitted it into two methods (one for deserialization and one for forwarding). Yet, the message type is only known within this class so it has to stay here (alternative would be generating a lambda function).

alsora · 2022-06-10T08:34:34Z

rclcpp/include/rclcpp/generic_subscription.hpp

@@ -119,6 +123,61 @@ class GenericSubscription : public rclcpp::SubscriptionBase
        options.event_callbacks.message_lost_callback,
        RCL_SUBSCRIPTION_MESSAGE_LOST);
    }
+
+    // Setup intra process publishing if requested.
+    if (rclcpp::detail::resolve_use_intra_process(options, *node_base)) {


most of this seems identical to what is already done in subscription.hpp... can we avoid this duplication?

I think generic subscprition is not needed after the changes anymore. But I wanted to minimize the code changes for the first PR.

I would prefer to have this moved to the SubscriptionBase class as a protected helper function.
I'm afraid of these going out of sync by the time a new PR is produced.

I removed GenericSubscprition in general to avoid duplicated code, perhaps this breaks with some existing code in other repositories

alsora · 2022-06-10T08:35:37Z

rclcpp/include/rclcpp/generic_subscription.hpp

+    callback_([callback](
+        std::shared_ptr<const rclcpp::SerializedMessage> serialized_message,
+        const rclcpp::MessageInfo & message_info) mutable {
+        callback.dispatch(serialized_message, message_info);


is the linter happy with this indentation? it feels strange that the body is on the same line as the arguments.

moreover, why is this mutable?

mutable is needed to get a copy of "callback" to avoid memory issues

jacobperron · 2022-06-13T21:02:43Z

rclcpp/include/rclcpp/create_publisher.hpp

+ * The NodeT type only needs to have a method called get_node_topics_interface()
+ * which returns a shared_ptr to a NodeTopicsInterface.


I think this comment was intended for the new function above.

jacobperron · 2022-06-13T21:05:06Z

rclcpp/include/rclcpp/create_publisher.hpp

+  // Extract the NodeTopicsInterface from the NodeT.
+  using rclcpp::node_interfaces::get_node_topics_interface;
+  auto node_topics = get_node_topics_interface(node);
+


It would be nice to support the QoS overrides feature here too, via the parameters interface. Is there a reason this was omitted?

you are right, done

rclcpp/include/rclcpp/experimental/subscription_intra_process_base.hpp

jacobperron · 2022-06-13T21:24:36Z

rclcpp/include/rclcpp/experimental/subscription_intra_process_base.hpp

+    const rclcpp::SerializedMessage * serialized_message,
+    IntraProcessManager * intraprocess_manager,
+    uint64_t intra_process_publisher_id,
+    void * untyped_allocator,
+    const std::vector<uint64_t> & take_ownership_subscriptions,
+    const std::vector<uint64_t> & take_shared_subscriptions) = 0;


Could you document these parameters in the docblock? Thanks :)

jacobperron · 2022-06-13T22:20:47Z

rclcpp/include/rclcpp/experimental/subscription_intra_process_buffer.hpp

+      if constexpr (std::is_same<SubscribedType,
+        rclcpp::SerializedMessage>::value ||
+        std::is_same<SubscribedType, rclcpp::SerializedMessage>::value)


We're checking the same condition twice here. Maybe you mean for one of these to check ROSMessageType?

jacobperron · 2022-06-13T22:59:02Z

rclcpp/include/rclcpp/publisher.hpp

+      get_subscription_count() > get_intra_process_subscription_count();
+
+    if (intra_process_is_enabled_ && inter_process_publish_needed) {
+      this->do_intra_process_ros_message_publish(std::move(msg));


Do we need to handle inter-process communication separately from intra-process here (similar to how we publish non-serialized messages)?

I think for serialized message it's currently needed

DensoADAS · 2022-06-22T08:01:35Z

updated to review

alsora · 2022-06-22T08:16:09Z

rclcpp/include/rclcpp/experimental/subscription_intra_process_buffer.hpp

+
+    return true;
+
+    return false;


double return statement

alsora · 2022-06-22T10:09:20Z

rclcpp/include/rclcpp/generic_subscription.hpp

-  // The type support library should stay loaded, so it is stored in the GenericSubscription
-  std::shared_ptr<rcpputils::SharedLibrary> ts_lib_;
-};
+typedef rclcpp::Subscription<rclcpp::SerializedMessage> GenericSubscription;


love this!
nitpick: if you happen to do other changes, please consider to replace typedef with using

DensoADAS · 2022-06-27T15:45:58Z

I think I processed all comments

DensoADAS · 2022-07-07T09:05:07Z

while testing I found two bugs which I fixed

DensoADAS · 2022-07-19T12:50:45Z

@alsora are further steps required from us? thank you

DensoADAS · 2023-05-10T06:24:27Z

What is missing for approval? Thank you 😄

alsora · 2023-05-22T10:46:11Z

Hi @DensoADAS, this is a 1-year old PR and it looks like it has been completely rebased and/or reworked.
It will require a full review from scratch before proceeding.

I'll try to do it this week.

DensoADAS · 2023-05-30T05:03:38Z

Hi @DensoADAS, this is a 1-year old PR and it looks like it has been completely rebased and/or reworked. It will require a full review from scratch before proceeding.

I'll try to do it this week.

@alsora Did you have already time for the review? Thank you.

DensoADAS · 2023-06-12T07:22:21Z

Hi @DensoADAS, this is a 1-year old PR and it looks like it has been completely rebased and/or reworked. It will require a full review from scratch before proceeding.

I'll try to do it this week.

any updates?

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

…base.hpp Signed-off-by: Joshua Hampp <[email protected]> Co-authored-by: Jacob Perron <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

…age and subscriber has a ros message type for intra process communication Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

…mmunication Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

Signed-off-by: Joshua Hampp <[email protected]>

…with IPM Signed-off-by: Joshua Hampp <[email protected]>

Signed-off-by: Joshua Hampp <[email protected]>

clalancette requested a review from jacobperron June 2, 2022 13:33

clalancette assigned jacobperron Jun 2, 2022

alsora reviewed Jun 10, 2022

View reviewed changes

jacobperron reviewed Jun 13, 2022

View reviewed changes

DensoADAS force-pushed the feature/ipm_with_rclcpp_serialized_messages branch from 719f26f to d08f3dd Compare June 20, 2022 09:12

alsora reviewed Jun 22, 2022

View reviewed changes

audrow changed the base branch from master to rolling June 28, 2022 14:20

DensoADAS force-pushed the feature/ipm_with_rclcpp_serialized_messages branch from 6df6d15 to fb485c7 Compare May 8, 2023 09:03

DensoADAS requested review from ivanpauno, hidmic and wjwwood as code owners May 8, 2023 09:03

DensoADAS force-pushed the feature/ipm_with_rclcpp_serialized_messages branch 2 times, most recently from 7793029 to c9a2120 Compare May 8, 2023 11:54

jacobperron removed their assignment May 9, 2023

DensoADAS force-pushed the feature/ipm_with_rclcpp_serialized_messages branch from 907f836 to 60bfac4 Compare May 22, 2023 09:19

alsora self-assigned this May 22, 2023

Joshua Hampp and others added 12 commits January 3, 2024 09:49

added ipm for generic_subscription

bb09e50

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

renamed to SplitSubscriptionsIndices and changed datatype to unsigned

4a34d97

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

updated comments

c8fd558

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

using const modifier

4cbc678

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

Update rclcpp/include/rclcpp/experimental/subscription_intra_process_…

9b5fd63

…base.hpp Signed-off-by: Joshua Hampp <[email protected]> Co-authored-by: Jacob Perron <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

Update rclcpp/include/rclcpp/experimental/subscription_intra_process_…

9e16b43

…base.hpp Signed-off-by: Joshua Hampp <[email protected]> Co-authored-by: Jacob Perron <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

renamed to Indices

de29d0e

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

fixed comment

795a281

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

added comment block

ea9245e

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

added failure case at top

f45d12b

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

enabled qos override

b4236b3

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

fix double return

7880677

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

DensoADAS force-pushed the feature/ipm_with_rclcpp_serialized_messages branch from 780f372 to a8e279c Compare January 3, 2024 08:49

Joshua Hampp and others added 15 commits January 3, 2024 09:50

removed specialization of GenericSubscription

158ab2c

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

splitted deserialization and message forwarding

098b6dd

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

using instead of typedef

048c458

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

fixed intra process publish of serialized message

667d8d2

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

throwing error when subscription callback uses rclcpp::SerializedMess…

c9b148b

…age and subscriber has a ros message type for intra process communication Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

updated test to ipm issue

c2501cf

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

added additional methods for create_generic_subscription

e98c158

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

fixed duplicated messages for ipm and serialized messages

245c868

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

added unit test for generic publisher/subscriber with intraprocess co…

3978f2a

…mmunication Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

fixes due to rebase

16fc15f

Signed-off-by: Joshua Hampp <[email protected]> Signed-off-by: Joshua Hampp <[email protected]>

updated to rolling

280e7be

Signed-off-by: Joshua Hampp <[email protected]>

updated SubscriptionTopicStatistics to current master

9d37234

Signed-off-by: Joshua Hampp <[email protected]>

added test_intra_process_subscriber to test serialized communication …

2115b26

…with IPM Signed-off-by: Joshua Hampp <[email protected]>

extended test for shared callback to cover more use cases

4f6e466

Signed-off-by: Joshua Hampp <[email protected]>

added test for SubscriptionIntraProcess with unique subscriber

a8e279c

Signed-off-by: Joshua Hampp <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ipm with rclcpp serialized messages #1931

Feature/ipm with rclcpp serialized messages #1931

DensoADAS commented May 17, 2022

alsora Jun 10, 2022

alsora Jun 10, 2022

DensoADAS Jun 20, 2022

alsora Jun 10, 2022

alsora Jun 10, 2022 •

edited

Loading

DensoADAS Jun 20, 2022

alsora Jun 22, 2022

DensoADAS Jun 23, 2022

alsora Jun 10, 2022

DensoADAS Jun 20, 2022

alsora Jun 22, 2022

DensoADAS Jun 22, 2022 •

edited

Loading

alsora Jun 10, 2022

DensoADAS Jun 20, 2022

jacobperron Jun 13, 2022

jacobperron Jun 13, 2022

DensoADAS Jun 20, 2022

jacobperron Jun 13, 2022

jacobperron Jun 13, 2022

jacobperron Jun 13, 2022

DensoADAS Jun 20, 2022

DensoADAS commented Jun 22, 2022

alsora Jun 22, 2022

alsora Jun 22, 2022 •

edited

Loading

DensoADAS commented Jun 27, 2022

DensoADAS commented Jul 7, 2022

DensoADAS commented Jul 19, 2022

DensoADAS commented May 10, 2023

alsora commented May 22, 2023

DensoADAS commented May 30, 2023

DensoADAS commented Jun 12, 2023

		@@ -199,41 +254,40 @@ class IntraProcessManager
		"Calling do_intra_process_publish for invalid or no longer existing publisher id");

		* The NodeT type only needs to have a method called get_node_topics_interface()
		* which returns a shared_ptr to a NodeTopicsInterface.

Feature/ipm with rclcpp serialized messages #1931

Are you sure you want to change the base?

Feature/ipm with rclcpp serialized messages #1931

Conversation

DensoADAS commented May 17, 2022

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

alsora Jun 10, 2022 • edited Loading

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

DensoADAS Jun 22, 2022 • edited Loading

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

Choose a reason for hiding this comment

DensoADAS commented Jun 22, 2022

Choose a reason for hiding this comment

alsora Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

DensoADAS commented Jun 27, 2022

DensoADAS commented Jul 7, 2022

DensoADAS commented Jul 19, 2022

DensoADAS commented May 10, 2023

alsora commented May 22, 2023

DensoADAS commented May 30, 2023

DensoADAS commented Jun 12, 2023

alsora Jun 10, 2022 •

edited

Loading

DensoADAS Jun 22, 2022 •

edited

Loading

alsora Jun 22, 2022 •

edited

Loading