Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Split SteeringOdometry into ForwardKinematics and InverseKinematics #1475

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nakul-py
Copy link
Contributor

@nakul-py nakul-py commented Jan 8, 2025

Description

This pull request refactors the SteeringOdometry class into separate ForwardKinematics and InverseKinematics classes to improve code clarity and maintainability.

Fixes #1461

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Sorry that I haven't responded earlier in the linked issue. I added some requirements there, please consider them, and also consider installing pre-commit because there are again issues with it.

@nakul-py
Copy link
Contributor Author

nakul-py commented Jan 8, 2025

I run pre-commit run --all-files all have passed.

check python ast.........................................................Passed
check for case conflicts.................................................Passed
check docstring is first.................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check xml................................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
fix utf-8 byte order marker..............................................Passed
pyupgrade................................................................Passed
pydocstyle...............................................................Passed
black....................................................................Passed
flake8...................................................................Passed
clang-format.............................................................Passed
ament_cppcheck...........................................................Passed
ament_cpplint............................................................Passed
ament_lint_cmake.........................................................Passed
ament_copyright..........................................................Passed
doc8.....................................................................Passed
rst ``code`` is two backticks............................................Passed
rst directives end with two colons.......................................Passed
rst ``inline code`` next to normal text..................................Passed
codespell................................................................Passed
Validate GitHub Workflows................................................Passed
Validate GitHub Actions..............................(no files to check)Skipped
Validate Dependabot Config (v2)..........................................Passed

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@nakul-py I think you need to run the pre-commit run --all-files command again.

The pre-commit only changes the files that are staged but not the unstaged one. I think when you ran this command. They are unstaged changed.

Right now, if you run it, it should show you the pre-commit changes after running the command.

Please also make sure it builds. I see that the changes in this PR are not compiling

steering_controllers_library.cpp:53:30: error: no matching function for call to ‘steering_kinematics::SteeringKinematics::SteeringKinematics()’

steering_kinematics.hpp:70:32: error: ‘class ForwardKinematics’ has no member named ‘calculate’
   
steering_kinematics.hpp:63:63: error: no matching function for call to ‘rcpputils::RollingMeanAccumulator<double>::RollingMeanAccumulator()’
   
steering_kinematics.hpp:63:63: error: no matching function for call to ‘rcpputils::RollingMeanAccumulator<double>::RollingMeanAccumulator()’
   
forward_kinematics.hpp:26:3: error: ‘Odometry’ does not name a type

@nakul-py
Copy link
Contributor Author

Hey @saikishor i am trying to building the project by colcon build then this error comes and i cannot solve this issue
image

@saikishor
Copy link
Member

I'm not aware of the tcb_span package. Probably, you are mixing some packages. Better to only build the ros2_control packages.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

There are several build errors now

/workspaces/ros2_rolling_ws/src/ros2_controllers/steering_controllers_library/include/steering_controllers_library/forward_kinematics.hpp:26:3: error: ‘Odometry’ does not name a type
   26 |   Odometry calculate(
      |   ^~~~~~~~

please fix them before you push.

regarding tcb_span, this is part of generate_parameter_library. Have you run rosdep to install the dependencies?

In general: What is your approach now with adding three classes: kinematics, forward-, and inverse kinematics? I'd

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be safe to delete completely.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor odometry class of steering controllers library
3 participants