ros2doctor: add node and parameter info to report #1161
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds node and parameter discovery capabilities to the ros2doctor diagnostic tool, enabling users to view active nodes and their parameters in the system report.
Key changes:
- Introduces NodeCheck/NodeReport classes to discover and report on active ROS 2 nodes, including detection of duplicate node names
- Introduces ParameterCheck/ParameterReport classes to query and report parameters for each discovered node
- Updates setup.py entry points to register the new check and report classes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| ros2doctor/setup.py | Adds entry points for NodeCheck, ParameterCheck, NodeReport, and ParameterReport; fixes missing trailing comma |
| ros2doctor/ros2doctor/api/node.py | Implements node discovery with duplicate detection check and node list reporting |
| ros2doctor/ros2doctor/api/parameter.py | Implements parameter discovery by querying list_parameters service for each node with reporting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
|
|
||
| def call_list_parameters(node, node_name, namespace='/'): |
There was a problem hiding this comment.
There should be two blank lines before module-level functions according to PEP 8 style guidelines. This helps maintain consistency with the rest of the codebase.
| class ParameterCheck(DoctorCheck): | ||
| """Check for nodes without parameter services.""" | ||
|
|
||
| def category(self): | ||
| return 'parameter' | ||
|
|
||
| def check(self): | ||
| result = Result() | ||
| with NodeStrategy(None) as node: | ||
| try: | ||
| node_names_and_namespaces = node.get_node_names_and_namespaces() | ||
| except Exception: | ||
| node_names_and_namespaces = [] | ||
| with DirectNode(None) as param_node: | ||
| for node_name, namespace in node_names_and_namespaces: | ||
| response = call_list_parameters(param_node.node, node_name, namespace) | ||
| if response is None: | ||
| full_name = f"{namespace.rstrip('/')}/{node_name}" | ||
| doctor_warn(f'Node {full_name} has no parameter services.') | ||
| result.add_warning() | ||
| return result | ||
|
|
||
|
|
||
| class ParameterReport(DoctorReport): | ||
| """Report parameter related information.""" | ||
|
|
||
| def category(self): | ||
| return 'parameter' | ||
|
|
||
| def report(self): | ||
| report = Report('PARAMETER LIST') | ||
| with NodeStrategy(None) as node: | ||
| try: | ||
| node_names_and_namespaces = node.get_node_names_and_namespaces() | ||
| except Exception: | ||
| node_names_and_namespaces = [] | ||
| if not node_names_and_namespaces: | ||
| report.add_to_report('total nodes checked', 0) | ||
| report.add_to_report('total parameter count', 0) | ||
| report.add_to_report('parameter', 'none') | ||
| return report | ||
|
|
||
| total_param_count = 0 | ||
| nodes_checked = 0 | ||
|
|
||
| with DirectNode(None) as param_node: | ||
| for node_name, namespace in sorted(node_names_and_namespaces): | ||
| nodes_checked += 1 | ||
| response = call_list_parameters(param_node.node, node_name, namespace) | ||
| if response and hasattr(response, 'result') and response.result: | ||
| param_names = response.result.names if hasattr(response.result, 'names') else [] | ||
| if param_names: | ||
| total_param_count += len(param_names) | ||
| full_name = f"{namespace.rstrip('/')}/{node_name}" | ||
| for param_name in sorted(param_names): | ||
| report.add_to_report('parameter', param_name) | ||
| report.add_to_report('node', full_name) | ||
|
|
||
| report.add_to_report('total nodes checked', nodes_checked) | ||
| report.add_to_report('total parameter count', total_param_count) | ||
| return report |
There was a problem hiding this comment.
The new ParameterCheck and ParameterReport classes lack test coverage. Given that other API modules like TopicCheck, TopicReport, and ServiceReport have comprehensive test coverage in test_api.py, these new classes should also have corresponding unit tests to verify their behavior, especially the complex parameter querying logic.
BhuvanB404
force-pushed
the
feature/node-parameter-doctor-report
branch
3 times, most recently
from
4a08d58 to
a7ffe55
Compare
BhuvanB404
changed the title
|
@fujitatomoya this is the approach I have used with less dependencies outside of ros2cli. Do provide your suggestions and review |
|
there are some flake8 warnings https://build.ros2.org/job/Rpr__ros2cli__ubuntu_noble_amd64/346/testReport/ros2doctor.ros2doctor.test/test_flake8/test_flake8/, besides that we need to add the tests to make sure those sub-commands are working as expected. |
Add NodeCheck/NodeReport and ParameterCheck/ParameterReport so the ros2doctor report shows active nodes and their parameters. - Add `ros2doctor/api/node.py` (node discovery, NodeCheck, NodeReport) - Add `ros2doctor/api/parameter.py` (parameter discovery, ParameterCheck, ParameterReport) - Update entry points in `ros2doctor/setup.py`: - `ros2doctor.checks: NodeCheck` and `ParameterCheck` - `ros2doctor.report: NodeReport` and `ParameterReport` Why: This enhancement provides valuable insights into the active nodes and their parameters within a ROS 2 system. By including this information in the `ros2 doctor` report sections in the `ros2 doctor --report` output, giving users better visibility into the runtime ROS 2 system configuration. Fixes: ros2#1090 Signed-off-by: BhuvanB404 <bhuvanb404@gmail.com> Signed-off-by: BhuvanB404 <bhuvanb1408@gmail.com>
BhuvanB404
force-pushed
the
feature/node-parameter-doctor-report
branch
from
e983cc1 to
9e0af3c
Compare
|
This is the output for |
BhuvanB404
force-pushed
the
feature/node-parameter-doctor-report
branch
from
9e0af3c to
4ca06e6
Compare
|
@fujitatomoya i have added tests for these. If any suggestions, do provide : ) |
BhuvanB404
changed the title
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
overall lgtm with green CI. i got a few comments.
ros2doctor/ros2doctor/api/node.py
Outdated
| with NodeStrategy(None) as node: | ||
| node_list = get_node_names(node=node, include_hidden_nodes=True) | ||
| node_names = [n.full_name for n in node_list] | ||
| if has_duplicates(node_names): |
There was a problem hiding this comment.
does this make more sense and useful?
from collections import Counter
def find_duplicates(values):
"""Return values that appear more than once."""
counts = Counter(values)
return [v for v, c in counts.items() if c > 1]There was a problem hiding this comment.
Ya , makes sense, removes my useless double check. Thank for the suggestion :)
| ready = client.wait_for_services(timeout_sec=2.0) | ||
| if not ready: | ||
| return None | ||
| future = client.list_parameters() |
There was a problem hiding this comment.
i am not sure if listing parameters are sufficient for ros2doctor.
maybe it should print the parameter values that are set as well? for example, use_sim_time parameter can be list but we are not able to see if that is set or not. i think for the ros2doctor would be useful to see the parameter variables for this case? what do you think?
There was a problem hiding this comment.
providing state information for the parameters sounds more useful than current implementation . I'll add it 👍
| @@ -0,0 +1,67 @@ | |||
| # Copyright 2019 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
??? did you copy this fixture from somewhere?
| # Copyright 2019 Open Source Robotics Foundation, Inc. | |
| # Copyright 2026 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
For tests, I mainly referred to ros2node and ros2param . Since the testing suite would be similar. So I copied the complex_node.py from there. It already had a launcher for different node types, so i copied the fixtures from there. If its not how it should be done, do suggest changes :)
BhuvanB404
force-pushed
the
feature/node-parameter-doctor-report
branch
from
4ca06e6 to
7dfeb4a
Compare
- Add and update tests: test_node.py, test_parameter.py, test_api.py, common.py Signed-off-by: BhuvanB404<bhuvanb1408@gmail.com> Signed-off-by: BhuvanB404 <bhuvanb1408@gmail.com>
BhuvanB404
force-pushed
the
feature/node-parameter-doctor-report
branch
from
7dfeb4a to
c62a5d4
Compare
|
@fujitatomoya sry for pinging again. I have pushed the changes, pls review. |
2 participants
Description
Add NodeCheck/NodeReport and ParameterCheck/ParameterReport so the ros2doctor report shows active nodes and their parameters.
ros2doctor/api/node.py(node discovery, NodeCheck, NodeReport)ros2doctor/api/parameter.py(parameter discovery, ParameterCheck, ParameterReport)ros2doctor/setup.py:ros2doctor.checks: NodeCheckandParameterCheckros2doctor.report: NodeReportandParameterReportWhy:
This enhancement provides valuable insights into the active nodes and their parameters within a ROS 2 system. By including this information in the
ros2 doctorreport sections in theros2 doctor --reportoutput, giving users better visibility into the runtime ROS 2 system configuration.Fixes: #1090
Is this user-facing behavior change?
Yes
Did you use Generative AI?
Additional Information