Dnae adas/generic subscriber and publisher #1077

DensoADAS · 2020-04-21T13:10:10Z

using SerializedMessage and support in intraprocess manager to create a generic subscriber / publisher without previously known message type

DensoADAS · 2020-04-21T13:26:41Z

@Karsten1987 I did not create a spezialzation of Publisher for SerializedMessages as I disklike copying so much redudant code. I redesigned it so the changes to the Publisher are reduced. I hope it does not break to much with the current api. (But I have also a version with a spezialized publisher in backup)

DensoADAS · 2020-04-21T13:34:51Z

depends on #1076

ivanpauno · 2020-04-21T19:14:22Z

A few questions before reviewing, I'm probably missing some context from #973 discussion:

what's the goal of the "generic" publisher/subscription?
how they differ from Publisher/Subscription?

Thanks!

ivanpauno · 2020-04-21T21:24:52Z

rclcpp/include/rclcpp/create_publisher.hpp

+create_generic_publisher(
+  NodeT & node,
+  const std::string & topic_name,
+  const rosidl_message_type_support_t & type_support,


Why both MessageT template parameter and the type_support argument are needed?

Naming should also be more clear.
Maybe, just an override of create_publisher?

I think MessageT is used for the allocator

rclcpp/include/rclcpp/create_subscription.hpp

ivanpauno · 2020-04-21T21:27:59Z

rclcpp/include/rclcpp/publisher.hpp

-    if (RCL_RET_OK != status) {
-      rclcpp::exceptions::throw_from_rcl_error(status, "failed to publish message");
-    }
+    generic_publisher::do_inter_process_publish(publisher_handle_, msg);


I don't understand why the two new free functions are needed, and why a "normal" Publisher is calling the functions in generic_publisher namespace.
@DensoADAS can you expain?

for the SerializedMessage the original "do_inter_process_publish" will fail as it's no ROS message

can we just are an override for a serialized message in the Publisher class?

rclcpp/include/rclcpp/publisher_factory.hpp

rclcpp/include/rclcpp/subscription_factory.hpp

Karsten1987

Given that there is no generic publisher type, I would think it makes sense best to not mention this and keep the notation to publisher.

rclcpp/include/rclcpp/create_publisher.hpp

rclcpp/include/rclcpp/create_subscription.hpp

rclcpp/include/rclcpp/publisher.hpp

DensoADAS · 2020-04-22T04:26:12Z

A few questions before reviewing, I'm probably missing some context from #973 discussion:
* what's the goal of the "generic" publisher/subscription?

* how they differ from `Publisher`/`Subscription`?
Thanks!

The "generic" publisher/subscriber allows communication without knowing the message type to compile time. Therefore a serialized message can bei published or received and the endpoint has to handle the content itself. This is used for example in rosbag2.

This implementation supports now intraprocess communication when using the C++ version of serialized messages which make the memory handling more safe.

ivanpauno · 2020-04-22T14:26:36Z

The "generic" publisher/subscriber allows communication without knowing the message type to compile time. Therefore a serialized message can bei published or received and the endpoint has to handle the content itself. This is used for example in rosbag2.

Ah ok. So in those cases, you use MessageT=SerializedMessageBase, and you don't need to know the message type at compile time, right?

* implemented interface to deserialize a given serialized message to ros message Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

* serialized publisher uses correct method to forward content to subscriber * non-serialized publisher can serialize message for serialized subscriber Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

…cess_message Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

…y needed for the next PR) Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* Reflect changes in rclcpp API Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com> * Revert earlier fix made in rclcpp Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

* added constructor with typesupport * added more generic do_inter_process_publish Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

…d subscribers Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Karsten1987 · 2020-04-22T16:06:59Z

CI (build everything, test rclcpp):

Linux
Linux-aarch64
macOS
Windows

DensoADAS · 2020-04-22T18:43:45Z

Switching back to create_generic_... would solve this. Do you have another suggestion?

DensoADAS · 2020-04-24T14:39:01Z

@Karsten1987 I lost a little bit the track of the conversation. What's the current status?

Karsten1987 · 2020-04-24T15:57:16Z

In my opinion, #1076 (comment) this is where we stand. I think the intraprocess manager should be sorted out before pushing forward with this change.

* removed specialization to rclcpp::Serialization Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

* allocate message * getter for serialization * provide untyped message Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

* two extra subscription/publisher pairs (for serialized ones) * added methods for (de)serialization * added methods for messages without known allocator * splitted up publishing with same type and not matching message type Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Karsten1987 · 2020-04-29T18:03:47Z

Thanks @DensoADAS for iterating over this PR.

Unfortunately, we've passed API freeze for Foxy and after talking with @jacobperron (the Foxy lead), we've decided to table this for G-Turtle.

nyxaria · 2020-08-17T08:03:20Z

Is a generic subscriber available in Foxy? I don't think I can use ROS2 until it is in. :/

jacobperron · 2020-08-17T17:31:00Z

Is a generic subscriber available in Foxy?

Not in rclcpp. Unless something like this PR can be done in an ABI compatible way, I don't think we can expect it to land in Foxy.

As mentioned on ROS Answers, you can do this in Python and/or copy the C++ implementation from rosbag2 for your application.

ivanpauno reviewed Apr 21, 2020

View reviewed changes

Karsten1987 reviewed Apr 21, 2020

View reviewed changes

