Node logging in moveit_core #2503

tylerjw · 2023-10-31T02:08:06Z

Description

codecov · 2023-10-31T03:45:32Z

Codecov Report

Attention: 360 lines in your changes are missing coverage. Please review.

Comparison is base (bddab3f) 50.87% compared to head (2cec4ef) 50.46%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
moveit_core/planning_scene/src/planning_scene.cpp	25.00%	39 Missing ⚠️
moveit_core/robot_state/src/robot_state.cpp	9.10%	30 Missing ⚠️
moveit_core/robot_model/src/robot_model.cpp	46.16%	28 Missing ⚠️
..._detection/src/allvalid/collision_env_allvalid.cpp	8.70%	21 Missing ⚠️
...istance_field/src/collision_env_distance_field.cpp	44.45%	20 Missing ⚠️
...cessing/src/time_optimal_trajectory_generation.cpp	16.67%	20 Missing ⚠️
moveit_core/robot_state/src/conversions.cpp	22.73%	17 Missing ⚠️
...kinematic_constraints/src/kinematic_constraint.cpp	60.98%	16 Missing ⚠️
...raint_samplers/src/default_constraint_samplers.cpp	42.86%	12 Missing ⚠️
moveit_core/robot_model/src/joint_model_group.cpp	25.00%	12 Missing ⚠️
... and 42 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2503      +/-   ##
==========================================
- Coverage   50.87%   50.46%   -0.41%     
==========================================
  Files         388      387       -1     
  Lines       32253    32148     -105     
==========================================
- Hits        16407    16220     -187     
- Misses      15846    15928      +82

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tylerjw · 2023-11-02T22:41:28Z

This one is still not ready... seeing weird test failures locally, will resume working on this tomorrow.

mergify · 2023-11-07T10:58:02Z

This pull request is in conflict. Could you fix it @tylerjw?

mergify · 2023-11-21T04:33:14Z

This pull request is in conflict. Could you fix it @tylerjw?

mergify · 2023-11-21T15:11:36Z

This pull request is in conflict. Could you fix it @tylerjw?

mergify · 2023-11-28T13:38:20Z

This pull request is in conflict. Could you fix it @tylerjw?

sea-bass

Things look good, except I have a question about the mix and match of PascalCase and snake_case in all the new logger names that were added. Is there a pattern I missed here for deciding which goes where?

Besides that, a sprinkling of minor comments, some of them not even due to the PR but things I noticed.

moveit_core/collision_detection/src/collision_octomap_filter.cpp

moveit_core/collision_distance_field/src/collision_env_distance_field.cpp

moveit_core/exceptions/src/exceptions.cpp

moveit_core/robot_trajectory/test/test_robot_trajectory.cpp

moveit_ros/move_group/src/default_capabilities/clear_octomap_service_capability.cpp

moveit_ros/moveit_servo/src/utils/command.cpp

moveit_ros/perception/point_containment_filter/src/shape_mask.cpp

moveit_ros/warehouse/src/save_as_text.cpp

tylerjw · 2023-12-03T02:16:09Z

The snake case are functions from ROS where they use the style god intended. The pascal case is from moveit's clang-tidy rules because we match ROS 1 style.

Signed-off-by: Tyler Weaver <[email protected]>

v4hn · 2023-12-04T19:42:15Z

doc/MIGRATION_GUIDE.md

-Once you have a child logger you can use it in logging macros:
-```C++
-RCLCPP_INFO(logger_, "Very important info message");
+RCLCPP_INFO(moveit::getLogger("my_namespace"), "Very important info message");


Honestly very stupid question:

Suggested change

RCLCPP_INFO(moveit::getLogger("my_namespace"), "Very important info message");

moveit::logInfo("my_namespace", "Very important info message");

or

Suggested change

RCLCPP_INFO(moveit::getLogger("my_namespace"), "Very important info message");

MOVEIT_INFO("my_namespace", "Very important info message");

do read loads better, right?

I would clearly support a readable wrapper over an awkward combination of MoveIt internal static magic (definitely not what god intended) TOGETHER with C macro magic (which was clearly invented by demons).

Here is the issues I see with those two approaches.

functions

The macros capture the file, function, and line number from the place where the macro is called. If we wrapped the ros logging system in our own functions we would loose that.

our own macros

This is a more ergonomic solution but would require me to write macros that wrap the ros macros. That would be a lot of code to write, test, and maintain. I think this approach is better given that tradeoff.

Fair enough. We could probably limit a convenience macro wrapper to the plain variants for almost all uses. That approach works well in OMPL and surely others. But then the question would be whether stream or printf-style and combinations might quickly blow up again if people disagree.

Still, an argument slot that only ever contains one generic term in correct usage should not exist either.

Between a rock and a hard place by design.

Yea, I think in this case the ROS 2 logging interface is just worse from a usability standpoint. I want to reduce the amount of abstractions I maintain so I don't want to maintain a set of wrapper logging macros. For that reason I think what is in this PR is the best compromise for now.

sea-bass

just found 1 thing

moveit_ros/move_group/src/default_capabilities/state_validation_service_capability.cpp

…n_service_capability.cpp Co-authored-by: Sebastian Castro <[email protected]>

tylerjw force-pushed the logger_moveit_core branch from 57ced77 to 6e1f7f4 Compare October 31, 2023 02:10

henningkayser requested review from henningkayser and sjahr October 31, 2023 14:36

tylerjw force-pushed the logger_moveit_core branch from 6358f2e to 886a724 Compare November 2, 2023 21:57

tylerjw force-pushed the logger_moveit_core branch from a1a2eda to 4929a9d Compare November 2, 2023 22:56

tylerjw force-pushed the logger_moveit_core branch from 4929a9d to 01b21f0 Compare November 20, 2023 22:15

tylerjw force-pushed the logger_moveit_core branch from 01b21f0 to bd7dbba Compare November 21, 2023 04:34

tylerjw force-pushed the logger_moveit_core branch 2 times, most recently from 8f942af to 239c933 Compare November 27, 2023 16:24

tylerjw force-pushed the logger_moveit_core branch from 29b959f to aa250dc Compare December 1, 2023 02:22

sea-bass reviewed Dec 3, 2023

View reviewed changes

tylerjw force-pushed the logger_moveit_core branch from 1433e80 to b5ad365 Compare December 4, 2023 18:21

tylerjw and others added 7 commits December 4, 2023 11:21

Node logging in moveit_core

c42f73f

Signed-off-by: Tyler Weaver <[email protected]>

prototype in logger_from_child_dut

2503535

Signed-off-by: Tyler Weaver <[email protected]>

yet another refactor

d83b147

update migration guide

368c617

Signed-off-by: Tyler Weaver <[email protected]>

rebase

e1c0709

Signed-off-by: Tyler Weaver <[email protected]>

Fix clang-tidy wanrings

c5c7e81

apply changes from review

3e3a622

Signed-off-by: Tyler Weaver <[email protected]>

tylerjw force-pushed the logger_moveit_core branch from b5ad365 to 3e3a622 Compare December 4, 2023 18:21

v4hn reviewed Dec 4, 2023

View reviewed changes

Merge branch 'main' into logger_moveit_core

ece2ab2

sea-bass approved these changes Dec 5, 2023

View reviewed changes

moveit_ros/move_group/src/default_capabilities/state_validation_service_capability.cpp Outdated Show resolved Hide resolved

Update moveit_ros/move_group/src/default_capabilities/state_validatio…

2cec4ef

…n_service_capability.cpp Co-authored-by: Sebastian Castro <[email protected]>

tylerjw enabled auto-merge (squash) December 5, 2023 16:34

tylerjw merged commit 74debfd into main Dec 5, 2023
11 of 12 checks passed

tylerjw deleted the logger_moveit_core branch December 5, 2023 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node logging in moveit_core #2503

Node logging in moveit_core #2503

tylerjw commented Oct 31, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

tylerjw commented Nov 2, 2023

mergify bot commented Nov 7, 2023

mergify bot commented Nov 21, 2023

mergify bot commented Nov 21, 2023

mergify bot commented Nov 28, 2023

sea-bass left a comment

tylerjw commented Dec 3, 2023

v4hn Dec 4, 2023

tylerjw Dec 4, 2023

v4hn Dec 4, 2023

tylerjw Dec 5, 2023

sea-bass left a comment

	RCLCPP_INFO(moveit::getLogger("my_namespace"), "Very important info message");
	moveit::logInfo("my_namespace", "Very important info message");

	RCLCPP_INFO(moveit::getLogger("my_namespace"), "Very important info message");
	MOVEIT_INFO("my_namespace", "Very important info message");

Node logging in moveit_core #2503

Node logging in moveit_core #2503

Conversation

tylerjw commented Oct 31, 2023

Description

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

tylerjw commented Nov 2, 2023

mergify bot commented Nov 7, 2023

mergify bot commented Nov 21, 2023

mergify bot commented Nov 21, 2023

mergify bot commented Nov 28, 2023

sea-bass left a comment

Choose a reason for hiding this comment

tylerjw commented Dec 3, 2023

v4hn Dec 4, 2023

Choose a reason for hiding this comment

tylerjw Dec 4, 2023

Choose a reason for hiding this comment

functions

our own macros

v4hn Dec 4, 2023

Choose a reason for hiding this comment

tylerjw Dec 5, 2023

Choose a reason for hiding this comment

sea-bass left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2023 •

edited

Loading