Add controlling topic expected frequencies and tolerances through ROS 2 parameters #21
Conversation
Greptile SummaryThis PR implements ROS 2 parameter-based configuration for topic monitoring, allowing users to specify expected frequencies and tolerances through parameter files or dynamic parameter commands instead of only through service calls. Key Changes
Implementation QualityThe implementation demonstrates careful attention to edge cases and proper ROS 2 parameter lifecycle management. Parameter validation prevents invalid states, and the separation of internal state updates from parameter updates (via the Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant LaunchFile
participant MonitorNode as GreenwaveMonitor
participant ParamServer as Parameter Server
participant UIAdaptor
participant Diagnostics as Message Diagnostics
Note over User,Diagnostics: Startup Flow with Parameter Configuration
User->>LaunchFile: Launch with parameter file
LaunchFile->>MonitorNode: Start with topics.<topic>.expected_frequency<br/>and topics.<topic>.tolerance
MonitorNode->>MonitorNode: Constructor initializes with<br/>allow_undeclared_parameters=true
MonitorNode->>ParamServer: Automatically declare parameters from overrides
MonitorNode->>MonitorNode: Register on_parameter_change callback
MonitorNode->>ParamServer: Subscribe to /parameter_events
MonitorNode->>MonitorNode: load_topic_parameters_from_overrides()
MonitorNode->>ParamServer: list_parameters() and get values
loop For each topic with frequency parameter
MonitorNode->>Diagnostics: add_topic(topic)
MonitorNode->>Diagnostics: setExpectedDt(freq, tolerance)
end
User->>UIAdaptor: Launch ncurses dashboard
UIAdaptor->>ParamServer: create_timer for initial params fetch
UIAdaptor->>ParamServer: list_parameters() when services ready
ParamServer-->>UIAdaptor: Parameter names
UIAdaptor->>ParamServer: get_parameters(topic param names)
ParamServer-->>UIAdaptor: Parameter values
UIAdaptor->>UIAdaptor: Parse and store expected_frequencies
UIAdaptor->>ParamServer: Subscribe to /parameter_events
Note over User,Diagnostics: Dynamic Parameter Update Flow
User->>ParamServer: ros2 param set topics./imu.expected_frequency 100
ParamServer->>MonitorNode: on_parameter_change callback
MonitorNode->>MonitorNode: Check updating_params_internally_ flag
MonitorNode->>MonitorNode: Validate parameter value (>0 for freq, >=0 for tol)
alt Valid parameter
MonitorNode->>MonitorNode: parse_topic_param_name()
MonitorNode->>MonitorNode: set_topic_expected_frequency()
MonitorNode->>Diagnostics: setExpectedDt(freq, tolerance)
MonitorNode-->>ParamServer: Return success
else Invalid parameter
MonitorNode-->>ParamServer: Return error with reason
end
ParamServer->>UIAdaptor: ParameterEvent message on /parameter_events
UIAdaptor->>UIAdaptor: Parse parameter name and value
UIAdaptor->>UIAdaptor: Update expected_frequencies map
Note over User,Diagnostics: Parameter Deletion Flow
User->>ParamServer: ros2 param delete topics./imu.expected_frequency
ParamServer->>MonitorNode: ParameterEvent with deleted_parameters
MonitorNode->>MonitorNode: on_parameter_event callback
MonitorNode->>Diagnostics: clearExpectedDt()
ParamServer->>UIAdaptor: ParameterEvent on /parameter_events
UIAdaptor->>UIAdaptor: Remove from expected_frequencies map
Note over User,Diagnostics: Service Call with Parameter Sync
User->>MonitorNode: set_expected_frequency service call
MonitorNode->>MonitorNode: handle_set_expected_frequency()
MonitorNode->>MonitorNode: set_topic_expected_frequency(update_parameters=true)
MonitorNode->>MonitorNode: Set updating_params_internally_ = true
MonitorNode->>Diagnostics: setExpectedDt(freq, tolerance)
MonitorNode->>ParamServer: declare_or_set_parameter() for freq and tol
MonitorNode->>MonitorNode: Set updating_params_internally_ = false
ParamServer->>UIAdaptor: ParameterEvent on /parameter_events
UIAdaptor->>UIAdaptor: Update expected_frequencies map
|
bmchalenv
force-pushed
the
topics_parameters
branch
from
6ddeeab to
4a3f4d3
Compare
… 2 parameters Signed-off-by: Blake McHale <bmchale@nvidia.com>
…y time Signed-off-by: Blake McHale <bmchale@nvidia.com>
bmchalenv
force-pushed
the
topics_parameters
branch
from
0caeff7 to
596cc53
Compare
…cing with parameters Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
greenwave_monitor/src/greenwave_monitor.cpp, line 282-286 (link)logic: If
declare_or_set_parameter()throws an exception,updating_params_internally_remainstrue, potentially blocking future parameter updates.
14 files reviewed, 1 comment
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Additional Comments (5)
-
greenwave_monitor/CMakeLists.txt, line 121 (link)style: using
file(GLOB ...)can cause build issues if test files are added/removed without re-running CMake configure stepNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
greenwave_monitor/src/greenwave_monitor.cpp, line 282-291 (link)logic: Exception safety issue: if parameter operations throw after setting
updating_params_internally_ = true, the flag remains true permanently, blocking future parameter updates -
greenwave_monitor/greenwave_monitor/test_utils.py, line 153-154 (link)logic: Setting empty string as topic when topics list is empty could cause issues. Is this intended behavior when an empty topics list is provided - should it default to an empty string topic or skip the topics parameter entirely?
-
greenwave_monitor/greenwave_monitor/test_utils.py, line 76-78 (link)style: Service availability timeout is hardcoded to 5.0s while function parameter allows configuring timeout_sec for the overall operation
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
greenwave_monitor/include/greenwave_monitor.hpp, line 83 (link)syntax: Typo in comment: 'on_paramter_change' should be 'on_parameter_change'
14 files reviewed, 5 comments
There was a problem hiding this comment.
Additional Comments (1)
-
greenwave_monitor/greenwave_monitor/ui_adaptor.py, line 326-327 (link)logic: substring match could incorrectly process events from unrelated nodes
14 files reviewed, 1 comment
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Additional Comments (1)
-
greenwave_monitor/src/greenwave_monitor.cpp, line 283-292 (link)style: The
updating_params_internally_flag lacks thread-safety mechanisms (atomic or mutex). If this node runs with a multi-threaded executor, race conditions could occur:- Thread A sets flag to true (line 283)
- Thread B checks flag (line 308) before Thread A completes
- Both threads could process parameters simultaneously
ROS 2 uses single-threaded executors by default, so this is safe for typical usage. However, consider using
std::atomic<bool>for robustness if multi-threaded executors might be used.
14 files reviewed, 1 comment
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Additional Comments (1)
-
greenwave_monitor/test/parameters/test_param_tol_only.py, line 82 (link)style: requesting expected_count=1 when expecting 0 results - this parameter should be 0 for clearer intent
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
14 files reviewed, 1 comment
sgillen
left a comment
sgillen
left a comment
There was a problem hiding this comment.
Did a cursory review, but as we discussed offline, the plan will be to
- remove the services entirely and use params for everything
- move all the parameter logic to message_diagnostics.hpp (which we should rename greenwave_diagnostics.hpp btw)
(edited)
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Test: only expected_frequency specified, tolerance defaults to 5%.""" |
There was a problem hiding this comment.
Let's label the test files + label strings more clearly since params are dynamic now, maybe startup_params when that is what you are testing.
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR successfully migrates from a service-based approach to a ROS 2 parameter-based approach for configuring topic monitoring frequencies and tolerances. The implementation is comprehensive and well-tested.
Key Changes
- Removed
SetExpectedFrequencyservice interface in favor of ROS 2 parameters - Parameters use
greenwave_diagnostics.<topic_name>.expected_frequencyandgreenwave_diagnostics.<topic_name>.tolerancenaming convention - Added event-driven parameter callbacks in
GreenwaveDiagnosticsclass for dynamic updates - Enhanced
GreenwaveUiAdaptorto query and track parameters from all nodes at startup - Added
get_topics_from_parameters()to discover topics from parameter namespace - Comprehensive test coverage with 8 new parameter test files covering YAML config, dynamic updates, validation, and edge cases
Critical Issue Found
Documentation contains incorrect parameter names - docs/images/SERVICES.md documents the parameter prefix as topics. but the actual implementation uses greenwave_diagnostics. - this will cause all user commands in the documentation to fail. This must be fixed before merge.
Architecture Benefits
- More idiomatic ROS 2 approach using standard parameter system
- Parameters can be set via YAML files, launch files, or
ros2 param setcommands - Parameters persist across node restarts when configured via launch/YAML
- Dynamic reconfiguration without service calls
- Integration with existing publisher nodes that have built-in
GreenwaveDiagnostics
Confidence Score: 3/5
- This PR has significant documentation errors that will break user workflows, but the code implementation itself is solid
- Score reflects critical documentation issues that must be fixed: all parameter examples use wrong prefix (
topics.instead ofgreenwave_diagnostics.). The C++ and Python implementations are well-architected with proper thread safety, comprehensive test coverage (8 new test files), and clean integration. A minor typo exists in a log message. Once documentation is corrected, this would be a 4/5. docs/images/SERVICES.mdrequires immediate correction - all parameter names are wrong and will cause user commands to fail
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| docs/images/SERVICES.md | 2/5 | Critical documentation errors: parameter prefix is greenwave_diagnostics. not topics. - all examples will fail if users follow them |
| greenwave_monitor/include/greenwave_diagnostics.hpp | 4/5 | Major refactor adding parameter-based frequency configuration with event-driven updates; minor typo in log message |
| greenwave_monitor/src/greenwave_monitor.cpp | 4/5 | Implements parameter discovery at startup and integrates with GreenwaveDiagnostics parameter system; clean implementation |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 4/5 | Adds comprehensive parameter event handling and initial parameter fetching from all nodes; well-structured async queries |
| greenwave_monitor/test/parameters/test_param_dynamic.py | 5/5 | Comprehensive test coverage for dynamic parameter changes including edge cases and validation |
Sequence Diagram
sequenceDiagram
participant User
participant LaunchFile
participant MonitorNode
participant GreenwaveDiagnostics
participant PublisherNode
participant ParameterServer
Note over User,ParameterServer: Scenario 1: Configuration via YAML at Launch
User->>LaunchFile: Launch with YAML config
LaunchFile->>MonitorNode: Start with parameters file
MonitorNode->>ParameterServer: declare_parameters_from_overrides()
ParameterServer-->>MonitorNode: Parameters declared
MonitorNode->>MonitorNode: get_topics_from_parameters()
MonitorNode->>ParameterServer: list_parameters("greenwave_diagnostics")
ParameterServer-->>MonitorNode: [greenwave_diagnostics./topic.expected_frequency, ...]
MonitorNode->>MonitorNode: add_topic() for each discovered topic
MonitorNode->>GreenwaveDiagnostics: new GreenwaveDiagnostics(topic_name, config)
GreenwaveDiagnostics->>ParameterServer: declare_parameter(freq_param_name)
GreenwaveDiagnostics->>ParameterServer: set_parameter(freq) [triggers callback]
GreenwaveDiagnostics->>GreenwaveDiagnostics: onParameterEvent() -> setExpectedDt()
Note over GreenwaveDiagnostics: Diagnostics now active with expected frequency
Note over User,ParameterServer: Scenario 2: Dynamic Parameter Update
User->>ParameterServer: ros2 param set greenwave_diagnostics./topic.expected_frequency 30.0
ParameterServer->>GreenwaveDiagnostics: onParameterChange() [validation]
GreenwaveDiagnostics-->>ParameterServer: SetParametersResult{successful=true}
ParameterServer->>GreenwaveDiagnostics: /parameter_events [ParameterEvent msg]
GreenwaveDiagnostics->>GreenwaveDiagnostics: onParameterEvent() -> setExpectedDt()
Note over GreenwaveDiagnostics: Expected frequency updated, diagnostics reconfigured
Note over User,ParameterServer: Scenario 3: Publisher Node with Built-in Diagnostics
User->>LaunchFile: Launch publisher with greenwave_diagnostics params
LaunchFile->>PublisherNode: Start with greenwave_diagnostics./topic.* params
PublisherNode->>GreenwaveDiagnostics: new GreenwaveDiagnostics(topic_name)
GreenwaveDiagnostics->>ParameterServer: declare parameters & subscribe to events
User->>MonitorNode: Call manage_topic service to add topic
MonitorNode->>MonitorNode: get_publishers_info_by_topic()
MonitorNode->>PublisherNode: Check for greenwave_diagnostics./topic.enabled param
MonitorNode->>ParameterServer: set_parameter(enabled=true) on PublisherNode
ParameterServer->>PublisherNode: Parameter update event
PublisherNode->>GreenwaveDiagnostics: onParameterEvent() -> enabled=true
Note over PublisherNode: Publisher now publishes diagnostics to /diagnostics
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Overview
This PR refactors the greenwave_monitor from a service-based API to a parameter-based API for controlling topic monitoring and expected frequencies. The changes replace ManageTopic and SetExpectedFrequency services with ROS 2 parameters following the pattern greenwave_diagnostics.<topic>.(enabled|expected_frequency|tolerance).
Key Changes
Architecture Shift: Moved from service calls to parameter-based configuration, aligning with ROS 2 best practices and enabling declarative configuration via YAML files.
New Components:
greenwave_diagnostics.hpp: New header-only library for per-topic diagnostics with parameter callbacks- Parameter naming:
greenwave_diagnostics.<topic>.enabled|expected_frequency|tolerance - External node tracking to prevent duplicate monitoring across nodes
API Changes:
- Removed: Service interfaces
ManageTopic.srvandSetExpectedFrequency.srv - Added: Parameter-based control with validation callbacks
- Enhanced: YAML configuration support with nested parameter syntax
Issues Found
Critical (Must Fix Before Merge)
- Missing include:
greenwave_diagnostics.hppusesrcpputils::join()without including the header - will cause compilation failure - Missing dependency:
package.xmldoesn't declarercpputilsdependency - will cause build failure
Non-Critical Issues
- Typo: "Iniital" should be "Initial" in log message (line 743 of greenwave_diagnostics.hpp)
- Logic inconsistency:
ui_adaptor.pycomment says NaN frequencies are "cleared" but they're actually stored in the dictionary (lines 489-492) - Style improvement: Parameter validation could use
continuestatements for cleaner logic flow (lines 624-636 of greenwave_diagnostics.hpp)
Testing
The PR includes comprehensive test coverage:
test_parameters.py: 9 test classes covering startup, YAML config, dynamic parameters, and multi-topic scenariostest_greenwave_diagnostics.cpp: Unit tests for C++ diagnostics functionality- Integration tests validate parameter synchronization across nodes
Positive Aspects
- Well-structured refactoring with clear separation of concerns
- Excellent test coverage demonstrating various configuration scenarios
- Documentation updated to reflect new API
- Backwards compatibility maintained via existing
topicsparameter list - Proper mutex protection for thread-safe parameter updates
Confidence Score: 3/5
- This PR has critical build issues that must be fixed before merging, but the core logic is sound
- Score of 3 reflects that while the refactoring is well-designed with good test coverage, there are two critical issues (missing include and dependency) that will cause immediate build failures. Once these are fixed, the PR should be safe to merge. The non-critical issues are style/consistency concerns that don't affect functionality.
- Pay close attention to greenwave_monitor/include/greenwave_diagnostics.hpp and greenwave_monitor/package.xml - both need updates to fix critical build issues
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 3/5 | New header-only diagnostics class with parameter-based configuration. Contains critical missing include for rcpputils::join and typo in log message. Parameter validation logic works but could be cleaner. |
| greenwave_monitor/package.xml | 2/5 | Missing rcpputils dependency required by greenwave_diagnostics.hpp. This will cause build failures. |
| greenwave_monitor/src/greenwave_monitor.cpp | 4/5 | Refactored from service-based to parameter-based topic management. Replaces ManageTopic and SetExpectedFrequency services with parameter events. Logic appears sound with proper validation and external node tracking. |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 3/5 | Python UI adapter for parameter-based diagnostics. Contains logic inconsistency where NaN frequency values are stored rather than cleared despite comment suggesting otherwise. |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive test suite for parameter-based configuration covering startup behavior, multiple topics, YAML config, and dynamic parameter changes. Well-structured tests. |
Sequence Diagram
sequenceDiagram
participant User
participant ROS2_Param_System as ROS 2 Parameter System
participant GreenwaveMonitor
participant GreenwaveDiagnostics
participant Publisher_Node as Publisher Node
Note over User,Publisher_Node: Startup: Topic Configuration from YAML/Launch
User->>GreenwaveMonitor: Launch with parameters<br/>(greenwave_diagnostics./topic.enabled: true)
GreenwaveMonitor->>GreenwaveMonitor: fetch_external_topic_map()
GreenwaveMonitor->>Publisher_Node: Query parameters via SyncParametersClient
Publisher_Node-->>GreenwaveMonitor: Return topic parameters
GreenwaveMonitor->>GreenwaveMonitor: get_topics_from_parameters()
GreenwaveMonitor->>GreenwaveMonitor: add_topic() for each configured topic
GreenwaveMonitor->>GreenwaveDiagnostics: Create diagnostics object
GreenwaveDiagnostics->>ROS2_Param_System: Declare/set freq, tol, enabled parameters
GreenwaveDiagnostics->>ROS2_Param_System: Subscribe to /parameter_events
Note over User,Publisher_Node: Runtime: Enable Monitoring for New Topic
User->>ROS2_Param_System: Set greenwave_diagnostics./new_topic.enabled = true
ROS2_Param_System->>GreenwaveMonitor: on_parameter_change() (validation)
GreenwaveMonitor->>GreenwaveMonitor: validate_add_topic()<br/>(check publishers, external nodes)
GreenwaveMonitor-->>ROS2_Param_System: Return validation result
ROS2_Param_System->>GreenwaveMonitor: ParameterEvent (new_parameters)
GreenwaveMonitor->>GreenwaveMonitor: internal_on_parameter_event()
GreenwaveMonitor->>GreenwaveMonitor: execute_add_topic()
GreenwaveMonitor->>GreenwaveDiagnostics: Create diagnostics object
Note over User,Publisher_Node: Runtime: Set Expected Frequency
User->>ROS2_Param_System: Set greenwave_diagnostics./topic.expected_frequency = 30.0
ROS2_Param_System->>GreenwaveDiagnostics: onParameterChange() (validation)
GreenwaveDiagnostics->>GreenwaveDiagnostics: Validate freq > 0 or NaN
GreenwaveDiagnostics-->>ROS2_Param_System: Return validation result
ROS2_Param_System->>GreenwaveDiagnostics: ParameterEvent (changed_parameters)
GreenwaveDiagnostics->>GreenwaveDiagnostics: onParameterEvent()<br/>setExpectedDt()
GreenwaveDiagnostics->>GreenwaveDiagnostics: Enable time diagnostics
Note over User,Publisher_Node: Runtime: Message Flow & Diagnostics
Publisher_Node->>GreenwaveMonitor: Publish message on /topic
GreenwaveMonitor->>GreenwaveDiagnostics: updateDiagnostics(timestamp)
GreenwaveDiagnostics->>GreenwaveDiagnostics: Check frame rate vs expected
GreenwaveMonitor->>GreenwaveDiagnostics: publishDiagnostics() (timer callback)
GreenwaveDiagnostics->>ROS2_Param_System: Publish to /diagnostics topic
Additional Comments (1)
|
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds ROS 2 parameter-based configuration for topic monitoring, enabling users to specify expected frequencies and tolerances via parameters instead of only service calls. The implementation supports both flat (topics./my_topic.expected_frequency) and nested YAML (topics: { /my_topic: { expected_frequency: 100 } }) parameter formats. The ncurses dashboard integrates with the new parameter system, reading initial configurations and responding to dynamic updates. Most of the implementation is solid with comprehensive test coverage.
Confidence Score: 1/5
- High risk due to logic bug that stores invalid NaN frequency values
- The PR contains a critical logic error in
ui_adaptor.pyline 491 where the conditionif value > 0 or math.isnan(value)will store NaN frequencies in the cache instead of clearing them as intended by the comment. This bug causes the expected_frequencies dictionary to contain invalid (NaN, tolerance) entries, leading to incorrect behavior in UI display logic. While the C++ implementation appears correct, this Python bug affects the ncurses frontend's ability to properly track cleared frequency parameters. greenwave_monitor/greenwave_monitor/ui_adaptor.pyrequires immediate fix to line 491 logic bug
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 1/5 | Critical logic bug at line 491: NaN frequency values incorrectly stored in cache instead of being cleared |
| greenwave_monitor/src/greenwave_monitor.cpp | 3/5 | Adds parameter-based topic configuration; implements validation and external topic tracking correctly |
| greenwave_monitor/include/greenwave_diagnostics.hpp | 3/5 | Adds ROS 2 parameter support for frequency, tolerance, and enabled flags with validation |
Sequence Diagram
sequenceDiagram
participant User
participant YAML/Launch
participant MonitorNode
participant ParamServer
participant GreenwaveDiag
participant PublisherNode
Note over User,PublisherNode: Startup: Parameter Initialization
YAML/Launch->>MonitorNode: Load parameters (topics./imu.expected_frequency: 100)
MonitorNode->>MonitorNode: get_topics_from_parameters()
MonitorNode->>MonitorNode: validate_add_topic(/imu)
MonitorNode->>GreenwaveDiag: Create diagnostics instance
GreenwaveDiag->>ParamServer: Declare enabled/freq/tol parameters
GreenwaveDiag->>GreenwaveDiag: setExpectedDt(100, 5)
Note over User,PublisherNode: Runtime: Dynamic Parameter Update
User->>ParamServer: Set topics./camera.expected_frequency=30
ParamServer->>MonitorNode: on_parameter_change()
MonitorNode->>MonitorNode: validate_add_topic(/camera)
MonitorNode->>MonitorNode: Store in pending_validations_
ParamServer-->>MonitorNode: SetParametersResult (success)
ParamServer->>MonitorNode: ParameterEvent (new_parameters)
MonitorNode->>MonitorNode: execute_add_topic()
MonitorNode->>GreenwaveDiag: Create diagnostics for /camera
GreenwaveDiag->>GreenwaveDiag: setExpectedDt(30, 5)
Note over User,PublisherNode: External Node Conflict Prevention
PublisherNode->>ParamServer: Set topics./imu.enabled=true
ParamServer->>MonitorNode: ParameterEvent (external node)
MonitorNode->>MonitorNode: external_on_parameter_event()
MonitorNode->>MonitorNode: Store in external_topic_to_node_
User->>MonitorNode: Try to monitor /imu
MonitorNode->>MonitorNode: validate_add_topic(): Check external_topic_to_node_
MonitorNode-->>User: Error: Already monitored by PublisherNode
| if value > 0 or math.isnan(value): | ||
| self.expected_frequencies[topic_name] = (value, current[1]) |
There was a problem hiding this comment.
The condition if value > 0 or math.isnan(value) will store NaN values in expected_frequencies, causing invalid cache entries. The comment states "Treat NaN or non-positive as 'cleared'", but NaN values are stored rather than cleared.
Issue: When a frequency parameter is set to NaN (which signals clearing), line 492 executes and stores (NaN, current[1]) in the cache. This causes incorrect behavior in get_expected_frequency_str() (line 577) which checks if freq <= 0.0 or math.isnan(freq) and returns '-'.
Expected behavior: NaN should remove the entry from the cache, not store it.
| if value > 0 or math.isnan(value): | |
| self.expected_frequencies[topic_name] = (value, current[1]) | |
| if field == 'freq': | |
| # Treat NaN or non-positive as "cleared" | |
| if value > 0 and not math.isnan(value): | |
| self.expected_frequencies[topic_name] = (value, current[1]) | |
| elif math.isnan(value): | |
| self.expected_frequencies.pop(topic_name, None) |
| tol = config.get('tol', DEFAULT_TOLERANCE_PERCENT) | ||
| if freq > 0 and not math.isnan(freq): | ||
| self.expected_frequencies[topic_name] = (freq, tol) |
There was a problem hiding this comment.
The condition if freq > 0 and not math.isnan(freq) correctly validates frequency values before storing them. However, this logic should match the handling in _on_parameter_event() at line 491 where NaN values are incorrectly stored. Consider extracting this validation into a helper method to ensure consistency.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| self._pending_node_queries: Dict[str, dict] = {} | ||
|
|
||
| self.manage_topic_client = self.node.create_client( | ||
| ManageTopic, | ||
| manage_service_name | ||
| ) | ||
| # Query initial parameters after a short delay to let nodes come up | ||
| self._initial_params_timer = self.node.create_timer( | ||
| 2.0, self._fetch_initial_parameters_callback) |
There was a problem hiding this comment.
Verify that _pending_node_queries cleanup occurs even if async callbacks fail. Currently _cleanup_node_query() is called in exception handlers, but ensure all error paths are covered to prevent memory leaks from orphaned client references.
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Overview
This PR implements a significant architectural change to the Greenwave Monitor by replacing service-based topic management with a ROS 2 parameter-based approach. This makes the system more idiomatic to ROS 2 and enables easier configuration through YAML files and launch parameters.
Key Changes
1. Parameter-Based Topic Configuration
- Replaces
ManageTopicandSetExpectedFrequencyservices with ROS 2 parameters - Topics are now configured via parameters with naming convention:
greenwave_diagnostics.<topic_name>.(expected_frequency|tolerance|enabled) - Supports both flat parameter syntax (
"greenwave_diagnostics./topic.expected_frequency": 100) and nested YAML syntax - Parameters can be set at launch time, via YAML files, or dynamically at runtime
2. Core Component Refactoring
- greenwave_diagnostics.hpp: Renamed from
message_diagnostics.hpp, added comprehensive parameter handling with validation callbacks - greenwave_monitor.cpp: Refactored to use parameters instead of services, added external topic tracking to prevent conflicts
- ui_adaptor.py: New Python module providing clean API for UI frontends to interact with parameter-based monitoring
3. Test Coverage
- Added extensive test suite (
test_parameters.py) covering startup behavior, dynamic changes, YAML config, validation, and multi-topic scenarios - Enhanced C++ unit tests (
test_greenwave_diagnostics.cpp) for parameter validation and edge cases - Tests verify parameter rejection for invalid values (negative frequency, non-numeric types, etc.)
Critical Issue Found
Bug in external_on_parameter_event: The function does not process changed_parameters, only new_parameters and deleted_parameters. This means when an external node changes an enabled parameter from true to false, the external_topic_to_node_ map is not updated, potentially blocking other nodes from monitoring that topic.
Architecture Benefits
- More ROS 2 Idiomatic: Parameters are the standard ROS 2 way to configure node behavior
- Better Tooling Support: Can use standard
ros2 paramCLI and rqt_reconfigure - Declarative Configuration: YAML files provide clear, version-controlled configuration
- Dynamic Reconfiguration: Parameters can be changed at runtime without service calls
- Better Integration: UI tools can subscribe to parameter events for automatic updates
Backwards Compatibility
The PR removes the service-based API entirely. The old topics parameter (list of strings) still works for basic topic monitoring, but advanced features (frequency/tolerance) require the new parameter structure.
Confidence Score: 3/5
- This PR has significant architectural improvements but contains a logic bug that could cause operational issues in multi-node scenarios
- Score of 3/5 reflects: (1) A confirmed logic bug in external_on_parameter_event that fails to handle changed_parameters, which could block topic monitoring in production; (2) Otherwise well-designed parameter system with comprehensive validation and extensive test coverage; (3) Breaking API change that removes service-based interface entirely; (4) Good test coverage but the existing tests don't catch the external parameter event bug
- Pay close attention to greenwave_monitor/src/greenwave_monitor.cpp (lines 214-236) - the external_on_parameter_event function needs to be fixed to handle changed_parameters
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/src/greenwave_monitor.cpp | 3/5 | Major refactoring to use ROS parameters instead of services for topic management. Contains a logic bug in external parameter event handling that fails to process changed_parameters properly. |
| greenwave_monitor/include/greenwave_diagnostics.hpp | 4/5 | Renamed from message_diagnostics.hpp. Added comprehensive parameter handling for frequency, tolerance, and enabled state. Parameter validation and callbacks are well-implemented with proper mutex protection. |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 4/5 | New UI adapter for parameter-based topic monitoring. Handles parameter events comprehensively, maintains topic-to-node mapping, and provides clean API for frontends. Well-structured threading and locking. |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive test suite for parameter functionality covering startup behavior, multiple topics, YAML configuration, dynamic parameter changes, validation, and enable/disable functionality. |
| greenwave_monitor/test/test_greenwave_diagnostics.cpp | 5/5 | Extensive unit tests for greenwave_diagnostics including frame rate calculation, parameter validation, frequency/tolerance handling, and config impacts. Tests cover edge cases and boundary values thoroughly. |
Sequence Diagram
sequenceDiagram
participant User
participant GreenwaveMonitor
participant GreenwaveDiagnostics
participant PublisherNode
participant ParameterServer
Note over User,ParameterServer: Topic Monitoring Setup (Startup)
User->>GreenwaveMonitor: Launch with YAML config
GreenwaveMonitor->>GreenwaveMonitor: fetch_external_topic_map()
GreenwaveMonitor->>GreenwaveMonitor: get_topics_from_parameters()
loop For each configured topic
GreenwaveMonitor->>GreenwaveMonitor: validate_add_topic()
GreenwaveMonitor->>GreenwaveDiagnostics: Create instance
GreenwaveDiagnostics->>ParameterServer: Declare parameters (freq, tol, enabled)
GreenwaveDiagnostics->>ParameterServer: Subscribe to parameter events
GreenwaveMonitor->>GreenwaveMonitor: create_generic_subscription()
end
Note over User,ParameterServer: Runtime Parameter Change
User->>ParameterServer: Set greenwave_diagnostics./topic.expected_frequency
ParameterServer->>GreenwaveDiagnostics: Parameter change callback (validation)
GreenwaveDiagnostics-->>ParameterServer: Validation result
ParameterServer->>GreenwaveDiagnostics: Parameter event (if successful)
GreenwaveDiagnostics->>GreenwaveDiagnostics: setExpectedDt(freq, tolerance)
Note over User,ParameterServer: Enable/Disable Monitoring
User->>ParameterServer: Set greenwave_diagnostics./topic.enabled = true
ParameterServer->>GreenwaveMonitor: on_parameter_change (validation)
GreenwaveMonitor->>GreenwaveMonitor: validate_add_topic()
GreenwaveMonitor-->>ParameterServer: Validation result
ParameterServer->>GreenwaveMonitor: on_parameter_event (if successful)
GreenwaveMonitor->>GreenwaveMonitor: execute_add_topic()
GreenwaveMonitor->>GreenwaveDiagnostics: Create diagnostics instance
Note over User,ParameterServer: Message Flow
PublisherNode->>GreenwaveMonitor: Publish message on topic
GreenwaveMonitor->>GreenwaveDiagnostics: updateDiagnostics(timestamp)
GreenwaveDiagnostics->>GreenwaveDiagnostics: Check frequency/jitter/timestamps
GreenwaveMonitor->>GreenwaveDiagnostics: publishDiagnostics()
GreenwaveDiagnostics->>User: Publish to /diagnostics topic
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements ROS 2 parameter-based configuration for topic monitoring, replacing the previous service-based approach. Topics can now be configured using greenwave_diagnostics.<topic_name>.expected_frequency, greenwave_diagnostics.<topic_name>.tolerance, and greenwave_diagnostics.<topic_name>.enabled parameters.
Key architectural changes:
-
New
GreenwaveDiagnosticsclass: Extracted diagnostics logic into a standalone header-only class (greenwave_diagnostics.hpp) that handles per-topic parameter management, validation, and diagnostics computation. Each instance manages parameters for a single topic. -
Parameter-based topic control: The
enabledparameter controls whether a topic is actively monitored. Setting it totruetriggers topic validation and subscription creation;falseremoves the topic from monitoring. -
Bidirectional parameter synchronization: Service calls previously used for configuration have been removed. All configuration now flows through ROS 2 parameters with proper validation callbacks.
-
External node coordination: The monitor node tracks which topics are being monitored by other nodes to prevent duplicate monitoring, using a startup discovery phase and runtime parameter event monitoring.
-
Dynamic parameter updates: Frequency and tolerance can be updated at runtime. Invalid values (negative tolerance, non-positive frequency, non-numeric types) are rejected with clear error messages.
The implementation includes comprehensive test coverage with both Python integration tests (test_parameters.py) and C++ unit tests (test_greenwave_diagnostics.cpp). The UI adaptor has been updated to work with the new parameter-based API.
Confidence Score: 4/5
- Safe to merge after fixing the missing rcpputils dependency
- The PR has one blocking issue: missing rcpputils dependency in package.xml. The code uses rcpputils::join but doesn't declare the dependency, which will cause build failures. Otherwise, the implementation is well-tested with comprehensive test coverage, proper parameter validation, and clean architecture. The previous review comments about documentation have been addressed.
- greenwave_monitor/package.xml - missing rcpputils dependency
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 4/5 | New header-only diagnostics class with parameter handling; uses rcpputils::join but dependency not in package.xml |
| greenwave_monitor/src/greenwave_monitor.cpp | 5/5 | Monitor node refactored to use parameter-based topic management; replaces service-based approach |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 5/5 | Python UI adaptor updated to work with parameter-based configuration |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive test suite for parameter-based topic configuration |
| greenwave_monitor/test/test_greenwave_diagnostics.cpp | 5/5 | New C++ unit tests for GreenwaveDiagnostics class parameter handling |
| greenwave_monitor/config/greenwave_monitor.yaml | 5/5 | Example config file demonstrating nested parameter syntax |
| docs/images/SERVICES.md | 5/5 | Documentation updated to reflect parameter-based API; no issues found |
| greenwave_monitor/CMakeLists.txt | 5/5 | Build configuration updated with new test targets |
| greenwave_monitor/package.xml | 4/5 | Package manifest; missing rcpputils dependency used in greenwave_diagnostics.hpp |
Sequence Diagram
sequenceDiagram
participant User
participant MonitorNode as GreenwaveMonitor
participant DiagObj as GreenwaveDiagnostics
participant ParamSvc as Parameter Service
participant Publisher as Topic Publisher
Note over User,Publisher: Startup Phase
User->>MonitorNode: Launch with YAML config
MonitorNode->>MonitorNode: fetch_external_topic_map()
MonitorNode->>MonitorNode: get_topics_from_parameters()
MonitorNode->>MonitorNode: Merge topics from params
loop For each topic
MonitorNode->>MonitorNode: validate_add_topic()
MonitorNode->>MonitorNode: execute_add_topic()
MonitorNode->>DiagObj: new GreenwaveDiagnostics()
DiagObj->>ParamSvc: declare_parameter(freq, tol, enabled)
DiagObj->>DiagObj: Register parameter callbacks
MonitorNode->>Publisher: create_generic_subscription()
end
Note over User,Publisher: Runtime Operation
Publisher->>MonitorNode: Publish message
MonitorNode->>DiagObj: updateDiagnostics(timestamp)
DiagObj->>DiagObj: Calculate frame rate & jitter
User->>ParamSvc: ros2 param set .../topic.expected_frequency 100.0
ParamSvc->>DiagObj: onParameterChange() [validation]
alt Valid parameter
DiagObj-->>ParamSvc: success=true
ParamSvc->>DiagObj: onParameterEvent() [execution]
DiagObj->>DiagObj: setExpectedDt(freq, tol)
else Invalid parameter
DiagObj-->>ParamSvc: success=false, reason
end
User->>ParamSvc: ros2 param set .../topic2.enabled true
ParamSvc->>MonitorNode: on_parameter_change() [validation]
MonitorNode->>MonitorNode: validate_add_topic()
alt Topic valid
MonitorNode-->>ParamSvc: success=true
ParamSvc->>MonitorNode: internal_on_parameter_event()
MonitorNode->>MonitorNode: execute_add_topic()
MonitorNode->>DiagObj: new GreenwaveDiagnostics()
else Topic not found
MonitorNode-->>ParamSvc: success=false
end
Note over MonitorNode,DiagObj: Timer callback (1 Hz)
MonitorNode->>DiagObj: publishDiagnostics()
DiagObj->>DiagObj: Publish to /diagnostics topic
Additional Comments (1)
Add |
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds ROS 2 parameter-based configuration for topic monitoring, allowing users to specify expected frequencies and tolerances through parameter files or dynamic commands instead of only through service calls. Parameters use the greenwave_diagnostics.<topic_name>.(expected_frequency|tolerance|enabled) naming convention with bidirectional synchronization between service calls and parameter changes. The ncurses dashboard reads parameters at startup and updates dynamically when parameters change.
Confidence Score: 3/5
- Safe to merge after adding missing rcpputils dependency
- The implementation is well-structured with comprehensive test coverage. The main issue is a missing build dependency (rcpputils) that will cause compilation failures in certain build configurations. The parameter validation logic correctly rejects invalid inputs, and the synchronization between parameters and diagnostics is properly implemented with mutex protection. Once the dependency is added, the code should function correctly.
- greenwave_monitor/package.xml requires rcpputils dependency addition
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 3/5 | Adds parameter-based configuration for topic monitoring with expected frequency and tolerance |
| greenwave_monitor/src/greenwave_monitor.cpp | 4/5 | Implements parameter event handling and external node monitoring coordination |
| greenwave_monitor/package.xml | 2/5 | Package manifest missing rcpputils dependency used in greenwave_diagnostics.hpp |
Sequence Diagram
sequenceDiagram
participant User
participant ParamFile as Parameter File/CLI
participant Monitor as GreenwaveMonitor
participant Diag as GreenwaveDiagnostics
participant ParamEvents as /parameter_events
User->>ParamFile: Set greenwave_diagnostics./topic.expected_frequency
ParamFile->>Monitor: Parameter override at startup
Monitor->>Diag: Create GreenwaveDiagnostics(config)
Diag->>Diag: Register parameter callbacks
Diag->>Diag: setParameterOrDefault() triggers callbacks
Diag->>ParamEvents: Publish parameter event
Diag->>Diag: onParameterEvent() updates expected_dt
User->>Monitor: ros2 param set expected_frequency
Monitor->>Diag: Parameter change validation
Diag-->>Monitor: Validation result
Monitor->>ParamEvents: Publish parameter event (if successful)
Diag->>Diag: onParameterEvent() updates expected_dt
User->>Monitor: ros2 param set enabled false
Monitor->>Monitor: on_parameter_change() validation
Monitor->>ParamEvents: Publish parameter event
Monitor->>Monitor: internal_on_parameter_event() removes topic
Additional Comments (1)
|
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds ROS 2 parameter-based configuration for topic monitoring, allowing users to specify expected frequencies and tolerances through parameter files or dynamic parameter commands instead of only through service calls. The implementation provides bidirectional synchronization between parameters and monitoring state.
Key Changes
- Parameter Structure: Topics configured via
greenwave_diagnostics.<topic_name>.expected_frequencyandgreenwave_diagnostics.<topic_name>.toleranceparameters - Dynamic Parameter Handling: Full runtime parameter modification with validation (frequency must be positive, tolerance non-negative, only numeric types accepted)
- Topic Enable/Disable: New
greenwave_diagnostics.<topic_name>.enabledparameter controls monitoring on/off - UI Integration: ncurses dashboard reads and displays expected frequencies from parameters
- Comprehensive Tests: Extensive test coverage for YAML config, dynamic changes, validation, and multi-topic scenarios
Issues Found
- Critical: Missing
rcpputilsinclude ingreenwave_diagnostics.hppwill cause compilation failure (line 642 usesrcpputils::joinwithout include) - Critical: Missing
rcpputilsdependency inpackage.xmlwill cause build failures in isolation - Logic Error: Python
ui_adaptor.pystores NaN values instead of clearing frequency entries (line 491) - Minor: Typo "Iniital" in log message (line 739)
- Style: Inconsistent idempotency in enabled parameter handling
Confidence Score: 3/5
- This PR has compilation and logic issues that must be fixed before merging
- Score reflects critical build-breaking issues (missing rcpputils include and dependency) and a logic error in Python NaN handling that could cause incorrect behavior. The core implementation is sound with good test coverage, but these issues prevent safe merging.
- Pay close attention to
greenwave_monitor/include/greenwave_diagnostics.hpp(missing include),greenwave_monitor/package.xml(missing dependency), andgreenwave_monitor/greenwave_monitor/ui_adaptor.py(NaN logic error)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 3/5 | Adds ROS 2 parameter support for topic frequency/tolerance configuration. Issues: missing rcpputils include (compilation failure), typo in log message, parameter deletion explicitly rejected but should align with clearing behavior. |
| greenwave_monitor/src/greenwave_monitor.cpp | 4/5 | Implements enabled parameter handling for topic monitoring control. Minor style issue with idempotency - enabling already-enabled topic succeeds silently but disabling non-monitored topic fails. |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 3/5 | Adds parameter-based topic control for UI. Critical logic error in NaN handling - stores NaN values instead of clearing them from expected_frequencies dict. |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive test coverage for parameter-based configuration including YAML, dynamic changes, validation, and multi-topic scenarios. Well-structured tests. |
| greenwave_monitor/package.xml | 2/5 | Missing rcpputils dependency - code uses rcpputils::join but dependency not declared, will cause build failures in isolation. |
| docs/images/SERVICES.md | 5/5 | Updated documentation for parameter-based configuration. Clear examples and usage patterns documented. |
Sequence Diagram
sequenceDiagram
participant User
participant GreenwaveMonitor
participant GreenwaveDiagnostics
participant ParameterService
participant ParameterEvents
participant UiAdaptor
Note over User,UiAdaptor: Setting Expected Frequency via Parameter
User->>ParameterService: ros2 param set greenwave_diagnostics./topic.expected_frequency 100.0
ParameterService->>GreenwaveDiagnostics: onParameterChange(freq=100.0)
GreenwaveDiagnostics->>GreenwaveDiagnostics: Validate: freq > 0 or isNaN
alt validation fails
GreenwaveDiagnostics-->>ParameterService: SetParametersResult(successful=false)
ParameterService-->>User: Error message
else validation succeeds
GreenwaveDiagnostics-->>ParameterService: SetParametersResult(successful=true)
ParameterService->>ParameterEvents: Publish ParameterEvent(changed_parameters)
ParameterEvents->>GreenwaveDiagnostics: onParameterEvent(changed)
GreenwaveDiagnostics->>GreenwaveDiagnostics: setExpectedDt(100.0, tolerance)
ParameterEvents->>UiAdaptor: onParameterEvent(changed)
UiAdaptor->>UiAdaptor: Update expected_frequencies dict
ParameterService-->>User: Success
end
Note over User,UiAdaptor: Enabling/Disabling Topic Monitoring
User->>ParameterService: ros2 param set greenwave_diagnostics./topic.enabled true
ParameterService->>GreenwaveMonitor: on_parameter_change(enabled=true)
GreenwaveMonitor->>GreenwaveMonitor: validate_add_topic()
alt topic validation fails
GreenwaveMonitor-->>ParameterService: SetParametersResult(successful=false)
ParameterService-->>User: Error message
else validation succeeds
GreenwaveMonitor->>GreenwaveMonitor: Store in pending_validations_
GreenwaveMonitor-->>ParameterService: SetParametersResult(successful=true)
ParameterService->>ParameterEvents: Publish ParameterEvent(new_parameters)
ParameterEvents->>GreenwaveMonitor: internal_on_parameter_event()
GreenwaveMonitor->>GreenwaveMonitor: execute_add_topic()
GreenwaveMonitor->>GreenwaveMonitor: Create subscription
GreenwaveMonitor->>GreenwaveDiagnostics: new GreenwaveDiagnostics()
GreenwaveDiagnostics->>GreenwaveDiagnostics: Declare frequency/tolerance params
ParameterEvents->>UiAdaptor: onParameterEvent(new)
UiAdaptor->>UiAdaptor: Update topic_to_node map
ParameterService-->>User: Success
end
Note over User,UiAdaptor: UI Adaptor Reading Parameters at Startup
UiAdaptor->>UiAdaptor: _fetch_initial_parameters_callback()
loop For each node
UiAdaptor->>ParameterService: ListParameters(prefix="greenwave_diagnostics")
ParameterService-->>UiAdaptor: Parameter names
UiAdaptor->>ParameterService: GetParameters(names)
ParameterService-->>UiAdaptor: Parameter values
UiAdaptor->>UiAdaptor: Parse and store in expected_frequencies
end
| if (greenwave_diagnostics_.find(topic) == greenwave_diagnostics_.end()) { | ||
| result.successful = false; | ||
| result.reason = "Topic not being monitored: " + topic; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
inconsistent idempotency - disabling non-monitored topic fails, but enabling already-monitored topic succeeds silently
Lines 145-148 allow enabling an already-enabled topic (silently continues), but lines 159-163 reject disabling a non-monitored topic with an error. Consider making both operations idempotent for consistency:
| if (greenwave_diagnostics_.find(topic) == greenwave_diagnostics_.end()) { | |
| result.successful = false; | |
| result.reason = "Topic not being monitored: " + topic; | |
| return result; | |
| } | |
| if (greenwave_diagnostics_.find(topic) == greenwave_diagnostics_.end()) { | |
| // Topic not monitored, disabling is idempotent - silently succeed | |
| continue; | |
| } |
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR successfully migrates the Greenwave Monitor from a service-based architecture to a parameter-based configuration system, enabling more flexible and ROS-native topic monitoring control.
Key Architectural Changes
Service Removal: Completely removes the greenwave_monitor_interfaces package with its ManageTopic and SetExpectedFrequency services, replacing them with ROS 2 parameters following the naming convention greenwave_diagnostics.<topic_name>.(expected_frequency|tolerance|enabled).
Bidirectional Synchronization: Implements a two-way sync mechanism where:
- Parameter changes trigger monitoring updates via
onParameterEvent - Service-like operations (add/remove topics, set frequencies) now update parameters
- Both the monitor node and publisher nodes can manage their own topic parameters
External Node Tracking: The monitor node discovers and tracks topics being monitored by other nodes through the external_topic_to_node_ map, preventing duplicate monitoring and respecting ownership boundaries.
Implementation Quality
The implementation includes comprehensive validation (positive frequencies, non-negative tolerances, type checking) and extensive test coverage. The Python UI adaptor demonstrates good async patterns for parameter discovery across multiple nodes.
Critical Issues Found
- Build Failure: Missing
rcpputilsinclude and dependency forrcpputils::join()used in error message formatting (line 650 of greenwave_diagnostics.hpp) - Potential KeyError: Python code attempts to delete dictionary keys without checking existence in parameter deletion handler (ui_adaptor.py:511)
Non-Critical Issues
- Typo in warning message: "Iniital" → "Initial"
- Parameter deletion is rejected entirely rather than treated as "clear" operation
- Synchronous validation in parameter callback can block up to 0.5 seconds during publisher discovery
The feature is well-designed and thoroughly tested, but the missing dependency will cause immediate build failures that must be resolved before merge.
Confidence Score: 2/5
- This PR has a critical build failure due to missing rcpputils dependency and a potential runtime crash in Python code
- Score of 2 reflects that while the feature is well-designed with excellent test coverage, there is a guaranteed compilation failure (missing rcpputils include/dependency) and a potential runtime crash (KeyError in Python). The missing dependency alone would prevent the code from building. Once these issues are fixed, the PR would be much safer (4-5 range) as the core logic is sound.
- Critical attention needed for greenwave_monitor/package.xml (add rcpputils dependency), greenwave_monitor/include/greenwave_diagnostics.hpp (add include), and greenwave_monitor/greenwave_monitor/ui_adaptor.py (fix KeyError in line 511)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 3/5 | Added parameter-based configuration system with validation and synchronization. Missing rcpputils include/dependency. Contains typo in warning message. |
| greenwave_monitor/src/greenwave_monitor.cpp | 4/5 | Replaced service-based topic management with parameter-based system. Implements validation, external node tracking, and deferred topic addition via pending_validations. |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 4/5 | Enhanced UI adaptor with async parameter querying, topic-to-node mapping, and bidirectional parameter synchronization. Well-structured implementation. |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive test suite covering parameter validation, dynamic updates, YAML config, and edge cases. Well-organized. |
| greenwave_monitor/package.xml | 2/5 | Removed greenwave_monitor_interfaces dependency but missing rcpputils dependency needed by code. |
Sequence Diagram
sequenceDiagram
participant User
participant MonitorNode as GreenwaveMonitor
participant PublisherNode as Publisher Node
participant Diagnostics as GreenwaveDiagnostics
participant ParamEvents as /parameter_events
Note over User,ParamEvents: Startup: Topic Discovery & Parameter Loading
MonitorNode->>MonitorNode: fetch_external_topic_map()
MonitorNode->>PublisherNode: Query parameters via SyncParametersClient
PublisherNode-->>MonitorNode: Return greenwave_diagnostics.* params
MonitorNode->>MonitorNode: Build external_topic_to_node_ map
MonitorNode->>MonitorNode: get_topics_from_parameters()
MonitorNode->>MonitorNode: Merge with 'topics' list parameter
loop For each topic
MonitorNode->>MonitorNode: validate_add_topic(topic)
MonitorNode->>MonitorNode: execute_add_topic(topic)
MonitorNode->>Diagnostics: Create GreenwaveDiagnostics
Diagnostics->>Diagnostics: Register onParameterChange callback
Diagnostics->>ParamEvents: Subscribe to /parameter_events
Diagnostics->>Diagnostics: setParameterOrDefault() for freq/tol/enabled
end
Note over User,ParamEvents: Runtime: Parameter-Based Configuration
User->>MonitorNode: ros2 param set enabled=true
MonitorNode->>MonitorNode: on_parameter_change()
MonitorNode->>MonitorNode: validate_add_topic() (with retries)
alt Validation succeeds
MonitorNode->>MonitorNode: Store in pending_validations_
MonitorNode-->>User: Success
ParamEvents->>MonitorNode: ParameterEvent (new/changed)
MonitorNode->>MonitorNode: internal_on_parameter_event()
MonitorNode->>MonitorNode: execute_add_topic() from pending
MonitorNode->>Diagnostics: Create GreenwaveDiagnostics
else Validation fails
MonitorNode-->>User: Error (no publisher found)
end
User->>Diagnostics: ros2 param set expected_frequency=30.0
Diagnostics->>Diagnostics: onParameterChange() validation
alt Frequency valid (positive or NaN)
Diagnostics-->>User: Success
ParamEvents->>Diagnostics: ParameterEvent
Diagnostics->>Diagnostics: onParameterEvent()
Diagnostics->>Diagnostics: setExpectedDt(freq, tolerance)
Diagnostics->>Diagnostics: Enable time diagnostics
else Frequency invalid (≤0)
Diagnostics-->>User: Error: must be positive or NaN
end
Note over User,ParamEvents: Runtime: Message Monitoring
PublisherNode->>Diagnostics: updateDiagnostics(timestamp)
Diagnostics->>Diagnostics: Check enabled flag
alt Enabled
Diagnostics->>Diagnostics: Calculate frame rates & jitter
Diagnostics->>Diagnostics: Check vs expected_dt if set
Diagnostics->>Diagnostics: Update status (OK/WARN/ERROR)
else Disabled
Diagnostics->>Diagnostics: Skip update
end
loop Every 1 second
MonitorNode->>Diagnostics: publishDiagnostics()
Diagnostics->>ParamEvents: Publish to /diagnostics topic
end
Note over User,ParamEvents: Runtime: Disabling Monitoring
User->>MonitorNode: ros2 param set enabled=false
MonitorNode->>MonitorNode: on_parameter_change()
MonitorNode->>MonitorNode: Check not external topic
MonitorNode-->>User: Success
ParamEvents->>MonitorNode: ParameterEvent (changed)
MonitorNode->>MonitorNode: internal_on_parameter_event()
MonitorNode->>MonitorNode: remove_topic()
MonitorNode->>Diagnostics: Destroy GreenwaveDiagnostics
Note right of Diagnostics: Parameters persist for re-enable
Additional Comments (1)
|
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements ROS 2 parameter-based configuration for topic monitoring in the Greenwave Monitor system. It replaces the previous service-based approach with a more idiomatic ROS 2 parameter system.
Key Changes
Parameter Structure: Topics can now be configured using greenwave_diagnostics.<topic_name>.(expected_frequency|tolerance|enabled) parameters, supporting both flat dotted notation and nested dictionary format in YAML files.
Architecture Shift: The entire greenwave_monitor_interfaces package has been removed, eliminating custom service definitions (ManageTopic.srv, SetExpectedFrequency.srv) in favor of standard ROS 2 parameter services.
Bidirectional Synchronization: Parameter changes trigger monitoring updates, and the system maintains consistency between parameter state and runtime behavior.
New Components:
GreenwaveDiagnosticsclass now manages its own parameters with validation and callbacksGreenwaveUiAdaptorprovides thread-safe parameter-based UI integration- Comprehensive test coverage in
test_parameters.pyandtest_greenwave_diagnostics.cpp
Critical Issues Found
Compilation Failure: The code uses rcpputils::join() but neither includes the header nor declares the dependency in package.xml. This will prevent the package from building.
Runtime Crashes: Two instances of unsafe dictionary deletion in ui_adaptor.py will cause KeyError exceptions when parameters are modified before diagnostics messages arrive.
Implementation Quality
The parameter validation logic is well-designed with proper type checking, range validation, and atomic updates. The synchronization between onParameterChange() (validation) and onParameterEvent() (execution) correctly prevents partial parameter updates. The external node tracking mechanism properly handles multi-node monitoring scenarios.
Confidence Score: 2/5
- Not safe to merge - contains compilation-blocking issues (missing rcpputils dependency) and runtime crashes (KeyError exceptions)
- The PR has two P0 blocking issues: (1) missing rcpputils include and dependency causes compilation failure, and (2) potential KeyError exceptions in Python code will cause runtime crashes. Additionally, there are two P1 issues with the same KeyError pattern that need fixing.
- greenwave_monitor/include/greenwave_diagnostics.hpp (missing include), greenwave_monitor/package.xml (missing dependency), greenwave_monitor/greenwave_monitor/ui_adaptor.py (KeyError bugs on lines 484 and 511)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 3/5 | New header-only diagnostics class with parameter management. Uses rcpputils::join without including header or dependency. |
| greenwave_monitor/package.xml | 3/5 | Removed greenwave_monitor_interfaces dependency. Missing rcpputils dependency needed by greenwave_diagnostics.hpp. |
| greenwave_monitor/src/greenwave_monitor.cpp | 4/5 | Refactored to use parameter-based topic management. Proper validation and synchronization logic for parameter events. |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 3/5 | UI adaptor for parameter-based monitoring. Potential KeyError on line 484 and 511 when deleting non-existent keys. |
| greenwave_monitor/CMakeLists.txt | 5/5 | Build configuration updated to use header-only greenwave_diagnostics library. No issues found. |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive parameter testing added. Covers various scenarios including dynamic parameter changes. |
| docs/images/SERVICES.md | 5/5 | Documentation updated to reflect parameter-based configuration. Clear examples provided. |
| greenwave_monitor/include/greenwave_monitor.hpp | 5/5 | Monitor node header updated with parameter-based topic management interfaces. Clean implementation. |
Sequence Diagram
sequenceDiagram
participant User
participant ROS2_Param as ROS2 Parameter Service
participant Monitor as GreenwaveMonitor
participant Diag as GreenwaveDiagnostics
participant UI as ncurses/UI Adaptor
Note over User,UI: Topic Monitoring Setup
User->>ROS2_Param: ros2 param set greenwave_diagnostics./topic.enabled true
ROS2_Param->>Monitor: on_parameter_change(enabled=true)
Monitor->>Monitor: validate_add_topic()
Monitor-->>ROS2_Param: SetParametersResult(success=true)
ROS2_Param->>Monitor: ParameterEvent(new_parameters)
Monitor->>Monitor: execute_add_topic()
Monitor->>Diag: create GreenwaveDiagnostics
Diag->>Diag: declare parameters
Diag->>ROS2_Param: set default parameters
Note over User,UI: Frequency Configuration
User->>ROS2_Param: ros2 param set greenwave_diagnostics./topic.expected_frequency 30.0
ROS2_Param->>Diag: onParameterChange()
Diag->>Diag: validate frequency > 0
Diag-->>ROS2_Param: SetParametersResult(success=true)
ROS2_Param->>Diag: ParameterEvent(changed_parameters)
Diag->>Diag: setExpectedDt(30.0, tolerance)
Note over User,UI: Runtime Monitoring
Monitor->>Monitor: topic_callback(msg)
Monitor->>Diag: updateDiagnostics(timestamp)
Diag->>Diag: check frequency/jitter
Monitor->>Diag: publishDiagnostics()
Diag->>UI: DiagnosticArray on /diagnostics
UI->>UI: update UI display
Note over User,UI: Topic Removal
User->>ROS2_Param: ros2 param set greenwave_diagnostics./topic.enabled false
ROS2_Param->>Monitor: on_parameter_change(enabled=false)
Monitor->>Monitor: validate removal
Monitor-->>ROS2_Param: SetParametersResult(success=true)
ROS2_Param->>Monitor: ParameterEvent(changed_parameters)
Monitor->>Monitor: remove_topic()
Monitor->>Monitor: destroy subscription
Note over Diag: Parameters persist for future use
Additional Comments (1)
|
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Overview
This PR implements parameter-based configuration for topic monitoring frequencies and tolerances. Instead of only service calls, users can now configure topics through ROS 2 parameters (both flat dotted notation and nested YAML format), with full bidirectional synchronization between parameters and service calls.
Key Changes
- Parameter Structure: Topics are configured using
greenwave_diagnostics.<topic_name>.expected_frequency,greenwave_diagnostics.<topic_name>.tolerance, andgreenwave_diagnostics.<topic_name>.enabledparameters - Bidirectional Synchronization: Parameter changes trigger monitoring updates; both representations stay in sync
- Dynamic Parameter Handling: Full support for runtime parameter modification with validation (positive frequency, non-negative tolerance, numeric types only)
- Interface Changes: Removed custom service interfaces (ManageTopic.srv, SetExpectedFrequency.srv) in favor of standard parameter-based control
- Python Frontend: ncurses dashboard now reads parameters and supports dynamic reconfiguration
Critical Issues Found
- Missing rcpputils dependency - Code uses
rcpputils::join()but the dependency is not declared in package.xml and the header is not included in greenwave_diagnostics.hpp. This will cause compilation failures.
Code Quality
- Comprehensive test coverage with 16+ test cases covering startup behavior, multiple topics, YAML configuration, dynamic parameters, and edge cases
- Proper parameter validation (non-positive frequencies rejected, negative tolerances rejected, non-numeric types rejected, parameter deletion rejected)
- Thread-safe implementation with mutex locks for shared state
- Good error handling and logging throughout
Confidence Score: 2/5
- This PR has critical build issues that must be resolved before merging. Missing rcpputils dependency in both package.xml and header includes will prevent compilation.
- Score of 2 reflects a single but critical blocker: the missing rcpputils dependency. The underlying functionality appears well-designed with comprehensive tests and proper parameter validation, but the code cannot compile in its current state. The rcpputils::join() function is used in the parameter validation error handling path (line 650 of greenwave_diagnostics.hpp), and both the C++ header include and package.xml dependency are missing. This is a compile-time error that prevents the code from building.
- greenwave_monitor/include/greenwave_diagnostics.hpp (add missing #include), greenwave_monitor/package.xml (add missing rcpputils dependency)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 2/5 | Header-only library for diagnostics configuration with parameter callbacks. Critical issue: missing #include <rcpputils/join.hpp> which will cause compilation failure when rcpputils::join() is called on line 650. |
| greenwave_monitor/package.xml | 2/5 | Package manifest missing critical rcpputils dependency. The code uses rcpputils::join() in greenwave_diagnostics.hpp but the dependency is not declared, causing build failures. |
| greenwave_monitor/src/greenwave_monitor.cpp | 4/5 | Main monitor node implementation with parameter handling, topic validation, and subscription management. Implementation is solid with proper mutex locking, error handling, and lifecycle management. |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive test suite covering parameter-based configuration with multiple test scenarios including YAML loading, dynamic parameter changes, and edge cases. Tests are well-organized and thorough. |
Sequence Diagram
sequenceDiagram
actor User
participant Launch as ROS Launch
participant Monitor as greenwave_monitor
participant Diag as GreenwaveDiagnostics
participant Param as Parameter Events
User->>Launch: Launch with parameters
Launch->>Monitor: Load greenwave_diagnostics.* parameters
Monitor->>Monitor: get_topics_from_parameters()
Monitor->>Diag: Create GreenwaveDiagnostics per topic
Diag->>Diag: declare_parameter(freq, tol, enabled)
Diag->>Param: Subscribe to parameter events
User->>Monitor: ros2 param set greenwave_diagnostics./topic.expected_frequency 50.0
Monitor->>Param: Parameter change event
Diag->>Diag: onParameterChange() validation
Diag->>Param: Subscribe for parameter event
Diag->>Diag: onParameterEvent() applies change
Diag->>Diag: setExpectedDt() or clearExpectedDt()
Monitor->>Diag: Message arrives on topic
Monitor->>Diag: updateDiagnostics(msg_timestamp)
Diag->>Diag: Check against expected frequency
Monitor->>Diag: publishDiagnostics()
Additional Comments (2)
|
Signed-off-by: Blake McHale <bmchale@nvidia.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements ROS 2 parameter-based configuration for topic monitoring, replacing service-based configuration with a more idiomatic ROS 2 approach. The implementation allows users to specify expected frequencies and tolerances through parameter files or dynamic parameter commands.
Key Changes:
- Parameter Structure: Topics configured using
greenwave_diagnostics.<topic>.expected_frequency,greenwave_diagnostics.<topic>.tolerance, andgreenwave_diagnostics.<topic>.enabledparameters - Bidirectional Sync: Service-like behavior maintained through parameter callbacks that validate and apply changes
- Dynamic Parameters: Full runtime modification support with validation (frequency must be positive, tolerance non-negative)
- Multi-Node Support: External node tracking prevents duplicate monitoring across multiple monitor instances
- Backward Compatibility: Original
topicslist parameter still supported for initialization
Critical Issues Found:
- Missing
rcpputilsdependency in package.xml will cause build failures - Inconsistent NaN handling in ui_adaptor.py stores NaN values instead of clearing them as documented
Confidence Score: 2/5
- Not safe to merge due to missing dependency that will cause build failures
- The missing rcpputils dependency is a blocking issue that will cause compilation failures when building the package. The NaN handling inconsistency is a logic bug that could cause unexpected behavior in the UI.
- greenwave_monitor/package.xml (missing dependency), greenwave_monitor/greenwave_monitor/ui_adaptor.py (NaN handling bug)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/package.xml | 0/5 | Missing rcpputils dependency that is required by greenwave_diagnostics.hpp |
| greenwave_monitor/include/greenwave_diagnostics.hpp | 4/5 | Uses rcpputils::join for error message formatting in parameter validation |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 3/5 | Inconsistent NaN handling in parameter event processing stores NaN instead of clearing |
| greenwave_monitor/src/greenwave_monitor.cpp | 5/5 | Parameter-based topic monitoring implementation with external node tracking |
| greenwave_monitor/test/test_parameters.py | 5/5 | Comprehensive test coverage for parameter-based configuration including dynamic updates |
Sequence Diagram
sequenceDiagram
participant User
participant ROSParam as ROS Parameter System
participant Monitor as GreenwaveMonitor
participant Diag as GreenwaveDiagnostics
participant UI as GreenwaveUiAdaptor
Note over User,UI: Initialization Phase
User->>Monitor: Launch with YAML config
Monitor->>ROSParam: declare parameters from overrides
Monitor->>Monitor: fetch_external_topic_map()
Monitor->>Monitor: get_topics_from_parameters()
loop For each topic
Monitor->>Diag: Create GreenwaveDiagnostics
Diag->>ROSParam: declare/set freq/tol/enabled params
Diag->>Diag: Register parameter callbacks
end
Monitor->>ROSParam: Register parameter callbacks
Note over User,UI: Dynamic Parameter Changes
User->>ROSParam: set greenwave_diagnostics./topic.expected_frequency
ROSParam->>Diag: onParameterChange (validate)
Diag-->>ROSParam: validation result
ROSParam->>Diag: onParameterEvent (execute)
Diag->>Diag: setExpectedDt(freq, tol)
Note over User,UI: Topic Enable/Disable
User->>ROSParam: set greenwave_diagnostics./topic.enabled=false
ROSParam->>Monitor: on_parameter_change (validate)
Monitor-->>ROSParam: validation result
ROSParam->>Monitor: internal_on_parameter_event
Monitor->>Monitor: remove_topic()
Monitor->>Diag: destroy diagnostics object
Note over User,UI: UI Integration
UI->>ROSParam: Subscribe to /parameter_events
ROSParam-->>UI: Parameter changes
UI->>UI: Update expected_frequencies cache
UI->>Monitor: Subscribe to /diagnostics
Monitor-->>UI: Diagnostic messages
UI->>UI: Update ui_diagnostics
| if field == 'freq': | ||
| # Treat NaN or non-positive as "cleared" | ||
| if value > 0 or math.isnan(value): | ||
| self.expected_frequencies[topic_name] = (value, current[1]) |
There was a problem hiding this comment.
[P1] Inconsistent NaN handling. The comment says "Treat NaN or non-positive as 'cleared'" but the condition if value > 0 or math.isnan(value): stores NaN values in expected_frequencies. This contradicts the intended behavior. If NaN should clear the frequency, it shouldn't be stored. This causes line 406 (if freq > 0 and not math.isnan(freq):) and line 583 (if freq <= 0.0 or math.isnan(freq):) to treat NaN inconsistently.
Additional Comments (1)
|
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR replaces service-based configuration with ROS 2 parameter-based topic monitoring. Topics can now be configured via greenwave_diagnostics.<topic>.(enabled|expected_frequency|tolerance) parameters, supporting both runtime changes and launch-time YAML configuration.
Major Changes
- Removed service interfaces (
ManageTopic,SetExpectedFrequency) andgreenwave_monitor_interfacespackage entirely - Added parameter-based configuration with validation in
GreenwaveDiagnosticsclass - Each monitored topic gets its own parameter namespace with
enabled,expected_frequency, andtolerancefields - Python UI adaptor updated to work with new parameter-based system
- Comprehensive test coverage added for parameter handling
Issues Found
- Critical: Buffer overflow risk in
GetTimestampFromSerializedMessage- accessesbuffer[4-11]without bounds check - Critical: Missing
rcpputilsdependency in package.xml (code usesrcpputils::join) - Logic bug: Python ui_adaptor.py inconsistent NaN handling - stores NaN values despite comment saying they represent "cleared"
- Performance: Each
GreenwaveDiagnosticsinstance creates redundant subscription to/parameter_events(N subscriptions for N topics)
Positive Aspects
- Well-tested with comprehensive C++ and Python tests
- Clear parameter validation with helpful error messages
- Proper mutex protection for shared diagnostics state
- Good documentation in SERVICES.md explaining new parameter structure
Confidence Score: 2/5
- Not safe to merge - contains critical buffer overflow vulnerability and missing dependency
- Score reflects two critical issues that will cause failures: (1) buffer overflow in timestamp extraction can crash the node with malformed messages, (2) missing rcpputils dependency will cause build failures. Additionally, the NaN handling bug in Python could cause UI issues. These must be fixed before merging.
greenwave_monitor/src/greenwave_monitor.cpp(buffer overflow),greenwave_monitor/package.xml(missing dependency),greenwave_monitor/greenwave_monitor/ui_adaptor.py(logic bug)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| greenwave_monitor/include/greenwave_diagnostics.hpp | 3/5 | Added parameter-based configuration (frequency, tolerance, enabled) with validation and event handling. Each instance registers callbacks and subscribes to parameter events. Missing rcpputils dependency causes build issues. |
| greenwave_monitor/src/greenwave_monitor.cpp | 2/5 | Implements parameter-based topic management. Buffer overflow risk in GetTimestampFromSerializedMessage (no bounds check before accessing buffer[4-11]). Multiple subscriptions to /parameter_events per topic may cause performance issues. |
| greenwave_monitor/greenwave_monitor/ui_adaptor.py | 3/5 | Python UI adaptor for parameter-based monitoring. Logic inconsistency: line 496 stores NaN frequencies despite comment saying NaN means "cleared". Otherwise handles parameter events and topic toggling correctly. |
| greenwave_monitor/package.xml | 1/5 | Missing <depend>rcpputils</depend> dependency. Code uses rcpputils::join in greenwave_diagnostics.hpp:651 but dependency not declared, will cause build failures. |
Sequence Diagram
sequenceDiagram
participant User
participant YAML/CLI
participant GreenwaveMonitor
participant GreenwaveDiagnostics
participant ParameterEvents
participant TopicPublisher
Note over User,TopicPublisher: Initialization Phase
YAML/CLI->>GreenwaveMonitor: Launch with parameters
GreenwaveMonitor->>GreenwaveMonitor: deferred_init()
GreenwaveMonitor->>GreenwaveMonitor: get_topics_from_parameters()
loop For each topic with params
GreenwaveMonitor->>GreenwaveDiagnostics: Create instance
GreenwaveDiagnostics->>GreenwaveDiagnostics: Declare/set freq, tol, enabled params
GreenwaveDiagnostics->>ParameterEvents: Subscribe to /parameter_events
GreenwaveDiagnostics->>GreenwaveDiagnostics: Register param callback
end
Note over User,TopicPublisher: Runtime Parameter Changes
User->>GreenwaveMonitor: ros2 param set greenwave_diagnostics./topic.expected_frequency 30.0
GreenwaveMonitor->>GreenwaveDiagnostics: onParameterChange() validates
alt Valid parameter
GreenwaveMonitor-->>User: Success
ParameterEvents->>GreenwaveDiagnostics: Parameter event published
GreenwaveDiagnostics->>GreenwaveDiagnostics: onParameterEvent() applies change
GreenwaveDiagnostics->>GreenwaveDiagnostics: setExpectedDt(freq, tol)
else Invalid parameter
GreenwaveMonitor-->>User: Error reason
end
Note over User,TopicPublisher: Topic Monitoring via enabled Parameter
User->>GreenwaveMonitor: ros2 param set greenwave_diagnostics./new_topic.enabled true
GreenwaveMonitor->>GreenwaveMonitor: on_parameter_change() validates topic
GreenwaveMonitor->>GreenwaveMonitor: validate_add_topic()
alt Topic has publishers
GreenwaveMonitor->>GreenwaveMonitor: Pend validation
ParameterEvents->>GreenwaveMonitor: Parameter committed
GreenwaveMonitor->>GreenwaveMonitor: execute_add_topic()
GreenwaveMonitor->>GreenwaveDiagnostics: Create new instance
GreenwaveMonitor->>TopicPublisher: Subscribe to topic
else No publishers
GreenwaveMonitor-->>User: Error: No publishers found
end
Note over User,TopicPublisher: Message Flow and Diagnostics
loop Every message
TopicPublisher->>GreenwaveMonitor: Publish message
GreenwaveMonitor->>GreenwaveDiagnostics: updateDiagnostics(timestamp)
GreenwaveDiagnostics->>GreenwaveDiagnostics: Check freq/tolerance/jitter
end
loop Every 1 second
GreenwaveMonitor->>GreenwaveDiagnostics: publishDiagnostics()
GreenwaveDiagnostics->>ParameterEvents: Publish to /diagnostics
end
| if field == 'freq': | ||
| # Treat NaN or non-positive as "cleared" | ||
| if value > 0 or math.isnan(value): | ||
| self.expected_frequencies[topic_name] = (value, current[1]) |
There was a problem hiding this comment.
inconsistent NaN handling - comment says "Treat NaN or non-positive as 'cleared'" but if value > 0 or math.isnan(value): stores NaN values
if NaN means "cleared", don't store it:
| if field == 'freq': | |
| # Treat NaN or non-positive as "cleared" | |
| if value > 0 or math.isnan(value): | |
| self.expected_frequencies[topic_name] = (value, current[1]) | |
| if field == 'freq': | |
| # Store positive frequencies, clear for NaN or non-positive | |
| if value > 0 and not math.isnan(value): | |
| self.expected_frequencies[topic_name] = (value, current[1]) | |
| else: | |
| self.expected_frequencies.pop(topic_name, None) |
| // Subscribe to parameter events | ||
| param_event_subscription_ = node_.create_subscription<rcl_interfaces::msg::ParameterEvent>( | ||
| "/parameter_events", 10, | ||
| std::bind(&GreenwaveDiagnostics::onParameterEvent, this, std::placeholders::_1)); |
There was a problem hiding this comment.
each GreenwaveDiagnostics instance creates its own subscription to /parameter_events - with N monitored topics, creates N redundant subscriptions to the same topic
more efficient: single subscription at GreenwaveMonitor level that dispatches to individual diagnostics objects
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Additional Comments (1)
if add check: if (serialized_message_ptr->get_rcl_serialized_message().buffer_length < 12) {
return std::chrono::time_point<std::chrono::system_clock>();
} |
2 participants
Resolves #19 and gives the capability to specify expected frequencies and tolerance through ROS 2 parameters. The
topicsparameter can now be specified with subfieldstopics.<topic_name>.(expected_frequency|tolerance).The original topics parameter with a list still exists and can be used during initialization, but now parameters specified with the new subfields will also add those topics to the monitor node with the expected frequency and tolerance already set. They can be set in a ROS 2 parameter file like:
or
ncurses_dashboard also works with the parameters. It reads all parameters at the beginning and sets up the dashboard with expected frequencies already set. If parameters are updated it also updates at the same time.