Skip to content

Avoid std::stringstream#175

Merged
bmagyar merged 2 commits intoros-controls:masterfrom
matthew-reynolds:avoid_stringstream
Apr 30, 2021
Merged

Avoid std::stringstream#175
bmagyar merged 2 commits intoros-controls:masterfrom
matthew-reynolds:avoid_stringstream

Conversation

@matthew-reynolds
Copy link
Copy Markdown
Member

Similar to ros-controls/ros2_control#391

We rarely use the _STREAM variants of RCLCPP logging, so we might as well just avoid them altogether to avoid the std::stringstream.

While I was in there, I also corrected the printf format for size_t. We previously had a mix of %d and %u. For correctness and portability, it should be %zu, which is specifically for size_ts.

Copy link
Copy Markdown
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Just a few small suggestions. Let me know what do you think about them. Thanks!

});

RCLCPP_INFO_STREAM(get_node()->get_logger(), "configure successful");
RCLCPP_INFO(get_node()->get_logger(), "configure successful");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When you are already editing this, can you take a look at the following use?

Suggested change
RCLCPP_INFO(get_node()->get_logger(), "configure successful");
RCLCPP_INFO(node_->get_logger(), "configure successful");

if ((*joint_commands)->data.size() != command_interfaces_.size()) {
RCLCPP_ERROR_STREAM_THROTTLE(
RCLCPP_ERROR_THROTTLE(
get_node()->get_logger(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
get_node()->get_logger(),
node_->get_logger(),

@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Apr 29, 2021

I am wondering if there's a way to use logging properly without the bloat of get_logger() and other ugly stuff like that. RCUTILS_LOG_ERROR_NAMED seems to have a similar effect... any opinions/experience with this?

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #175 (4a638cc) into master (7187083) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
  Coverage   35.27%   35.28%              
==========================================
  Files         286       26     -260     
  Lines       24816     2256   -22560     
  Branches    16082     1462   -14620     
==========================================
- Hits         8755      796    -7959     
+ Misses       1452      132    -1320     
+ Partials    14609     1328   -13281     
Flag Coverage Δ
unittests 35.28% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ectory_controller/test/test_trajectory_actions.cpp
...ontrollers/src/joint_group_position_controller.cpp
...include/joint_trajectory_controller/tolerances.hpp
...ontrollers/src/joint_group_position_controller.cpp
...ers/joint_trajectory_controller/src/trajectory.cpp
...ler/test/test_load_joint_trajectory_controller.cpp
..._state_broadcaster/src/joint_state_broadcaster.cpp
...test/test_load_joint_group_position_controller.cpp
...ler/test/test_load_joint_trajectory_controller.cpp
..._controllers/src/joint_group_effort_controller.cpp
... and 302 more

@bmagyar bmagyar merged commit cafa448 into ros-controls:master Apr 30, 2021
@matthew-reynolds matthew-reynolds deleted the avoid_stringstream branch May 7, 2021 00:40
@matthew-reynolds
Copy link
Copy Markdown
Member Author

As I recall, the RCLCPP logging macros just take a string name from the Node's logger which is then passed down to the RCUTILS macros. The RCUTILS macros are basically the same as the ROS1 LOG_NAMED macros.

I think the cleanest and most standard is to stick with the RCLCPP macros, and to use node_ everywhere. We mostly use node_ internally and get_node() externally, which I think is the right move - It's not too verbose and it matches the standard ROS C++ approach. I've got a PR ready that I'll put up once I get approval from work...

gwalck pushed a commit to b-robotized-forks/ros2_controllers that referenced this pull request Jun 7, 2023
* remove controller loader abstraction layer
* Fix reload_controller_libraries_srv test
* Remove remaining comments
* Remove remaining mock

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Co-authored-by: Mateus Amarante <mateus.amarujo@gmail.com>
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.

4 participants