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.

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

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

Conversation

MCFurry
Copy link
Contributor

@MCFurry MCFurry commented May 6, 2024

Thanks for this nice steering_controllers_library!

Using it however, we noticed issues in the odometry calculation where our steered robot would spin incredibly fast.
There seems a small math error in the calculation which this PR fixes:
The integration expects distances and angles, while the accumulators expect a velocity.

Probably also relates to #937

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.

I haven't reviewed the original code yet, but isn't the angle here referring to the angle itself instead of angular velocity?

@MCFurry
Copy link
Contributor Author

MCFurry commented May 6, 2024

Well in all the functions that call update_odometry(linear_velocity, angular, dt), the angular input is calculated as:
tan(steer_pos_) * linear_velocity / wheelbase_ which has a velocity as outcome.
So it seems that angular is indeed a velocity here.

@saikishor
Copy link
Member

Well in all the functions that call update_odometry(linear_velocity, angular, dt), the angular input is calculated as:
tan(steer_pos_) * linear_velocity / wheelbase_ which has a velocity as outcome.
So it seems that angular is indeed a velocity here.

Fine! Thanks for the explanation. Do you mind adding tests for your changes?

@MCFurry
Copy link
Contributor Author

MCFurry commented May 6, 2024

Well in all the functions that call update_odometry(linear_velocity, angular, dt), the angular input is calculated as:
tan(steer_pos_) * linear_velocity / wheelbase_ which has a velocity as outcome.
So it seems that angular is indeed a velocity here.

Fine! Thanks for the explanation. Do you mind adding tests for your changes?

Sure thing! I added some very simple tests, please let me know if these are okay or if we want to test some more exotic situations?

@christophfroehlich
Copy link
Contributor

Please have a look at #954 and review what I've written there. I realized some wrong code too, but wanted to have the theory/nomenclature approved before I start fixing/refactoring.

@MCFurry
Copy link
Contributor Author

MCFurry commented May 7, 2024

Please have a look at #954 and review what I've written there. I realized some wrong code too, but wanted to have the theory/nomenclature approved before I start fixing/refactoring.

Very nice! I'll review that soon!

@MCFurry
Copy link
Contributor Author

MCFurry commented May 14, 2024

@christophfroehlich I just reviewed your documentation effort and that looks really nice! Great work!
Do you think merging this PR in the meantime would be a small step forwards already? It might make the Jazzy release still?

@christophfroehlich
Copy link
Contributor

I'd like to update the function documentation/docstring to make this point clear. But you deactivated the tick Maintainers are allowed to edit this pull request. I'll open a new PR on top of that one

@MCFurry
Copy link
Contributor Author

MCFurry commented May 15, 2024

I'd like to update the function documentation/docstring to make this point clear. But you deactivated the tick Maintainers are allowed to edit this pull request. I'll open a new PR on top of that one

Ahh I wasn't aware of that checkbox, it seems I also can't change that anymore?

@Timple
Copy link
Contributor

Timple commented May 24, 2024

@christophfroehlich Can you list the changes you wanted to make? Or create a pull request targetting this PR?
That way this PR can keep moving forward 🙂

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.

Thanks for the changes, the algorithm seems to be fine now. I made a follow-up PR #1149 to get a more consistent nomenclature.

@christophfroehlich
Copy link
Contributor

@Mergifyio backport humble iron

Copy link
Contributor

mergify bot commented May 24, 2024

backport humble iron

✅ Backports have been created

Co-authored-by: Christoph Fröhlich <[email protected]>
@christophfroehlich
Copy link
Contributor

@Timple @MCFurry please have a look at the failing pre-commit jobs (a missing newline at the end of a file). The other failing jobs are not related. fyi, we need another review of someone with write access (@bmagyar or @destogl). In the meantime, could you maybe leave a review on #954? ;)

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.

The current fix and the tests look good to me.

Thank you 👍🏽

@MCFurry
Copy link
Contributor Author

MCFurry commented May 27, 2024

I'd like to update the function documentation/docstring to make this point clear. But you deactivated the tick Maintainers are allowed to edit this pull request. I'll open a new PR on top of that one

Ahh I wasn't aware of that checkbox, it seems I also can't change that anymore?

@Timple @MCFurry please have a look at the failing pre-commit jobs (a missing newline at the end of a file). The other failing jobs are not related. fyi, we need another review of someone with write access (@bmagyar or @destogl). In the meantime, could you maybe leave a review on #954? ;)

Done!

@bmagyar bmagyar merged commit d2e926b into ros-controls:master May 27, 2024
5 of 18 checks passed
mergify bot pushed a commit that referenced this pull request May 27, 2024
mergify bot pushed a commit that referenced this pull request May 27, 2024
christophfroehlich pushed a commit that referenced this pull request May 27, 2024
christophfroehlich pushed a commit that referenced this pull request May 27, 2024
henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
@Rayman Rayman deleted the fix/steering_controllers_library-odometry-calculation branch September 10, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants