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 51.88679% with 51 lines in your changes missing coverage. Please review.

Project coverage is 87.75%. Comparing base (86dd7d2) to head (812d103).

❗ Current head 812d103 differs from pull request most recent head 5bc3c93

Please upload reports for the commit 5bc3c93 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1585      +/-   ##
==========================================
- Coverage   87.79%   87.75%   -0.04%     
==========================================
  Files         102      102              
  Lines        8764     8797      +33     
  Branches      787      789       +2     
==========================================
+ Hits         7694     7720      +26     
- Misses        792      798       +6     
- Partials      278      279       +1

Flag	Coverage Δ
unittests	`87.75% <51.88%> (-0.04%)`	⬇️

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

Files	Coverage Δ
..._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%> (ø)`
hardware_interface/src/actuator.cpp	`75.00% <100.00%> (ø)`
hardware_interface/src/sensor.cpp	`70.40% <100.00%> (ø)`
hardware_interface/src/system.cpp	`75.00% <100.00%> (ø)`
...dware_interface/test/test_component_interfaces.cpp	`96.16% <100.00%> (+0.12%)`	⬆️
... and 3 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

👍

bmagyar · 2024-06-27T05:45:17Z

hardware_interface/include/hardware_interface/actuator_interface.hpp

+ rclcpp::Clock get_clock() const
+ {
+ if (clock_interface_)
+ {
+ return *(clock_interface_->get_clock());
+ }
+ else
+ {
+ return rclcpp::Clock(RCL_ROS_TIME);
+ }
+ }


isn't this misleading? If we have no clock we shouldn't return any...

Suggested change

rclcpp::Clock get_clock() const

{

if (clock_interface_)

{

return *(clock_interface_->get_clock());

}

else

{

return rclcpp::Clock(RCL_ROS_TIME);

}

}

std::optional<rclcpp::Clock> get_clock() const

{

return clock_interface_ ? std::optional<rclcpp::Clock>{*(clock_interface_->get_clock())} : std::nullopt;

}

Better yet, just use the existing sharedptr

Suggested change

rclcpp::Clock get_clock() const

{

if (clock_interface_)

{

return *(clock_interface_->get_clock());

}

else

{

return rclcpp::Clock(RCL_ROS_TIME);

}

}

rclcpp::node_interfaces::NodeClockInterface::SharedPtr get_clock() const

{

return clock_interface_;

}

Sure, maybe shared_ptr is better and makes sense.

Or maybe even better yet... can we validate the clock interface sharedptr we are getting and turn it into a clock? Is that dangerous?

*(clock_interface_->get_clock()); That's what I'm doing here right?

Yes but what I mean is we should do this at the top and not even accept invalid clocks

@bmagyar Yes, I just pushed the changes!

With the new changes, we will need to push a fix to ros2_controllers as well inorder to fix some load tests. For instance this one

https://github.com/ros-controls/ros2_controllers/blob/b81be48fb1b6101a3c601c0ea11c5393e5a7026e/imu_sensor_broadcaster/test/test_load_imu_sensor_broadcaster.cpp#L34-L37

Before:

controller_manager::ControllerManager cm( std::make_unique<hardware_interface::ResourceManager>( ros2_control_test_assets::minimal_robot_urdf), executor, "test_controller_manager");

Now:

controller_manager::ControllerManager cm( executor, ros2_control_test_assets::minimal_robot_urdf, "test_controller_manager");

Take a look at the changes and let me know if they look fine to you. If yes, I will open a PR for the controllers as well.

While making these modifications, I was under the dilemma to still have the NodeClockInterface and NodeLoggingInterface or just replace them with the rclcpp::Clock::SharedPtr and rclcpp::Logger so, it is easy on the tests, but just in case, I left the same.

Here is the PR of ros2_controllers: ros-controls/ros2_controllers#1184

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

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

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 Jun 27, 2024

bmagyar Jun 27, 2024

saikishor Jun 27, 2024

bmagyar Jun 27, 2024

saikishor Jun 27, 2024

bmagyar Jun 27, 2024

saikishor Jun 27, 2024 •

edited

Loading

saikishor Jun 27, 2024

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

Are you sure you want to change the base?

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 Jun 27, 2024

Choose a reason for hiding this comment

bmagyar Jun 27, 2024

Choose a reason for hiding this comment

saikishor Jun 27, 2024

Choose a reason for hiding this comment

bmagyar Jun 27, 2024

Choose a reason for hiding this comment

saikishor Jun 27, 2024

Choose a reason for hiding this comment

bmagyar Jun 27, 2024

Choose a reason for hiding this comment

saikishor Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

saikishor Jun 27, 2024

Choose a reason for hiding this comment

saikishor commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

saikishor Jun 27, 2024 •

edited

Loading