Skip to content

Add timestamps to rmw-internal structures.#200

Closed
iluetkeb wants to merge 12 commits intoros2:masterfrom
boschresearch:feature/waitset_timestamp
Closed

Add timestamps to rmw-internal structures.#200
iluetkeb wants to merge 12 commits intoros2:masterfrom
boschresearch:feature/waitset_timestamp

Conversation

@iluetkeb
Copy link
Copy Markdown
Contributor

@iluetkeb iluetkeb commented Mar 10, 2020

This goes together with ros2/rcl#591

@iluetkeb iluetkeb mentioned this pull request Mar 10, 2020
Closed
@ralph-lange ralph-lange mentioned this pull request Mar 11, 2020
Closed
@ralph-lange
Copy link
Copy Markdown

ralph-lange commented Mar 11, 2020

Connects to ros2/design#259.

wjwwood
wjwwood reviewed Mar 12, 2020
View reviewed changes
Copy link
Copy Markdown
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:

rmw/rmw/include/rmw/types.h

Line 345 in 71b000c

// 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

@iluetkeb iluetkeb mentioned this pull request Mar 13, 2020
Closed
3 tasks
@iluetkeb
Copy link
Copy Markdown
Contributor Author

iluetkeb commented Mar 13, 2020

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 :-)

wjwwood
wjwwood reviewed Mar 13, 2020
View reviewed changes
wjwwood
wjwwood reviewed Mar 13, 2020
View reviewed changes
@iluetkeb iluetkeb marked this pull request as ready for review March 20, 2020 20:33
wjwwood
wjwwood reviewed Mar 21, 2020
View reviewed changes
wjwwood
wjwwood approved these changes Mar 23, 2020
View reviewed changes
Copy link
Copy Markdown
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 iluetkeb mentioned this pull request Apr 1, 2020
Closed
@ralph-lange ralph-lange mentioned this pull request Apr 7, 2020
Closed
iluetkeb and others added 12 commits April 8, 2020 19:48
@iluetkeb
Add timestamps to rmw-internal structures.
bd03136 
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb @wjwwood
Update rmw/include/rmw/types.h
d0bd679 
Co-Authored-By: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb @wjwwood
Update rmw/include/rmw/types.h
cd1ca7f 
Co-Authored-By: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb
Style fix
64db65d 
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb
re-enabling received timestamp
c4d9522 
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb
Remove const
ee4a60f 
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb
Add documentation comments.
af7370a 
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb
Fix comment
a4a5423 
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb @wjwwood
Update rmw/include/rmw/types.h
53e8b40 
Co-Authored-By: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb @wjwwood
typo fix
0528a98 
Co-Authored-By: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb @wjwwood
comma continuation
0d26a3f 
Co-Authored-By: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb @wjwwood
full stop
fc8e920 
Co-Authored-By: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb iluetkeb marked this pull request as draft April 14, 2020 15:18
@iluetkeb iluetkeb mentioned this pull request Apr 14, 2020
Merged
@iluetkeb iluetkeb closed this Apr 14, 2020
@iluetkeb
Copy link
Copy Markdown
Contributor Author

iluetkeb commented Apr 14, 2020

Closed in favor of #214

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

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@methylDragon methylDragon Awaiting requested review from methylDragon methylDragon will be requested when the pull request is marked ready for review methylDragon is a code owner

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno will be requested when the pull request is marked ready for review ivanpauno is a code owner

Assignees

@wjwwood wjwwood

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iluetkeb @ralph-lange @wjwwood @claireyywang