Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rcl/rosidl arrays - need some help understanding #77

Closed
firesurfer opened this issue Aug 25, 2016 · 13 comments
Closed

rcl/rosidl arrays - need some help understanding #77

firesurfer opened this issue Aug 25, 2016 · 13 comments
Labels
question Further information is requested

Comments

@firesurfer
Copy link
Contributor

I'm currently trying to implement arrays in my .net wrapper. At the moment I'm running into some problems. Let's take an int8 array for example:

On C side the struct for the array contains three entries:

typedef struct rosidl_generator_c__int8__Array
{
    int8* data;
    size_t size;
    size_t capacity;
}rosidl_generator_c__int8__Array;

As far as I understand the initialization code for the arrays I have to insert a pointer into the data field, and insert the amount of array elements into the size field. The capacity field is the same value as the size field. Is this correct?

@esteve Are there any other traps you could imagine when passing arrays from .net to native code that invoke this error?

terminate called after throwing an instance of 'eprosima::fastcdr::exception::NotEnoughMemoryException'
what(): Not enough memory in the buffer stream

At the moment I',m doing the marshalling by hand: For example for the int8 array:

[StructLayout(LayoutKind.Sequential)]
    public struct rosidl_generator_c__primitive_array_int8
    {   
        public IntPtr Data ;
        UIntPtr Size;
        UIntPtr Capacity;


        public rosidl_generator_c__primitive_array_int8(byte[] _Data)
        {

            Data = Marshal.AllocHGlobal (_Data.Length);//Marshal.SizeOf<byte> ()*_Data.Length);
            Size = (UIntPtr) _Data.Length;
            Capacity = Size;
            Marshal.Copy (_Data, 0, Data, _Data.Length);
        }
        public byte[] Array
        {
            get{ byte[] tempArray = new byte[(int)Size]; 
                Marshal.Copy (Data, tempArray, 0, (int)Size);
                return tempArray;}
        }
        public int ArraySize
        {
            get{return (int)Size;}
        }
        public int ArrayCapacity
        {
            get{ return (int)Capacity;}
        }
    }
@dirk-thomas
Copy link
Member

