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

Updating templates with developments from mecanum_controller #106

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

Updating templates with developments from mecanum_controller #106

wants to merge 42 commits into from

Conversation

Robotgir
Copy link
Contributor

@Robotgir Robotgir commented Feb 23, 2023 •
edited by destogl
Loading

created a ros2_controller package from this template and all tests pass, tests specific to ref_timeout are not included as they are being addressed in #73

@mergify
Copy link

mergify bot commented Feb 23, 2023

This pull request is in conflict. Could you fix it @Robotgir?

destogl
destogl requested changes Mar 21, 2023
View reviewed changes
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Some changes still needed, not sure if this is working as it should

templates/ros2_control/controller/dummy_chainable_controller.cpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/dummy_chainable_controller.cpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/dummy_chainable_controller.cpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/dummy_chainable_controller.cpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/dummy_controller.yaml
@@ -29,3 +29,12 @@ dummy_controller:
forbidden_interface_name_prefix: null
}
}
reference_names: {
Copy link
Member

Choose a reason for hiding this comment

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

I am uncertain if this is necessary here or if we should have this in the separate file for the chainable controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

separate file idea sounds better to me as this is not always the case

Copy link
Member

Choose a reason for hiding this comment

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

@Robotgir are you planing to fix this?

templates/ros2_control/controller/dummy_package_namespace/dummy_chainable_controller.hpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/dummy_package_namespace/dummy_controller.hpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/test_dummy_chainable_controller.cpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/test_dummy_chainable_controller.hpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/test_dummy_chainable_controller.cpp Outdated Show resolved Hide resolved
@destogl destogl requested a review from muritane March 21, 2023 16:19
@destogl
Copy link
Member

destogl commented Mar 21, 2023

@Robotgir can you test this through one more time, please?

@Robotgir Robotgir mentioned this pull request Mar 23, 2023
Open
@Robotgir Robotgir force-pushed the updating-templates-from-mecanum branch from 306a752 to d2911e5 Compare March 23, 2023 18:45
Robotgir
Robotgir commented Mar 23, 2023
View reviewed changes
.clang-format Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added changes intentionally as you wanted to check them for new config file from ros2_contol

Copy link
Member

Choose a reason for hiding this comment

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

This should be added into a new PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#118

@Robotgir Robotgir force-pushed the updating-templates-from-mecanum branch from d2911e5 to 3f3e1db Compare March 31, 2023 14:40
@mergify mergify bot mentioned this pull request Mar 31, 2023
Closed
@Robotgir
Copy link
Contributor Author

Robotgir commented Apr 3, 2023

@Robotgir can you test this through one more time, please?

@destogl i tested this again an as i said loading controller test alone fails in both standard and chain able cases

@mergify
Copy link

mergify bot commented May 12, 2023

This pull request is in conflict. Could you fix it @Robotgir?

@destogl destogl requested a review from gwalck May 16, 2023 05:17
@destogl
Copy link
Member

destogl commented May 16, 2023

@gwalck please also resolve all deprecation warnings from the generated code

destogl
destogl reviewed May 16, 2023
View reviewed changes
Copy link
Member

@destogl destogl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Propose also to change the following:

auto current_ref = input_ref_.readFromRT(); to

auto current_ref = *(reference_.readFromRT());

And all changed following from this. It should clean up the code a bit.

gwalck
gwalck approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@gwalck gwalck left a comment

Choose a reason for hiding this comment

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

I have reviewed the changes, added multiple fixes to remove deprecated + fixed all tests

templates/ros2_control/controller/dummy_chainable_controller.cpp Outdated Show resolved Hide resolved
templates/ros2_control/controller/test_dummy_chainable_controller_preceeding.cpp Outdated Show resolved Hide resolved
@mergify Mergify
Copy link

mergify bot commented Feb 26, 2024

This pull request is in conflict. Could you fix it @Robotgir?

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

@destogl destogl destogl requested changes

@gwalck gwalck gwalck approved these changes

@muritane muritane Awaiting requested review from muritane

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@Robotgir @destogl @gwalck