search

eclipse-cyclonedds / cyclonedds

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

About dds_takecdr() (and it's missing twin brother dds_readcdr()) #356

Open TheFixer opened 4 years ago

TheFixer commented 4 years ago

Hi,

I noticed a few issues regarding the implementation of dds_takecdr().

  1. There is no documentation about dds_takecdr(), and its twin brother dds_readcdr() is missing
  2. The signature of dds_takecdr() seems to deviate from the interface of other take-implementation as far as the position of the dds_sample_info_t parameter is concerned. In most other take() implementations it is after the maxs parameter, in dds_takecdr() it is before the maxs parameter. To me this is unnecessarily confusing.

I am happy to address issue 1 and provide a PR for it if you think this makes sense. I doubt whether you think it is worthwhile to address 2 since this would involve an API change.

Can you let me know if it makes sense to provide a PR for issue 1? Can you also let me know what your position is regarding issue 2?

eboasson commented 4 years ago

I'd be happy to accept a PR that addresses 1, and in principle I would be happy with one addressing 2 as well. I doubt there are many users of dds_takecdr but there is at least one (https://github.com/ros2/rmw_cyclonedds).

I have another suggestion for dealing with point 2. I remember takecdr got introduced as a quick hack to test performance for invoking operations via a unix stream socket and it sort of managed to escape that old experiment and made its way into the API. Back then, Cyclone really only dealt with CDR serialised data, but today the only remnant of that is the message validation code that requires the "encoding" field to be set to CDR.

So, it would arguably make sense to switch to a new name, say read_ref and take_ref, because what they really do is hand out references to the internal abstracted samples. (This is something different from the "loaned samples" in the spec, because those are in application representation.) In those new functions, the parameters can have the correct order and dds_takecdr can then become a deprecated wrapper.

Does that make sense?

TheFixer commented 4 years ago

@eboasson Yes, that makes sense. I'll try to address these issues one by one in different PRs.