Fix correct usage of angular velocity in update_odometry() function

[MCFurry]

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

[saikishor]

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

[MCFurry]

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]

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]

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]

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]

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]

@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]

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]

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]

@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 🙂

[christophfroehlich]

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]

@Mergifyio backport humble iron

[mergify]

backport humble iron

✅ Backports have been created

Details

• #1153 Fix correct usage of angular velocity in update_odometry() function (backport #1118) has been created for branch humble
• #1154 Fix correct usage of angular velocity in update_odometry() function (backport #1118) has been created for branch iron

[christophfroehlich]

@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? ;)

[saikishor]

The current fix and the tests look good to me.

Thank you 👍🏽

[MCFurry]

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!