It looks like the primitive array struct lacks the necessary documentation. Please take a look at the similar struct for strings (https://github.com/ros2/rosidl/blob/8f507b9a656eea1a7ed196b34d7fa5bd1311052e/rosidl_generator_c/include/rosidl_generator_c/string.h#L23-L30). The capacity contains the actually allocated size of data. Size contains the number of valid entries in the array.

In the case where all your elements contain valid data both numbers are equal. But in the case of a dynamically filled array capacity might be bigger then size.

I should use the provided functions to initialize / finalize any array: https://github.com/ros2/rosidl/blob/8f507b9a656eea1a7ed196b34d7fa5bd1311052e/rosidl_generator_c/include/rosidl_generator_c/primitives_array_functions.h#L30-L36

@firesurfer
Copy link
Contributor Author

Perhaps I got another bug. Perhaps I'm using the function the wrong way.

I got the following struct:

typedef struct test_msgs__msg__Dummy
{
  bool thisisabool;
  int8_t thisisaint8;
  uint8_t thisiauint8;
  int16_t thisisaint16;
  uint16_t thisisauint16;
  int32_t thisisaint32;
  uint32_t thisisauint32;
  int64_t thisisaint64;
  uint64_t thisisauint64;
  float thisafloat32;
  double thisisfloat64;
  rosidl_generator_c__String thisisastring;
  rosidl_generator_c__String thisisanotherstring;
  rosidl_generator_c__int8__Array thisisaint8array;
} test_msgs__msg__Dummy;

As described above I'm using .net P/Invoke on the rcl_publish function to publish this struct from C#.
I got a subscription in a c++ client running that simply prints out all values of the message. All the primitive members and the strings are working fine, but the array is making troubles.

The c++ client dies with the above mentioned exception:

terminate called after throwing an instance of 'eprosima::fastcdr::exception::NotEnoughMemoryException'
  what():  Not enough memory in the buffer stream

I ensured that the rosidl_generator_c__int8__Array is correctly passed into the native code, by adding some debug code to the rcl_publish function.

rcl_publish(const rcl_publisher_t * publisher, const void * ros_message)
{
  test_msgs__msg__Dummy * dummyStruct = (test_msgs__msg__Dummy*)ros_message;
  puts("Some native debug output:");
  printf("Pointer: %u  \n",dummyStruct->thisisaint8array.data);
  printf("Array size: %u \n", dummyStruct->thisisaint8array.size);
  printf("Array capacity: %u \n", dummyStruct->thisisaint8array.capacity);

The memory address and the size/capacity values are the same as on the C# side.
For example: I got an int8 array with one element: In this case size and capacity should both be one?
I' sure that I'm passing the correct data to the rcl_publish function.

@wjwwood
Copy link
Member

wjwwood commented Aug 26, 2016

How big are you making the array? It may be that the current rmw implementation for FastRTPS doesn't support messages that are that large. We're actively working on that here:

ros2/rmw_fastrtps#51

You can try to see if that is the issue by making the arrays smaller or by trying Connext or OpenSplice with your example.

@firesurfer
Copy link
Contributor Author

Sorry for the late answer. I had some trouble with the email notifications.

I tested my wrapper with an int8_t array with 1,2 and 3 elements. This should'nt be to large. Publishing the same message from C++ over FastRTPS works.

@dirk-thomas
Copy link
Member

Without more information it is hard to suggest anything specific. Maybe try to break in the beginning of the serialization code to check what exact data it is trying to serialize (https://github.com/eProsima/ROS-RMW-Fast-RTPS-cpp/blob/e336a8b0f93c8179cc2b4a62d02272403ed356bb/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L309-L472). That might help in narrowing down where the problem is.

@firesurfer
Copy link
Contributor Author

I think I found the problem. I think it's located in the message deserialisation:
I put a debug output to: https://github.com/eProsima/ROS-RMW-Fast-RTPS-cpp/blob/e336a8b0f93c8179cc2b4a62d02272403ed356bb/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.h#L497
which prints out printf("Size: %u \n", member->array_size_);.In my case this array_size is 0. This shouldn't be a problem because I'm using a dynamic allocated array.
But apparently there's no case which handles arrays with a dynamic size like in deserialization function for c++.

@dirk-thomas
Copy link
Member

@richiprosima It looks like the mentioned case is missing for the C type support. Can you please suggest a fix for this or create a PR.

@esteve
Copy link
Member

esteve commented Aug 31, 2016

This should already be addressed in #45

On Aug 31, 2016 18:15, "Dirk Thomas" notifications@github.com wrote:

@richiprosima https://github.com/richiprosima It looks like the
mentioned case is missing for the C type support. Can you please suggest a
fix for this or create a PR.


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

@esteve
Copy link
Member

esteve commented Aug 31, 2016

Sorry, I meant ros2/rmw_fastrtps#45

On Aug 31, 2016 18:32, "Esteve Fernandez" esteve@apache.org wrote:

This should already be addressed in #45

On Aug 31, 2016 18:15, "Dirk Thomas" notifications@github.com wrote:

@richiprosima https://github.com/richiprosima It looks like the
mentioned case is missing for the C type support. Can you please suggest a
fix for this or create a PR.


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

@dirk-thomas
Copy link
Member

@esteve It looks like the referenced PR needs to be rebased and has one pending comment. Once it is mergable again I can trigger new CI builds for it.

@esteve
Copy link
Member

esteve commented Aug 31, 2016

I responded to your comment regarding printf in
ros2/rmw_fastrtps#45 (comment)

Though I agree with you about removing the printf calls, I also want to
be consistent with what seems to be eProsima's codestyle guidelines.
@richiprosima, are you ok with removing the printf calls when catching an
exception? Do you have codestyle guidelines about this or the printf
calls are just a personal preference?

On Aug 31, 2016 18:48, "Dirk Thomas" notifications@github.com wrote:

@esteve https://github.com/esteve It looks like the referenced PR needs
to be rebased and has one pending comment. Once it is mergable again I can
trigger new CI builds for it.


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

@richiprosima
Copy link

Go on removing printf. It is not about codestyle. Surely I forgot to remove it.

@firesurfer
Copy link
Contributor Author

I think this can be closed because it was addressed in: ros2/rmw_fastrtps#45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants