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

Add liveliness and lease duration to pub and echo #665

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from

Conversation

NourS-d
Copy link

@NourS-d NourS-d commented Sep 1, 2021

Closes Issue #614

@NourS-d NourS-d force-pushed the saeed/add-liveliness-and-duration branch from 728ebed to d375c91 Compare September 1, 2021 20:02
@NourS-d
Copy link
Author

NourS-d commented Sep 1, 2021

Pub

$ ros2 topic pub /demo/test_demo geometry_msgs/Twist '{linear: {x: -0.0}}' --qos-liveliness-lease-duration 20e9

Echo

$ ros2 topic echo /demo/test_demo geometry_msgs/Twist --qos-liveliness-lease-duration 20e9 
1630525300.053282 [0]       ros2: using network interface wlp2s0 (udp/192.168.43.167) selected arbitrarily from: wlp2s0, docker0
linear:
  x: -0.0
  y: 0.0
  z: 0.0
angular:
  x: 0.0
  y: 0.0
  z: 0.0
---
$ ros2 topic echo /demo/test_demo geometry_msgs/Twist --qos-liveliness-lease-duration 30e9 
1630525306.729698 [0]       ros2: using network interface wlp2s0 (udp/192.168.43.167) selected arbitrarily from: wlp2s0, docker0
linear:
  x: -0.0
  y: 0.0
  z: 0.0
angular:
  x: 0.0
  y: 0.0
  z: 0.0
---
$ ros2 topic echo /demo/test_demo geometry_msgs/Twist --qos-liveliness-lease-duration 10e9 
1630525312.813764 [0]       ros2: using network interface wlp2s0 (udp/192.168.43.167) selected arbitrarily from: wlp2s0, docker0
[WARN] [1630525313.416047065] [_ros2cli_121115]: New publisher discovered on topic '/demo/test_demo', offering incompatible QoS. No messages will be received from it. Last incompatible policy: LIVELINESS

@NourS-d NourS-d force-pushed the saeed/add-liveliness-and-duration branch from d375c91 to 54bc7ed Compare September 1, 2021 20:30
@NourS-d NourS-d marked this pull request as ready for review September 1, 2021 20:31
@NourS-d
Copy link
Author

NourS-d commented Sep 1, 2021

@audrow Hello, I created a PR for this since it looked relatively straightforward.
#614 (comment)
Do you believe some special reasoning is needed for liveliness?

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

just one nit, could you fix it?

ros2topic/ros2topic/verb/echo.py Outdated Show resolved Hide resolved
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.

Just a couple nitpicks - thanks for the PR!

ros2topic/ros2topic/api/__init__.py Outdated Show resolved Hide resolved
ros2topic/test/test_qos_conversions.py Outdated Show resolved Hide resolved
@audrow audrow self-assigned this Sep 9, 2021
@NourS-d NourS-d force-pushed the saeed/add-liveliness-and-duration branch from 54bc7ed to 03dfdba Compare September 10, 2021 19:44
@audrow
Copy link
Member

audrow commented Sep 10, 2021

@ros-pull-request-builder retest this please. (I'm not sure if the last change broke something, I doubt it did.)

@audrow
Copy link
Member

audrow commented Sep 10, 2021

Do you believe some special reasoning is needed for liveliness?

I'm not sure. @ivanpauno or @jacobperron, any idea?

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

I have a few comments/questions

ros2topic/ros2topic/api/__init__.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/api/__init__.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/api/__init__.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/api/__init__.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/api/__init__.py Outdated Show resolved Hide resolved
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.

Looks okay to me, with Ivan's comments addressed. One bug fix suggested below.

ros2topic/ros2topic/verb/echo.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/api/__init__.py Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

@NourS-d friendly ping

@NourS-d NourS-d force-pushed the saeed/add-liveliness-and-duration branch from 538c4b8 to 2295f64 Compare October 31, 2021 12:08
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM with green CI!

@ivanpauno
Copy link
Member

@ros-pull-request-builder retest this please

@ivanpauno
Copy link
Member

CI:

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

@ivanpauno
Copy link
Member

there are some new failures in CI, but maybe this just needs a rebase.
@NourS-d could you rebase with master?

If you can also check if you can reproduce the test failures locally that would be great.

@audrow audrow force-pushed the saeed/add-liveliness-and-duration branch 2 times, most recently from 98abe76 to 2b72a29 Compare December 23, 2021 00:19
@audrow
Copy link
Member

audrow commented Dec 23, 2021

I rebased - it seems that the same error is present in the failing PR job. It seems to be a key error.

Traceback (most recent call last):
  File "/tmp/ws/src/ros2cli/ros2topic/test/test_cli.py", line 760, in test_topic_pub_print_every_two
    assert topic_command.wait_for_output(functools.partial(
AssertionError: Output does not match: Failed to load entry point 'pub': cannot import name 'nonnegative_int' from 'ros2topic.api' (/tmp/ws/install_isolated/ros2topic/lib/python3.8/site-packages/ros2topic/api/__init__.py)
  Traceback (most recent call last):
    File "/tmp/ws/install_isolated/ros2cli/bin/ros2", line 33, in <module>
      sys.exit(load_entry_point('ros2cli==0.15.0', 'console_scripts', 'ros2')())
    File "/tmp/ws/install_isolated/ros2cli/lib/python3.8/site-packages/ros2cli/cli.py", line 50, in main
      add_subparsers_on_demand(
    File "/tmp/ws/install_isolated/ros2cli/lib/python3.8/site-packages/ros2cli/command/__init__.py", line 250, in add_subparsers_on_demand
      extension.add_arguments(
    File "/tmp/ws/install_isolated/ros2topic/lib/python3.8/site-packages/ros2topic/command/topic.py", line 29, in add_arguments
      add_subparsers_on_demand(
    File "/tmp/ws/install_isolated/ros2cli/lib/python3.8/site-packages/ros2cli/command/__init__.py", line 237, in add_subparsers_on_demand
      extension = command_extensions[name]
  KeyError: 'pub'

@fujitatomoya
Copy link
Collaborator

cannot import name 'nonnegative_int' from 'ros2topic.api' (/tmp/ws/install_isolated/ros2topic/lib/python3.8/site-packages/ros2topic/api/init.py)

nonnegative_int has been removed, i think it should be moved to ros2topic/ros2topic/api/__init__.py.

@audrow
Copy link
Member

audrow commented Apr 27, 2022

@NourS-d, would you like to get this in?

@audrow audrow removed their request for review April 27, 2022 22:37
@NourS-d
Copy link
Author

NourS-d commented Apr 28, 2022 via email

@audrow
Copy link
Member

audrow commented Apr 28, 2022

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Apr 28, 2022

rebase

✅ Branch has been successfully rebased

@clalancette clalancette force-pushed the saeed/add-liveliness-and-duration branch from 2b72a29 to 85bc897 Compare April 28, 2022 16:24
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:23
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.

6 participants