-
Notifications
You must be signed in to change notification settings - Fork 49
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
[ROS2] Port kindr_ros and kindr_msgs #26
base: ros2
Are you sure you want to change the base?
Conversation
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.
Very nice, thank you!
# TODO(SivertHavso): add back code coverage: | ||
# find_package(cmake_code_coverage QUIET) | ||
# if(cmake_code_coverage_FOUND) | ||
# add_gtest_coverage(TEST_BUILD_TARGETS ${PROJECT_NAME}-test) | ||
# endif() |
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.
Is this something you intended to do before merging the MR?
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.
It will have to wait until I've finished porting the elevation_mapping package. I'd say merge it now since it "only" affects quality assurance and not the API or ABI. The tests themself still run and there shouldn't be a notable difference in code coverage between the ROS1 and ROS2 branch for now.
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.
Out of curiosity, why is this a dependency of the porting of elevation_mapping
? This is a downstream dependency, right?
@SivertHavso We internally discussed the matter of formatting of code: As we are using this style for most of our closed source and open source code, we decided that it would make sense to use it also on the ROS2 branches. Would you mind adopting it after merging this PR? |
@remod Sure, in the process of doing it now. On my ROS2 working branch I've got it set to run clang-format, cppcheck, gtest, lint_cmake, and xmllint automatically (through the corresponding ament_cmake packages) as part of testing now, and have disabled the cpplint and uncrustify linters because they expect a different style. I'm getting a lot of failures from clang_format with your style though. For example clang-format wants: template <typename PrimType_>
inline static void convertFromRosGeometryMsg(const geometry_msgs::msg::Point& geometryPointMsg, kindr::Position<PrimType_, 3>& position) {
position.x() = static_cast<PrimType_>(geometryPointMsg.x);
position.y() = static_cast<PrimType_>(geometryPointMsg.y);
position.z() = static_cast<PrimType_>(geometryPointMsg.z);
} instead of the current: kindr_ros/kindr_ros/include/kindr_ros/RosGeometryMsgPhysicalQuantities.hpp Lines 84 to 92 in f0bd43c
I get the same results running ament_clang_format or clang-format directly for both the master branch and my ROS2 working branch, "12 files with 218 code style divergences". I'm quite sure it's loading your custom configuration because settings such as Can you confirm whether the master branch is formatted correctly according to your style please? |
No you are right, this package is not formatted according to our guidelines yet. I'll do that today! |
Summary of non-ROS2 specific changes. These could potentially be cherry picked into the ROS1 branch to avoid future merge conflicts:
ROS2 specific changes:
All 18 gtests run and pass for me. I've targeted ROS2 Galactic, but I see not reason why it shouldn't work with ROS2 Foxy.
The rviz plugins haven't been ported yet.
Partly resolves #22