Use parameter yaml on startup for configuring greenwave monitor + rename to greenwave diagnostics #22

bmchalenv · 2026-01-13T05:47:23Z

Add support for ROS parameter YAML on startup and rename message diagnostics to greenwave diagnostics to match parameter yaml group name. Look at the example.yaml if you want to see how it is integrated.

Fixes a few bugs/makes the system robust also. Specifically #16 .

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps · 2026-01-13T05:53:05Z

Greptile Summary

This PR adds YAML-based parameter configuration for the greenwave monitor and renames message_diagnostics to greenwave_diagnostics throughout the codebase to better align with the parameter namespace.

Key Changes:

Parameter Loading: Topics can now be configured via YAML with expected_frequency and tolerance parameters under the greenwave_diagnostics namespace
Deferred Initialization: Topic discovery is now deferred 100ms after node construction to allow the ROS graph to settle, improving reliability when topics don't exist immediately
Destructor Cleanup: Both GreenwaveMonitor and MinimalPublisher now have explicit destructors that cancel timers and clear diagnostics before the base Node destructor runs, fixing issue Greenwave monitor sometimes exits with return code -11, causes flaky test. #16 (exit code -11 crash)
Namespace Rename: Complete rename from message_diagnostics to greenwave_diagnostics for consistency with the parameter group name
Improved Topic Discovery: Changed from get_topic_names_and_types() to get_publishers_info_by_topic() for more reliable type discovery with configurable retries

Bug Fixes:

Fixes Greenwave monitor sometimes exits with return code -11, causes flaky test. #16: The explicit destructors prevent accessing invalid node state during shutdown by canceling timers and clearing resources in the correct order
Negative tolerance values are now clamped to 0.0 with a warning
Topics with invalid expected_frequency (≤ 0) are monitored but without frequency validation settings
Empty topic strings from the default parameter value are now skipped

Testing:

The PR includes comprehensive test coverage for YAML parameter loading, including validation of integer parameter conversion, negative parameter handling, and proper monitoring behavior for various configuration scenarios.

Confidence Score: 4/5

This PR is safe to merge with careful attention to the destructor implementation
The PR successfully addresses issue Greenwave monitor sometimes exits with return code -11, causes flaky test. #16 with explicit destructors and adds valuable YAML configuration support. The implementation is well-tested with comprehensive test coverage. Score is 4 rather than 5 due to the complexity of the destructor cleanup order and the init_timer_ self-cancellation pattern, which while likely safe, could benefit from additional stress testing under rapid shutdown scenarios
greenwave_monitor/src/greenwave_monitor.cpp - Pay attention to the destructor cleanup order and init_timer_ callback self-cancellation pattern

Important Files Changed

Filename	Overview
greenwave_monitor/src/greenwave_monitor.cpp	Adds YAML parameter loading, deferred initialization, and destructor cleanup to fix exit code -11 crash
greenwave_monitor/include/greenwave_monitor.hpp	Adds destructor, deferred_init method, and updates function signatures for parameter loading
greenwave_monitor/include/minimal_publisher_node.hpp	Adds destructor with timer cancellation and diagnostics cleanup
greenwave_monitor/test/test_greenwave_monitor.py	Adds YAML parameter loading tests and expands test coverage for configuration scenarios

Sequence Diagram

sequenceDiagram
    participant User
    participant ROS2
    participant GreenwaveMonitor
    participant InitTimer
    participant TopicDiscovery
    participant Diagnostics

    User->>ROS2: Launch with YAML config
    ROS2->>GreenwaveMonitor: Constructor(options)
    GreenwaveMonitor->>GreenwaveMonitor: allow_undeclared_parameters(true)
    GreenwaveMonitor->>GreenwaveMonitor: automatically_declare_parameters_from_overrides(true)
    GreenwaveMonitor->>GreenwaveMonitor: declare_parameter("topics", {""})
    GreenwaveMonitor->>GreenwaveMonitor: create_wall_timer(1s, timer_callback)
    GreenwaveMonitor->>InitTimer: create_wall_timer(100ms, deferred_init)
    Note over InitTimer: Wait 100ms for ROS graph to settle
    
    InitTimer->>GreenwaveMonitor: deferred_init()
    GreenwaveMonitor->>GreenwaveMonitor: add_topics_from_parameters()
    GreenwaveMonitor->>GreenwaveMonitor: list_parameters("greenwave_diagnostics", 10)
    GreenwaveMonitor->>GreenwaveMonitor: Parse topic names from params
    GreenwaveMonitor->>GreenwaveMonitor: get_parameter("topics")
    
    loop For each topic
        GreenwaveMonitor->>TopicDiscovery: find_topic_type(topic, retries=5, wait=0.5s)
        TopicDiscovery->>ROS2: get_publishers_info_by_topic(topic)
        ROS2-->>TopicDiscovery: publisher info
        TopicDiscovery-->>GreenwaveMonitor: topic type
        GreenwaveMonitor->>GreenwaveMonitor: create_subscription(topic)
        GreenwaveMonitor->>Diagnostics: create GreenwaveDiagnostics
        
        alt Has expected_frequency param
            GreenwaveMonitor->>GreenwaveMonitor: get_double_param(freq_param)
            GreenwaveMonitor->>GreenwaveMonitor: get_double_param(tol_param)
            alt expected_frequency > 0
                GreenwaveMonitor->>Diagnostics: setExpectedDt(frequency, tolerance)
            else expected_frequency <= 0
                GreenwaveMonitor->>GreenwaveMonitor: WARN: skipping parameter settings
            end
        end
    end
    
    GreenwaveMonitor->>GreenwaveMonitor: create_service(manage_topic)
    GreenwaveMonitor->>GreenwaveMonitor: create_service(set_expected_frequency)
    
    loop Every 1 second
        GreenwaveMonitor->>Diagnostics: publishDiagnostics()
        Diagnostics->>ROS2: publish to /diagnostics
    end
    
    User->>ROS2: Shutdown (SIGINT)
    ROS2->>GreenwaveMonitor: ~GreenwaveMonitor()
    GreenwaveMonitor->>GreenwaveMonitor: timer_->cancel()
    GreenwaveMonitor->>GreenwaveMonitor: init_timer_->cancel()
    GreenwaveMonitor->>Diagnostics: greenwave_diagnostics_.clear()
    GreenwaveMonitor->>GreenwaveMonitor: subscriptions_.clear()
    Note over GreenwaveMonitor: Clean shutdown prevents exit code -11

greptile-apps

_{5 files reviewed, 5 comments}

_{Edit Code Review Agent Settings | Greptile}

greenwave_monitor/greenwave_monitor/ui_adaptor.py

greenwave_monitor/src/greenwave_monitor.cpp

greenwave_monitor/test/test_greenwave_monitor.py

greenwave_monitor/test/test_topic_monitoring_integration.py

greenwave_monitor/src/greenwave_monitor.cpp

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps

_{5 files reviewed, 5 comments}

_{Edit Code Review Agent Settings | Greptile}

greenwave_monitor/src/greenwave_monitor.cpp

greenwave_monitor/greenwave_monitor/ui_adaptor.py

greenwave_monitor/greenwave_monitor/ncurses_frontend.py

greenwave_monitor/config/example.yaml

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps

_{5 files reviewed, 5 comments}

_{Edit Code Review Agent Settings | Greptile}

greenwave_monitor/src/greenwave_monitor.cpp

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greenwave_monitor/src/greenwave_monitor.cpp

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greenwave_monitor/src/greenwave_monitor.cpp

sgillen · 2026-01-13T23:18:21Z

This also closes #19

sgillen · 2026-01-13T22:10:57Z

greenwave_monitor/config/example.yaml

+        expected_frequency: 100.0
+        tolerance: 10.0
+      /image_topic:
+        expected_frequency: 0.0  # Invalid parameter settings are ignored


Let's elaborate - ignored and we use the default "report topic frequency but don't check it" behavior

I think we should include the string topic here as well? Also with expected_frequency = 0?

Nit: We're missing the copyright header here.

I left out the string topic on purpose so that it gets picked up by the topics:= parameter. I still think that parameter is useful for just starting the monitor without frequency and tolerance set. Wanted to make sure there was still an example.

I moved topics:= into the YAML so that users can reference it.

sgillen · 2026-01-13T22:15:04Z

greenwave_monitor/examples/example.launch.py

            executable='greenwave_monitor',
            name='greenwave_monitor',
            output='log',
