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

Add Mecanum Drive Controller #512

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

Add Mecanum Drive Controller #512

wants to merge 94 commits into from

Conversation

destogl
Copy link
Member

@destogl destogl commented Jan 27, 2023 •
edited by christophfroehlich
Loading

@destogl destogl force-pushed the add-mecanum-drive-controller branch from a22266c to 002b436 Compare January 27, 2023 23:01
destogl
destogl commented Jan 27, 2023
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023 •
edited by codecov bot
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.88%. Comparing base (0b43291) to head (f6de20a).
Report is 33 commits behind head on master.

❗ Current head f6de20a differs from pull request most recent head 3da18a8

Please upload reports for the commit 3da18a8 to get more accurate results.

Additional details and impacted files 
@@             Coverage Diff             @@
##           master     #512       +/-   ##
===========================================
- Coverage   71.86%   30.88%   -40.98%     
===========================================
  Files          41        7       -34     
  Lines        3650      832     -2818     
  Branches     1794      505     -1289     
===========================================
- Hits         2623      257     -2366     
+ Misses        707      133      -574     
- Partials      320      442      +122
Flag Coverage Δ
unittests 30.88% <ø> (-40.98%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 43 files with indirect coverage changes

@destogl destogl mentioned this pull request Jan 28, 2023
Merged
destogl
destogl commented Jan 31, 2023
View reviewed changes
mecanum_drive_controller/test/test_load_mecanum_drive_controller.cpp Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/test/test_mecanum_drive_controller.cpp Outdated Show resolved Hide resolved
@destogl destogl force-pushed the add-mecanum-drive-controller branch from 9b036ae to 43b183f Compare February 8, 2023 16:36
@destogl destogl force-pushed the add-mecanum-drive-controller branch from 936bb9e to 48415bf Compare March 8, 2023 18:45
@mergify
Copy link
Contributor

mergify bot commented Mar 28, 2023

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

christophfroehlich
christophfroehlich requested changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM, suggestions only for small changes like typos etc.

mecanum_drive_controller/src/mecanum_drive_controller.yaml Outdated Show resolved Hide resolved
mecanum_drive_controller/src/mecanum_drive_controller.yaml Outdated Show resolved Hide resolved
mecanum_drive_controller/src/mecanum_drive_controller.yaml Outdated Show resolved Hide resolved
mecanum_drive_controller/src/odometry.cpp Show resolved Hide resolved
mecanum_drive_controller/src/odometry.cpp Outdated Show resolved Hide resolved
mecanum_drive_controller/doc/userdoc.rst Outdated Show resolved Hide resolved
mecanum_drive_controller/doc/userdoc.rst Outdated Show resolved Hide resolved
mecanum_drive_controller/doc/userdoc.rst Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Jul 27, 2023

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

destogl
destogl commented Jun 27, 2024
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Pratham-Pandey
Pratham-Pandey approved these changes Jul 9, 2024
View reviewed changes
Copy link

@Pratham-Pandey Pratham-Pandey left a comment

Choose a reason for hiding this comment

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

The controller is working(robot cart is moving as expected).
The odometry frame is not being published. Try out this gazebo_ros2_control demo to check.

@Pratham-Pandey
Copy link

@christophfroehlich I think my review would not count as I do not have write access to the repo.

@saikishor
Copy link
Member

@Pratham-Pandey every review counts. If you are an expert on this, we appreciate it if you can review the changes.

Thank you

@Pratham-Pandey
Copy link

@Pratham-Pandey every review counts. If you are an expert on this, we appreciate it if you can review the changes.

Thank you

Have reviewed it already.

@luis-camero luis-camero mentioned this pull request Jul 11, 2024
Closed
@luis-camero
Copy link

@christophfroehlich, does the WaitSet issue in the test framework need to be resolved before this PR can be merged? Bypassing the subscriber/publisher results in all tests passing.

@bmagyar would you accept a PR to Humble with the same "hack" to support Twist and TwistStamped as in the Diff. Drive Controller in #143.

@ahcorde ahcorde mentioned this pull request Jul 22, 2024
Open
@luis-camero
Copy link

@destogl could you take a look at the PR I made to your fork. I think there's an issue with the odometry topic.

@Pratham-Pandey
Copy link

@luis-camero is odom frame available in your case in rviz? In my case, the odom frame is not even listed in rviz Fixed Frame dropdown menu. Could you share the controller yaml file and the URDF?

@luis-camero luis-camero mentioned this pull request Jul 23, 2024
Open
@luis-camero
Copy link

@luis-camero is odom frame available in your case in rviz? In my case, the odom frame is not even listed in rviz Fixed Frame dropdown menu. Could you share the controller yaml file and the URDF?

@Pratham-Pandey, take a look at the PR I made.

HopeCollector
HopeCollector reviewed Jul 30, 2024
View reviewed changes
mecanum_drive_controller/test/test_mecanum_drive_controller_preceeding.cpp

TEST_F(MecanumDriveControllerTest, when_controller_is_configured_expect_all_parameters_set)
{
SetUpController();
Copy link

Choose a reason for hiding this comment

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

Setting up the controller at the beginning will cause the test to fail later because the controller parameter is already configured in the function, according to my test, moving it to line 46, the test works fine.

Copy link

Choose a reason for hiding this comment

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

It looks like I opened a review by mistake, feel free to delete the review I submitted if needed!

@luis-camero
Odometry Position Frame (#21)
57fac95 
* Add hardware interface testing dependency

* Use orientation w.r.t. odom to set position
@destogl destogl added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels Jul 31, 2024
@bmagyar
Copy link
Member

bmagyar commented Jul 31, 2024

@destogl I think there were a few API breaks since those tests were written ;)

@Pratham-Pandey
Copy link

Can this controller be used to move to a specified coordinate or do i need to implement a separate controller (for example a PID controller) to do this?

@christophfroehlich
Copy link
Contributor

Can this controller be used to move to a specified coordinate or do i need to implement a separate controller (for example a PID controller) to do this?

Usually, there is some navigation task involved. See the nav2 docs, the nav2 controllers publish a stamped twist as far as I understood, which you can then feed into this ros2_controller.

christophfroehlich
christophfroehlich requested changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

And some more: StoglRobotics-forks#23

@destogl but when_reference_msg_received_expect_updated_commands_and_status_message still fails

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

@HopeCollector HopeCollector HopeCollector left review comments

@christophfroehlich christophfroehlich christophfroehlich requested changes

@Pratham-Pandey Pratham-Pandey Pratham-Pandey approved these changes

@anfemosa anfemosa Awaiting requested review from anfemosa

@livanov93 livanov93 Awaiting requested review from livanov93

@mhubii mhubii Awaiting requested review from mhubii

@VX792 VX792 Awaiting requested review from VX792

@bmagyar bmagyar Awaiting requested review from bmagyar

@Robotgir Robotgir Awaiting requested review from Robotgir

@erickisos erickisos Awaiting requested review from erickisos

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.