Skip to content

Service timestamps#217

Merged
wjwwood merged 3 commits intoros2:masterfrom
boschresearch:feature/services_timestamps
Apr 24, 2020
Merged

Service timestamps#217
wjwwood merged 3 commits intoros2:masterfrom
boschresearch:feature/services_timestamps

Conversation

@iluetkeb
Copy link
Copy Markdown
Contributor

Proposal to add timestamps on service request and response (for ros2/design#259)

This requires a few related PR's (see below for mentions).

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.

This looks reasonable, but it will definitely require changes in rclcpp and rclpy.

@iluetkeb
Copy link
Copy Markdown
Contributor Author

iluetkeb commented Apr 22, 2020

This looks reasonable, but it will definitely require changes in rclcpp and rclpy.

What kind of changes? There are compatibility methods in rcl, so you can still call it the old way.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 22, 2020

There is no "nice" way to get this information in C++, we'd need new functions and callback types for rclcpp Clients/Services. It is completely inaccessible in Python atm, AFAICT.

@wjwwood wjwwood self-assigned this Apr 22, 2020
@wjwwood wjwwood added the enhancement New feature or request label Apr 22, 2020
@iluetkeb
Copy link
Copy Markdown
Contributor Author

There is no "nice" way to get this information in C++, we'd need new functions and callback types for rclcpp Clients/Services.

The "user", as in a person writing a ROS node, shouldn't need it. AFAICT, the user cannot currently get the message_info_t either, right? At least I could not find a way to create a subscription callback receiving it.

For the executor, the rcl-functions should suffice, shouldn't they?

I mean, I'm happy to add a better way to access this, but currently I don't understand the use-case.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 23, 2020

Without python code to convert the new struct into a pyobj the executor, which is in python, cannot use it either. Either way it needs to be in python so user could use it if they wanted.

@iluetkeb
Copy link
Copy Markdown
Contributor Author

Got it. That’s in the works already, I’ll push after dinner.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 23, 2020

Sorry you were speaking about c++, still the user should have the option of looking at the time stamps in think. It would require new callback signatures to expose that to them. Also for the case of using it without an executor the user needs all this information to make decisions about when to execute it.

…for ros2/design#259

Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
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 24, 2020

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 24, 2020

New CI after linter fixes:

@wjwwood wjwwood merged commit f618306 into ros2:master Apr 24, 2020
wjwwood pushed a commit to ros2/rmw_fastrtps that referenced this pull request Apr 24, 2020
* remember the sample info on requests and responses

Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>

* This implements services timestamps

Requires ros2/rmw#217
Realizes the 2nd part of ros2/design#259

Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>

* use rmw_time_point_value_t instead of rmw_time_t

Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>

* snake_case

Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb iluetkeb deleted the feature/services_timestamps branch April 24, 2020 10:24
@ros-discourse
Copy link
Copy Markdown

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros2-middleware-change-proposal/15863/7

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants