Add logger and clock interfaces from ResourceManager to HardwareComponent interfaces #1585

saikishor · 2024-06-24T16:35:35Z

As discussed in the WG meeting on 19th June 2024, I've added the logging and the clock interfaces to ResourceManager and HardwareComponents. I've also modified all the RCUTILS logging to RCLCPP logging.

[INFO] [1719246718.869627728] [resource_manager]: Loading hardware 'MockHardwareSystem' 
[INFO] [1719246718.869987243] [resource_manager]: Loaded hardware 'MockHardwareSystem' from plugin 'mock_components/GenericSystem'
[INFO] [1719246718.870008000] [resource_manager]: Initialize hardware 'MockHardwareSystem' 
[WARN] [1719246718.870122036] [resource_manager.hardware_component.system.MockHardwareSystem]: Custom interface with following offset 'actual_position' does not exist. Offset will not be applied
[INFO] [1719246718.870130775] [resource_manager]: Successful initialization of hardware 'MockHardwareSystem'
[INFO] [1719246718.870288251] [resource_manager]: 'configure' hardware 'MockHardwareSystem'

Thank you

codecov · 2024-06-24T16:45:54Z

Codecov Report

Attention: Patch coverage is 65.48223% with 68 lines in your changes missing coverage. Please review.

Project coverage is 86.96%. Comparing base (bb85eae) to head (9d72500).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1585      +/-   ##
==========================================
+ Coverage   86.94%   86.96%   +0.01%     
==========================================
  Files         107      107              
  Lines        9306     9379      +73     
  Branches      863      867       +4     
==========================================
+ Hits         8091     8156      +65     
- Misses        899      903       +4     
- Partials      316      320       +4

Flag	Coverage Δ
unittests	`86.96% <65.48%> (+0.01%)`	⬆️

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

Files	Coverage Δ
.../include/controller_manager/controller_manager.hpp	`100.00% <ø> (ø)`
...er/test/test_controller_manager_with_namespace.cpp	`100.00% <100.00%> (+2.94%)`	⬆️
...t_controllers_chaining_with_controller_manager.cpp	`99.14% <100.00%> (+0.28%)`	⬆️
...ler_manager/test/test_hardware_management_srvs.cpp	`96.02% <100.00%> (-0.06%)`	⬇️
..._interface/include/hardware_interface/actuator.hpp	`100.00% <ø> (ø)`
.../include/hardware_interface/actuator_interface.hpp	`100.00% <100.00%> (ø)`
...re_interface/include/hardware_interface/sensor.hpp	`66.66% <ø> (ø)`
...ce/include/hardware_interface/sensor_interface.hpp	`100.00% <100.00%> (ø)`
...re_interface/include/hardware_interface/system.hpp	`100.00% <ø> (ø)`
...ce/include/hardware_interface/system_interface.hpp	`100.00% <100.00%> (ø)`
... and 12 more

hardware_interface/src/resource_manager.cpp

christophfroehlich

LGTM!

How can we promote this new feature? Please add something to the release notes, maybe also hardware components docs. Possibly we can use this in the demos as well?

saikishor · 2024-06-25T10:21:42Z

How can we promote this new feature? Please add something to the release notes, maybe also hardware components docs. Possibly we can use this in the demos as well?

Yes, we can use them in the demos as well. I've used it inside the MockHardwareSystem in this PR.

mergify · 2024-06-25T10:56:45Z

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

…ctor

…rceStorage

…d clock interfaces

christophfroehlich

👍

hardware_interface/include/hardware_interface/actuator_interface.hpp

bmagyar · 2024-07-03T17:57:47Z

@ahcorde are we allowed one more breaking change please please?

destogl

It looks great, just a few small comments we can resolve fairly quickly.

controller_manager/include/controller_manager/controller_manager.hpp

controller_manager/src/controller_manager.cpp

destogl · 2024-07-03T16:42:41Z

hardware_interface/include/hardware_interface/actuator_interface.hpp

@@ -73,7 +75,8 @@ class ActuatorInterface : public rclcpp_lifecycle::node_interfaces::LifecycleNod
 public:
  ActuatorInterface()
  : lifecycle_state_(rclcpp_lifecycle::State(
-      lifecycle_msgs::msg::State::PRIMARY_STATE_UNKNOWN, lifecycle_state_names::UNKNOWN))
+      lifecycle_msgs::msg::State::PRIMARY_STATE_UNKNOWN, lifecycle_state_names::UNKNOWN)),
+    actuator_logger_(rclcpp::get_logger("actuator_interface"))


Shouldn't we here use some better name? Or better yet, do we need this actually? As we always have to call init first.

You need to define something, if not it complains with the following error. That's the reason, I declared basically the name of the class

error: ‘rclcpp::Logger::Logger()’ is private within this context

hardware_interface/src/resource_manager.cpp

destogl · 2024-07-03T17:14:59Z

hardware_interface/src/resource_manager.cpp

+  // Logger and Clock interfaces
+  rclcpp::node_interfaces::NodeClockInterface::SharedPtr clock_interface_;
+  rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr logger_interface_;
+  rclcpp::Logger rm_logger_ = rclcpp::get_logger("resource_manager");


maybe to add this above to the constructor to if - else statement. This would increase clarity. (I mean the right side).

hardware_interface/src/resource_manager.cpp

Co-authored-by: Dr. Denis <denis@stoglrobotics.de>

destogl

Thanks for clarifications!

hardware_interface/src/resource_manager.cpp

github-actions bot requested review from bmagyar, destogl, fmauch, mcbed, olivier-stasse, peterdavidfagan and VX792 June 24, 2024 16:35

christophfroehlich reviewed Jun 24, 2024

View reviewed changes

hardware_interface/src/resource_manager.cpp Outdated Show resolved Hide resolved

christophfroehlich requested changes Jun 25, 2024

View reviewed changes

saikishor added 16 commits June 25, 2024 13:36

Add logging interface as an argument into the ResourceManager constru…

cd7fe30

…ctor

Add get_logger and get_clock methods in the ResourceManager and Resou…

a424ec2

…rceStorage

Propagate the interfaces to the HW component

806430e

Add the init API in the hardware interfaces to parse the logging an…

73cc794

…d clock interfaces

comment to avoid unused parameter warning

a391558

Add documentation to the get_logger and get_clock methods

4dbcf0c

remove the default nullptr args to the logging and clock interface

3565f8b

Consider the nullptr and return valid Logger and Clock

7760461

Change RCUTILS logging to RCLCPP logging

fe46906

rename ResourceManager to resource_manager in the logger

e037d10

change the variables order for the vtable

b313af4

Add rm_logger variable to avoid asking for logger every instance

578408d

remove NodeLoggingInterface and parse Logger directly

1fdcbbb

change the include headers of unused logging interface

4eecf45

enable logger services for the controller_manager

2baf0b1

Update Release Notes

812d103

saikishor force-pushed the propagate/logger branch from 5ce5a44 to 812d103 Compare June 25, 2024 11:47

christophfroehlich previously approved these changes Jun 25, 2024

View reviewed changes

bmagyar reviewed Jun 27, 2024

View reviewed changes

hardware_interface/include/hardware_interface/actuator_interface.hpp Outdated Show resolved Hide resolved

saikishor added 2 commits June 27, 2024 13:49

return shared pointer of clock instead of clock itself

4ff14e5

make the clock and logger interfaces mandatory arguments and fix tests

5bc3c93

saikishor dismissed christophfroehlich’s stale review via 5bc3c93 June 27, 2024 12:08

saikishor mentioned this pull request Jun 27, 2024

added changes corresponding to the logger and clock propagation in ResourceManager ros-controls/ros2_controllers#1184

Merged

bmagyar previously approved these changes Jul 3, 2024

View reviewed changes

destogl reviewed Jul 3, 2024

View reviewed changes

Merge branch 'master' into propagate/logger

2bf8a1d

christophfroehlich previously approved these changes Jul 4, 2024

View reviewed changes

saikishor added 2 commits July 4, 2024 19:36

add similar comment on the version conditioning

038945c

add the default rm_logger_ to the if-else as per MR suggestion

303c846

saikishor dismissed stale reviews from christophfroehlich and bmagyar via 303c846 July 4, 2024 20:58

saikishor and others added 2 commits July 4, 2024 22:58

print default value of the update_rate

ec89bf5

Co-authored-by: Dr. Denis <denis@stoglrobotics.de>

print default update rate in other instances as well

9d72500

christophfroehlich approved these changes Jul 6, 2024

View reviewed changes

bmagyar approved these changes Jul 6, 2024

View reviewed changes

saikishor mentioned this pull request Jul 8, 2024

Propagate the node clock and logging interface to ResourceManager ros-controls/gz_ros2_control#368

Merged

destogl approved these changes Jul 8, 2024

View reviewed changes

hardware_interface/src/resource_manager.cpp Show resolved Hide resolved

hardware_interface/src/resource_manager.cpp Show resolved Hide resolved

destogl merged commit 2b758eb into ros-controls:master Jul 8, 2024
18 of 21 checks passed

saikishor deleted the propagate/logger branch July 8, 2024 16:20

saikishor mentioned this pull request Jul 8, 2024

Propagate the node clock and logging interface to ResourceManager ros-controls/gazebo_ros2_control#357

Merged

mergify bot mentioned this pull request Jul 9, 2024

Propagate the node clock and logging interface to ResourceManager (backport #368) ros-controls/gz_ros2_control#373

Merged

saikishor mentioned this pull request Jul 17, 2024

Add "NodeLoggingInterface" Access to HW and create dedicated logger accessible from get_logger() method inside the component. #1619

Closed

sea-bass mentioned this pull request Jul 18, 2024

Remove mimic tags and rename sim_ignition to sim_gazebo PickNikRobotics/ros2_robotiq_gripper#54

Merged

christophfroehlich mentioned this pull request Jul 24, 2024

Refactor logging ros-controls/ros2_control_demos#543

Merged

danzimmerman mentioned this pull request Oct 29, 2024

ResourceManager needs node logging interface on Jazzy danzimmerman/dz_minimal_controller#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logger and clock interfaces from ResourceManager to HardwareComponent interfaces #1585

Add logger and clock interfaces from ResourceManager to HardwareComponent interfaces #1585

saikishor commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

christophfroehlich left a comment

saikishor commented Jun 25, 2024

mergify bot commented Jun 25, 2024

christophfroehlich left a comment

bmagyar commented Jul 3, 2024

destogl left a comment

destogl Jul 3, 2024

saikishor Jul 4, 2024

destogl Jul 3, 2024

saikishor Jul 3, 2024

saikishor Jul 4, 2024

destogl left a comment

Add logger and clock interfaces from ResourceManager to HardwareComponent interfaces #1585

Add logger and clock interfaces from ResourceManager to HardwareComponent interfaces #1585

Conversation

saikishor commented Jun 24, 2024 • edited Loading

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

christophfroehlich left a comment

Choose a reason for hiding this comment

saikishor commented Jun 25, 2024

mergify bot commented Jun 25, 2024

christophfroehlich left a comment

Choose a reason for hiding this comment

bmagyar commented Jul 3, 2024

destogl left a comment

Choose a reason for hiding this comment

destogl Jul 3, 2024

Choose a reason for hiding this comment

saikishor Jul 4, 2024

Choose a reason for hiding this comment

destogl Jul 3, 2024

Choose a reason for hiding this comment

saikishor Jul 3, 2024

Choose a reason for hiding this comment

saikishor Jul 4, 2024

Choose a reason for hiding this comment

destogl left a comment

Choose a reason for hiding this comment

saikishor commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading