Skip to content

Fill in message_info timestamps#163

Merged
wjwwood merged 3 commits intoros2:masterfrom
boschresearch:feature/message_info_timestamp
Apr 23, 2020
Merged

Fill in message_info timestamps#163
wjwwood merged 3 commits intoros2:masterfrom
boschresearch:feature/message_info_timestamp

Conversation

@iluetkeb
Copy link
Copy Markdown
Contributor

This fills the timestamps in the message_info.

Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
Comment thread rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated
@iluetkeb iluetkeb closed this Apr 23, 2020
@iluetkeb iluetkeb reopened this Apr 23, 2020
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 23, 2020

I'm curious why you changed this, to not have the received timestamp? Does it matter so long as it is consistent within a single implementation?

I don't really mind either way, but it seemed like the received timestamp was more useful than wrong.

@iluetkeb
Copy link
Copy Markdown
Contributor Author

iluetkeb commented Apr 23, 2020

I'm curious why you changed this, to not have the received timestamp?

The timestamps are intended to see which event came first. However, when you just take system time at the moment of calling rcl_take_request, the timestamps will merely reflect the call order and so carry no value (you could have gotten the same result by calling it yourself just prior to the call).

In contrast, Fast-RTPS since 1.10 takes a real received timestamp when it receives the data and stores it in the SampleInfo, from where the rmw_fastrtps implementation of take_request copies it.

For this reason, I think it is important to communicate to the user whether the middleware really supports a genuine received timestamp or not. 0 is used to signify that it doesn't.

@wjwwood wjwwood merged commit 27bc819 into ros2:master Apr 23, 2020
@eboasson
Copy link
Copy Markdown
Collaborator

Thanks @iluetkeb for helping with implementing this also for cyclone dds, it really makes a difference to me. I just wanted to add a suggestion on how to get a receive time stamp without having to rely on Cyclone to provide it.

The RMW layer relies on a custom sample implementation to perform direct ROS2-to-CDR conversion and to get access to the serialized form for rmw_take_serialized_message and any received message will be turned into a sample instance as soon as it is ready to be inserted in the reader history caches. If that’s what you mean by “the receive time stamp” (as I pointed out in ros2/design#259 (comment) there are very many versions), then one way to do it would be to store that time stamp in this custom sample: just add it to

class serdata_rmw : public ddsi_serdata
{
protected:
size_t m_size {0};
/* first two bytes of data is CDR encoding
second two bytes are encoding options */
std::unique_ptr<byte[]> m_data {nullptr};
public:
serdata_rmw(const ddsi_sertopic * topic, ddsi_serdata_kind kind);
void resize(size_t requested_size);
size_t size() const {return m_size;}
void * data() const {return m_data.get();}
};

It won’t be returned in the sample info because cyclone currently only implements the fields defined in the standard, but that can be worked around by using dds_takecdr instead of dds_take, like when taking a serialized message:

while (dds_takecdr(sub->enth, &dcmn, 1, &info, DDS_ANY_STATE) == 1) {

That gives you a reference to the sample that you can then deserialize by calling ddsi_serdata_to_sample on it. That way you get everything.

(I don’t know what the rules are for loaned samples, but with this sample implementation, and for those topics where the CDR representation matches the in-memory representation and where there are no endianness issues, the same trick would be feasible.)

@iluetkeb iluetkeb deleted the feature/message_info_timestamp branch April 24, 2020 16:01
@iluetkeb
Copy link
Copy Markdown
Contributor Author

Thanks @eboasson, I've now put this into an issue for future reference: #167

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants