Skip to content

Autostart parameter services#478

Merged
sloretz merged 4 commits intomasterfrom
auto_start_parameter_service
May 25, 2018
Merged

Autostart parameter services#478
sloretz merged 4 commits intomasterfrom
auto_start_parameter_service

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented May 24, 2018 •
edited
Loading

This automatically starts parameter services when a node is constructed. This is one step closer to a future where the services are created by rcl. If the user doesn't have to start the services, then they won't notice when rcl is starting them instead of rclcpp.

I refactored create_service() to get rid of ParameterServices requiring a pointer to Node so that it could be started by NodeParameters.

@sloretz sloretz added the in progress Actively being worked on (Kanban column) label May 24, 2018
@sloretz sloretz self-assigned this May 24, 2018
This was referenced May 24, 2018
Merged
Merged
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 24, 2018

CI

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

@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 24, 2018
@sloretz sloretz mentioned this pull request May 24, 2018
Closed
10 tasks
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented May 24, 2018

Looks like a failing build on Windows:

01:27:19 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\src\rclcpp\node_interfaces\node_parameters.cpp(31): error C2653: 'node_interfaces': is not a class or namespace name [C:\J\workspace\ci_windows\ws\build\rclcpp\rclcpp.vcxproj]
01:27:19 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\src\rclcpp\node_interfaces\node_parameters.cpp(32): error C2653: 'node_interfaces': is not a class or namespace name [C:\J\workspace\ci_windows\ws\build\rclcpp\rclcpp.vcxproj]
01:27:19 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\src\rclcpp\node_interfaces\node_parameters.cpp(33): error C2653: 'node_interfaces': is not a class or namespace name [C:\J\workspace\ci_windows\ws\build\rclcpp\rclcpp.vcxproj]
01:27:19 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\src\rclcpp\node_interfaces\node_parameters.cpp(35): error C2511: 'rclcpp::node_interfaces::NodeParameters::NodeParameters(const rclcpp::node_interfaces::NodeParameters::SharedPtr,const rclcpp::node_interfaces::NodeParameters::SharedPtr,const rclcpp::node_interfaces::NodeParameters::SharedPtr,bool)': overloaded member function not found in 'rclcpp::node_interfaces::NodeParameters'node_timers.cpp [C:\J\workspace\ci_windows\ws\build\rclcpp\rclcpp.vcxproj]
01:27:19   
01:27:19   C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\include\rclcpp/node_interfaces/node_parameters.hpp(44): note: see declaration of 'rclcpp::node_interfaces::NodeParameters'
01:27:19 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\src\rclcpp\node_interfaces\node_parameters.cpp(35): error C2671: 'rclcpp::node_interfaces::NodeParameters::{ctor}': static member functions do not have 'this' pointers [C:\J\workspace\ci_windows\ws\build\rclcpp\rclcpp.vcxproj]
01:27:19 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\src\rclcpp\node_interfaces\node_parameters.cpp(36): error C2550: 'rclcpp::node_interfaces::NodeParameters::{ctor}': constructor initializer lists are only allowed on constructor definitions [C:\J\workspace\ci_windows\ws\build\rclcpp\rclcpp.vcxproj]
01:27:19 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\src\rclcpp\node_interfaces\node_parameters.cpp(43): error C2664: 'std::shared_ptr<rclcpp::Publisher<rcl_interfaces::msg::ParameterEvent,std::allocator<void>>> rclcpp::create_publisher<MessageT,AllocatorT,PublisherT>(rclcpp::node_interfaces::NodeTopicsInterface *,const std::string &,const rmw_qos_profile_t &,bool,std::shared_ptr<AllocatorT>)': cannot convert argument 1 from 'rclcpp::node_interfaces::NodeParameters *' to 'rclcpp::node_interfaces::NodeTopicsInterface *' [C:\J\workspace\ci_windows\ws\build\rclcpp\rclcpp.vcxproj]

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 24, 2018

Partial run of windows CI to check if issues are fixed Build Status

@mikaelarguedas
Copy link
Copy Markdown
Member

mikaelarguedas commented May 24, 2018

Should we provide an option for users to not start the parameter server automatically ?

Or do we skip it for no until you get a chance to use the API provided in ros2/rcl#194?

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 24, 2018 •
edited
Loading

Not starting parameter services also means opting out of features like use_sim_time too (edit: oops, actually use_sim_time could still be used once we have initialization from a yaml file). I vote to skip it for now.

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 24, 2018

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

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented May 24, 2018

@sloretz I actually think it's pretty important to be able to avoid creating the ROS part of the parameters service. That system is still pretty heavy weight at the moment. In fact @serge-nikulin already mentioned they'd like this option to disable any ROS interfaces related to parameters.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented May 24, 2018

I would be fine with it being in a follow up pr, however. I just think it's worth doing soon.

wjwwood
wjwwood approved these changes May 24, 2018
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.

lgtm, just had one comment/question.

rclcpp/src/rclcpp/parameter_service.cpp Outdated

get_parameters_service_ = create_service<rcl_interfaces::srv::GetParameters>(
node_base, node_services,
std::string(node_name) + "/" + parameter_service_names::get_parameters,
Copy link
Copy Markdown
Member

@wjwwood wjwwood May 24, 2018

Choose a reason for hiding this comment

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

node_name is already a string? Or is it because it's const?

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz May 25, 2018

Choose a reason for hiding this comment

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

Neither, I didn't pay close enough attention when replacing node->get_name() with node_name 07c0b03

<string> defines an operator+ taking a const string and a c string http://en.cppreference.com/w/cpp/string/basic_string/operator%2B

mikaelarguedas
mikaelarguedas reviewed May 25, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

code change looks good to me:

2 things that should be done before considering merging:

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 25, 2018

@wjwwood added bool start_parameter_services to node constructor in c95925d

CI

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

wjwwood
wjwwood approved these changes May 25, 2018
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.

Lgtm, thanks for making the change.

This was referenced May 25, 2018
Merged
Merged
Merged
Merged
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 25, 2018

@mikaelarguedas turtlebot 2 demo Build Status

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 25, 2018

CI looks good and prs #478, ros2/demos#236, ros2/system_tests#270 are approved.

Turtlebot demo PRs passed CI too, but those prs (ros2/turtlebot2_demo#83, ros2/depthimage_to_laserscan#7, ros2/navigation#27, ros2/teleop_twist_joy#7) aren't approved. I'll merge the prs in ros2.repos now and wait for approvals on the turtlebot2 demo repos separately.

@sloretz sloretz merged commit d82ce96 into master May 25, 2018
@sloretz sloretz deleted the auto_start_parameter_service branch May 25, 2018 20:08
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented May 30, 2018

I am not sure of this patch is related but in the last nightly this test failure showed up and this PR is the only merge changed in this area recently.

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 30, 2018

Seems like this PR might be the culprit. --retest-until-fail 1000 passed on my machine using the commit prior. Using master with --retest-until-fail 100 finds a failure most of the time. Investigating.

@sloretz sloretz mentioned this pull request May 30, 2018
Merged
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented May 30, 2018

@dirk-thomas created PR #483

@mikaelarguedas mikaelarguedas mentioned this pull request Jun 8, 2018
Merged
@mikaelarguedas mikaelarguedas mentioned this pull request Aug 16, 2018
Closed
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
@alsora @wjwwood
included header in logging_rosout.c (ros2#478)
aa6cd91 
Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikaelarguedas mikaelarguedas mikaelarguedas left review comments

@wjwwood wjwwood wjwwood approved these changes

Assignees

@sloretz sloretz

Labels

in review Waiting for review (Kanban column)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sloretz @mjcarroll @mikaelarguedas @wjwwood @dirk-thomas