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

Move test nodes from the ros2_control_demos repository. #294

Merged
merged 5 commits into from
Feb 23, 2022
Merged

Move test nodes from the ros2_control_demos repository. #294

merged 5 commits into from
Feb 23, 2022

Conversation

destogl
Copy link
Member

@destogl destogl commented Feb 9, 2022 •
edited
Loading

Moving the controllers' test nodes where they belong to.
They should be also released here in the future.

It would be cool to backport this to galactic and foxy too.

related to ros-controls/ros2_control_demos#150

@destogl destogl self-assigned this Feb 9, 2022
@destogl destogl requested a review from bmagyar February 9, 2022 08:45
@destogl destogl mentioned this pull request Feb 21, 2022
Closed
bmagyar
bmagyar reviewed Feb 22, 2022
View reviewed changes
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

I know it sounds a little silly but since it's now a shipped piece of code, we should add a follow-up ticket to add tests to this. I know it's hard to test but still...

Also, do you think launch files (xml and python) would be a good addition or better to keep it minimal for now?

ros2_controllers_test_nodes/package.xml Show resolved Hide resolved
..._controllers_test_nodes/ros2_controllers_test_nodes/publisher_forward_position_controller.py Outdated Show resolved Hide resolved
..._controllers_test_nodes/ros2_controllers_test_nodes/publisher_joint_trajectory_controller.py Outdated Show resolved Hide resolved
@destogl
Copy link
Member Author

destogl commented Feb 22, 2022

Also, do you think launch files (xml and python) would be a good addition or better to keep it minimal for now?

I would keep it small for now. The main purpose first is to unblock other releases using this. We can then later see what does make sense to move here. What is actually used and how it can be generalized.

And +1 for tests! But in the follow-up

@destogl @bmagyar
Apply suggestions from code review
a9b3df7 
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
@destogl
Copy link
Member Author

destogl commented Feb 22, 2022

Thanks for the review. When merging, we should add @livanov93 as co-author because he implemented "initial position"-check

@destogl destogl requested a review from bmagyar February 22, 2022 21:37
destogl and others added 3 commits February 22, 2022 22:42
@destogl @livanov93
Add Lovro as co-author
7c632a5 
Co-authored-by: livanov93 <lovro.ivanov@gmail.com>
@destogl
Update ci.yml
50d1111
@destogl
Update lint.yml
a5b5a48
@livanov93
Copy link
Contributor

@destogl Thanks for remembering the co-author thing.

livanov93
livanov93 approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@livanov93 livanov93 left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@bmagyar bmagyar merged commit e7d0517 into master Feb 23, 2022
@bmagyar bmagyar deleted the add-controller-test-nodes branch February 23, 2022 11:37
@bmagyar
Copy link
Member

bmagyar commented Feb 23, 2022

@Mergifyio backport to galactic

mergify bot pushed a commit that referenced this pull request Feb 23, 2022
@destogl
Move test nodes from the ros2_control_demos repository. (#294)
f501be9 
Co-authored-by: Lovro Ivanov <lovro.ivanov@gmail.com>
(cherry picked from commit e7d0517)
@mergify mergify bot mentioned this pull request Feb 23, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2022

backport to galactic

✅ Backports have been created

bmagyar pushed a commit that referenced this pull request Feb 23, 2022
@mergify @destogl
Move test nodes from the ros2_control_demos repository. (#294) (#298)
03fc3e7 
Co-authored-by: Lovro Ivanov <lovro.ivanov@gmail.com>
(cherry picked from commit e7d0517)

Co-authored-by: Denis Štogl <denis@stogl.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar bmagyar approved these changes

@livanov93 livanov93 livanov93 approved these changes

Assignees

@destogl destogl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@destogl @livanov93 @bmagyar