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

Final batch of examples #327

Merged
merged 17 commits into from Nov 30, 2021
Merged

Final batch of examples #327

merged 17 commits into from Nov 30, 2021

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Oct 13, 2021

Moved from ros2/launch_ros#269
Signed-off-by: Aditya Pande aditya050995@gmail.com

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

@ivanpauno if I make number_of_nodes as a launch argument, I won't be able to use it in generate_launch_description() to launch nodes based on that number

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Oct 14, 2021

TODO :

  • Split this example into rosbag2 and wait_for_node action ✔️
  • For rosbag2, make sure the bag file is written consistently in a temp directory ✔️
  • For wait_for_node action : Write a complete class like we did for WaitForTopics complete with docstrings, so that we can move it to launch_testing_ros later. (Add a TODO in the comment) ✔️
  • Remove duplicate examples from original repos once we're done with "moving" examples

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@ivanpauno
Copy link
Member

@ivanpauno if I make number_of_nodes as a launch argument, I won't be able to use it in generate_launch_description() to launch nodes based on that number

You can, it's just a bit tricky.
We should have something in launch to make it easier, let me see if I can provide an snippet showing how to do it.

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
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.

Thanks for adding these examples! Most of my comments below are minor nitpicks.

launch_testing/launch_testing_examples/README.md Outdated Show resolved Hide resolved
launch_testing/launch_testing_examples/README.md Outdated Show resolved Hide resolved
launch_testing/launch_testing_examples/README.md Outdated Show resolved Hide resolved
launch_testing/launch_testing_examples/package.xml Outdated Show resolved Hide resolved

def test_delay(self):
"""Delay the shutdown of processes so that rosbag can record some messages."""
time.sleep(3)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a potential gap in launch_testing. It would be nice if there was a launch_testing feature that could delay the shutdown event. I know there is a KeepAlive action, but I don't know if that is exactly what we want here.

Copy link
Member

Choose a reason for hiding this comment

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

You can just add a Timer action in the launch description with a 3s period that does nothing.
That will keep the launch service alive and delay the shutdown.

It's still kind of a hack though.

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 @jacobperron comments addressed

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
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.

LGTM with green CI

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Oct 27, 2021

CI :

build : --packages-above-and-dependencies launch_testing_examples
test : --packages-above launch_testing_examples

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

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Oct 27, 2021

CI round 2 :
build : --packages-above-and-dependencies launch_testing_examples
test: --packages-above launch_testing_examples

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

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

CI round 3 :

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

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Oct 28, 2021

Not sure why but Windows CI can't find the metadata.yaml file generated by the rosbag.

@ivanpauno
Copy link
Member

Not sure why but Windows CI can't find the metadata.yaml file generated by the rosbag.

I have no idea, I would recommend testing on a windows VM.

Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Nov 30, 2021

CI : Disabled the test record_rosabag_launch_test.py on windows

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

Macos rerun : Build Status

@adityapande-1995
Copy link
Contributor Author

rosbag_record_launch_test.py has been disabled on windows for now due to CI. Relevant issue : ros2/rosbag2#926

Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995 adityapande-1995 merged commit 35b181c into master Nov 30, 2021
@delete-merged-branch delete-merged-branch bot deleted the aditya/add_examples_final branch November 30, 2021 23:51
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.

None yet

3 participants