Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ros2doctor: add topic check #341

Merged
merged 6 commits into from
Oct 1, 2019
Merged

ros2doctor: add topic check #341

merged 6 commits into from
Oct 1, 2019

Conversation

claireyywang
Copy link
Contributor

@claireyywang claireyywang commented Sep 25, 2019 •
edited
Loading

Check for cases like pub without sub or sub without pub using ros2topic and ros2cli API. Report contains same content as ros2 topic info outputs. Also adds unit tests for TopicCheck and TopicReport.

add topic check&report
024a478 
Signed-off-by: claireyywang <clairewang@openrobotics.org>
@claireyywang claireyywang added the in review Waiting for review (Kanban column) label Sep 25, 2019
@claireyywang claireyywang self-assigned this Sep 25, 2019
claireyywang added 2 commits September 30, 2019 11:48
add topic check&report
434be9c 
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Merge branch 'claire/ros2doctor-topic' of https://github.com/ros2/ros…
9095d55 
…2cli into claire/ros2doctor-topic

Signed-off-by: claireyywang <clairewang@openrobotics.org>
jacobperron
jacobperron reviewed Sep 30, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Can you provide some steps to test this feature? I'm not sure what changes to expect in the output.

ros2doctor/ros2doctor/api/topic.py Outdated
from ros2doctor.api import DoctorReport
from ros2doctor.api import Report
from ros2doctor.api import Result
from ros2topic.api import get_topic_names
Copy link
Member

Choose a reason for hiding this comment

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

We should declare a dependency to ros2topic in the package.xml.
But, IMO, it would be nicer if we didn't have to depend on another CLI tool and instead call the rclpy API directly.

ros2doctor/ros2doctor/api/topic.py Outdated
result = Result()
to_be_checked = _get_topics()
for topic in to_be_checked:
with DirectNode(topic) as node:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the overhead is for repeatedly creating and destroying DirectNode, but in any case, it looks like we can safely use the same instance for the duration of the for-loop. I recommend restructuring like:

with DirecNode(topic) as node:
    for topic in to_be_checked:

ros2doctor/ros2doctor/api/topic.py Outdated
if not to_be_reported:
report.add_to_report('topic', 'none')
for topic in to_be_reported:
with DirectNode(topic) as node:
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above. Consider changing the scope of the node context.

claireyywang added 2 commits September 30, 2019 17:03
add topic test, tbc
e96072c 
Signed-off-by: claireyywang <clairewang@openrobotics.org>
add topic report unit test
3c6a1f8 
Signed-off-by: claireyywang <clairewang@openrobotics.org>
@claireyywang
Copy link
Contributor Author

Can you provide some steps to test this feature? I'm not sure what changes to expect in the output.

ros2 doctor -r should output (when no topics are publishing/subscribing)

TOPIC LIST
topic               : none
publisher count     : 0
subscirber count    : 0

ros2 doctor --report-fail --include-warning when publishing on /chatter should output

/home/clairewang/ros2_ws/install/ros2doctor/lib/python3.6/site-packages/ros2doctor/api/format.py: 76: UserWarning: Distribution eloquent is not fully supported or tested. To get more consistent features, download a stable version at https://index.ros.org/doc/ros2/Installation/
/home/clairewang/ros2_ws/install/ros2doctor/lib/python3.6/site-packages/ros2doctor/api/format.py: 76: UserWarning: Publisher without subscriber detected on /chatter.

2/3 checks failed

Failed modules are  platform topic

  PLATFORM INFORMATION
system           : Linux
platform info    : Linux-5.0.0-29-generic-x86_64-with-Ubuntu-18.04-bionic
release          : 5.0.0-29-generic
processor        : x86_64

  ROS 2 INFORMATION
distribution name      : eloquent
distribution type      : ros2
distribution status    : prerelease
release platforms      : {'ubuntu': ['bionic']}

  TOPIC LIST
topic               : /chatter
publisher count     : 1
subscriber count    : 0

jacobperron
jacobperron approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM with green CI.

ros2doctor/ros2doctor/api/topic.py Outdated Show resolved Hide resolved
correct docstring
f6137a5 
Signed-off-by: claireyywang <clairewang@openrobotics.org>
@claireyywang
Copy link
Contributor Author

claireyywang commented Oct 1, 2019 •
edited
Loading

Linux Build Status
Linux-aaaaaaaarch64 Build Status
OSX Build Status
Windows Build Status

@claireyywang claireyywang merged commit 3c30b34 into master Oct 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the claire/ros2doctor-topic branch October 1, 2019 23:19
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
@methylDragon
Implement None check for ComposableNodeContainer (ros2#341)
35b2ca9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron approved these changes

@sloretz sloretz Awaiting requested review from sloretz

Assignees

@claireyywang claireyywang

Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@claireyywang @jacobperron