Integrate pal_statistics for introspection of controllers, hardware components and more #1918

saikishor · 2024-12-05T23:35:19Z

Added integration of pal_statistics into the ros2_control infrastructure

You can find more examples at #1897

codecov · 2024-12-05T23:51:52Z

Codecov Report

Attention: Patch coverage is 87.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.60%. Comparing base (4ad5709) to head (5f91488).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...re_interface/include/hardware_interface/handle.hpp	50.00%	3 Missing and 3 partials ⚠️
hardware_interface/src/resource_manager.cpp	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1918      +/-   ##
==========================================
- Coverage   88.61%   88.60%   -0.01%     
==========================================
  Files         122      122              
  Lines       13304    13357      +53     
  Branches     1207     1215       +8     
==========================================
+ Hits        11789    11835      +46     
- Misses       1062     1064       +2     
- Partials      453      458       +5

Flag	Coverage Δ
unittests	`88.60% <87.50%> (-0.01%)`	⬇️

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

Files with missing lines	Coverage Δ
...controller_interface/controller_interface_base.hpp	`92.30% <ø> (ø)`
...roller_interface/src/controller_interface_base.cpp	`88.57% <100.00%> (+6.09%)`	⬆️
.../include/controller_manager/controller_manager.hpp	`100.00% <ø> (ø)`
controller_manager/src/controller_manager.cpp	`76.12% <100.00%> (-0.24%)`	⬇️
.../include/hardware_interface/actuator_interface.hpp	`92.24% <100.00%> (+0.27%)`	⬆️
...ce/include/hardware_interface/sensor_interface.hpp	`95.65% <100.00%> (+0.26%)`	⬆️
...ce/include/hardware_interface/system_interface.hpp	`82.35% <100.00%> (+0.53%)`	⬆️
hardware_interface/src/actuator.cpp	`81.29% <100.00%> (+0.62%)`	⬆️
hardware_interface/src/sensor.cpp	`76.98% <100.00%> (+0.95%)`	⬆️
hardware_interface/src/system.cpp	`81.29% <100.00%> (+0.62%)`	⬆️
... and 2 more

... and 1 file with indirect coverage changes

christophfroehlich

I could write an AI agent commenting on PRs 😆

Please update the docs
Please update the release_notes

saikishor · 2024-12-06T19:08:23Z

I could write an AI agent commenting on PRs 😆

Please update the docs

Please update the release_notes

Sure! Boss 😆😆

mergify · 2024-12-07T18:02:50Z

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

christophfroehlich

Thanks! I tested this successfully and discussed some details with Sai, just add some notes here:

pal_statistics name/values pair is parsed by plotjuggler to be able to select the values by name without a custom message layout/no need for sending the names on a regular basis.

https://github.com/facontidavide/PlotJuggler/blob/b1ad0896844e848c2495e09a6e5c3548685adedd/plotjuggler_plugins/ParserROS/ros_parser.cpp#L81-L90

statistics from [Diagnostics] Add diagnostics of execution time and periodicity of the controllers and controller_manager #1871 will be added in a later PR.
we have to fix clang so that it doesn't check system packages, maybe with this setup

I added some minor comments below.

hardware_interface/package.xml

doc/debugging.rst

doc/introspection.rst

christophfroehlich

Approving as we have fixed clang now.

.github/workflows/jazzy-semi-binary-build.yml

.github/workflows/rolling-semi-binary-build.yml

saikishor · 2024-12-10T17:16:43Z

Approving as we have fixed clang now.

Thank you @christophfroehlich

controller_interface/src/controller_interface_base.cpp

doc/introspection.rst

controller_interface/src/controller_interface_base.cpp

doc/introspection.rst

mergify · 2024-12-20T14:52:02Z

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

MarqRazz

Thanks for detailed answers @saikishor, I think this looks like a great addition!

saikishor · 2024-12-20T21:41:04Z

Thanks for detailed answers @saikishor, I think this looks like a great addition!

Thanks to you for taking time and reviewing it

saikishor · 2024-12-20T23:34:06Z

Conflicts resolved! ;)

…ses and add helper macros

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

…ecycle node

destogl

A few comments to clarify before continuing.

controller_interface/src/controller_interface_base.cpp

destogl · 2025-01-01T17:31:23Z

controller_interface/include/controller_interface/controller_interface_base.hpp

@@ -26,6 +26,7 @@
 #include "hardware_interface/loaned_command_interface.hpp"
 #include "hardware_interface/loaned_state_interface.hpp"

+#include "pal_statistics/pal_statistics_utils.hpp"


This adds a new dependency to almost everything. Should we at first have this removed by on compiler flags and one can enable it if required.

I think the change is minimal, or may be I can add it to the hardware_interface/introspection and add it eveywhere, that way it is minimal

destogl · 2025-01-01T17:35:15Z

controller_manager/src/controller_manager.cpp

@@ -283,6 +284,8 @@ ControllerManager::ControllerManager(
  init_controller_manager();
 }

+ControllerManager::~ControllerManager() { CLEAR_ALL_REGISTRIES(); }


I am not a fan of global macros if those are not required. If this is the only way, OK, but then I would propose to have a more explicit name, e.g., CLEAR_ALL_INTROSPECTION_REGISTRIES.

This global macro is from PAL statistics. The macros are added for simplistic integration. May be I can add another macro, that can call this.

Done. I added more explicit name as you requested

destogl · 2025-01-01T17:36:00Z

controller_manager/src/controller_manager.cpp

@@ -321,6 +324,10 @@ void ControllerManager::init_controller_manager()
  diagnostics_updater_.add(
    "Controller Manager Activity", this,
    &ControllerManager::controller_manager_diagnostic_callback);
+  INITIALIZE_REGISTRY(


Also here, can we do this on an object? If not, can we rename this to INITIALIZE_INTROSPECTION_REGISTRY?

This is from the PAL statistics. I'm not sure that this can be renamed :(

Done. I've also changed this and others

doc/introspection.rst

destogl · 2025-01-01T17:56:21Z

hardware_interface/include/hardware_interface/actuator_interface.hpp

@@ -93,7 +94,7 @@ class ActuatorInterface : public rclcpp_lifecycle::node_interfaces::LifecycleNod
   */
  ActuatorInterface(const ActuatorInterface & other) = delete;

-  ActuatorInterface(ActuatorInterface && other) = default;
+  ActuatorInterface(ActuatorInterface && other) = delete;


Why do we need to delete this constructor? It is obviously not used, but if this is the reason for deletion, then why not do this in a separate PR.

@destogl I just checked the code. This is a part of the change as the pal_statistics::RegistrationsRAII stats_registrations_ is non moveable. As we already deleted the copies, I think it makes sense to remove the move operations as well

hardware_interface/include/hardware_interface/handle.hpp

destogl · 2025-01-01T18:10:51Z

hardware_interface/include/hardware_interface/handle.hpp

+        rclcpp::get_logger("command_interface"), "Registering handle: " << get_name());
+      std::function<double()> f = [this]()
+      { return value_ptr_ ? *value_ptr_ : std::numeric_limits<double>::quiet_NaN(); };
+      REGISTER_ENTITY(DEFAULT_REGISTRY_KEY, "command_interface." + get_name(), f);


Kind or too generic name. I understand this is from the pal_statistics, but maybe we can sub-specify it.

I'll take a look at it

Changed it with specific naming

destogl · 2025-01-01T18:10:59Z

hardware_interface/include/hardware_interface/handle.hpp

