ros2 / rmw_cyclonedds

ROS 2 RMW layer for Eclipse Cyclone DDS
Apache License 2.0
108 stars 89 forks source link

ros_data may changed when data invalid #434

Closed sgf201 closed 3 months ago

sgf201 commented 1 year ago

image

dds_take may change ros_data, when info.valid_data false Is that acceptable ros_data changed but *taken = false ??

eboasson commented 1 year ago

You have little choice if you want to map rmw_take_... on DDS' "take" operation, although that is probably a little wriggle room for implementation-defined behaviour there.

What is clear there is such a thing as "invalid data" in the result of a DDS read/take operation, which is used for indicating state changes that are not associated with data written by the application (see "2.2.2.5.1.4 Interpretation of the SampleInfo valid_data" in the spec.

The wriggle room is that the spec simply says:

To ensure corerctness and portability, the valid_data flag must be examined by the application prior to accessing the Data associated with the DataSample and if the flag is set to FALSE, the application should not access the Data associated with the DataSample, that is, the application should access only the SampleInfo.

but that doesn't specify what happens to that data. It merely says the contents are undefined.

In Cyclone we chose to make it guarantee that it is a (roughly) default-constructed sample with the key values correctly filled in, rather than leaving it untouched. Those key values have to be correctly filled in, because otherwise a program that simply takes everything as soon as it comes in (not an unreasonable bit of application code) can find itself unable to determine the key value after having taken an "invalid_data" sample. I'll spare you the details now, feel free to ask if you do want to know 🙂

And if you fill in the key values, then you need to decide what to do with the other fields ... Perhaps in the C++ API you could get away with leaving them untouched (because they'd be default-constructed to begin with), but that doesn't work in C. And since key values may require allocation (strings are not uncommon key fields), you need to be able to free it, and then with the existence of general free functions for application data, you have little choice but to initialise it.

I suspect there may be implementations that leave the data unchanged when "valid_data" is false. I think that's a mistake because of the complications that come from not being able to recover the key values, but it is within the DDS implementor's freedom.

So if you use Cyclone or the implementation you are using isn't explicitly stating that it doesn't modify the input when *taken is false, you have to assume it has changed. So then either you have to make an additional copy or you have to live with it possibly having been changed.