Fastdds type support extensions #67

MiguelCompany · 2021-03-17T09:43:00Z

This extends the initialization callback max_serialized_size so it returns whether the type support refers to a POD type.

This change is required in order to support zero-copy on rmw_fastrtps_cpp.

Connects to ros2/rmw_fastrtps#516
Connects to ros2/rmw_connextdds#14

hidmic · 2021-03-26T19:50:25Z

rosidl_typesupport_fastrtps_c/resource/msg__type_support_c.cpp.em

+  // Start considering the type is plain.
+  // Internal methods will set values to false when necessary.
+  full_bounded = true;
+  is_plain = true;


@MiguelCompany nit: consider moving this into max_serialized_size_*(). It's unusual for a function to not have a well defined output argument value.

I understand your point. However, as the internal methods may be called from other internal methods (in the case of composed types), a parent may have turned one of the booleans to false.

Consider the following messages:

# Inner.msg uint32 index uin8[10] data # Outer.msg string title Inner data

When calling _Outer__max_serialized_size, the following sequence of events will occur:

_Outer__max_serialized_size will set both booleans to true and call max_serialized_size_Outer

max_serialized_size_Outer will set both booleans to false due to the string title

max_serialized_size_Outer will call max_serialized_size_Inner due to the field data

max_serialized_size_Inner will not change the booleans

When calling _Inner__max_serialized_size, the following sequence of events will occur:

_Inner__max_serialized_size will set both booleans to true and call max_serialized_size_Inner

max_serialized_size_Inner will not change the booleans

In summary:

The public API on this package is the one defined for the callbacks.

For that API, this PR is fixing the code in order to always have a well-defined output argument value.

max_serialized_size_* should be considered internal, but needs to be exported in order for messages to be included on other messages

Hmm, that's a problem only because max_serialized_size_*() functions for composed types are given the same is_plain to mutate. I won't block on this, maybe it's best for performance, but I will point out it obscures the logic a bit. A logical AND of the is_plain variable for each field in a type is easier to read.

Ok. Fixed in e69b64e

hidmic · 2021-03-26T19:50:55Z

rosidl_typesupport_fastrtps_cpp/resource/msg__type_support.cpp.em

+  // Start considering the type is plain.
+  // Internal methods will set values to false when necessary.
+  full_bounded = true;
+  is_plain = true;


@MiguelCompany same as above.

Fixed in e69b64e

rosidl_typesupport_fastrtps_cpp/include/rosidl_typesupport_fastrtps_cpp/message_type_support.h

MiguelCompany · 2021-04-08T13:33:36Z

@hidmic I addressed your concerns

@clalancette May we run a CI on this?

clalancette · 2021-04-08T21:17:50Z

CI:

Linux
Linux-aarch64
macOS
Windows

MiguelCompany · 2021-04-09T05:10:28Z

@clalancette I think you forgot to include ros2/rmw_connextdds#14

clalancette · 2021-04-09T12:25:35Z

CI:

Linux
Linux-aarch64
macOS
Windows

MiguelCompany · 2021-04-12T05:50:06Z

@clalancette As stated in the description, this PR

Connects to ros2/rmw_fastrtps#516
Connects to ros2/rmw_connextdds#14

So both should be used on the build. Sorry for the confusion.

Thank you!

MiguelCompany · 2021-04-14T07:47:35Z

@clalancette Friendly ping

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2021-04-15T12:00:02Z

@clalancette I rebased this and added commit 7e709c2, in which I make the change binary compatible.

clalancette · 2021-04-20T20:13:16Z

Here's a CI with what should be all of the correct pieces in place:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2021-04-21T05:38:53Z

@clalancette I just fixed the linter errors on the generated code.

clalancette · 2021-04-21T16:47:56Z

Here's another try:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

clalancette · 2021-04-22T18:11:01Z

CI again:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

MiguelCompany · 2021-04-23T08:50:22Z

@clalancette eProsima/Fast-DDS#1931 fixes the assertions

clalancette · 2021-04-23T13:07:41Z

Windows Debug again:

MiguelCompany · 2021-04-26T05:26:40Z

Windows Debug again:

@clalancette The has been stalled for three days before even starting to build.

15:38:00 C:\J\workspace\ci_windows>rem Kill any running docker containers, which may be leftover from aborted jobs 
15:38:00 
15:38:00 C:\J\workspace\ci_windows>powershell -Command "if ($(docker ps -q) -ne $null) { docker stop $(docker ps -q)}"

clalancette · 2021-04-26T11:59:18Z

Let's try again:
Windows Debug again:

hidmic · 2021-05-05T12:41:01Z

rosidl_typesupport_fastrtps_cpp/include/rosidl_typesupport_fastrtps_cpp/message_type_support.h

+
+#define ROSIDL_TYPESUPPORT_FASTRTPS_UNBOUNDED_TYPE 0x00
+#define ROSIDL_TYPESUPPORT_FASTRTPS_BOUNDED_TYPE   0x01
+#define ROSIDL_TYPESUPPORT_FASTRTPS_PLAIN_TYPE     0x03


@MiguelCompany meta: why not an enum? You'd get type checking.

hidmic · 2021-05-05T12:44:09Z

@clalancette it looks like this PR's ready to go?

clalancette · 2021-05-05T15:25:21Z

@clalancette it looks like this PR's ready to go?

Yeah, I think this is ready to go in with ros2/rmw_fastrtps#523 and ros2/rmw_connextdds#14 , but I'd like to run one more CI on it. But I'm going to hold off on that until we sort out ros2/rclcpp#1657

clalancette · 2021-05-06T13:44:44Z

CI:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

clalancette · 2021-05-11T13:08:11Z

The warnings are all just an artifact of this needing to be rebased. The test failures look like known flakes. So I'm going to go ahead and merge this along with the rest of the PRs for this.

MiguelCompany mentioned this pull request Mar 17, 2021

Fastdds type support extensions ros2/rmw_fastrtps#516

Closed

MiguelCompany force-pushed the fastdds-type-support-extensions branch from dda708e to c83f80a Compare March 23, 2021 09:20

This was referenced Mar 26, 2021

Fastdds type support extensions eProsima/rmw_connextdds#1

Closed

Support extended signature for message_type_support_callbacks_t::max_serialized_size() from rosidl_typesupport_fastrtps_cpp ros2/rmw_connextdds#14

Merged

hidmic reviewed Mar 26, 2021

View reviewed changes

asorbini reviewed Mar 29, 2021

View reviewed changes

rosidl_typesupport_fastrtps_cpp/include/rosidl_typesupport_fastrtps_cpp/message_type_support.h Show resolved Hide resolved

MiguelCompany requested a review from hidmic April 8, 2021 05:30

hidmic approved these changes Apr 8, 2021

View reviewed changes

MiguelCompany added 4 commits April 15, 2021 07:44

Extend max_serialized_size interface to account for plain types.

4acc922

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Set initial values for output parameters.

f7ad6a3

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Added feature define.

cd4be91

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Always give value to output parameters.

565433c

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the fastdds-type-support-extensions branch from e69b64e to 81fdacd Compare April 15, 2021 05:45

Binary compatible interface.

7e709c2

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the fastdds-type-support-extensions branch from 81fdacd to 7e709c2 Compare April 15, 2021 07:45

Fix linter errors on generated code.

eb34af9

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany mentioned this pull request Apr 28, 2021

High CPU usage when using DDS intra-process communication ros2/rclcpp#1642

Open

hidmic reviewed May 5, 2021

View reviewed changes

clalancette merged commit 748be56 into ros2:master May 11, 2021

clalancette mentioned this pull request May 11, 2021

Loan messages implementation ros2/rmw_fastrtps#523

Merged

MiguelCompany deleted the fastdds-type-support-extensions branch May 11, 2021 13:43

MiguelCompany mentioned this pull request Jun 7, 2021

Correctly inform that a BoundedSequence is bounded #71

Merged

MiguelCompany mentioned this pull request Jul 9, 2021

[Galactic] Loan messages implementation ros2/rmw_fastrtps#547

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fastdds type support extensions #67

Fastdds type support extensions #67

MiguelCompany commented Mar 17, 2021 •

edited

Loading

hidmic Mar 26, 2021

MiguelCompany Mar 29, 2021

hidmic Mar 29, 2021

MiguelCompany Apr 8, 2021

hidmic Mar 26, 2021

MiguelCompany Mar 29, 2021

MiguelCompany Apr 8, 2021

MiguelCompany commented Apr 8, 2021

clalancette commented Apr 8, 2021

MiguelCompany commented Apr 9, 2021

clalancette commented Apr 9, 2021

MiguelCompany commented Apr 12, 2021

MiguelCompany commented Apr 14, 2021

MiguelCompany commented Apr 15, 2021

clalancette commented Apr 20, 2021

MiguelCompany commented Apr 21, 2021

clalancette commented Apr 21, 2021

clalancette commented Apr 22, 2021 •

edited

Loading

MiguelCompany commented Apr 23, 2021

clalancette commented Apr 23, 2021

MiguelCompany commented Apr 26, 2021

clalancette commented Apr 26, 2021

hidmic May 5, 2021

hidmic commented May 5, 2021

clalancette commented May 5, 2021

clalancette commented May 6, 2021 •

edited

Loading

clalancette commented May 11, 2021

Fastdds type support extensions #67

Fastdds type support extensions #67

Conversation

MiguelCompany commented Mar 17, 2021 • 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

MiguelCompany commented Apr 8, 2021

clalancette commented Apr 8, 2021

MiguelCompany commented Apr 9, 2021

clalancette commented Apr 9, 2021

MiguelCompany commented Apr 12, 2021

MiguelCompany commented Apr 14, 2021

MiguelCompany commented Apr 15, 2021

clalancette commented Apr 20, 2021

MiguelCompany commented Apr 21, 2021

clalancette commented Apr 21, 2021

clalancette commented Apr 22, 2021 • edited Loading

MiguelCompany commented Apr 23, 2021

clalancette commented Apr 23, 2021

MiguelCompany commented Apr 26, 2021

clalancette commented Apr 26, 2021

Choose a reason for hiding this comment

hidmic commented May 5, 2021

clalancette commented May 5, 2021

clalancette commented May 6, 2021 • edited Loading

clalancette commented May 11, 2021

MiguelCompany commented Mar 17, 2021 •

edited

Loading

clalancette commented Apr 22, 2021 •

edited

Loading

clalancette commented May 6, 2021 •

edited

Loading