Skip to content

Fix 'ros2 topic hz' #1003

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

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

Conversation

christophebedard
Copy link
Member

Fixes #1002

Follow-up to #998, which backported a commit from Rolling to Jazzy that included unrelated changes that Jazzy doesn't have:

  1. _rostopic_hz() does not expect the qos_args arg, since Jazzy doesn't have Add support for topic QOS for ros2topic bw, delay and hz #935
  2. args.topic_name contains a single topic name, since Jazzy doesn't have Support multiple topics via ros2 topic hz. #929, which also means that _rostopic_hz() only expects a single topic name, and not a list

Run a node:

$ ros2 run examples_rclcpp_minimal_publisher publisher_member_function

Before:

$ ros2 topic hz /topic
Traceback (most recent call last):
  File "/home/christophe.bedard/ros2_ws_jazzy/install/ros2cli/bin/ros2", line 33, in <module>
    sys.exit(load_entry_point('ros2cli', 'console_scripts', 'ros2')())
  File "/home/christophe.bedard/ros2_ws_jazzy/build/ros2cli/ros2cli/cli.py", line 91, in main
    rc = extension.main(parser=parser, args=args)
  File "/home/christophe.bedard/ros2_ws_jazzy/build/ros2topic/ros2topic/command/topic.py", line 41, in main
    return extension.main(args=args)
  File "/home/christophe.bedard/ros2_ws_jazzy/build/ros2topic/ros2topic/verb/hz.py", line 87, in main
    return main(args)
  File "/home/christophe.bedard/ros2_ws_jazzy/build/ros2topic/ros2topic/verb/hz.py", line 152, in main
    _rostopic_hz(node.node, topics, qos_args=args, window_size=args.window_size,
TypeError: _rostopic_hz() got an unexpected keyword argument 'qos_args'

If I only fix (1) and not (2), the topic name string gets split into single characters:

$ ros2 topic hz topic --filter 'True'
WARNING: topic [t] does not appear to be published yet

After:

$ ros2 topic hz topic --filter 'True'
average rate: 2.000
        min: 0.500s max: 0.500s std dev: 0.00011s window: 3
average rate: 2.000
        min: 0.499s max: 0.501s std dev: 0.00058s window: 6

Signed-off-by: Christophe Bedard <[email protected]>
@fujitatomoya
Copy link
Collaborator

i was wondering why we could not detect this failure, turns out CI parameter is wrong, ros2topic was not tested. https://ci.ros2.org/job/ci_launcher/15562/parameters/ i missed that, sorry.

@christophebedard
Copy link
Member Author

Yeah, I looked into that. Just a simple mistake.

Some of the CI jobs listed here in the original PR were unrelated to the PR: #998 (review). Only the ci_linux-aarch64 and ci_windows jobs tested the PR. The test_cli tests aren't run on Windows, but they ran in the ci_linux-aarch64 job and failed: https://ci.ros2.org/job/ci_linux-aarch64/17214/testReport/ros2topic.ros2topic.test/test_cli/test_cli/.

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.

lgtm with green CI.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Apr 4, 2025

Pulls: #1003
Gist: https://gist.githubusercontent.com/fujitatomoya/8395f8fbc368ec4dfa2a196a6add027c/raw/dd79551d329f529c5ffa0d783984e0baee8f6169/ros2.repos
BUILD args: --packages-above-and-dependencies ros2topic
TEST args: --packages-above ros2topic
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15595

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Signed-off-by: Christophe Bedard <[email protected]>
@christophebedard
Copy link
Member Author

There were linter issues :/ I'll abort and re-trigger the CI jobs

@christophebedard
Copy link
Member Author

christophebedard commented Apr 4, 2025

It doesn't like the data attribute:

'int(m.data.rpartition(\":\")[-1]) % 2 == 0',

    File "/home/jenkins-agent/workspace/ci_linux-aarch64/ws/install/ros2topic/lib/python3.12/site-packages/ros2topic/eval/__init__.py", line 78, in generic_visit
      raise ValidationException(
  ros2topic.eval.ValidationException: Attribute data is not allowed

See https://ci.ros2.org/job/ci_linux-aarch64/17257/testReport/ros2topic.ros2topic.test/test_cli/test_cli/.

Does this mean that the "safe evaluation" will reject most expressions that try to access a message's fields?

I'll have to pick this back up this weekend.

@christophebedard
Copy link
Member Author

Reverting the original change for now: #1004

@christophebedard
Copy link
Member Author

We'll probably end up addressing these issues in the new Rolling PR (#1001) and then backport it to Jazzy.

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.

2 participants