Fix steering controllers library code documentation and naming #1149

christophfroehlich · 2024-05-24T21:33:55Z

A follow-up to #1118, using consistent nomenclature introduced in #954

My commits do not change any logic or behavior, just make the code more readable.

saikishor

Few minor comments

ackermann_steering_controller/src/ackermann_steering_controller.cpp

bicycle_steering_controller/src/bicycle_steering_controller.cpp

steering_controllers_library/include/steering_controllers_library/steering_odometry.hpp

steering_controllers_library/src/steering_odometry.cpp

tricycle_steering_controller/src/tricycle_steering_controller.cpp

Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>

saikishor

LGTM. Thanks for addressing the comments!

christophfroehlich · 2024-05-28T19:53:43Z

@MCFurry @Timple As you have recently contributed to the steering controllers library: maybe you could have a look if you agree with these changes here?

saikishor

I've left comments regarding the new commits

steering_controllers_library/src/steering_odometry.cpp

saikishor

LGTM

Timple · 2024-05-30T06:36:52Z

tricycle_steering_controller/src/tricycle_steering_controller.cpp

    const double steer_position = state_interfaces_[STATE_STEER_AXIS].get_value();
    if (
-      !std::isnan(rear_right_wheel_value) && !std::isnan(rear_left_wheel_value) &&
-      !std::isnan(steer_position))
+      std::isfinite(traction_right_wheel_value) && std::isfinite(traction_left_wheel_value) &&


What does traction mean in this context?

We have front and rear wheel actuated tricycles.

steering_controller lib does implement tricycles with traction on the other axle as the steering wheel (different than the earlier tricycle_controller, assuming steering+traction no the same wheel), but with front or rear steering (subsequently, rear or front traction)

Here, the old variable name was rear_right/left_wheel_value independent of the params_.front_steering value. See the state_interface_configuration() method, where depending on the params_.front_steering the front or rear wheel names are exported as state interfaces, which is read here as state_interfaces_[STATE_TRACTION_RIGHT_WHEEL].get_value(). I just renamed the variable to be valid for both configurations.

Or have I misunderstood something?

steering_controller lib does implement tricycles with traction on the other axle as the steering wheel (different than the earlier tricycle_controller, assuming steering+traction on the same wheel)

Ah, this might be part of the confusion. But it only supports a subset of tricycles then. In that case this variable is in line with that.

exactly. I have plans to include the "old" tricycle controller into the steering lib, but step by step

qinqon

nit

ackermann_steering_controller/src/ackermann_steering_controller.cpp

Co-authored-by: Quique Llorente <ellorent@redhat.com>

* Update documentation and consolidate variable names * Simplify private methods and further update docs * Rename methods * Rename method and variables * Rename convert method * Rename variables and improve doc * Rename local variables * Use std::isfinite instead of !isnan Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> * Use a lowercase theta for heading Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> * Make some temporary variables const * Let update_from_position call update_from_velocity * Explicitly set variables with 0 in constructor * Fix docstring * Apply consistent variable naming Co-authored-by: Quique Llorente <ellorent@redhat.com> --------- Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> Co-authored-by: Quique Llorente <ellorent@redhat.com> (cherry picked from commit b245155)

#1164) * Update documentation and consolidate variable names * Simplify private methods and further update docs * Rename methods * Rename method and variables * Rename convert method * Rename variables and improve doc * Rename local variables * Use std::isfinite instead of !isnan Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> * Use a lowercase theta for heading Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> * Make some temporary variables const * Let update_from_position call update_from_velocity * Explicitly set variables with 0 in constructor * Fix docstring * Apply consistent variable naming Co-authored-by: Quique Llorente <ellorent@redhat.com> --------- Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> Co-authored-by: Quique Llorente <ellorent@redhat.com> (cherry picked from commit b245155) Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

#1165) * Update documentation and consolidate variable names * Simplify private methods and further update docs * Rename methods * Rename method and variables * Rename convert method * Rename variables and improve doc * Rename local variables * Use std::isfinite instead of !isnan Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> * Use a lowercase theta for heading Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> * Make some temporary variables const * Let update_from_position call update_from_velocity * Explicitly set variables with 0 in constructor * Fix docstring * Apply consistent variable naming Co-authored-by: Quique Llorente <ellorent@redhat.com> --------- Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> Co-authored-by: Quique Llorente <ellorent@redhat.com> (cherry picked from commit b245155) Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>

