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

Added README.md for image_tools #571

Closed
wants to merge 20 commits into from
Closed

Added README.md for image_tools #571

wants to merge 20 commits into from

Conversation

cardboardcode
Copy link
Contributor

@cardboardcode cardboardcode commented Jul 26, 2022 •
edited
Loading

Purpose of Pull Request

This pull request adds a README.md to image_tools as part of efforts to address the issue highlighted in #531.

Edits Made:

  1. Added instructional README.md under /image_tools directory.
  2. Added supplementary .png picture for verifying that the ROS2 package works.

@audrow audrow self-assigned this Jul 26, 2022
@audrow audrow self-requested a review July 26, 2022 17:01
@cardboardcode cardboardcode marked this pull request as draft August 13, 2022 16:19
@cardboardcode cardboardcode marked this pull request as ready for review January 23, 2023 09:13
@cardboardcode
Copy link
Contributor Author

Apologies for the long delay. I now have the bandwidth to see this Pull Request through now that I am on holiday.

Will respond to requests to edit/improve the added README.md promptly.

adityapande-1995
adityapande-1995 reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@adityapande-1995 adityapande-1995 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 this ! Looks good to me with the following minor comments resolved.

image_tools/README.md Outdated Show resolved Hide resolved
image_tools/README.md Outdated Show resolved Hide resolved
@cardboardcode
Copy link
Contributor Author

Hi @adityapande-1995 and @clalancette, thank you for the prompt review.

I have updated the current README.md draft based on your feedbacks in the following commits:
1 . b33a13c
2 . 9dad8f4

However, I have also further edited the draft to adhere to the style of instructions similar to the approved Pull Request 572.

Enquiries

  1. May I request for another quick 5-minute review?

  2. Would it be okay for me to force-push the commits as instructed by the DCO bot since it seems the signed commits are failing the checks?

@clalancette
Copy link
Contributor

  • Would it be okay for me to force-push the commits as instructed by the DCO bot since it seems the signed commits are failing the checks?

Yes, force-pushing is fine; it is pretty common practice, exactly to fix the DCO type issues.

cardboardcode and others added 20 commits January 24, 2023 00:00
@cardboardcode
➕ 📖 Added README.md for image_tools ROS2 package. Verified instructions.
1fa27c8 
Signed-off-by: Bey Hao Yun <[email protected]>
@jacobperron @cardboardcode
Change dependency from 'rosidl_cmake' to 'rosidl_default_generators' (r…
3cb7f35 
…os2#578)

This is the recommendation from the ROS 2 tutorials.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@jacobperron @cardboardcode
Exit with code 0 if ExternalShutdownException is raised (ros2#581)
8b6dd97 
* Exit with code 0 if ExternalShutdownException is raised

This is an expected exception from the executor on a keyboard interrupt, so I don't think it should be an error.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@cardboardcode @paudrow
Added README.md for dummy_map_server (ros2#572)
0381b27 
* ➕ 📖 Added README.md for dummy_map_server ROS2 package. Verified instructions.

Signed-off-by: Bey Hao Yun <[email protected]>

* Update dummy_robot/dummy_map_server/README.md

Decomposed sentence into smaller sizes.

Co-authored-by: Audrow Nash <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>

* Update dummy_robot/dummy_map_server/README.md

Reworded.

Co-authored-by: Audrow Nash <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>

* 🔨 Modified use of --packages-select to --packages-up-to based on feedback.

Signed-off-by: Bey Hao Yun <[email protected]>

* 🔨 Modified README.md further to adhere to feedback given.

Signed-off-by: Bey Hao Yun <[email protected]>

* 🔨 Modified README.md commands to adhere to higher cross-platform compatibility.

Signed-off-by: Bey Hao Yun <[email protected]>

* Update dummy_robot/dummy_map_server/README.md

Co-authored-by: Audrow Nash <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>

Signed-off-by: Bey Hao Yun <[email protected]>
Co-authored-by: Audrow Nash <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@jacobperron @cardboardcode
Remove action_msgs dependency (ros2#580)
46274b6 
It is now properly included in the rosidl dependency chain.

Signed-off-by: Jacob Perron <[email protected]>

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@ihasdapie @cardboardcode
Add demo for rclpy parameter client (ros2#566)
a693c57 
* WIP: python examples for async parameter client

Signed-off-by: Brian Chen <[email protected]>

* reorganize

Signed-off-by: Brian Chen <[email protected]>

* linting & use tempfile

Signed-off-by: Brian Chen <[email protected]>

* linting

Signed-off-by: Brian Chen <[email protected]>

* put async_param_server node in async_param_client

Signed-off-by: Brian Chen <[email protected]>

* address review comments

Signed-off-by: Brian Chen <[email protected]>

* delete param demo with static and non-static param

Signed-off-by: Brian Chen <[email protected]>

* tabs aren't valid yaml

Signed-off-by: Brian Chen <[email protected]>

* decrease demo complexity

Signed-off-by: Brian Chen <[email protected]>

* use parameter blackboard

Signed-off-by: Brian Chen <[email protected]>

* add warning if wait_for_services fails to demo

Signed-off-by: Brian Chen <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@clalancette @cardboardcode
Changelog.
d91c0d4 
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@clalancette @cardboardcode
0.22.0
97d20b1 
Signed-off-by: Bey Hao Yun <[email protected]>
@cardboardcode
fix memory leak (ros2#585)
3185617 
Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@clalancette @cardboardcode
Install the launch file for lifecycle_py. (ros2#586)
b873be2 
It wasn't being installed previously, which means it couldn't
be used with 'ros2 launch'

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@deepanshubansal01 @cardboardcode
Demo for pre and post set parameter callback support (ros2#565)
fd65651 
* local parameter callback support

Signed-off-by: deepanshu <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@clalancette @cardboardcode
Changelog.
15b84ab 
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@clalancette @cardboardcode
0.23.0
5d86c8a 
Signed-off-by: Bey Hao Yun <[email protected]>
@sloretz @cardboardcode
Make demo_nodes_cpp_native install stuff only when it builds (ros2#590)
f93181d 
* Make demo_nodes_cpp_native install stuff only when it builds

Signed-off-by: Shane Loretz <[email protected]>

* doing nothing -> skipping

Signed-off-by: Shane Loretz <[email protected]>

Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@audrow @cardboardcode
[rolling] Update maintainers - 2022-11-07 (ros2#589)
9d2d84c 
Signed-off-by: Bey Hao Yun <[email protected]>
@kagibson @paudrow @cardboardcode
Add README's for action_tutorials. (ros2#576)
7bb5f6e 
Co-authored-by: Audrow Nash <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@clalancette @cardboardcode
Update the demos to C++17. (ros2#594)
c908b8f 
The main reason to do this is so that we can compile the demos
with the clang static analyzer.  As of clang++-14 (what is in
Ubuntu 22.04), the default still seems to be C++14, so we need
to specify C++17 so that new things in the rclcpp headers work
properly.

Further, due to reasons I don't fully understand, I needed to
set CMAKE_CXX_STANDARD_REQUIRED in order for clang to really use
that version.  So set this as well.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Bey Hao Yun <[email protected]>
@cardboardcode
🔨 Adhered instructional details in image_tools README.md to previousl…
42143ad 
…y approved standards.

Signed-off-by: Bey Hao Yun <[email protected]>
@cardboardcode
🔨 Updated based on feedback to correct phrasing.
095f6b6 
Signed-off-by: Bey Hao Yun <[email protected]>
@cardboardcode
🔨 Updated based on feedback to change phrasing.
b8232d5 
Signed-off-by: Bey Hao Yun <[email protected]>
@cardboardcode cardboardcode force-pushed the add_image_tools_readme branch from 9dad8f4 to b8232d5 Compare January 23, 2023 16:00
audrow
audrow reviewed Jan 23, 2023
View reviewed changes
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.

I think that this PR should be rebased on a newly pulled rolling branch. That way, the PR doesn't have a bunch of unrelated commits.

@cardboardcode
Copy link
Contributor Author

cardboardcode commented Jan 23, 2023 •
edited
Loading

I think that this PR should be rebased on a newly pulled rolling branch. That way, the PR doesn't have a bunch of unrelated commits.

Will do. @audrow Should I create a new Pull Request for this added README.md?

Asking because it seems it is not possible to rebase it cleanly in the current Pull Request.

@audrow
Copy link
Member

audrow commented Jan 23, 2023

I think that this PR should be rebased on a newly pulled rolling branch. That way, the PR doesn't have a bunch of unrelated commits.

Will do. @audrow Should I create a new Pull Request for this added README.md?

Asking because it seems it is not possible to rebase it cleanly in the current Pull Request.

That sounds okay to me. You can mention this PR in the new PR so that it's easier to trace the feedback.

I think it would also be possible to start from the rolling branch, cherry-pick your commits here on to the rolling branch, and then force push to this branch, but whatever you think is easier.

@cardboardcode cardboardcode mentioned this pull request Jan 24, 2023
Merged
@cardboardcode
Copy link
Contributor Author

Created a new pull request with a cleaner set of commits that has been rebased from the updated rolling branch.

Closing. Please refer to Pull Request #596 for continuation of this thread.

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

@adityapande-1995 adityapande-1995 adityapande-1995 left review comments

@audrow audrow audrow left review comments

@clalancette clalancette clalancette left review comments

@mjeronimo mjeronimo Awaiting requested review from mjeronimo

Assignees

@audrow audrow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cardboardcode @clalancette @audrow @adityapande-1995 @jacobperron @ihasdapie @deepanshubansal01 @sloretz @kagibson