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

Thread safety fixes for footprint #3875

Merged

Conversation

LeifHookedWireless
Copy link
Contributor


Basic Info

Info Please fill out this column
Ticket(s) this addresses No ticket known
Primary OS tested on Ubuntu
Robotic platform tested on Peanut's in-house robot (sim)

Description of contribution in a few bullet points

  • Footprint calculation was not thread safe and could lead to crashes. e.g. attempting to do collision detection while footprint was updating.
  • Code inspection revealed that sensors PointCloud, Range, and Scan also appear to be unsafe.
  • Replaces Thready safety fixes #3863

Description of documentation updates required from your changes

  • Changed signature of calculateMinAndMaxDistances()

Future work that may be required in bullet points

  • None expected.

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@LeifHookedWireless LeifHookedWireless mentioned this pull request Oct 12, 2023
7 tasks
nav2_costmap_2d/src/layered_costmap.cpp Outdated Show resolved Hide resolved
nav2_collision_monitor/src/scan.cpp Outdated Show resolved Hide resolved
@LeifHookedWireless LeifHookedWireless changed the title Thread safety fixes for footprint, sensors Thread safety fixes for footprint Oct 13, 2023
@SteveMacenski
Copy link
Member

https://app.circleci.com/pipelines/github/ros-planning/navigation2/10204/workflows/72cd1995-6da4-4624-afd0-0cc42fcfb145/jobs/32392/tests

Linting problems

@SteveMacenski
Copy link
Member

@LeifHookedWireless retriggering CI, but you had a failure with obstacle tests in costmap 2D. If the new CI job also fails, that means this has a regression that you should look at.

But pending that, I approve this PR

@SteveMacenski
Copy link
Member

Yeah @LeifHookedWireless multiple tests are failing

@SteveMacenski SteveMacenski merged commit ea1902e into ros-navigation:main Nov 4, 2023
1 of 2 checks passed
jwallace42 pushed a commit to jwallace42/navigation2 that referenced this pull request Jan 3, 2024
* Don't assign bad values to radius, even temporarily.

* Thread safety:
 * Keep same smart pointer to object within a scope.

* Revert can_load_messages fixes. those will go in a different PR

* There's no easy way to pass references back in a thread-safe way.
Have calculateMinAndMaxDistances return values instead.

* Revert unnecessary changes "Thread safety:"

This reverts commit 98e2f8d.

* Protect access to footprint via mutex.

* Revert "Protect access to footprint via mutex."

This reverts commit 405cc17.

* Use atomic assignment for footprint.

* Add comment about atomic load/store.

* Initialize footprint. If accessed before assigned was returning a NULL
reference.

---------

Co-authored-by: Leif Terry <leif@peanutrobotics.com>
Signed-off-by: gg <josho.wallace@gmail.com>
Rayman added a commit to nobleo/teb_local_planner that referenced this pull request Apr 2, 2024
This PR fixes the change in API for `calculateMinAndMaxDistances` due to
the following PR: ros-navigation/navigation2#3875
enricosutera pushed a commit to enricosutera/navigation2 that referenced this pull request May 19, 2024
* Don't assign bad values to radius, even temporarily.

* Thread safety:
 * Keep same smart pointer to object within a scope.

* Revert can_load_messages fixes. those will go in a different PR

* There's no easy way to pass references back in a thread-safe way.
Have calculateMinAndMaxDistances return values instead.

* Revert unnecessary changes "Thread safety:"

This reverts commit 98e2f8d.

* Protect access to footprint via mutex.

* Revert "Protect access to footprint via mutex."

This reverts commit 405cc17.

* Use atomic assignment for footprint.

* Add comment about atomic load/store.

* Initialize footprint. If accessed before assigned was returning a NULL
reference.

---------

Co-authored-by: Leif Terry <leif@peanutrobotics.com>
Signed-off-by: enricosutera <enricosutera@outlook.com>
Marc-Morcos pushed a commit to Marc-Morcos/navigation2 that referenced this pull request Jul 4, 2024
* Don't assign bad values to radius, even temporarily.

* Thread safety:
 * Keep same smart pointer to object within a scope.

* Revert can_load_messages fixes. those will go in a different PR

* There's no easy way to pass references back in a thread-safe way.
Have calculateMinAndMaxDistances return values instead.

* Revert unnecessary changes "Thread safety:"

This reverts commit 98e2f8d.

* Protect access to footprint via mutex.

* Revert "Protect access to footprint via mutex."

This reverts commit 405cc17.

* Use atomic assignment for footprint.

* Add comment about atomic load/store.

* Initialize footprint. If accessed before assigned was returning a NULL
reference.

---------

Co-authored-by: Leif Terry <leif@peanutrobotics.com>
corot pushed a commit to rst-tu-dortmund/teb_local_planner that referenced this pull request Nov 8, 2024
This PR fixes the change in API for `calculateMinAndMaxDistances` due to
the following PR: ros-navigation/navigation2#3875
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.

2 participants