Fix ackermann steering odometry

[franzrammerstorfer]

This is IMHO a quick and easy fix for

#878

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dcd15ee) 46.86% compared to head (5181b95) 48.03%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #921      +/-   ##
==========================================
+ Coverage   46.86%   48.03%   +1.16%     
==========================================
  Files          41       41              
  Lines        3862     3862              
  Branches     1816     1816              
==========================================
+ Hits         1810     1855      +45     
+ Misses        792      745      -47     
- Partials     1260     1262       +2     

Flag	Coverage Δ
unittests	48.03% <100.00%> (+1.16%)	⬆️

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

Files	Coverage Δ
...ring_controllers_library/src/steering_odometry.cpp	62.27% <100.00%> (+13.17%)	⬆️

... and 2 files with indirect coverage changes

[christophfroehlich]

Hi @franzrammerstorfer! Thanks for the PR.

Some first comments from my side:

To fix the linter issues, you can check it locally by

• install pre-commit by pip3 install pre-commit
• pre-commit run--all

Why do you use EXPECT_FLOAT_EQ instead of EXPECT_DOUBLE_EQ?

Does it make sense to check these cases also with update_from_velocity and update_from_position, i.e., for params_.open_loop=false?

[franzrammerstorfer]

Hi @christophfroehlich!
Sorry for not listen to pre-commit checks, I did not read the instructions carefully enough. I think I fixed that with my last few commits. I misused EXPECT_FLOAT_EQ, bad habit, thanks for the hint.

Yes, you're right, there are a lot of things open to test -- but right now I just wanted to provide a quick fix for the Ackermann odometry, only rudimentary tested. Maybe we should open a new issue, tagged "help wanted" for proper testing of the steering lib -- starting with tests for the integrator functions, etc. But maybe first we should check for a redesign cf. #692

BTW, I just added this file 'test_steering_odometry.cpp', with some hard-coded tests. Is that the way to go, or do we want to have a more advanced testing style?

[christophfroehlich]

Yes, you're right, there are a lot of things open to test -- but right now I just wanted to provide a quick fix for the Ackermann odometry, only rudimentary tested. Maybe we should open a new issue, tagged "help wanted" for proper testing of the steering lib -- starting with tests for the integrator functions, etc. But maybe first we should check for a redesign cf. #692

ok, I'll create a new issue then.

BTW, I just added this file 'test_steering_odometry.cpp', with some hard-coded tests. Is that the way to go, or do we want to have a more advanced testing style?

Should the expectations of the FW kinematics be true for all the different kinematic configurations? Then we could change this to a parameterized test as we have with JTC

ros2_controllers/joint_trajectory_controller/test/test_trajectory_controller_utils.hpp

Lines 695 to 710 in 1d0d753

	// From the tutorial: https://www.sandordargo.com/blog/2019/04/24/parameterized-testing-with-gtest
	class TrajectoryControllerTestParameterized
	: public TrajectoryControllerTest,
	public ::testing::WithParamInterface<
	std::tuple<std::vector<std::string>, std::vector<std::string>>>
	{
	public:
	virtual void SetUp()
	{
	TrajectoryControllerTest::SetUp();
	command_interface_types_ = std::get<0>(GetParam());
	state_interface_types_ = std::get<1>(GetParam());
	}
	static void TearDownTestCase() { TrajectoryControllerTest::TearDownTestCase(); }
	};

ros2_controllers/joint_trajectory_controller/test/test_trajectory_controller.cpp

Lines 1840 to 1849 in 1d0d753

	// position controllers
	INSTANTIATE_TEST_SUITE_P(
	PositionTrajectoryControllers, TrajectoryControllerTestParameterized,
	::testing::Values(
	std::make_tuple(std::vector<std::string>({"position"}), std::vector<std::string>({"position"})),
	std::make_tuple(
	std::vector<std::string>({"position"}), std::vector<std::string>({"position", "velocity"})),
	std::make_tuple(
	std::vector<std::string>({"position"}),
	std::vector<std::string>({"position", "velocity", "acceleration"}))));

[christophfroehlich]

But we can for now just fix the CI and extend the tests in a follow-up PR.

[christophfroehlich]

@aleksandrsizmailovs and @roncapat, as you have commented on the issue: Is this PR fine for you?

[christophfroehlich]

LGTM

[roncapat]

Seems fine IMHO :)

[aleksandrsizmailovs]

@aleksandrsizmailovs and @roncapat, as you have commented on the issue: Is this PR fine for you?

Looks good to me.