-
Notifications
You must be signed in to change notification settings - Fork 552
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.
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
[rospy_tutorials] Add example of periodical publishing with rospy.Timer #34
Conversation
try: | ||
rospy.init_node('talker', anonymous=True) | ||
pub = rospy.Publisher('chatter', String, queue_size=10) | ||
timer = rospy.Timer(rospy.Duration(1. / 10), publish_callback) # 10Hz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Timer
expects a Time
as the first argument. While passing a Duration
works it is less intuitive imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was obviously wrong. Passing Time
doesn't work and the test fails with a stacktrace. But somehow the PR job didn't detect the problem.
I will look into the job. Once it reports the failure correctly we can make it pass by removing the second commit (which I asked for).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, is this line wrong?
https://github.com/ros/ros_comm/blob/kinetic-devel/clients/rospy/src/rospy/timer.py#L188
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrong docstring will be fixed via ros/ros_comm#878.
Fixed. |
@ros-pull-request-builder retest this please |
@@ -0,0 +1,4 @@ | |||
<launch> | |||
<node name="talker" pkg="rospy_tutorials" type="talker_timer.py" /> | |||
<test test-name="talker_listener_test" pkg="rospy_tutorials" type="talker_listener_test.py" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test-name
is the same as for another test. Please add a suffix (_with_timer
) to it in order to make it unique.
Fixed. |
<test test-name="talker_listener_test" pkg="rospy_tutorials" type="talker_listener_test.py" /> | ||
|
||
<test test-name="talker_listener_test_with_timer" | ||
name="talker_listener_test_with_timer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't have a name
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, should the name
be specified when the node name is used in testing to resolve name of private topic name or param name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know - you might want to try it.
But this test doesn't have anything like that.
Great, now the PR job detected the failing test correctly. Can you please revert the second commit now and use |
Fixed. |
Thank you for iterating on this and fixing the documentation. |
Thanks! |
As suggested in ros/ros_comm#864 (comment)