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

Print 'Infinite' for infinite durations in topic endpoint info #722

Merged
merged 3 commits into from
May 6, 2021
Merged

Print 'Infinite' for infinite durations in topic endpoint info #722

merged 3 commits into from
May 6, 2021

Conversation

emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Mar 19, 2021 •
edited
Loading

Sample output for ros2 topic pub /chatter std_msgs/String "data: hello":

$ ros2 topic info -v /chatter
Type: std_msgs/msg/String

Publisher count: 1

Node name: _ros2cli_30830
Node namespace: /
Topic type: std_msgs/msg/String
Endpoint type: PUBLISHER
GID: 2d.26.10.01.fb.93.9b.48.9d.7e.d3.24.00.00.08.03.00.00.00.00.00.00.00.00
QoS profile:
  Reliability: RELIABLE
  Durability: VOLATILE
  Lifespan: Infinite
  Deadline: Infinite
  Liveliness: AUTOMATIC
  Liveliness lease duration: Infinite

Subscription count: 0

Connected to ros2/ros2cli#616

@emersonknapp emersonknapp force-pushed the emersonknapp/topic-info-duration-infinite branch from 03fd46f to 7abda81 Compare March 22, 2021 18:53
@emersonknapp
Copy link
Contributor Author

@sloretz @ivanpauno as maintainers - requesting a review when you can

ivanpauno
ivanpauno approved these changes Mar 22, 2021
View reviewed changes
rclpy/rclpy/topic_endpoint_info.py Outdated Show resolved Hide resolved
rclpy/rclpy/topic_endpoint_info.py Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy_pybind11.cpp Outdated Show resolved Hide resolved
@emersonknapp emersonknapp force-pushed the emersonknapp/topic-info-duration-infinite branch from bf288c9 to 37e6e4f Compare March 31, 2021 01:22
@emersonknapp
Copy link
Contributor Author

@ivanpauno requesting re-review now that I was able to address all the comments, given the latest state of the pybind code

@emersonknapp
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/emersonknapp/8243fb1b94c37e0c9a07ec4c5edfb2a5/raw/bc58898af9aa8ad525c5d97279271f0b8c9f4734/ros2.repos
BUILD args: --packages-up-to rclpy
TEST args: --packages-select rclpy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8046

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

@ivanpauno
Copy link
Member

Running tests above rclpy, I'm afraid some ros2cli test might depend on how a duration is printed:

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

@emersonknapp emersonknapp mentioned this pull request Mar 31, 2021
Merged
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 31, 2021 •
edited
Loading

CI with packages-above rclpy testing, including ros2/ros2cli#616

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

@emersonknapp emersonknapp force-pushed the emersonknapp/topic-info-duration-infinite branch from 37e6e4f to a491ba1 Compare April 1, 2021 17:10
@ivanpauno
Copy link
Member

CI with packages-above rclpy testing, including ros2/ros2cli#616

It doesn't seem to be including ros2/ros2cli#616, again including it:

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

@emersonknapp emersonknapp force-pushed the emersonknapp/topic-info-duration-infinite branch from a491ba1 to f133bfc Compare April 5, 2021 17:47
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Apr 5, 2021 •
edited
Loading

I think the failures were because my branches were out of date with the fast development on rclpy right now

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

^ note: rviz_ogre_vendor timed out build on Windows - should I just try it again?

@ivanpauno
Copy link
Member

ivanpauno commented Apr 5, 2021 •
edited
Loading

^ note: rviz_ogre_vendor timed out build on Windows - should I just try it again?

👍

  • Windows Build Status

@emersonknapp
Copy link
Contributor Author

emersonknapp commented Apr 5, 2021 •
edited
Loading

CMake warning is from eclipse-cyclonedds/cyclonedds#741

It looks like we are good to merge this and ros2/ros2cli#616

@ivanpauno
Copy link
Member

Sorry, I wasn't available yesterday late (I work on UTC-3 time zone) and now we're past the API/feature freeze.

I'm not completely sure if this passes the post API/feature "safe to merge" criteria, I'm in favor of merging it as it makes ros2cli output more clear and I don't think this can break downstream code.

@hidmic could you do a second review?

@emersonknapp
Copy link
Contributor Author

Thanks @audrow ! Does this mean we're planning to get this into Galactic?

@audrow
Copy link
Member

audrow commented Apr 16, 2021

I believe so. Let me just confirm with someone more senior on the team.

@hidmic
Copy link
Contributor

hidmic commented Apr 16, 2021

@hidmic could you do a second review?

Argh, I didn't see this. My apologies.

hidmic
hidmic approved these changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM, but the line between bug fix and feature is quite blurry here. Feature freeze was almost two weeks ago. If we merge this for Galactic, we'd have to make an exception. @cottsay ?

rclpy/rclpy/duration.py Outdated Show resolved Hide resolved
@cottsay
Copy link
Member

cottsay commented Apr 16, 2021

If we merge this for Galactic, we'd have to make an exception

I agree. Can we wait until we branch galactic next week to merge this into rolling? And then consider it for backport into Galactic after we release it?

@emersonknapp emersonknapp force-pushed the emersonknapp/topic-info-duration-infinite branch from f133bfc to 5b8cccc Compare April 16, 2021 18:22
@emersonknapp
Copy link
Contributor Author

Whatever y'all want to do

@ivanpauno
Copy link
Member

This can be merged now.
Running an extra round of CI, just in case:

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

@ivanpauno ivanpauno merged commit 901fb47 into ros2:master May 6, 2021
@ivanpauno ivanpauno mentioned this pull request May 12, 2021
Merged
@DLu
Copy link

DLu commented Feb 2, 2024

Note: I came here to fix this, not realizing the problem was that I was running foxy. Thanks! Still, might be worth documenting:
ros2/ros2_documentation#4157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@audrow audrow audrow approved these changes

@hidmic hidmic hidmic approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@emersonknapp @ivanpauno @audrow @hidmic @cottsay @DLu @sloretz