[WIP] :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 NodeCheck(DoctorCheck): | ||
| """Check for duplicate node names.""" | ||
|
|
||
| def category(self): | ||
| return 'node' | ||
|
|
||
| def check(self): | ||
| result = Result() | ||
| with NodeStrategy(None) as node: | ||
| node_names = get_node_names(node, include_hidden_nodes=True) | ||
| if has_duplicates(node_names): | ||
| """ Count duplicates """ | ||
| name_counts = defaultdict(int) | ||
| for name in node_names: | ||
| name_counts[name] += 1 | ||
|
|
||
| duplicates = [name for name, count in name_counts.items() if count > 1] | ||
| for duplicate in duplicates: | ||
| doctor_warn(f'Duplicate node name: {duplicate}') | ||
| result.add_warning() | ||
| return result | ||
|
|
||
|
|
||
| class NodeReport(DoctorReport): | ||
| """Report node related information.""" | ||
|
|
||
| def category(self): | ||
| return 'node' | ||
|
|
||
| def report(self): | ||
| report = Report('NODE LIST') | ||
| with NodeStrategy(None) as node: | ||
| node_names = get_node_names(node, include_hidden_nodes=True) | ||
| if not node_names: | ||
| report.add_to_report('node count', 0) | ||
| report.add_to_report('node', 'none') | ||
| else: | ||
| report.add_to_report('node count', len(node_names)) | ||
| for node_name in sorted(node_names): | ||
| report.add_to_report('node', node_name) | ||
| return report |
There was a problem hiding this comment.
The new NodeCheck and NodeReport 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.
| 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
2 times, most recently
from
3e215aa to
4a08d58
Compare
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
4a08d58 to
a7ffe55
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Bhuvan B <bhuvanb06@protonmail.com>
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. |
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