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

Light Messages #187

Merged
merged 20 commits into from
Dec 28, 2021
Merged

Light Messages #187

merged 20 commits into from
Dec 28, 2021

Conversation

WilliamLewww
Copy link
Contributor

@WilliamLewww WilliamLewww commented Nov 29, 2021 •
edited
Loading

🎉 Light Messages

Depends on #189

Summary

Derived a light message for ROS2 from Ignition's light message.

ros2 topic pub --once /world/light_config ros_ign_interfaces/msg/Light "{name: 'sun'}"

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

ahcorde
ahcorde requested changes Nov 30, 2021
View reviewed changes
ros_ign_bridge/CMakeLists.txt Outdated Show resolved Hide resolved
@WilliamLewww WilliamLewww marked this pull request as draft November 30, 2021 19:06
WilliamLewww and others added 2 commits November 30, 2021 11:08
@WilliamLewww
Updated CMakeLists
182bf81 
Signed-off-by: William Lew <[email protected]>
@ahcorde
suggestions to PR gazebosim#187
3085ddc 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Copy link
Collaborator

ahcorde commented Dec 2, 2021

I made here some suggestions WilliamLewww#1 to this PR

@chapulina chapulina added the ROS 2 ROS 2 label Dec 2, 2021
@WilliamLewww WilliamLewww marked this pull request as ready for review December 6, 2021 18:32
chapulina
chapulina suggested changes Dec 9, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Did a quick pass

ros_ign_bridge/CMakeLists.txt Outdated Show resolved Hide resolved
ros_ign_bridge/CMakeLists.txt Outdated Show resolved Hide resolved
ros_ign_bridge/CMakeLists.txt Outdated Show resolved Hide resolved
ros_ign_bridge/package.xml Outdated Show resolved Hide resolved
ros_ign_interfaces/msg/Color.msg Outdated Show resolved Hide resolved
ros_ign_interfaces/msg/Light.msg Outdated Show resolved Hide resolved
ros_ign_image/CMakeLists.txt Outdated Show resolved Hide resolved
@WilliamLewww
Applied code suggestions
de91bbb 
Signed-off-by: William Lew <[email protected]>
@chapulina
Copy link
Contributor

Oh I forgot to mention that we should add tests for the new conversions, and document them in the README.

ahcorde
ahcorde previously requested changes Dec 11, 2021
View reviewed changes
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

linters https://github.com/ignitionrobotics/ros_ign/runs/4489943637?check_suite_focus=true

@WilliamLewww
Copy link
Contributor Author

It seems that the test_ros_subscriber is not working properly on the ros2 branch. I am going to make a separate pull request to fix the test.

https://github.com/ignitionrobotics/ros_ign/blob/5d9cfef3d58948a77b962d231e281c88ea88e188/ros_ign_bridge/CMakeLists.txt#L163

@WilliamLewww
Copy link
Contributor Author

Opened the following pull request to fix the ROS subscriber test: #189

@WilliamLewww WilliamLewww force-pushed the wlew/light_message branch 2 times, most recently from ccf734a to 6fbf510 Compare December 22, 2021 19:33
@WilliamLewww
Fixed style errors
a300f18 
Signed-off-by: William Lew <[email protected]>
@WilliamLewww
Fixed merge conflicts
65d8133 
Signed-off-by: William Lew <[email protected]>
@WilliamLewww
Fixed formatting errors
99e49db 
Signed-off-by: William Lew <[email protected]>
chapulina
chapulina suggested changes Dec 27, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Ouch, I forgot about this PR when I merged #192. Thanks for updating the PR for that! Let's be sure to merge this one before #194 so you don't need to change this PR again.

I just have some final minor comments, otherwise this looks good to me 👍

ros_ign_bridge/CMakeLists.txt Outdated Show resolved Hide resolved
ros_ign_image/CMakeLists.txt Outdated Show resolved Hide resolved
@chapulina chapulina mentioned this pull request Dec 27, 2021
Merged
@WilliamLewww
Added code suggestions
8735d1c 
Signed-off-by: William Lew <[email protected]>
chapulina
chapulina approved these changes Dec 28, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks! 💡

@chapulina chapulina merged commit dcd68ff into gazebosim:ros2 Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@ahcorde ahcorde ahcorde left review comments

Assignees
No one assigned
Labels
ROS 2 ROS 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@WilliamLewww @ahcorde @chapulina