Port transmission loader plugins #633

VX792 · 2022-01-31T21:14:39Z

My draft PR for the following issue: #599

Basic structure added for TransmissionLoader, including the SimpleTransmissionLoader plus tests for SimpleTransmissionLoader.
When I tested it everything seemed to work, but I'm not sure how I should proceed with the InvalidSpec testcase, since it segfaults while parsing the following:

transmission_interface/SimpleTransmission

The ROS Noetic version has a test case similar to this one. Should I just ignore this?
I'll probably use EXPECT_THROW for the rest.

Also, if I'm planning on porting the other transmission loaders too, should I open a separate PR for those?
Also, where should I place the URDFs I used for the tests?

-Rewrite the test cases for it based on the ROS Noetic version -Modify the package.xml and the CMakeLists.txt to accomodate the pluginlib

bmagyar · 2022-01-31T21:20:33Z

Awesome!
Could you add a few more details regarding that test case please, I'm not sure I got it.

Also, if I'm planning on porting the other transmission loaders too, should I open a separate PR for those?
A separate PR please, I intend to merge this as soon as we can.

Also, where should I place the URDFs I used for the tests?
You can put the URDFs here: https://github.com/ros-controls/ros2_control/tree/master/ros2_control_test_assets/include/ros2_control_test_assets

VX792 · 2022-01-31T21:28:23Z

Yes of course. I wanted to create something similar to the URDF used here, so there will be an invalid test case

https://github.com/ros-controls/ros_control/blob/noetic-devel/transmission_interface/test/urdf/simple_transmission_loader_invalid.urdf

but I think that parsing the first transmission gives us a segfault for some reason. I didn't encounter this problem when I set the mechanical reduction to zero, or to a string. In the latter case an exception is raised as expected.

bmagyar · 2022-02-04T12:16:51Z

ok so the crash is an issue with the parser, we should fix that but that could be done on a follow-up issue. Can you please comment out that test in the mean time please? leave a //TODO note above explaining when it can be reenabled

MinimalSpec and InvalidSpec test case added +preCommit

transmission_interface/CMakeLists.txt

transmission_interface/include/transmission_interface/simple_transmission_loader.hpp

transmission_interface/include/transmission_interface/transmission_loader.hpp

transmission_interface/src/simple_transmission_loader.cpp

transmission_interface/test/simple_transmission_loader_test.cpp

bmagyar · 2022-02-08T09:52:14Z

Pinging @saikishor for a due-diligence license approval from PAL.

…ransmission_loader.hpp Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

…sion_loader.hpp Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

saikishor · 2022-02-08T12:19:34Z

Pinging @saikishor for a due-diligence license approval from PAL.

LGTM

transmission_interface/CMakeLists.txt

destogl

Thanks for the contribution.
I added a lot of nit-picking and maybe some of my comments are missing the point, but I would like to get explained why some things are designed as they are.

This represents a very core of the transmission-management functionality, let's get it right.

transmission_interface/include/transmission_interface/transmission_interface_exception.hpp

transmission_interface/include/transmission_interface/transmission_loader.hpp

transmission_interface/package.xml

transmission_interface/ros2_control_plugins.xml

transmission_interface/test/simple_transmission_loader_test.cpp

destogl · 2022-02-12T19:18:06Z

transmission_interface/test/simple_transmission_loader_test.cpp

+int main(int argc, char ** argv)
+{
+  testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}


Is this really needed? We don't have this in the test files usually. At least the one using gmock instead of gtest library.

Suggested change

int main(int argc, char ** argv)

{

testing::InitGoogleTest(&argc, argv);

return RUN_ALL_TESTS();

}

I can fix this in #642 . The trick is to link your tests binary to a library called "gmock_main".

fixed over there

transmission_interface/include/transmission_interface/transmission_interface_exception.hpp

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

transmission_interface/test/simple_transmission_loader_test.cpp

…ls#618) (ros-controls#633) * Use branch name substitution for all links (ros-controls#618) (cherry picked from commit aece974) --------- Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Co-authored-by: Dr. Denis <denis@stoglrobotics.de>

VX792 added 2 commits January 31, 2022 21:56

Add simple transmission loader (in progress)

8d0ecc4

-Rewrite the test cases for it based on the ROS Noetic version -Modify the package.xml and the CMakeLists.txt to accomodate the pluginlib

Add one working test case for simple transmission loader

48e7d16

destogl linked an issue Feb 1, 2022 that may be closed by this pull request

Port TransmissionLoader from ROS Noetic over to ROS2 #599

Closed

3 tasks

VX792 added 2 commits February 4, 2022 21:00

Add the other two test cases

a4ecec4

MinimalSpec and InvalidSpec test case added +preCommit

Make pre-commit happy + copyright changes

625eff2

VX792 force-pushed the port-transmission-loader-plugins branch from 731aec2 to 625eff2 Compare February 4, 2022 20:57

VX792 added 2 commits February 7, 2022 10:44

Make pre-commit happy

462944a

Remove tinyxml.h from transmission_loader headers

7950048

VX792 marked this pull request as ready for review February 7, 2022 11:35

bmagyar requested review from bmagyar and destogl February 8, 2022 08:29

bmagyar requested changes Feb 8, 2022

View reviewed changes

VX792 and others added 8 commits February 8, 2022 11:16

Update transmission_interface/include/transmission_interface/simple_t…

53fb2c6

…ransmission_loader.hpp Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Update transmission_interface/include/transmission_interface/transmis…

dd6f726

…sion_loader.hpp Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Update transmission_interface/include/transmission_interface/transmis…

12b72ee

…sion_loader.hpp Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Update transmission_interface/include/transmission_interface/transmis…

dd899ee

…sion_loader.hpp Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Update transmission_interface/src/simple_transmission_loader.cpp

23cb42c

Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Update transmission_interface/src/simple_transmission_loader.cpp

143d042

Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Update transmission_interface/package.xml

249eac8

Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

Update transmission_interface/src/simple_transmission_loader.cpp

ccaf44f

Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

std::cerr + remove spaces from CMakeLists.txt

0e227cc

bmagyar reviewed Feb 9, 2022

View reviewed changes

transmission_interface/CMakeLists.txt Outdated Show resolved Hide resolved

one last space to go

d0e3f35

bmagyar approved these changes Feb 12, 2022

View reviewed changes

bmagyar mentioned this pull request Feb 12, 2022

Fix transmission loader tests #642

Merged

destogl requested changes Feb 12, 2022

View reviewed changes

bmagyar and others added 3 commits February 14, 2022 08:54

Tidy up includes

f5fb827

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

Small touchups

aebcee9

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

Update transmission_interface/test/simple_transmission_loader_test.cpp

bf31ae8

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

bmagyar mentioned this pull request Feb 15, 2022

Mimic joints independent of the combination of command interfaces ros-controls/gazebo_ros2_control#111

Closed

destogl mentioned this pull request Feb 16, 2022

Move test-URDF from transmission interface to ros2_control_test_assets #648

Open

destogl approved these changes Feb 16, 2022

View reviewed changes

transmission_interface/test/simple_transmission_loader_test.cpp Show resolved Hide resolved

transmission_interface/test/simple_transmission_loader_test.cpp Show resolved Hide resolved

destogl merged commit f5ef068 into ros-controls:master Feb 16, 2022

wmmc88 mentioned this pull request Feb 16, 2022

Port TransmissionLoader from ROS Noetic over to ROS2 #599

Closed

3 tasks

henrygerardmoore mentioned this pull request Jul 19, 2024

Unused header cleanup #1627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port transmission loader plugins #633

Port transmission loader plugins #633

VX792 commented Jan 31, 2022

bmagyar commented Jan 31, 2022

VX792 commented Jan 31, 2022

bmagyar commented Feb 4, 2022

bmagyar commented Feb 8, 2022

saikishor commented Feb 8, 2022

destogl left a comment

destogl Feb 12, 2022

bmagyar Feb 14, 2022

bmagyar Feb 15, 2022

Port transmission loader plugins #633

Port transmission loader plugins #633

Conversation

VX792 commented Jan 31, 2022

bmagyar commented Jan 31, 2022

VX792 commented Jan 31, 2022

bmagyar commented Feb 4, 2022

bmagyar commented Feb 8, 2022

saikishor commented Feb 8, 2022

destogl left a comment

Choose a reason for hiding this comment

destogl Feb 12, 2022

Choose a reason for hiding this comment

bmagyar Feb 14, 2022

Choose a reason for hiding this comment

bmagyar Feb 15, 2022

Choose a reason for hiding this comment