Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timestamps to rmw-internal structures. #200

Closed
wants to merge 12 commits into from
Closed

Add timestamps to rmw-internal structures. #200

wants to merge 12 commits into from

Conversation

iluetkeb
Copy link
Contributor

This goes together with ros2/rcl#591

@ralph-lange
Copy link

Connects to ros2/design#259.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should (while we're at it) re-enable this field in the message info, as the main reason it was disabled was because we didn't know how to get the arrival timestamp in each case, which we will have to figure out for this:

// const rmw_time_t received_timestamp;

Also, feel free to suggest a different name if you think arrival_timestamp sounds better, or something similar.

Also, we should consider any changes to tests and documentation. One specific place I know is that we should mention this change in the release/migration notes for foxy:

https://github.com/ros2/ros2_documentation/blob/master/source/Releases/Release-Foxy-Fitzroy.rst#changes-since-the-eloquent-release

rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
@iluetkeb iluetkeb mentioned this pull request Mar 13, 2020
3 tasks
@iluetkeb
Copy link
Contributor Author

Also, we should consider any changes to tests and documentation.

In principle, I would test that a) allocation and deallocation works and b) that the fields are filled properly.

However, it seems that a) happens in rcl and b) happens in the rmw implementation. So it doesn't appear as if I can test anything in rmw.

Suggestions to the contrary very welcome, I want this to be solid :-)

rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
@iluetkeb iluetkeb marked this pull request as ready for review March 20, 2020 20:33
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating.

iluetkeb and others added 12 commits April 8, 2020 19:48
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Co-Authored-By: William Woodall <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Co-Authored-By: William Woodall <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Co-Authored-By: William Woodall <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Co-Authored-By: William Woodall <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Co-Authored-By: William Woodall <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
Co-Authored-By: William Woodall <[email protected]>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <[email protected]>
@iluetkeb iluetkeb marked this pull request as draft April 14, 2020 15:18
@iluetkeb
Copy link
Contributor Author

Closed in favor of #214

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