Serialize and deserialize arrays #45

esteve · 2016-06-29T09:51:15Z

This PR serializes and deserializes unbounded arrays. Tested locally with two Python scripts (listener and talker), using different permutations of OpenSplice and FastRTPS.

dirk-thomas · 2016-06-29T14:55:30Z

rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h

@@ -259,6 +259,7 @@ void serialize_array(
            printf("vector overcomes the maximum length\n");
            throw std::runtime_error("vector overcomes the maximum length");
        }
+        ser << (int32_t)data.size;


Instead of replicating the wire format here this should use the API of Cdr to perform the serialization. I guess in this case it should call serializeSequence instead of serializing the size and calling serializeArray.

dirk-thomas · 2016-06-30T01:41:33Z

rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h

+
+        auto & string_array_field = *reinterpret_cast<rosidl_generator_c__String__Array *>(field);
+        if(!rosidl_generator_c__String__Array__init(&string_array_field, cpp_string_vector.size())) {
+            printf("unable to initialize rosidl_generator_c__String array\n");


All error messages should go to stderr. But since the exception is thrown anyway I would say the error message is redundant and can be removed.

wjwwood · 2016-06-30T21:26:57Z

Thanks for the patch @esteve! I agree that the printf statements, while useful when debugging, are probably better left out.

With that comment addressed the changes lgtm. I'll just start some CI (assuming that removing the printf's won't impact the result):

Linux:
- /
OS X:
- /

wjwwood · 2016-06-30T23:02:39Z

rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h

@@ -514,6 +514,12 @@ void deserialize_array(
    (void)call_new;
    if (member->array_size_ && !member->is_upper_bound_) {
        deser.deserializeArray((T*)field, member->array_size_);
+    } else {
+        auto & data = *reinterpret_cast<typename GenericCArray<T>::type *>(field);
+        int32_t dsize;


Looks like cppcheck is concerned about this not be initialized:

http://ci.ros2.org/job/ci_linux/1606/testReport/(root)/projectroot/cppcheck/

You could either give it a default value or use the no lint comment to squelch cppcheck.

esteve · 2016-07-06T09:15:19Z

@wjwwood the printf statements next to throwing exceptions seem to be a recurring pattern in eProsima's code:

https://github.com/eProsima/ROS-RMW-Fast-RTPS-cpp/blob/master/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L402

https://github.com/eProsima/ROS-RMW-Fast-RTPS-cpp/blob/master/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L481

https://github.com/eProsima/ROS-RMW-Fast-RTPS-cpp/blob/master/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L239

so I'm going to leave the printf statements in this PR, just for consistency, though I agree they all should be removed since they don't add much value.

esteve · 2016-09-01T07:47:31Z

@richiprosima I've rebased this and removed the printf calls, this should be mergeable now.

dirk-thomas · 2016-09-01T15:35:18Z

Can you please trigger the CI builds for these changes.

wjwwood · 2016-09-01T19:50:51Z

CI:

Linux:
OS X:

dirk-thomas · 2016-09-01T20:37:36Z

Windows:

firesurfer · 2016-09-06T13:50:37Z

I tested this PR with my .Net wrapper and it seems to work fine, apart from passing c-typesupport messages to cpp.

What works:

Sending and recieving messages in program(s) that use c-typesupport
Recieving messages sent from a cpp program in a program that uses the same messages but with c-typesupport.

What doesn't work:

Sending messages from a program that uses messages with c-typesupport and recieving them in a cpp program.

esteve · 2016-09-09T11:48:24Z

@firesurfer thanks for testing this PR. That's weird, once the messages go through the wire, the typesupport should matter. Do you have a concrete example which I can test? I recall I tested this between rclpy (c-typesupport) and rclpp (c++-typesupport), and it seemed to work, but I'll check again.

firesurfer · 2016-09-09T13:36:37Z

@esteve I tested it only with my C# wrapper so far. In my testing workspace ( https://github.com/firesurfer/rclcs_testing_ws ) I got one program that uses in C# - test_cs.exe (c-typesupport) and one program that uses c++ - test_cpp (c++-typesupport). Sending a message from the C# program to another program that uses the C# wrapper works. But sending a message from the C# wrapper to the C++ program doesn't work.

On the other side a program that uses the C# wrapper receives the message published from a C++ program. I haven't had the time yet to investigate this issue further but I think I can find some time to have a deeper look at it tomorrow.

esteve · 2016-09-09T13:50:38Z

@firesurfer do you have the message definition you used? I can test it out and see if I can find the problem.

firesurfer · 2016-09-09T13:52:21Z

@esteve this is my dummy message: https://github.com/firesurfer/rclcs_testing_ws/blob/master/src/test_msgs/msg/Dummy.msg

esteve · 2016-09-09T13:54:32Z

@firesurfer thanks!

mikaelarguedas · 2016-09-14T22:31:33Z

CI with fastrtps whitelisted in test_communication
Master:

This PR:

mikaelarguedas · 2016-09-16T01:47:24Z

TD;DR this PR does fix the serialisation/deserialisation of dynamic primitive array messages (DynamicArrayPrimitives and BoundedArrayPrimitives) but doesn't fix the other arrays test cases, it can be merged as is without resulting in any regression.

@esteve do you prefer:

to merge this as is and we'll create a follow-up PR to address the remaining array test failures
allow us to push commits on your branch and merge this PR once it fixes all the array serialization problems ?

Here is the results of my investigation for the remaining test failures:

All non array test failures (Nested/Primitives) are due to rclpy.spin_once() behaving differently with fastrtps and other rmw implementations, need more investigation to figure out why, quick fix: relax test timeout
StaticArrayPrimitives fails because fixed size string arrays are not serialized/deserialized properly (the function serializeArray is only applied the first rosidl_generator_c__String element and the rest of the array is dropped (c.f. https://github.com/esteve/ROS-RMW-Fast-RTPS-cpp/blob/a4d829308e18d3c3d24b58f903f803bd8f1ada18/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L267)). It uses the size of the String array instead of the size of the char array so the communication works only if you have an array longer than the individual strings in that array
StaticArrayNested IMO same string (de)serialization problem as StaticArrayPrimitives, works well if we remove strings from the equation
DynamicArrayNested feature not implemented (c.f. this todo)
BoundedArrayNested I assume it is the same as DynamicArrayNested but we may uncover more modifications to be done once the previous test case is fixed

esteve · 2016-09-16T09:25:55Z

Thank you so much for looking into this @mikaelarguedas!

I think it'd be fine to just merge this PR and continue any remaining bits
in a follow-up PR, so that @firesurfer and I can keep working on our stuff.
What's in this PR, while not complete, is still some progress.

On Sep 16, 2016 03:47, "Mikael Arguedas" [email protected] wrote:

TD;DR this PR does fix the serialisation/deserialisation of dynamic
primitive array messages (DynamicArrayPrimitives and
BoundedArrayPrimitives) but doesn't fix the other arrays test cases, it
can be merged as is without resulting in any regression.

@esteve https://github.com/esteve do you prefer:

to merge this as is and we'll create a follow-up PR to address the
remaining array test failures

allow us to push commits on your branch
https://github.com/blog/2247-improving-collaboration-with-forks and
merge this PR once it fixes all the array serialization problems ?

Here is the results of my investigation for the remaining test failures:

All non array test failures (Nested/Primitives) are due to
rclpy.spin_once() behaving differently with fastrtps and other rmw
implementations, need more investigation to figure out why, quick fix:
relax test timeout

StaticArrayPrimitives fails because fixed size string arrays are not
serialized/deserialized properly (the function serializeArray is only
applied the first rosidl_generator_c__String element and the rest of
the array is dropped (c.f. https://github.com/esteve/ROS-
RMW-Fast-RTPS-cpp/blob/a4d829308e18d3c3d24b58f903f803
bd8f1ada18/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/
TypeSupport_impl.h#L267
https://github.com/esteve/ROS-RMW-Fast-RTPS-cpp/blob/a4d829308e18d3c3d24b58f903f803bd8f1ada18/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L267)).
It uses the size of the String array instead of the size of the char array
so the communication works only if you have an array longer than the
individual strings in that array

StaticArrayNested IMO same string (de)serialization problem as
StaticArrayPrimitives, works well if we remove strings from the
equation

DynamicArrayNested feature not implemented (c.f. this todo
https://github.com/eProsima/ROS-RMW-Fast-RTPS-cpp/blob/master/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L305
)

BoundedArrayNested I assume it is the same as DynamicArrayNested but
we may uncover more modifications to be done once the previous test case is
fixed

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#45 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACDVIOcRlEGX1RVF878mh2RClBpLFYoks5qqfUtgaJpZM4JA9Vl
.

dirk-thomas · 2016-09-16T16:46:46Z

After talking to @mikaelarguedas we will merge this as-is and go from there. Thanks.

mikaelarguedas · 2016-09-29T01:02:38Z

@esteve @firesurfer #58 may solve some of the issues you encountered. The tests are passing in both rclcpp -> rclpy and rclpy -> rclcpp for all message types

esteve added the in review Waiting for review (Kanban column) label Jun 29, 2016

dirk-thomas reviewed Jun 29, 2016
View reviewed changes

dirk-thomas mentioned this pull request Jun 29, 2016

add a bunch of tests for rcl and rosidl_generator_c messages ros2/system_tests#122

Merged

dirk-thomas reviewed Jun 30, 2016
View reviewed changes

wjwwood reviewed Jun 30, 2016
View reviewed changes

esteve mentioned this pull request Aug 31, 2016

rcl/rosidl arrays - need some help understanding ros2/rcl#77

Closed

esteve added 2 commits September 16, 2016 11:20

Serialize and deserialize arrays

67d8301

Removed printf calls

4bb5ec8

dirk-thomas merged commit 71aed1a into ros2:master Sep 16, 2016

dirk-thomas removed the in review Waiting for review (Kanban column) label Sep 16, 2016

esteve deleted the array-serialization branch September 16, 2016 17:19

mikaelarguedas mentioned this pull request Sep 21, 2016

serialize deserialize arrays #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize and deserialize arrays #45

Serialize and deserialize arrays #45

esteve commented Jun 29, 2016

dirk-thomas Jun 29, 2016

esteve Jun 29, 2016

dirk-thomas Jun 30, 2016

wjwwood commented Jun 30, 2016

wjwwood Jun 30, 2016

esteve Jul 6, 2016

esteve commented Jul 6, 2016

esteve commented Sep 1, 2016

dirk-thomas commented Sep 1, 2016

wjwwood commented Sep 1, 2016

dirk-thomas commented Sep 1, 2016

firesurfer commented Sep 6, 2016

esteve commented Sep 9, 2016

firesurfer commented Sep 9, 2016

esteve commented Sep 9, 2016

firesurfer commented Sep 9, 2016

esteve commented Sep 9, 2016

mikaelarguedas commented Sep 14, 2016

mikaelarguedas commented Sep 16, 2016

esteve commented Sep 16, 2016

dirk-thomas commented Sep 16, 2016

mikaelarguedas commented Sep 29, 2016

Serialize and deserialize arrays #45

Serialize and deserialize arrays #45

Conversation

esteve commented Jun 29, 2016

dirk-thomas Jun 29, 2016

Choose a reason for hiding this comment

esteve Jun 29, 2016

Choose a reason for hiding this comment

dirk-thomas Jun 30, 2016

Choose a reason for hiding this comment

wjwwood commented Jun 30, 2016

wjwwood Jun 30, 2016

Choose a reason for hiding this comment

esteve Jul 6, 2016

Choose a reason for hiding this comment

esteve commented Jul 6, 2016

esteve commented Sep 1, 2016

dirk-thomas commented Sep 1, 2016

wjwwood commented Sep 1, 2016

dirk-thomas commented Sep 1, 2016

firesurfer commented Sep 6, 2016

esteve commented Sep 9, 2016

firesurfer commented Sep 9, 2016

esteve commented Sep 9, 2016

firesurfer commented Sep 9, 2016

esteve commented Sep 9, 2016

mikaelarguedas commented Sep 14, 2016

mikaelarguedas commented Sep 16, 2016

esteve commented Sep 16, 2016

dirk-thomas commented Sep 16, 2016

mikaelarguedas commented Sep 29, 2016