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

Support DDS XTypes creation #445

Open
wants to merge 33 commits into
base: rolling
Choose a base branch
from

Conversation

Splinter1984
Copy link

@Splinter1984 Splinter1984 commented Mar 28, 2023

Description:

This changes allow us to register ros_types as DDSXtypes.
In example this allow to use cyclonedds cli tool to access to middleware of ros2.

Example (subscribe to ros2 topic):

~$ cyclonedds subscribe rt/chatter
   0:00:01 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Entities discovered: 1

 Subscribing, CTRL-C to quit
String_(data='Hello World: 1')
String_(data='Hello World: 2')
String_(data='Hello World: 3')

Example (display type information):

~$ cyclonedds typeof rt/chatter
   0:00:01 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Entities discovered: 1

As defined in participant(s) 0110283f-9e45-f5f7-b391-ffe7000001c1
module std_msgs {                                                                                                                                         
    module msg {                                                                                                                                          
        module dds_ {                                                                                                                                     
            @final                                                                                                                                        
            struct String_ {                                                                                                                              
                string data;                                                                                                                              
            };                                                                                                                                            
        };                                                                                                                                                
    };                                                                                                                                                    
};
Depends on:

@Splinter1984 Splinter1984 marked this pull request as draft March 28, 2023 09:58
@Splinter1984 Splinter1984 marked this pull request as ready for review March 29, 2023 14:03
Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this @Splinter1984! It took forever for me to sit down and give it a proper review, but now I have. This is a lovely addition to the RMW layer and while I do have some comments, they really are about small things. I do admit that I haven't spend enough attention on all the error paths yet. Perhaps you can have a look at the comments and then we can include that aspect in a second round?

I am also quite sure that the code layout check will fail. Almost every PR seems to end up with a final round through uncrustify to meet the code formatting rules anway, so that's really not something to worry about. 🙂

@ivanpauno if you think I am wrong in assuming that doing a release for Cyclone and updating rolling to that new release would be something that the ROS 2 community would appreciate, then please do say so!

@@ -24,7 +24,9 @@
#include "Serialization.hpp"
#include "TypeSupport2.hpp"
#include "bytewise.hpp"
#include "dds/ddsi/q_radmin.h"
#include "dds/ddsi/ddsi_radmin.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we do a new release of Cyclone (which is very much overdue anyway), then we could make these changes at the same time as switching ROS 2 rolling over to that new Cyclone version. That way we don't have to work on making the RMW build with 0.10.x.

Alternatively we could make sure dds/dds.h is included first, then look at whether (for example) DDS_DYNAMIC_TYPE_SPEC is defined, and if is not:

  • use the old include files
  • do #define ddsi_rdataa nn_rdata, &c.
  • stub out all the dynamic type

That way we'll have a single RMW layer that will work with all versions. I don't think it brings much benefit beyond decoupling merging this PR and the Cyclone release + changing ROS 2 rolling over to that new release. But simplifying those processes is definitely worth considering.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be great to leave compatibility with older versions.

@@ -685,11 +749,351 @@ static std::string get_type_name(const char * type_support_identifier, void * ty
}
}


extern "C" dds_dynamic_type_descriptor_t get_dynamic_type_descriptor_prim(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite see why this needs to be extern "C"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. extern "C" useless here.

extern "C" dds_dynamic_type_descriptor_t get_dynamic_type_descriptor_prim(
dds_dynamic_type_kind_t kind, const char *name, uint32_t num_bounds, const uint32_t *bounds, dds_dynamic_type_kind_t type)
{
return{kind,name,{},{},num_bounds,bounds,DDS_DYNAMIC_TYPE_SPEC_PRIM(type),{},};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is no better way to do this. The C example/test code uses designated initializers to set the fields that are required (and have the rest auto-initialized to 0), if that's still not supported in C++ perhaps doing:

dds_dynamic_type_descriptor t desc{};
desc.kind = kind;
...

works out better. I'm not quite sure, but writing it out as a struct initializer like this looks fragile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted your suggestions.

if (!member->is_upper_bound_)
{
ddt = dds_dynamic_type_create(dds_ppant,
get_dynamic_type_descriptor_prim(DDS_DYNAMIC_ARRAY, member->name_, 1, (uint32_t*)(&member->array_size_), type));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the actual type of array_size_ is size_t, so casting it to a pointer to uint32_t will break things quite badly on big-endian 64-bit machines.

I've checked and dds_dynamic_type_create in turn calls ddsi_dynamic_type_create_array, which copies the data. Given that it is a single-element array, changing it to:

uint32_t array_size = static_cast<uint32_t>(member->array_size_);
ddt = dds_dynamic_type_create(dds_ppant, 
        get_dynamic_type_descriptor_prim(DDS_DYNAMIC_ARRAY, member->name_, 1, &array_size_, type));

should solve that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply your suggestions.

RCUTILS_LOG_ERROR("failed to add type member sequence/array with prim type");
}

template<typename MemberType>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must be overlooking something obvious, because it looks like this is so close to the function above that it might as well not exist!

(But then surely the compiler would reject it. So I must be the one who is wrong here.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need two instances of the function. one for adding primitive types, one for more complex types.

dds_typeinfo_t *type_info;
auto rc = dds_dynamic_type_register(&dt, &type_info);
if (rc != DDS_RETCODE_OK)
RMW_SET_ERROR_MSG("dds_dynamic_type_register failed to register type");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to propagate the error out here, I think. I suspect continuing after an error will crash ... (This applies in a few more places.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and change all the same moments to assert.

if (rc != DDS_RETCODE_OK)
RMW_SET_ERROR_MSG("dds_create_topic_descriptor failed to create descriptor");

if (st)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever get here when st == nullptr? It seems to me there is not much reason to.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right this step is not needed here.

@@ -514,6 +516,9 @@ static void sertype_rmw_free(struct ddsi_sertype * tpcmn)
tp->type_support.type_support_ = NULL;
}

free((void*)tp->type_information.data);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are allocated by ddsrt_memdup, right? Then they should be freed with ddsrt_free instead of free. (It won't matter on Linux, but on Windows it gets tricky with how they deal with DLLs.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. Change to ddsrt_free.

if (member->is_upper_bound_)
{
ddt = dds_dynamic_type_create(dds_ppant,
get_dynamic_type_descriptor(DDS_DYNAMIC_STRING8, nullptr, 1, (uint32_t*)(&member->array_size_), {}));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the same thing I mentioned above about size_t vs uint32_t applies as well.

if (ret != DDS_RETCODE_OK)
RCUTILS_LOG_ERROR("failed to add dynamic type member");
} else {
dynamic_type_add_array(dstruct, dds_ppant, member, ddt);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised it checks for is_array_ but apparently doesn't need to deal with is_upper_bound_ or array_size_. Is that really true?

The ROS_TYPE_STRING case is again subtly different in this regard ...

I should check the rosidl definitions, I know, but for now, perhaps you can just double-check? ☺️

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I made a big mistake. I didn't check string_upper_bound.

@clalancette
Copy link
Contributor

@ivanpauno if you think I am wrong in assuming that doing a release for Cyclone and updating rolling to that new release would be something that the ROS 2 community would appreciate, then please do say so!

@eboasson I think a CycloneDDS update would be great. I'd say go ahead and make the CycloneDDS releases. Once that is done, we can do a PR to https://github.com/ros2/ros2 (like we did in ros2/ros2#1402) to update ROS 2, and run CI on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants