-
Notifications
You must be signed in to change notification settings - Fork 298
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.
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
Adding MecanumDrive plugin with Odom and Tf, with Tests. #2297
Conversation
Thanks for your contribution @muttistefano. Do you mind fixing the linter issues and signing off on your commit (see https://github.com/gazebosim/gz-sim/pull/2297/checks?check_run_id=20866188364). |
Hi @azeey , thanks for the answer. |
There are some trailing white spaces in the code (see https://github.com/gazebosim/gz-sim/actions/runs/7765649621/job/21405089973?pr=2297). |
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
Hello ! |
If I'm not mistaken, the checks are fine, the only thing missing is the review from @mjcarroll . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@muttistefano I think you've accidentally checked in a few files and directories (e.g. log
, .vscode
) to the pull request. Could you please remove them?
My bad; it should be fixed now. |
Can you fix DCO? |
using namespace std::chrono_literals; | ||
|
||
/// \brief Test MecanumDrive system | ||
class MecanumDriveTest : public InternalFixture<::testing::TestWithParam<int>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to run this multiple times. Can you change this to
class MecanumDriveTest : public InternalFixture<::testing::TestWithParam<int>> | |
class MecanumDriveTest : public InternalFixture<::testing::Test> |
and change all the TEST_P
in this file to TEST_F
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has not been addressed yet.
Not a priority here, can be done separately
///////////////////////////////////////////////// | ||
// See: https://github.com/gazebosim/gz-sim/issues/1175 | ||
// See: https://github.com/gazebosim/gz-sim/issues/630 | ||
TEST_P(MecanumDriveTest, GZ_UTILS_TEST_ENABLED_ONLY_ON_LINUX(PublishCmd)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is copied from the DiffDrive test, but can we first try to enable it and see if it doesn't work on the other platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't quite get what do you mean, since my testing skills are weak.
do you mean changing GZ_UTILS_TEST_ENABLED_ONLY_ON_LINUX to something that enables the tests on other OSs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think something like:
TEST_P(MecanumDriveTest, GZ_UTILS_TEST_ENABLED_ONLY_ON_LINUX(PublishCmd)) | |
TEST_P(MecanumDriveTest, PublishCmd) |
let's also not block on this
Hi @muttistefano, any updates on this PR? |
I will try to finish the pull today |
Signed-off-by: Stefano Mutti <[email protected]> Signed-off-by: Stefano Mutti <[email protected]>
Signed-off-by: Stefano Mutti <[email protected]>
I messed up the pull a bit; let me know if it's ok or if I should do something. |
Hi, I am very interested in this PR, as I am running a project using this plugin. However, missing odom and tf (also the disappeared cmd_vel) topics introduce trouble of interacting with ROS via the bridge. I am working on ign-sim-6 due to ROS2 humble. Any update on this? |
I think i solved all the requests. |
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Hello, is this fix available now if yes do i need to just use apt update or is there another process? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iche033 do you mind to take a look ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested running mecanum_drive world echo'ing the odom messages, and appears to be working as expected.
I left some minor comments only but the PR is good as is. Given that the PR has been around for a while, we can get this merged first and address the remaining comments separately.
using namespace std::chrono_literals; | ||
|
||
/// \brief Test MecanumDrive system | ||
class MecanumDriveTest : public InternalFixture<::testing::TestWithParam<int>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has not been addressed yet.
Not a priority here, can be done separately
///////////////////////////////////////////////// | ||
// See: https://github.com/gazebosim/gz-sim/issues/1175 | ||
// See: https://github.com/gazebosim/gz-sim/issues/630 | ||
TEST_P(MecanumDriveTest, GZ_UTILS_TEST_ENABLED_ONLY_ON_LINUX(PublishCmd)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think something like:
TEST_P(MecanumDriveTest, GZ_UTILS_TEST_ENABLED_ONLY_ON_LINUX(PublishCmd)) | |
TEST_P(MecanumDriveTest, PublishCmd) |
let's also not block on this
"/model/foo/cmdvel", "/model/bar/odom"); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
@@ -26,9 +26,10 @@ | |||
#include <set> | |||
#include <string> | |||
#include <vector> | |||
#include <chrono> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include headers in alphabetically order
some comments can be addressed in a separate PR
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gz-sim8 #2297 +/- ##
===========================================
+ Coverage 65.75% 69.02% +3.27%
===========================================
Files 327 342 +15
Lines 31233 33300 +2067
===========================================
+ Hits 20537 22986 +2449
+ Misses 10696 10314 -382 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎉 New feature
Summary
Adding MecanumDrive plugin with Odom and Tf, with Tests.
Relative to #1665 , but targets gz-sim8, fixes an include error in the plugin and add tests.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