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

Avoid creating a dedicated tf thread #97

Merged
merged 4 commits into from
Feb 7, 2023
Merged

Conversation

ivanpauno
Copy link
Collaborator

@ivanpauno ivanpauno commented Feb 3, 2023

Related to #35 (comment)

Summary

This marginaly improves cpu usage, and avoids the creation of an unnecessary thread.

It also changes the code to lookup for tf transforms without a timeout, as it's not allowed if not using a dedicated thread.
It's also not necessary in this case, as we're using a message filter that makes sure that the callback is called when the transforms are available.

Checklist

  • Read the contributing guidelines.
  • Configured pre-commit and ran colcon test locally.
  • Signed all commits for DCO.
  • Added tests (regression tests for bugs, coverage of new code for features).
  • Updated documentation (as needed).
  • Checked that CI is passing.

This marginaly improves cpu usage, and avoids the creation of an unnecessary thread.

Lookup for tf transforms without a timeout, as it's not allowed if not using a dedicated thread.
It's also not necessary in this case, as we're using a message filter that makes sure that the callback is called when the transforms are available.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the enhancement New feature or request label Feb 3, 2023
@ivanpauno ivanpauno self-assigned this Feb 3, 2023
glpuga
glpuga previously approved these changes Feb 3, 2023
Copy link
Collaborator

@glpuga glpuga left a comment

Choose a reason for hiding this comment

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

Nice catch, LGTM

Maybe the commit and the PR name should both be "Avoid creating a dedicated tf thread" ?

@glpuga
Copy link
Collaborator

glpuga commented Feb 3, 2023

nit: maybe adding this rationale shaped as a comment at the point where the listener is created would be useful:

It also changes the code to lookup for tf transforms without a timeout, as it's not allowed if not using a dedicated thread. It's also not necessary in this case, as we're using a message filter that makes sure that the callback is called when the transforms are available.

@ivanpauno ivanpauno changed the title Avoid creating a dedicated tf frame Avoid creating a dedicated tf thread Feb 6, 2023
@ivanpauno
Copy link
Collaborator Author

Maybe the commit and the PR name should both be "Avoid creating a dedicated tf thread" ?

That was the name I thought I wrote 😂

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@glpuga
Copy link
Collaborator

glpuga commented Feb 6, 2023

Also notice that the commit message still needs to be ammended.

Screenshot from 2023-02-06 17-30-01

nahueespinosa
nahueespinosa previously approved these changes Feb 6, 2023
beluga_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
Co-authored-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Copy link
Member

@nahueespinosa nahueespinosa left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivanpauno ivanpauno merged commit 9bcd5c1 into main Feb 7, 2023
@ivanpauno ivanpauno deleted the ivanpauno/avoid-tf-thread branch February 7, 2023 13:09
@ivanpauno
Copy link
Collaborator Author

Also notice that the commit message still needs to be ammended.

Fixed when squash-merging

@nahueespinosa nahueespinosa added cpp Related to C++ code ros Related to ROS labels Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp Related to C++ code enhancement New feature or request ros Related to ROS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants