Expose raw CDR stream for publish and subscribe #259

Karsten1987 · 2017-10-23T18:21:57Z

Pretty complicated for what it is, but it seems to be the only doable way for now.

The idea here is to cast a custom raw data stream connext_static_message_handle object to a known dds message type. This fake sample is then passed the serialize function where we can depending on which pointer is set cast it back to the underlying type (either the original dds message or the custom raw data).
Unfortunately, we also have to modify the generated serialize function to deal with this fake sample.

dirk-thomas · 2017-10-23T18:42:07Z

...support_connext_cpp/include/rosidl_typesupport_connext_cpp/connext_static_message_handle.hpp

+  const void * untyped_ros_message = nullptr;
+  void * untyped_dds_message = nullptr;
+  const char * raw_message = nullptr;  // making this void for not including rmw dep
+  const size_t * raw_message_length;  // making this void for not including rmw dep


The comment is weird. I don't seen anything void in these two lines?

dirk-thomas · 2017-10-23T18:44:34Z

rosidl_typesupport_connext_cpp/resource/msg__type_support.cpp.em

@@ -149,35 +149,52 @@ convert_ros_message_to_dds(
 bool
 publish__@(spec.base_type.type)(
  void * untyped_topic_writer,
-  const void * untyped_ros_message)
+  ConnextStaticMessageHandle * message_handle)


Why should this not be const anymore? Why should the untyped_dds_message be stored in this handle? Couldn't it be just a local variable?

dirk-thomas · 2017-10-23T18:46:01Z

rosidl_typesupport_connext_cpp/rosidl_typesupport_connext_cpp/__init__.py

    return 0


+def _get_serialization_code(msg_name, indentation):
+    val = ("{{\n"


In order to avoid an indentation which is dependent on the variable name val add a newline after the open parenthesis.

dirk-thomas · 2017-10-23T18:46:58Z

rosidl_typesupport_connext_cpp/rosidl_typesupport_connext_cpp/__init__.py

+
+
+def _modify_plugin_serialize_function(pkg_name, msg_name, lines):
+    # set include correctly - line 49 is the last generated include


Since this seems pretty fragile can you add asserts to ensure that the line actually contains an include and that the next line doesn't.

wjwwood

Looks like you should go through this and find any use of a symmetric function, e.g. create_data/delete_data or malloc/free (should be rmw_allocate/rmw_free), and audit their use to ensure you call the clean up function in all cases.

wjwwood · 2018-03-08T01:49:10Z

rmw_connext_cpp/CMakeLists.txt

+    "connext_static_raw_data_support.cpp"
+  )
+
+  # ament_lint_auto_find_test_dependencies()


This should be worked into a comment about why it's not being used, or should be removed imo.

wjwwood · 2018-03-08T02:00:47Z

rosidl_typesupport_connext_c/resource/msg__type_support_c.cpp.em

+  // TODO(karsten1987): This allocation has to be preallocated
+  // or at least bring in a custom allocator
+  cdr_stream->raw_message =
+    reinterpret_cast<char *>(malloc(sizeof(char) * cdr_stream->message_length));


Should use rmw_allocate for now, at the very least.

wjwwood · 2018-03-08T02:01:11Z

rmw_connext_cpp/src/rmw_publish.cpp

+    RMW_SET_ERROR_MSG("failed to publish message");
+    return RMW_RET_ERROR;
+  }
+  free(cdr_stream.raw_message);


Also, rmw_free.

wjwwood · 2018-03-08T02:05:26Z

rmw_connext_cpp/src/rmw_publish.cpp

+  }
+  if (!publish(topic_writer, &cdr_stream)) {
+    RMW_SET_ERROR_MSG("failed to publish message");
+    return RMW_RET_ERROR;


cdr_stream.raw_message would be leaked here.

wjwwood · 2018-03-08T02:06:57Z

rmw_connext_cpp/src/rmw_publish.cpp

+    return RMW_RET_ERROR;
+  }
+
+  ConnextStaticCDRStream cdr_stream;


Should this or could this be const?

do you mean with static initialization ?

No, I think I was just confused.

wjwwood · 2018-03-08T02:16:49Z

rosidl_typesupport_connext_c/resource/msg__type_support_c.cpp.em

+  if (@(spec.base_type.type)_Plugin_deserialize_from_cdr_buffer(
+      dds_message, cdr_stream->raw_message, cdr_stream->message_length) != RTI_TRUE)
+  {
+    fprintf(stderr, "deserialize from cdr buffer failed\n");


This should use the rcutils logging API at the very least, otherwise perhaps it should be the error state message? Not sure which as I'm not sure what is expected of this function by the caller.

wjwwood · 2018-03-08T02:17:41Z

rosidl_typesupport_connext_c/resource/msg__type_support_c.cpp.em

+      dds_message, cdr_stream->raw_message, cdr_stream->message_length) != RTI_TRUE)
+  {
+    fprintf(stderr, "deserialize from cdr buffer failed\n");
+    return false;


Is dds_message leaked here? I think delete_data needs to be called on it right?

wjwwood · 2018-03-08T02:19:00Z

rosidl_typesupport_connext_cpp/resource/msg__type_support.cpp.em

+  }
+
+  // convert ros to dds
+  if (!convert_ros_message_to_dds(ros_message, *dds_message)) {
    return false;


I think dds_message is leaked here too.

wjwwood · 2018-03-08T02:19:30Z

rosidl_typesupport_connext_cpp/resource/msg__type_support.cpp.em

+  cdr_stream->raw_message =
+    reinterpret_cast<char *>(malloc(sizeof(char) * cdr_stream->message_length));
+  // call the function again and fill the buffer this time
+  if (@(spec.base_type.type)_Plugin_serialize_to_cdr_buffer(cdr_stream->raw_message, &cdr_stream->message_length, dds_message) != RTI_TRUE) {
    return false;


cdr_stream->raw_message is leaked?

Maybe also dds_message.

wjwwood · 2018-03-08T02:19:44Z

rosidl_typesupport_connext_cpp/resource/msg__type_support.cpp.em

+  // TODO(karsten1987): This allocation has to be preallocated
+  // or at least bring in a custom allocator
+  cdr_stream->raw_message =
+    reinterpret_cast<char *>(malloc(sizeof(char) * cdr_stream->message_length));


Needs to use rmw_allocate for now.

Karsten1987 · 2018-06-07T21:18:22Z

rmw_connext_cpp/bin/apply-patch.py

+# limitations under the License.
+
+# originated from the stackoverflow post:
+# https://stackoverflow.com/a/40967337


@dirk-thomas do you think this note is going to be ok as is?

Well, you can't just put OSRF copyright and Apachae license on the code copied from Stackoverflow. I guess you need to "reimplement the function if we can't find any API which does it for us.

Apparently the code is public domain: https://gist.github.com/noporpoise/16e731849eb1231e86d78f9dfeca3abc#file-unifieddiff-py-L3

Which means that IMO it's fine to license it as Apache 2.0.
It may be worth putting the 2016 copyright to the original author that's listed in the file ("Isaac Turner") to give credit even if it's public domain

The copyright must be put here, and better attribution (maybe the GitHub username), also the public domain license should be mentioned here.

I'd recommend:

# Based on `unifieddiff.py` by @noporpoise which is licensed as `Public domain (CC0)` # see: https://gist.github.com/noporpoise/16e731849eb1231e86d78f9dfeca3abc

…appy

wjwwood · 2018-06-12T22:08:19Z

rmw_connext_cpp/bin/apply-patch.py

+# limitations under the License.
+
+# originated from the stackoverflow post:
+# https://stackoverflow.com/a/40967337


The copyright must be put here, and better attribution (maybe the GitHub username), also the public domain license should be mentioned here.

I'd recommend:

# Based on `unifieddiff.py` by @noporpoise which is licensed as `Public domain (CC0)` # see: https://gist.github.com/noporpoise/16e731849eb1231e86d78f9dfeca3abc

wjwwood · 2018-06-12T22:18:56Z

rmw_connext_cpp/src/rmw_publish.cpp

+    return RMW_RET_ERROR;
+  }
+
+  ConnextStaticCDRStream cdr_stream;


No, I think I was just confused.

* use size_t * fix generated code’s support for size_t

dirk-thomas · 2020-07-28T22:58:37Z

rosidl_typesupport_connext_cpp/resource/msg__type_support.cpp.em

+  }
+  if (cdr_stream->buffer_capacity < cdr_stream->buffer_length) {
+    cdr_stream->allocator.deallocate(cdr_stream->buffer, cdr_stream->allocator.state);
+    cdr_stream->buffer = static_cast<char *>(cdr_stream->allocator.allocate(cdr_stream->buffer_length, cdr_stream->allocator.state));


After allocating a bigger buffer the capacity of cdr_stream is never updated. See http://build.ros2.org/view/Rci/job/Rci__nightly-connext_ubuntu_focal_amd64/29/testReport/rclcpp/TestSerializedMessage/serialization/ for a test failing for a while.

See ros2/rosidl_typesupport_connext#58 for the proposed fix.

Karsten1987 added the in progress Actively being worked on (Kanban column) label Oct 23, 2017

Karsten1987 self-assigned this Oct 23, 2017

dirk-thomas reviewed Oct 23, 2017

View reviewed changes

Karsten1987 force-pushed the expose_cdr branch 2 times, most recently from e83132c to c79816a Compare November 16, 2017 17:30

Karsten1987 force-pushed the expose_cdr branch from c8145a6 to 5e72c19 Compare February 8, 2018 17:48

Karsten1987 force-pushed the expose_cdr branch from 70f5a77 to 52399cf Compare February 16, 2018 01:00

Karsten1987 force-pushed the expose_cdr branch from cc1ce49 to 255f5c3 Compare February 27, 2018 16:53

Karsten1987 force-pushed the expose_cdr branch from 75284ee to c260f85 Compare March 6, 2018 17:32

wjwwood suggested changes Mar 8, 2018

View reviewed changes

Karsten1987 force-pushed the expose_cdr branch 2 times, most recently from 793fb6a to 07aa7b9 Compare March 21, 2018 00:03

wjwwood mentioned this pull request Mar 21, 2018

Demo nodes for raw publish and subscribe ros2/demos#185

Merged

Karsten1987 force-pushed the expose_cdr branch from 07aa7b9 to 14aa666 Compare April 25, 2018 17:45

Karsten1987 commented Jun 7, 2018

View reviewed changes

Karsten1987 and others added 15 commits June 11, 2018 22:30

publish_raw function with type cast

350fcc4

modify generated serialize function to cope with type castings

4b2691d

WIP publish and subscribe raw

dd840c7

changes for version 5.3.0

e9f37e7

wip - memory corruption

95cfa46

rtiddsgen 2.5

dd5f792

make it work

f4a4b80

comment about pre-allocation

7825f9c

publish raw with connext cpp

4704116

rewrite type support callback structure

aa2aee1

linters

7654bbd

remove unused files

b2a02ff

linters

62ceb14

[style] changes to the generated raw data idl for making CI linters h…

8b61ffb

…appy

rosidl linters

042cca6

Karsten1987 and others added 12 commits June 11, 2018 22:34

raw generated code for raw data idl

3a44598

patch for functional differences to generated code

8b66d9c

linters again

4b90edc

[style] changes to generated code again

a0601f9

add patch files

3265fb6

stuff from our hackathon

53b99d8

use patched version of generated files

90e4227

remove unused files

750ab52

add rmw_serialize functions

26f9b4e

deallocate memory of temporary cdr stream

1e5484d

linters

3fac8cf

cleanup allocated memory

a810982

Karsten1987 force-pushed the expose_cdr branch from 156b573 to a810982 Compare June 12, 2018 08:16

wjwwood approved these changes Jun 12, 2018

View reviewed changes

Karsten1987 added 2 commits June 13, 2018 10:09

address comments

d5f525f

raw->serialized

988371d

wjwwood approved these changes Jun 14, 2018

View reviewed changes

wjwwood and others added 2 commits June 14, 2018 15:03

use size_t (#289)

9cea055

* use size_t * fix generated code’s support for size_t

fix uncrustify

e74a72d

wjwwood approved these changes Jun 15, 2018

View reviewed changes

wjwwood added 3 commits June 15, 2018 16:46

fix remaining cases where Windows messes up max()

8eaeb9e

fix warnings on Windows

f778624

fix second argument that needs casting

27cd763

Karsten1987 merged commit 861b8f3 into master Jun 16, 2018

dirk-thomas removed the in progress Actively being worked on (Kanban column) label Jun 16, 2018

Karsten1987 deleted the expose_cdr branch June 16, 2018 08:35

dirk-thomas mentioned this pull request Jul 27, 2018

*_raw function ros2/rmw#125

Merged

dirk-thomas mentioned this pull request Jul 28, 2020

fix capacity of serialized messages ros2/rosidl_typesupport_connext#58

Merged

dirk-thomas reviewed Jul 28, 2020

View reviewed changes

fujitatomoya mentioned this pull request Nov 12, 2020

[Requesting Help] Support ContentFilteredTopic on rmw_connext #476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose raw CDR stream for publish and subscribe #259

Expose raw CDR stream for publish and subscribe #259

Karsten1987 commented Oct 23, 2017 •

edited

Loading

dirk-thomas Oct 23, 2017

dirk-thomas Oct 23, 2017

dirk-thomas Oct 23, 2017

dirk-thomas Oct 23, 2017

wjwwood left a comment

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

Karsten1987 May 14, 2018

wjwwood Jun 12, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

wjwwood Mar 8, 2018

Karsten1987 Jun 7, 2018

dirk-thomas Jun 7, 2018

mikaelarguedas Jun 7, 2018

mikaelarguedas Jun 7, 2018

wjwwood Jun 12, 2018

wjwwood Jun 12, 2018

wjwwood Jun 12, 2018

dirk-thomas Jul 28, 2020



		def _modify_plugin_serialize_function(pkg_name, msg_name, lines):
		# set include correctly - line 49 is the last generated include

Expose raw CDR stream for publish and subscribe #259

Expose raw CDR stream for publish and subscribe #259

Conversation

Karsten1987 commented Oct 23, 2017 • 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

wjwwood 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

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

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

Karsten1987 commented Oct 23, 2017 •

edited

Loading