Skip to content

Make diagnostic aggregator composable#435

Open
nnarain wants to merge 3 commits into
ros:ros2from
nnarain:issue321-composable
Open

Make diagnostic aggregator composable#435
nnarain wants to merge 3 commits into
ros:ros2from
nnarain:issue321-composable

Conversation

@nnarain
Copy link
Copy Markdown

@nnarain nnarain commented Feb 2, 2025

Heya.

This PR makes the diagnostic_aggregator node composable (see #321 )

Summary of changes:

  • Have the Aggregator class implement get_node_base_interface so it can be registered as a component
  • Register the aggregator as a rclcpp_component
  • analyzer_group.cpp is moved to the analyzers library
    • The plugins xml file incorrectly lists the AnalyzerGroup plugin as existing the analyzers library. When composing the nodes I found that AnalyzerGroup would not load. I assume it has worked up to this point because everything was linked into the same executable.
    • Consequently this means the top level analyzer group is loaded as a plugin and therefore requires a class loader
  • Added compose example launch file.

@ct2034 ct2034 added enhancement This tackles a new feature of the code (and not a bug) ros2 PR tackling a ROS2 branch labels Feb 10, 2025
@frozenreboot
Copy link
Copy Markdown

Hi @nnarain,

Is this PR still being worked on?
If you are busy, I’d be happy to pick it up, resolve the merge conflicts, and submit a new PR referencing your work.
Please let me know!

@nnarain
Copy link
Copy Markdown
Author

nnarain commented Jan 3, 2026

hey @frozenreboot

Ya I should be able to get back to this. If I don't find the time this week I'll give you a ping.

Hey @ct2034 any initial feedback on this PR?

* the aggregator will report the error and publish it in the aggregated output.
*/
class Aggregator
class Aggregator : public rclcpp::Node
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not required for composable nodes.
Franky: I would advise against it. It's hard if you want this class as a member of another class.

This pr would be much smaller if you kept the node member and the logger member.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fair point on smaller PR

@nnarain nnarain force-pushed the issue321-composable branch from 7ea3c1d to 933726f Compare January 4, 2026 02:50
@nnarain nnarain force-pushed the issue321-composable branch from 933726f to 7e891e0 Compare January 4, 2026 17:53
Copy link
Copy Markdown
Collaborator

@ct2034 ct2034 left a comment

Choose a reason for hiding this comment

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

Thanks. This looks very good.
Could I please ask you to provide a small launch test. It could be based in the nice example diagnostic_aggregator/example/compose_example.launch.py.in.
And please also add a small section on this to the diagnostic_aggregator/README.md

@ct2034 ct2034 added the needs more work Someone has worked on this but more work is needed label May 21, 2026
@nnarain
Copy link
Copy Markdown
Author

nnarain commented May 21, 2026

Will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement This tackles a new feature of the code (and not a bug) needs more work Someone has worked on this but more work is needed ros2 PR tackling a ROS2 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants