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

Added async_send_request_return_request to return the originating request #157

Merged
merged 2 commits into from
Nov 19, 2015

Conversation

esteve
Copy link
Member

@esteve esteve commented Nov 18, 2015

Connects to #35

@esteve esteve added the in progress Actively being worked on (Kanban column) label Nov 18, 2015
@esteve
Copy link
Member Author

esteve commented Nov 18, 2015

I'm not sure about the name of the method, so I'd appreciate if anyone comes with a better one :-)

@esteve
Copy link
Member Author

esteve commented Nov 18, 2015

@tfoote let me know if this is what you had in mind or if it needs to be reworked.

@tfoote
Copy link
Contributor

tfoote commented Nov 18, 2015

That's what we talked about during the review.

Is it not possible to overload the existing method which would avoid the very verbose method name?

@@ -142,10 +163,39 @@ class Client : public ClientBase
return f;
}

SharedFutureWithRequest async_send_request_return_request(SharedRequest request)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this method? A noop callback doesn't need the request data.

@esteve
Copy link
Member Author

esteve commented Nov 19, 2015

I ended up using function_traits to fix a compilation bug on Windows because of the recurring issue we found with std::function. I've also added specializations for references and universal references to function_traits so that it can be used with lambdas that are not wrapped in std::function objects.

New CI:

http://ci.ros2.org/job/ros2_batch_ci_linux/606/
http://ci.ros2.org/job/ros2_batch_ci_osx/492/
http://ci.ros2.org/job/ros2_batch_ci_windows_connext_dynamic/2/
http://ci.ros2.org/job/ros2_batch_ci_windows_connext_static/2/
http://ci.ros2.org/job/ros2_batch_ci_windows_opensplice/674/

@tfoote
Copy link
Contributor

tfoote commented Nov 19, 2015

+1

The connext_dynamic job failed on the filename length. @wjwwood is that fix still pending?

@esteve
Copy link
Member Author

esteve commented Nov 19, 2015

I pushed one last change to move the function_traits templates to a separate namespace, I guess could open a separate PR for it. In any case, I triggered new CI jobs:

http://ci.ros2.org/job/ros2_batch_ci_linux/607/
http://ci.ros2.org/job/ros2_batch_ci_osx/493/
http://ci.ros2.org/job/ros2_batch_ci_windows_connext_dynamic/3/
http://ci.ros2.org/job/ros2_batch_ci_windows_connext_static/3/
http://ci.ros2.org/job/ros2_batch_ci_windows_opensplice/675/

@esteve esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 19, 2015
@tfoote
Copy link
Contributor

tfoote commented Nov 19, 2015

+1 the new name is much clearer. There probably will be usages in other repos of the old names?

@esteve
Copy link
Member Author

esteve commented Nov 19, 2015

function_traits is only used in rclcpp itself. I updated the test in system_tests to use async_send_request instead of the longer name.

@esteve
Copy link
Member Author

esteve commented Nov 19, 2015

I also updated the code to pass uncrustify.

esteve added a commit that referenced this pull request Nov 19, 2015
Added async_send_request_return_request to return the originating request
@esteve esteve merged commit 10ce7a3 into master Nov 19, 2015
@esteve esteve deleted the return-request branch November 19, 2015 02:11
@esteve esteve removed the in review Waiting for review (Kanban column) label Nov 19, 2015
@esteve
Copy link
Member Author

esteve commented Nov 19, 2015

03697d1

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
* ros2GH-157 Adapt error messages of python code to C++ error format

* ros2GH-157 Expose read ahead queue size property

* ros2GH-157 Fixes for CTRL-C during playback

- for large read-ahead-queue-size: Error message after closing
- for small read-ahead-queue-size: CTRL-C doesn't interrupt

* ros2GH-157 Clarify that -a or topics must be given

* ros2GH-157 Get rid of --spin-time argument from record

* ros2GH-157 Improve error output when info cannot read metadata

* python touchups
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Sep 20, 2024
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