Skip to content

Port rostopic bw#190

Merged
mjcarroll merged 3 commits intoros2:masterfrom
yechun1:port_rostopic_bw
Apr 17, 2019
Merged

Port rostopic bw#190
mjcarroll merged 3 commits intoros2:masterfrom
yechun1:port_rostopic_bw

Conversation

@yechun1
Copy link
Copy Markdown
Contributor

@yechun1 yechun1 commented Jan 11, 2019 •
edited
Loading

Port rostopic bw from ROS1 to ROS2, enable the tool to display bandwidth used by topic.

Fixed the issue: #132
This patch requires ros2/rclpy#242

@tfoote tfoote added the in review Waiting for review (Kanban column) label Jan 11, 2019
@yechun1 yechun1 mentioned this pull request Jan 11, 2019
Closed
3 tasks
mjcarroll
mjcarroll reviewed Jan 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Would it be possible to add some tests just to verify the basic behavior of the bw verb?

Probably something along the lines of the info test would be suitable (https://github.com/ros2/ros2cli/blob/master/ros2topic/test/test_info.py)

dirk-thomas
dirk-thomas reviewed Jan 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Please take some time to actually write a description for your PR even when it is short. Also mentioning that this patch requires ros2/rclpy#242 would be helpful.

LucidOne
LucidOne approved these changes Jan 11, 2019
View reviewed changes
Copy link
Copy Markdown

@LucidOne LucidOne left a comment

Choose a reason for hiding this comment

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

👍

ros2topic/ros2topic/verb/bw.py Outdated
def add_arguments(self, parser, cli_name):
arg = parser.add_argument(
'topic',
help='Topic name to be calcurated the delay')
Copy link
Copy Markdown

@LucidOne LucidOne Jan 11, 2019

Choose a reason for hiding this comment

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

This one is actually in the original import.
'Topic name to monitor for bandwidth utilization'

Copy link
Copy Markdown
Contributor Author

@yechun1 yechun1 Jan 15, 2019

Choose a reason for hiding this comment

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

thanks, fixed in c94f22e

@yechun1
Copy link
Copy Markdown
Contributor Author

yechun1 commented Jan 14, 2019

Would it be possible to add some tests just to verify the basic behavior of the bw verb?

@mjcarroll I try to add test code but meet a issue that colcon test will not stop. I think the bw is a monitor, this will lead the test run into loop. Do you have any idea based with below test code?

def test_bw_publishers_subscribers():
    args = Namespace()
    args.topic = '/test_ros2_topic_cli'
    args.window = 100
    s = StringIO()
    with redirect_stdout(s):
        bw_verb = BwVerb()
        bw_verb.main(args=args)
        expected_output = _generate_expected_output(0, 0, 0, 0, args.window)
        assert expected_output == s.getvalue().splitlines()

@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Apr 10, 2019

Hey @yechun1 , sorry for the delay. Can you rebase and --signoff your commits to appease the DCO and I will run a CI round on this?

yechun1 added 3 commits April 11, 2019 14:18
@yechun1
Copy original file for ros2topic bw porting
f7824ed 
This file is originally copied from: https://github.com/ros/ros_comm/blob/6e5016f4b2266d8a60c9a1e163c4928b8fc7115e/tools/rostopic/src/rostopic/__init__.py

Signed-off-by: Chris Ye <chris.ye@intel.com>
@yechun1
add rostopic bw original file link
8c20f79 
Signed-off-by: Chris Ye <chris.ye@intel.com>
@yechun1
port rostopic bw to ros2topic
1854c78 
enable ros2topic bw to display bandwidth used by topic.

Signed-off-by: Chris Ye <chris.ye@intel.com>
@yechun1
Copy link
Copy Markdown
Contributor Author

yechun1 commented Apr 11, 2019

Hi @mjcarroll , just rebased code and re-submitted.
Sorry I'm not familiar with test code, and did not add tests to verify the basic behavior of the bw verb.

@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Apr 17, 2019

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

@mjcarroll mjcarroll merged commit d0bfb5b into ros2:master Apr 17, 2019
@mjcarroll mjcarroll removed the in review Waiting for review (Kanban column) label Apr 17, 2019
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Apr 17, 2019

Thanks for the contribution!

@yechun1
Copy link
Copy Markdown
Contributor Author

yechun1 commented Apr 18, 2019

thanks for approval and merge.

@yechun1 yechun1 deleted the port_rostopic_bw branch April 18, 2019 00:44
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
@ivanpauno
Remove constructors arguments deprecated since Foxy (ros2#190)
a8c12a9 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

+2 more reviewers

@LucidOne LucidOne LucidOne approved these changes

@dirk-thomas dirk-thomas dirk-thomas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mjcarroll mjcarroll

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yechun1 @mjcarroll @LucidOne @dirk-thomas @tfoote