This was referenced May 24, 2024

Fix correct usage of angular velocity in update_odometry() function #1118

Merged

Fix steering controllers library kinematics #1150

Merged

christophfroehlich added 7 commits May 27, 2024 19:16

Update documentation and consolidate variable names

78d6004

Simplify private methods and further update docs

759264b

Rename methods

f85fc5b

Rename method and variables

8602f49

Rename convert method

bf9998b

Rename variables and improve doc

2531973

Rename local variables

77212df

christophfroehlich force-pushed the fix/steering_controllers_library-docs branch from 4b06b5f to 77212df Compare May 27, 2024 19:18

christophfroehlich marked this pull request as ready for review May 27, 2024 19:18

github-actions bot requested review from arne48, bmagyar, destogl, peterdavidfagan, progtologist, VanshGehlot and VX792 May 27, 2024 19:19

christophfroehlich requested a review from saikishor May 27, 2024 19:19

saikishor suggested changes May 27, 2024

View reviewed changes

christophfroehlich and others added 3 commits May 27, 2024 22:00

Use std::isfinite instead of !isnan

e115836

Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>

Use a lowercase theta for heading

6942c1a

Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>

Make some temporary variables const

4e5653d

christophfroehlich requested a review from saikishor May 27, 2024 21:43

saikishor approved these changes May 27, 2024

View reviewed changes

christophfroehlich added 2 commits May 28, 2024 19:47

Let update_from_position call update_from_velocity

f0b2780

Explicitly set variables with 0 in constructor

d8ce400

saikishor suggested changes May 29, 2024

View reviewed changes

Fix docstring

479d850

saikishor approved these changes May 30, 2024

View reviewed changes

Timple reviewed May 30, 2024

View reviewed changes

qinqon suggested changes Jun 4, 2024

View reviewed changes

ackermann_steering_controller/src/ackermann_steering_controller.cpp Outdated Show resolved Hide resolved

bmagyar approved these changes Jun 5, 2024

View reviewed changes

christophfroehlich added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels Jun 5, 2024

Apply consistent variable naming

089a803

Co-authored-by: Quique Llorente <ellorent@redhat.com>

christophfroehlich merged commit b245155 into master Jun 5, 2024
5 of 18 checks passed

christophfroehlich deleted the fix/steering_controllers_library-docs branch June 5, 2024 19:50

mergify bot mentioned this pull request Jun 5, 2024

Fix steering controllers library code documentation and naming (backport #1149) #1164

Merged

mergify bot mentioned this pull request Jun 5, 2024

Fix steering controllers library code documentation and naming (backport #1149) #1165

Merged

This was referenced Jul 5, 2024

Fix steering controllers library kinematics (backport #1150) #1194

Merged

Fix steering controllers library kinematics (backport #1150) #1195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix steering controllers library code documentation and naming #1149

Fix steering controllers library code documentation and naming #1149

christophfroehlich commented May 24, 2024 •

edited

Loading

saikishor left a comment

saikishor left a comment

christophfroehlich commented May 28, 2024

saikishor left a comment

saikishor left a comment

Timple May 30, 2024

christophfroehlich May 30, 2024 •

edited

Loading

Timple May 30, 2024

christophfroehlich May 30, 2024

qinqon left a comment

Fix steering controllers library code documentation and naming #1149

Fix steering controllers library code documentation and naming #1149

Conversation

christophfroehlich commented May 24, 2024 • edited Loading

saikishor left a comment

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

christophfroehlich commented May 28, 2024

saikishor left a comment

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

Timple May 30, 2024

Choose a reason for hiding this comment

christophfroehlich May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Timple May 30, 2024

Choose a reason for hiding this comment

christophfroehlich May 30, 2024

Choose a reason for hiding this comment

qinqon left a comment

Choose a reason for hiding this comment

christophfroehlich commented May 24, 2024 •

edited

Loading

christophfroehlich May 30, 2024 •

edited

Loading