Add support for taking a sequence of messages #148
Conversation
| memcpy( | ||
| message_info->publisher_gid.data, &info.publication_handle, | ||
| sizeof(info.publication_handle)); | ||
| } |
There was a problem hiding this comment.
What's the else case here? Why would that be null, and if it is, is that ok?
There was a problem hiding this comment.
message_info can't be null, I dare say. What I am more concerned about is what happens when !valid_data: that's a case the ROS2 applications don't expect. In Cyclone's case, the sample will be all-zero (except for the key fields, but there are none of those in ROS2).
You only get those in some edge cases: when the last publisher disappears, when a publisher loses its liveliness, stuff like that. I think those need to be hidden from the caller, just like rmw_take does.
There was a problem hiding this comment.
@eboasson Since we aren't currently controlling any allocation here, would it be sufficient to rearrange the sequences to keep all of the valid samples/infos at the beginning of the sequence, and indicate with the size field how many are valid?
There was a problem hiding this comment.
I went ahead and implemented it that way in ef76435
There was a problem hiding this comment.
@mjcarroll my apologies for not responding, but yes, that works as long as the upper layers don't mind the pointers getting reordered. I suppose those pointers ultimately come from the application and I don't know what the expectation of applications will be in this.
There was a problem hiding this comment.
In general I would expect the upper layers to do the allocation outside of the message_sequence structure, and only populate the pointers for the purpose of retrieving from the middleware. In that case, I can't imagine that reordering the pointers should cause substantial issues, as long as it is documented.
mjcarroll
force-pushed
the
take_sequence
branch
2 times, most recently
from
9c2c9a4 to
ef76435
Compare
jacobperron
left a comment
jacobperron
left a comment
There was a problem hiding this comment.
lgtm, but it would be good if @eboasson could do a final review.
eboasson
left a comment
eboasson
left a comment
There was a problem hiding this comment.
The logic for the message handling seems fine in principle, but I have added some comments for what I think are problems or where I am unsure about the intended behaviour.
| memcpy( | ||
| message_info->publisher_gid.data, &info.publication_handle, | ||
| sizeof(info.publication_handle)); | ||
| } |
There was a problem hiding this comment.
@mjcarroll my apologies for not responding, but yes, that works as long as the upper layers don't mind the pointers getting reordered. I suppose those pointers ultimately come from the application and I don't know what the expectation of applications will be in this.
rmw_cyclonedds_cpp/src/rmw_node.cpp
Outdated
| } | ||
|
|
||
| *taken = taken_msg.size(); | ||
| message_sequence->size = taken_msg.size(); |
There was a problem hiding this comment.
With this, I wonder if it wouldn't make more sense to simply remove taken: with the number of taken samples always stored in message_sequence->size it doesn't really add anything.
There was a problem hiding this comment.
I was trying to mirror the rmw_take API, which has a similar flag, but it would appear there is some duplication here. It was my intention that the API consumer would be explicitly checking the taken flag to determine if messages were taken before iterating over the structures.
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
mjcarroll
force-pushed
the
take_sequence
branch
from
91e04f5 to
5b86ff5
Compare
Implementation of ros2/rmw#212.
Signed-off-by: Michael Carroll [email protected]