-            parameters=[


I vote we have config file contain all topics, and leave a commented out version showing how to specify topic inline like before.

Let me know what you think about the new yaml. I like the idea of leaving a commented out version.

greenwave_monitor/include/greenwave_diagnostics.hpp

sgillen · 2026-01-13T23:42:07Z

greenwave_monitor/src/greenwave_monitor.cpp

+    }
+
+    std::string topic_name = remainder.substr(0, last_dot);
+    if (!topic_name.empty() && topic_name[0] == '/') {


So we are basically limiting ourselves to globally namespaces topics in the params? if we relax that condition does the topic resolution handle namespaces gracefully? or is it easier to just have this restriction?

It could work, I was trying to avoid complexity by not testing/implementing that in this MR. I'll try it out.

Nvm I recall why I didn't, it adds more complexity to all of our logic. We'd need to support it in add topic and greenwave diagnostics. There would have to be some form of discovery or way to find the namespace. For instance, "/camera0/image_raw" and "/camera1/image_raw", how do we tell which one to match the namespace too? I thought it was too much to handle for this MR and even my previous MR. My opinion is that should be done separately since it strays strongly from the point here.

sgillen · 2026-01-13T23:44:18Z

greenwave_monitor/src/greenwave_monitor.cpp

+      if (expected_frequency > 0.0) {
+        greenwave_diagnostics_[topic]->setExpectedDt(expected_frequency, tolerance);
+      } else {
+        RCLCPP_WARN(


I wonder if this should be RCLCPP_DEBUG instead? I can imagine cases where this just causes annoying console spam.

Also isn't expected_frequency == 0.0 expected behavior?

expected_frequency == 0.0 is bad, we reject it in set expected frequency service so I reject it here. Was trying to keep our logic the same across the board. If we want to allow a topic to be specified as 0 frequency, I'm fine with doing that.

Theoretically the WARN message should never appear unless the user has misconfigured their YAML.

If they want to not specify expected frequency they should use topics:= instead. Another alternative is to add back the enabled flag, that was part of the reason I had that parameter in the more complex parameter MR.

greenwave_monitor/src/greenwave_monitor.cpp

greenwave_monitor/test/test_greenwave_monitor.py

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps

_{15 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-14T04:18:16Z

greenwave_monitor/src/greenwave_monitor.cpp

+  init_timer_ = this->create_wall_timer(
+    100ms, [this]() {
+      init_timer_->cancel();
+      deferred_init();
+    });


logic: timer cancels itself then accesses this

the init_timer_ callback cancels itself on line 58, then immediately calls deferred_init() which accesses member variables. if the node is being destroyed during this callback, the timer cancellation in the destructor (line 48-50 in the header) may not prevent the callback from continuing to execute deferred_init(), potentially accessing destroyed node state

Suggested change

init_timer_ = this->create_wall_timer(

100ms, [this]() {

init_timer_->cancel();

deferred_init();

});

init_timer_ = this->create_wall_timer(

100ms, [this, timer_weak = std::weak_ptr<rclcpp::TimerBase>()]() mutable {

if (auto timer = timer_weak.lock()) {

timer->cancel();

deferred_init();

}

});

std::static_pointer_cast<rclcpp::WallTimer<std::function<void()>>>(init_timer_)->get_timer_handle();

bmchalenv added 4 commits January 12, 2026 19:49

Add support for loading greenwave monitor parameters from a list

b0f0ac5

Signed-off-by: Blake McHale <bmchale@nvidia.com>

Rename message diagnostics to greenwave diagnostics

7815ede

Signed-off-by: Blake McHale <bmchale@nvidia.com>

Fix linting

63f6661

Signed-off-by: Blake McHale <bmchale@nvidia.com>

Add tests

8e5e61c

Signed-off-by: Blake McHale <bmchale@nvidia.com>

bmchalenv changed the title ~~Use parameter yaml on startup for configuring greenwave monitor~~ Use parameter yaml on startup for configuring greenwave monitor + rename to greenwave diagnostics Jan 13, 2026

greptile-apps bot reviewed Jan 13, 2026

View reviewed changes

More tests

6368e9a

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps bot reviewed Jan 13, 2026

View reviewed changes

More tests and lint

c97ffc7

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps bot reviewed Jan 13, 2026

View reviewed changes

Add retry back to find topic

b5fd513

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps bot reviewed Jan 13, 2026

View reviewed changes

greenwave_monitor/src/greenwave_monitor.cpp Show resolved Hide resolved

Add catch for when context becomes invalid

4ad2656

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps bot reviewed Jan 13, 2026

View reviewed changes

greenwave_monitor/src/greenwave_monitor.cpp Show resolved Hide resolved

sgillen reviewed Jan 13, 2026

View reviewed changes

bmchalenv added 3 commits January 13, 2026 18:18

Add fixes for comments

e461081

Signed-off-by: Blake McHale <bmchale@nvidia.com>

Add an or negative

fc769b6

Signed-off-by: Blake McHale <bmchale@nvidia.com>

Fix lint

943b6a1

Signed-off-by: Blake McHale <bmchale@nvidia.com>

greptile-apps bot reviewed Jan 14, 2026

View reviewed changes

Use parameter yaml on startup for configuring greenwave monitor + rename to greenwave diagnostics #22

Are you sure you want to change the base?

Use parameter yaml on startup for configuring greenwave monitor + rename to greenwave diagnostics #22

Conversation

bmchalenv commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sgillen commented Jan 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmchalenv commented Jan 13, 2026 •

edited

Loading

greptile-apps bot commented Jan 13, 2026 •

edited

Loading