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

Fix steering controllers library kinematics #1150

Merged
merged 12 commits into from
Jul 3, 2024
Merged

Fix steering controllers library kinematics #1150

merged 12 commits into from
Jul 3, 2024

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented May 26, 2024 •
edited
Loading

A follow-up of #1149 to fix bugs of the steering library, namely

  • in open-loop mode, zero steering angle was used for IK calculation
    discussed in Add mobile robot kinematics 101 and improve steering library docs #954 (my fix breaks API of the library, but it had no information about open_loop. I could duplicate the get_commands() method to get_commands_open_loop(), but that would break ABI too?)
  • wrong wheelspeed was used (traction on steering wheel, instead of other axle was assumed)
  • Improve odometry of overdetermined measurements. use nonlinear dependency from wheel speed to calculate linear velocity (ideal) vs the old linear average:
    image

This should finally fix #937 #933 and #789

@christophfroehlich christophfroehlich mentioned this pull request May 26, 2024
Closed
@christophfroehlich christophfroehlich changed the title Fix/steering controllers library kinematics Fix steering controllers library kinematics May 26, 2024
@christophfroehlich christophfroehlich force-pushed the fix/steering_controllers_library-docs branch from 4b06b5f to 77212df Compare May 27, 2024 19:18
@christophfroehlich christophfroehlich force-pushed the fix/steering_controllers_library_kinematics branch 2 times, most recently from 498e09d to c421ad0 Compare May 28, 2024 19:54
Base automatically changed from fix/steering_controllers_library-docs to master June 5, 2024 19:50
@christophfroehlich
Fix IK
aa34f02 
- in open-loop mode, zero steering angle was used for IK calculation
- wrong wheelspeed was used (traction on steering wheel, instead of other axle was assumed)
@christophfroehlich
Fix controller tests and add notes
35f34d8
@christophfroehlich
Improve tests for other steering types
21afbc5
@christophfroehlich
Improve odometry of overdetermined measurements
8809f62
@christophfroehlich
Remove debug output
0209540
@christophfroehlich christophfroehlich force-pushed the fix/steering_controllers_library_kinematics branch from c421ad0 to 0209540 Compare June 5, 2024 20:01
@christophfroehlich christophfroehlich marked this pull request as ready for review June 5, 2024 20:32
@ros-controls ros-controls deleted a comment from mergify bot Jun 5, 2024
@christophfroehlich christophfroehlich mentioned this pull request Jun 8, 2024
Merged
@christophfroehlich christophfroehlich mentioned this pull request Jun 9, 2024
Closed
qinqon
qinqon suggested changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@qinqon qinqon left a comment

Choose a reason for hiding this comment

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

first pass, I will review the "Improve odometry of overdetermined measurements" later on

steering_controllers_library/src/steering_odometry.cpp Show resolved Hide resolved
steering_controllers_library/src/steering_odometry.cpp Show resolved Hide resolved
steering_controllers_library/src/steering_odometry.cpp Outdated Show resolved Hide resolved
steering_controllers_library/src/steering_odometry.cpp Outdated Show resolved Hide resolved
steering_controllers_library/src/steering_odometry.cpp Outdated Show resolved Hide resolved
steering_controllers_library/src/steering_odometry.cpp Show resolved Hide resolved
steering_controllers_library/include/steering_controllers_library/steering_odometry.hpp Outdated Show resolved Hide resolved
@christophfroehlich christophfroehlich mentioned this pull request Jun 19, 2024
Open
6 tasks
@qinqon qinqon mentioned this pull request Jun 19, 2024
Open
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 19, 2024

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

@christophfroehlich
Copy link
Contributor Author

fe2ba4b:

$ colcon test --packages-select steering_controllers_library ackermann_steering_controller bicycle_steering_
controller tricycle_steering_controller
Starting >>> steering_controllers_library
Finished <<< steering_controllers_library [0.38s]          
Starting >>> ackermann_steering_controller
Starting >>> bicycle_steering_controller
Starting >>> tricycle_steering_controller
Finished <<< ackermann_steering_controller [0.93s]                                                                                                 
Finished <<< bicycle_steering_controller [0.97s]
Finished <<< tricycle_steering_controller [0.96s]

Summary: 4 packages finished [1.76s]

qinqon
qinqon suggested changes Jun 23, 2024
View reviewed changes
steering_controllers_library/src/steering_odometry.cpp
Comment on lines +162 to +169
// overdetermined, we take the average
const double right_steer_pos_est = std::atan(
wheelbase_ * std::tan(right_steer_pos) /
(wheelbase_ - wheel_track_ / 2 * std::tan(right_steer_pos)));
const double left_steer_pos_est = std::atan(
wheelbase_ * std::tan(left_steer_pos) /
(wheelbase_ + wheel_track_ / 2 * std::tan(left_steer_pos)));
steer_pos_ = (right_steer_pos_est + left_steer_pos_est) * 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Calculations are fine, but I thin we can split it a little to re-use and improve readability having "atan" and "tan" at the same line is a little confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah that's fine since this is following the docs.

qinqon
qinqon approved these changes Jun 23, 2024
View reviewed changes
Copy link
Contributor

@qinqon qinqon left a comment

Choose a reason for hiding this comment

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

/lgtm

@destogl destogl merged commit df04492 into master Jul 3, 2024
4 of 18 checks passed
@destogl destogl deleted the fix/steering_controllers_library_kinematics branch July 3, 2024 17:43
wittenator pushed a commit to wittenator/ros2_controllers that referenced this pull request Jul 5, 2024
@christophfroehlich @wittenator
Fix steering controllers library kinematics (ros-controls#1150)
3ca1232
@wittenator wittenator mentioned this pull request Jul 5, 2024
Closed
@christophfroehlich christophfroehlich added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron labels Jul 5, 2024
mergify bot pushed a commit that referenced this pull request Jul 5, 2024
@christophfroehlich @mergify
Fix steering controllers library kinematics (#1150)
5f1fd99 
(cherry picked from commit df04492)
mergify bot pushed a commit that referenced this pull request Jul 5, 2024
@christophfroehlich @mergify
Fix steering controllers library kinematics (#1150)
eb2bb6d 
(cherry picked from commit df04492)
This was referenced Jul 5, 2024
Merged
Merged
bmagyar pushed a commit that referenced this pull request Jul 6, 2024
@mergify
Fix steering controllers library kinematics (#1150) (#1195)
73b20e5
bmagyar pushed a commit that referenced this pull request Jul 15, 2024
@mergify
Fix steering controllers library kinematics (#1150) (#1194)
143bfc9
This was referenced Jul 18, 2024
Closed
Closed
henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
@destogl
[MockHardware] Remove all deprecated options and deprecated plugins f…
0863acd 
…rom the library. (ros-controls#1150)

* Rename HW for simpler debugging and remove deprecated initial value handling.
* Remove fake from parameters.
* Remove FakeSystem
* Cleanup fake from tests.
@destogl destogl mentioned this pull request Aug 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar bmagyar approved these changes

@qinqon qinqon qinqon approved these changes

@bijoua29 bijoua29 Awaiting requested review from bijoua29

@destogl destogl Awaiting requested review from destogl

@erickisos erickisos Awaiting requested review from erickisos

@moriarty moriarty Awaiting requested review from moriarty

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

@progtologist progtologist Awaiting requested review from progtologist

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

Successfully merging this pull request may close these issues.

Odomentry not properly working with ackermann steering
4 participants
@christophfroehlich @qinqon @bmagyar @destogl