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

Service timestamps #217

Merged
merged 3 commits into from
Apr 24, 2020
Merged

Service timestamps #217

merged 3 commits into from
Apr 24, 2020

Conversation

iluetkeb
Copy link
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
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
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
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
@wjwwood wjwwood added this to In review in Foxy API Freeze Apr 22, 2020
@iluetkeb
Copy link
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
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
Contributor Author

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

@wjwwood
Copy link
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
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>
@wjwwood wjwwood moved this from In review to Done in Foxy API Freeze Apr 24, 2020
@iluetkeb iluetkeb deleted the feature/services_timestamps branch April 24, 2020 10:24
@ros-discourse
Copy link

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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants