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.

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

ros2 topic pub starts publishing right away. #626

Merged
merged 1 commit into from
May 12, 2021

Conversation

fujitatomoya
Copy link
Collaborator

address #624

Signed-off-by: Tomoya Fujita [email protected]

@fujitatomoya
Copy link
Collaborator Author

@clalancette @ivanpauno what do you think? i'd like to hear your opinion.

@clalancette
Copy link
Contributor

I think we should punt on this for now; the issue isn't that important and I don't think we should change it for Galactic. I'll add this to H-Turtle board instead.

Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

The change looks good and I tested that it worked on my machine.

I think maybe the true solution is to change the timer behavior in rcl, but this might be an easier way to make ros2 topic pub perform how the user expects.

ros2topic/ros2topic/verb/pub.py Show resolved Hide resolved
@audrow audrow self-assigned this Apr 23, 2021
@audrow
Copy link
Member

audrow commented Apr 23, 2021

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

I think maybe the true solution is to change the timer behavior in rcl

I don't think that's a good idea.
e.g.: if you want a "one-shot timer", what you currently do is to register a timer and cancel it from the callback.
If the first call happens immediatly that wouldn't work.

@fujitatomoya
Copy link
Collaborator Author

e.g.: if you want a "one-shot timer", what you currently do is to register a timer and cancel it from the callback.

yeah i am with @ivanpauno on this. application can have more control for use cases.

@audrow
Copy link
Member

audrow commented May 7, 2021

Rerunning CI to double check that things are good:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya force-pushed the topic-20210419-ros2topic-pub branch from c0bba6d to 5e807e6 Compare May 9, 2021 02:37
@fujitatomoya
Copy link
Collaborator Author

I believe that rebase required to include #616, retry CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

@ros-pull-request-builder retest this please

@ivanpauno
Copy link
Member

@fujitatomoya could you confirm if the Rpr failure is unrelated or not?

Besides that failure, this seems to be ready to be merged.

@fujitatomoya
Copy link
Collaborator Author

@ivanpauno thanks!

https://build.ros2.org/job/Rpr__ros2cli__ubuntu_focal_amd64/223/testReport/ros2topic.ros2topic.test/test_cli/test_cli/ is not related to the fix. it is failure of ros2 topic info not pub, i do not see the relationship from the test implementation. besides, i cannot reproduce this failure on my local environment.

(Just FYI, https://build.ros2.org/job/Rpr__ros2cli__ubuntu_focal_amd64/223/testReport/ros2topic.ros2topic.test/test_cli/test_cli/ does have the expected output which is Type: std_msgs/msg/String\n\nPublisher count: 1\n\nNode name: talker_node\nNode namespace: /\nTopic type: std_msgs/ms...ds\n Liveliness: AUTOMATIC\n Liveliness lease duration: 9223372036854775807 nanoseconds\n\nSubscription count: 0\n\n. i do not really see why this is failing.)

@ivanpauno
Copy link
Member

Ah, I know the problem

Liveliness lease duration: 9223372036854775807

ros2/rclpy#722, that change wasn't released into rolling when the Rpr builder last run (and the tests here were already updated).

It should pass now:

@ros-pull-request-builder retest this please.

@ivanpauno ivanpauno merged commit edbc160 into ros2:master May 12, 2021
@fujitatomoya
Copy link
Collaborator Author

@ivanpauno thanks for the support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants