eclipse-cyclonedds / cyclonedds

Eclipse Cyclone DDS project
https://projects.eclipse.org/projects/iot.cyclonedds
Other
849 stars 351 forks source link

Dynamic message type in c/c++ to facilitate publishing raw video frames whilst taking advantage of zero copy. #1437

Closed mangodan2003 closed 1 year ago

mangodan2003 commented 1 year ago

Hi, I am new to DDS, so please excuse me in that I do not yet know all the correct terminology. I am aware that within the python implementation there are examples of howto dynamically create a message type without the need for the Idl file. I am told that to take advantage of zero copy the size of the payload needs to be known in advance, which seemingly is not possible within the current c/c++ Idl implementation, unless one just reserves a huge array that can cater for the largest likely frame size. Can anybody shed any light on what would be involved in dynamically constructing a very basic msg structure that contains just a single array of bytes where the size can be specified at runtime by the publishing service?

eboasson commented 1 year ago

Long live open source! 😁

It can't be done officially, but it can be made to work if you're willing to do some dirty tricks. In C, you moreover need to be willing to work around a current limitation of the C API. Everything in this comment is completely untested, but I'm pretty sure it'll behave exactly as I think in C. With C++ I am always a little more worried 🤣

C++

I think you could turn the IDL compiler output in a template where you pass in the size of the array. For example:

@topic @final struct a { long x; octet data[1234567]; };

yields a very simple .hpp and .cpp file, where only the former has some straightforward dependencies on the size. So changing all occurrences of 1234567 into a template parameter should work.

The bit I'd be worried about is the type metadata, but if you're willing to use a build of Cyclone without type discovery (and so also without type assignability checking), then that problem simply disappears.

C

That "current limitation" is that the C API doesn't yet properly support taking/reading with "loans" where you get a pointer to shared memory instead of a copy of it. It doesn't mean you can't do zero-copy in C (the C++ binding & the ROS 2 RMW implementation do it, and do so on top of the C API), but it does mean one needs more tricks then I'd want to impose on anyone. The good news is that this problem is about to be lifted, thanks to @reicheratwork who did some pretty fundamental work to clean up a variety of things, including the shared memory integration and this particular detail in the API. (It isn't yet a PR, but I can already tell you that it is solid except for some polishing, some testing and a few backwards compatibility considerations.)

But also, once more: zero-copy in C is possible today. You just have to use the takecdr function and look at the Iceoryx chunk that it references. That'll all change a bit though. As I said, you need to be willing to do some dirty tricks to make it happen with the current code base.

After you've accepted those tricks, the next step is to patch the IDL compiler output. If the type is as simple as you're describing, you could do that by creating a prototype IDL file and patching the output at run-time. If you take the same example:

@topic @final struct a { long x; octet data[1234567]; };

yields in the .h file:

typedef struct a {
  int32_t x;
  uint8_t data[1234567];
} a;

which you could rewrite to

typedef struct a {
  int32_t x;
  uint8_t data[];
} a;

and then you have a proper C type with a flexible array member at the end. The .c file contains the type description used by the serialiser, and it is just a list of opcodes, some flags and a few sizes and related things. We've managed to keep this backwards compatible going back to the first commit and I have no intention of breaking that compatibility so it'll probably keep working for another couple of years (even if I'll never claim it to be a stable interface 😁):

static const uint32_t a_ops [] =
{
  /* a */
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_SGN, offsetof (a, x),
  DDS_OP_ADR | DDS_OP_TYPE_ARR | DDS_OP_SUBTYPE_1BY, offsetof (a, data), 1234567u,
  DDS_OP_RTS
};
const dds_topic_descriptor_t a_desc =
{
  .m_size = sizeof (a),
  .m_align = dds_alignof (a),
  .m_flagset = DDS_TOPIC_FIXED_SIZE | DDS_TOPIC_XTYPES_METADATA,
  .m_nkeys = 0u,
  .m_typename = "a",
  .m_keys = NULL,
  .m_nops = 3,
  .m_ops = a_ops,
  .m_meta = "",
  .type_information = { .data = TYPE_INFO_CDR_a, .sz = TYPE_INFO_CDR_SZ_a },
  .type_mapping = { .data = TYPE_MAP_CDR_a, .sz = TYPE_MAP_CDR_SZ_a }
};

That a_desc is what you pass to dds_create_topic and just by looking at it, it is pretty obvious which fields need changing: m_size, and the 1234567u in a_ops.

The other thing is the XTypes metadata, there are two ways of dealing with that: Cyclone doesn't require that — thanks to that decision to retain backwards compatibility! — and clearing the DDS_TOPIC_XTYPES_METADATA flag suffices to take care of that problem.

mangodan2003 commented 1 year ago

Brilliant, thanks for all the info :)

sjames commented 1 year ago

@eboasson I'm eagerly awaiting the work from @reicheratwork to become public. Ive managed to get the Rust binding (https://github.com/sjames/cyclonedds-rs) working with shared memory, but there are corner cases that break. I will need to revamp the Rust binding once these changes are public. Any plans to make it available soon?

eboasson commented 1 year ago

Any plans to make it available soon?

It is only dependent on my giving it focussed instead of scattered attention ... This is the sort of change that really needs to be merged sooner rather than later, if only for reasons of overhead in keeping it up-to-date. Therefore, the short answer is simply: yes 🙂