DensoADAS force-pushed the dnae_adas/generic_subscriber_and_publisher branch from e0fb512 to b5bce53 Compare April 22, 2020 07:13

Joshua Hampp and others added 18 commits April 22, 2020 17:03

* updated interface to SubscriptionIntraProcessBase

9d88f0e

* implemented interface to deserialize a given serialized message to ros message Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

added support for SerializedMessage in Intraprocessmanger, so:

1dad3ad

* serialized publisher uses correct method to forward content to subscriber * non-serialized publisher can serialize message for serialized subscriber Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

updated unit test for intraprocess manager to match new interface

c552c0b

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

updated is_serialized_message_class traits

2513cc2

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

renamed provide_serialized_intra_process_message to provide_intra_pro…

fd6a20e

…cess_message Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

fixed Serialization class for non-supported type (yes this is actuall…

f0c8dd3

…y needed for the next PR) Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

updated to (de)seriaizle method

facb43d

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

fix build regression (ros2#1078)

08a3251

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Reflect changes in rclcpp API (ros2#1079)

0577f39

* Reflect changes in rclcpp API Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com> * Revert earlier fix made in rclcpp Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

extended publisher:

9ae2da1

* added constructor with typesupport * added more generic do_inter_process_publish Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

added create_generic_subscription and create_generic_publisher

9490d9f

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

added test for intra process communication with generic publishers an…

3b430bf

…d subscribers Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

fixed date

88a6927

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

fixed unintended change

63ad2fe

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

replaced generic_publisher

86f8ef9

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

removed unneeded template typename

62635ab

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

renamed create_generic_... to create_

d676d12

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

updated to new SerializedMessage

aeb69bf

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

DensoADAS force-pushed the dnae_adas/generic_subscriber_and_publisher branch from b5bce53 to aeb69bf Compare April 22, 2020 15:38

emersonknapp mentioned this pull request Apr 24, 2020

Export generic_subscription.hpp/cpp and generic_publisher.hpp/cpp for external use ros2/rosbag2#378

Closed

Joshua Hampp added 7 commits April 29, 2020 18:40

* added get_type_support_handle to rclcpp::Serialization

8d2de84

* removed specialization to rclcpp::Serialization Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

extended subscription_intra_process(_base):

e2c19bb

* allocate message * getter for serialization * provide untyped message Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

update unit test for subscription_intra_process(_base)

370061b

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

simplified code

2e5a7bd

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

fixed modifier

6b3d9c7

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

updated comments

f5c3362

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>

Karsten1987 mentioned this pull request Aug 23, 2020

deserialize SerializedMessage into arbitrary message type on runtime? #1283

Closed

clalancette assigned wjwwood Mar 29, 2021

wjwwood removed their assignment Mar 29, 2021

clalancette added the backlog label Mar 28, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dnae adas/generic subscriber and publisher #1077

Dnae adas/generic subscriber and publisher #1077

DensoADAS commented Apr 21, 2020

DensoADAS commented Apr 21, 2020 •

edited

Loading

DensoADAS commented Apr 21, 2020 •

edited by ivanpauno

Loading

ivanpauno commented Apr 21, 2020 •

edited

Loading

ivanpauno Apr 21, 2020

ivanpauno Apr 21, 2020

DensoADAS Apr 22, 2020

ivanpauno Apr 21, 2020

DensoADAS Apr 22, 2020

ivanpauno Apr 22, 2020

Karsten1987 left a comment

DensoADAS commented Apr 22, 2020

ivanpauno commented Apr 22, 2020

Karsten1987 commented Apr 22, 2020 •

edited

Loading

DensoADAS commented Apr 22, 2020

DensoADAS commented Apr 24, 2020

Karsten1987 commented Apr 24, 2020

Karsten1987 commented Apr 29, 2020

nyxaria commented Aug 17, 2020

jacobperron commented Aug 17, 2020

Dnae adas/generic subscriber and publisher #1077

Are you sure you want to change the base?

Dnae adas/generic subscriber and publisher #1077

Conversation

DensoADAS commented Apr 21, 2020

DensoADAS commented Apr 21, 2020 • edited Loading

DensoADAS commented Apr 21, 2020 • edited by ivanpauno Loading

ivanpauno commented Apr 21, 2020 • edited Loading

ivanpauno Apr 21, 2020

Choose a reason for hiding this comment

ivanpauno Apr 21, 2020

Choose a reason for hiding this comment

DensoADAS Apr 22, 2020

Choose a reason for hiding this comment

ivanpauno Apr 21, 2020

Choose a reason for hiding this comment

DensoADAS Apr 22, 2020

Choose a reason for hiding this comment

ivanpauno Apr 22, 2020

Choose a reason for hiding this comment

Karsten1987 left a comment

Choose a reason for hiding this comment

DensoADAS commented Apr 22, 2020

ivanpauno commented Apr 22, 2020

Karsten1987 commented Apr 22, 2020 • edited Loading

DensoADAS commented Apr 22, 2020

DensoADAS commented Apr 24, 2020

Karsten1987 commented Apr 24, 2020

Karsten1987 commented Apr 29, 2020

nyxaria commented Aug 17, 2020

jacobperron commented Aug 17, 2020

DensoADAS commented Apr 21, 2020 •

edited

Loading

DensoADAS commented Apr 21, 2020 •

edited by ivanpauno

Loading

ivanpauno commented Apr 21, 2020 •

edited

Loading

Karsten1987 commented Apr 22, 2020 •

edited

Loading