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 rcl_action_client_options when creating action client. #1133

Merged
merged 4 commits into from
Jun 16, 2020

Conversation

fujitatomoya
Copy link
Collaborator

fix #1118.

@fujitatomoya
Copy link
Collaborator Author

I guess that it needs to be rebased.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@ros-pull-request-builder retest this please

@fujitatomoya
Copy link
Collaborator Author

source build with ros2/ros2@4eece71,

i do not see the following build error reported by CI,
http://build.ros2.org/job/Fpr__rclcpp__ubuntu_focal_amd64/80/console#console-section-15

08:03:23 /tmp/ws/src/rclcpp/rclcpp/src/rclcpp/context.cpp: In function ‘void rclcpp_logging_output_handler(const rcutils_log_location_t*, int, const char*, rcutils_time_point_value_t, const char*, __va_list_tag (*)[1])’:
08:03:23 /tmp/ws/src/rclcpp/rclcpp/src/rclcpp/context.cpp:66:12: error: ‘rcl_logging_multiple_output_handler’ was not declared in this scope; did you mean ‘rcutils_logging_console_output_handler’?
08:03:23    66 |     return rcl_logging_multiple_output_handler(
08:03:23       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
08:03:23       |            rcutils_logging_console_output_handler
08:03:23 /tmp/ws/src/rclcpp/rclcpp/src/rclcpp/context.cpp:67:56: error: return-statement with a value, in function returning ‘void’ [-fpermissive]
08:03:23    67 |       location, severity, name, timestamp, format, args);
08:03:23       |                                                        ^
08:03:23 /tmp/ws/src/rclcpp/rclcpp/src/rclcpp/context.cpp: In member function ‘virtual void rclcpp::Context::init(int, const char* const*, const rclcpp::InitOptions&)’:
08:03:23 /tmp/ws/src/rclcpp/rclcpp/src/rclcpp/context.cpp:146:13: error: ‘rcl_logging_configure_with_output_handler’ was not declared in this scope; did you mean ‘rcutils_logging_console_output_handler’?
08:03:23   146 |       ret = rcl_logging_configure_with_output_handler(
08:03:23       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
08:03:23       |             rcutils_logging_console_output_handler
08:03:23 make[2]: *** [CMakeFiles/rclcpp.dir/build.make:220: CMakeFiles/rclcpp.dir/src/rclcpp/context.cpp.o] Error 1
08:03:23 make[1]: *** [CMakeFiles/Makefile2:105: CMakeFiles/rclcpp.dir/all] Error 2
08:03:23 make: *** [Makefile:141: all] Error 2
08:03:23 ---

i do not think this is related to my fix.

@fujitatomoya
Copy link
Collaborator Author

@hidmic

friendly ping.

@jacobperron jacobperron added this to In progress in Galactic via automation May 28, 2020
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.

Seems reasonable to me, but it would be better to have a C++ based option structure.

rclcpp_action/include/rclcpp_action/client.hpp Outdated Show resolved Hide resolved
@wjwwood wjwwood requested a review from jacobperron June 2, 2020 03:35
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

We should hold for after the release of Foxy.

Galactic automation moved this from In progress to Reviewer approved Jun 2, 2020
@jacobperron
Copy link
Member

it would be better to have a C++ based option structure.

+1
If we do this we should also add a C++ structure for the server options. This sounds like a good follow-up PR.

@fujitatomoya
Copy link
Collaborator Author

@jacobperron @hidmic @wjwwood

merge conflicts are resolved, could we have a go for merge?

@jacobperron
Copy link
Member

jacobperron commented Jun 15, 2020

CI for packages above rclcpp_action:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Edit: The large number of linter failures are unrelated, I'll re-trigger once the issue has been resolved elsewhere.

Edit: I've triggered a new build.

@jacobperron jacobperron merged commit 696d9ed into ros2:master Jun 16, 2020
Galactic automation moved this from Reviewer approved to Done Jun 16, 2020
@jacobperron
Copy link
Member

@fujitatomoya Thanks for the patch!

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Jul 7, 2020
* add rcl_action_client_options for create_client.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* Capitalize comments and keep the default rcl_action_client_options.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* delete unnecessary default rcl_action_client_options_t.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Galactic
  
Done
Development

Successfully merging this pull request may close these issues.

User cannot setup qos in rclcpp_action create_client
4 participants