+    {
+      RCLCPP_INFO_STREAM(
+        rclcpp::get_logger("command_interface"), "Unregistering handle: " << get_name());
+      UNREGISTER_ENTITY(DEFAULT_REGISTRY_KEY, "command_interface." + get_name());


Kind of too generic.

Yeah, PAL statistics uses that. I'll what I can do wiht it

Done. Also changed it to be more generic

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

saikishor

@destogl Thanks for the review.

I've addressed all your comments in the PR.

saikishor · 2025-01-01T20:15:28Z

controller_interface/include/controller_interface/controller_interface_base.hpp

@@ -26,6 +26,7 @@
 #include "hardware_interface/loaned_command_interface.hpp"
 #include "hardware_interface/loaned_state_interface.hpp"

+#include "pal_statistics/pal_statistics_utils.hpp"


saikishor · 2025-01-01T20:16:05Z

controller_manager/src/controller_manager.cpp

@@ -283,6 +284,8 @@ ControllerManager::ControllerManager(
  init_controller_manager();
 }

+ControllerManager::~ControllerManager() { CLEAR_ALL_REGISTRIES(); }


Done. I added more explicit name as you requested

saikishor · 2025-01-01T20:16:22Z

controller_manager/src/controller_manager.cpp

@@ -321,6 +324,10 @@ void ControllerManager::init_controller_manager()
  diagnostics_updater_.add(
    "Controller Manager Activity", this,
    &ControllerManager::controller_manager_diagnostic_callback);
+  INITIALIZE_REGISTRY(


Done. I've also changed this and others

hardware_interface/include/hardware_interface/handle.hpp

saikishor · 2025-01-01T20:17:40Z

hardware_interface/include/hardware_interface/handle.hpp

+        rclcpp::get_logger("command_interface"), "Registering handle: " << get_name());
+      std::function<double()> f = [this]()
+      { return value_ptr_ ? *value_ptr_ : std::numeric_limits<double>::quiet_NaN(); };
+      REGISTER_ENTITY(DEFAULT_REGISTRY_KEY, "command_interface." + get_name(), f);


Changed it with specific naming

hardware_interface/include/hardware_interface/handle.hpp

saikishor · 2025-01-01T20:17:58Z

hardware_interface/include/hardware_interface/handle.hpp

+    {
+      RCLCPP_INFO_STREAM(
+        rclcpp::get_logger("command_interface"), "Unregistering handle: " << get_name());
+      UNREGISTER_ENTITY(DEFAULT_REGISTRY_KEY, "command_interface." + get_name());


Done. Also changed it to be more generic

github-actions bot requested review from aprotyas, arne48, christophfroehlich, DasRoteSkelett, destogl, mamueluth and qiayuanl December 5, 2024 23:35

christophfroehlich requested changes Dec 6, 2024

View reviewed changes

saikishor requested a review from christophfroehlich December 7, 2024 20:15

christophfroehlich reviewed Dec 9, 2024

View reviewed changes

hardware_interface/package.xml Outdated Show resolved Hide resolved

doc/debugging.rst Outdated Show resolved Hide resolved

doc/debugging.rst Outdated Show resolved Hide resolved

doc/debugging.rst Outdated Show resolved Hide resolved

christophfroehlich reviewed Dec 9, 2024

View reviewed changes

doc/introspection.rst Outdated Show resolved Hide resolved

saikishor mentioned this pull request Dec 9, 2024

Mark everything under /opt/ros a system header ros-controls/ros2_control_ci#191

Merged

christophfroehlich previously approved these changes Dec 10, 2024

View reviewed changes

.github/workflows/jazzy-semi-binary-build.yml Outdated Show resolved Hide resolved

.github/workflows/rolling-semi-binary-build.yml Outdated Show resolved Hide resolved

saikishor dismissed christophfroehlich’s stale review via 094028c December 17, 2024 07:16

MarqRazz reviewed Dec 20, 2024

View reviewed changes

controller_interface/src/controller_interface_base.cpp Show resolved Hide resolved

doc/introspection.rst Show resolved Hide resolved

controller_interface/src/controller_interface_base.cpp Show resolved Hide resolved

doc/introspection.rst Show resolved Hide resolved

MarqRazz approved these changes Dec 20, 2024

View reviewed changes

saikishor force-pushed the integrate/pal_statistics branch from b9d892c to 90d12f2 Compare December 20, 2024 23:33

saikishor added 6 commits December 30, 2024 16:15

add first integration of pal_statistics

1b503d3

namespace the introspection_data to the controller_manager node naming

aca20e4

Add integration into the controller and hardware components base clas…

07334de

…ses and add helper macros

remove execution time introspection

884c080

handle cases when the value_ptr_ is invalid

c181b5d

add CLEAR_ALL_REGISTRIES macro in destructor

a107a99

saikishor and others added 15 commits December 30, 2024 16:15

disable introspection on cleanup

180b127

rename macro to REGISTER_ROS2_CONTROL_INTROSPECTION

6581204

update release_notes

4eee6ad

add missing header in controller_interface_base.hpp

7e22d6a

Update debugging docs

a57b973

add more documentation about the topics

f2c4e0e

delete the move constructors

719c747

Add macro with also enable argument

6ed4be6

place dependencies alphabetically

81c0277

Update doc/debugging.rst

10fe456

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

move the documentation to introspection.rst

870da8a

update docs on the third argument to enable introspection

612158d

use clang jobs from christoph

d4923ae

Revert "Add macro with also enable argument"

447a273

Update doc/introspection.rst

bc04f2a

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

saikishor force-pushed the integrate/pal_statistics branch from 90d12f2 to da34b78 Compare December 30, 2024 15:15

Use the master CI for clang

aa19ef9

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

saikishor force-pushed the integrate/pal_statistics branch from da34b78 to aa19ef9 Compare December 31, 2024 10:16

saikishor added 2 commits December 31, 2024 11:37

remove the visibility_macro on get_name method

3838f45

return std::string instead of const ref as char * are returned by lif…

f201856

…ecycle node

destogl reviewed Jan 1, 2025

View reviewed changes

saikishor and others added 2 commits January 1, 2025 20:14

Apply suggestions from code review

fb80164

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

Add changes from Denis review comments

5f91488

saikishor commented Jan 1, 2025

View reviewed changes

saikishor requested a review from destogl January 1, 2025 20:18

Integrate pal_statistics for introspection of controllers, hardware components and more #1918

Are you sure you want to change the base?

Integrate pal_statistics for introspection of controllers, hardware components and more #1918

Conversation

saikishor commented Dec 5, 2024

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

christophfroehlich left a comment

Choose a reason for hiding this comment

saikishor commented Dec 6, 2024

mergify bot commented Dec 7, 2024

christophfroehlich left a comment • edited Loading

Choose a reason for hiding this comment

christophfroehlich left a comment

Choose a reason for hiding this comment

saikishor commented Dec 10, 2024

mergify bot commented Dec 20, 2024

MarqRazz left a comment

Choose a reason for hiding this comment

saikishor commented Dec 20, 2024

saikishor commented Dec 20, 2024

destogl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2024 •

edited

Loading

christophfroehlich left a comment •

edited

Loading