feat(behavior_path_planner): change planner data update method

[purewater0901]

Signed-off-by: yutaka [email protected]

Description

Currently, planner_data_ is copied incorrectly, and it might cause some serious issues. In this PR, I fixed the problem.

Related Issues

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

• There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

[codecov]

Codecov Report

Base: 11.28% // Head: 10.43% // Decreases project coverage by -0.85% ⚠️

Coverage data is based on head (3677707) compared to base (c6d63b7).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2510      +/-   ##
==========================================
- Coverage   11.28%   10.43%   -0.86%     
==========================================
  Files        1166     1266     +100     
  Lines       81863    88552    +6689     
  Branches    20947    21597     +650     
==========================================
+ Hits         9242     9243       +1     
- Misses      62758    69381    +6623     
- Partials     9863     9928      +65     

Flag	Coverage Δ		*Carryforward flag
differential	3.15% <0.00%> (?)
total	10.54% <0.00%> (-0.75%)	⬇️	Carriedforward from 7de7d6b

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
...havior_path_planner/behavior_path_planner_node.hpp	0.00% <ø> (ø)
...or_path_planner/src/behavior_path_planner_node.cpp	0.26% <0.00%> (+0.08%)	⬆️
...erception/traffic_light_classifier/src/nodelet.cpp	0.00% <0.00%> (ø)
...ehavior_path_planner/src/behavior_tree_manager.cpp	0.00% <0.00%> (ø)
...based_prediction/src/map_based_prediction_node.cpp	0.00% <0.00%> (ø)
...nner/src/scene_module/pull_out/pull_out_module.cpp	0.00% <0.00%> (ø)
...er/src/scene_module/avoidance/avoidance_module.cpp	0.00% <0.00%> (ø)
...er/src/scene_module/pull_over/pull_over_module.cpp	0.00% <0.00%> (ø)
.../src/scene_module/side_shift/side_shift_module.cpp	0.00% <0.00%> (ø)
...ner/src/scene_module/crosswalk/scene_crosswalk.cpp	0.00% <0.00%> (ø)
... and 105 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[VRichardJP]

Although it should not be a problem in practice, there is still a concurrency issue.
For example you have the 2 snippets:

void BehaviorPathPlannerNode::updatePlannerData()
{
  // update planner data (pose, map and route)
  planner_data_->self_pose = self_pose_listener_.getCurrentPose();
  if (has_received_map_) {
    planner_data_->route_handler->setMap(*map_ptr_);
    has_received_map_ = false;
  }
  //...

void BehaviorPathPlannerNode::onMap(const HADMapBin::ConstSharedPtr msg)
{
  map_ptr_ = msg;
  has_received_map_ = true;
}

When 2 threads are working in parallel, the following situation may occur:

1. Let's say a new map has been received, so we need to update route_handler
2. thread 1 enters updatePlannerData and reach the if (has_received_map_)
3. since has_received_map_ is true, the thread 1 execute planner_data_->route_handler->setMap(*map_ptr_)
4. at the same time, a new map is received. Thread 2 executes onMap: map_ptr_ points to new data, has_received_map_ is set to true.
5. thread 1 finally finishes with the setMap() line, and execute has_received_map_ = false

-> Consequence:

• a new map has been received: map_ptr_ points to a new map
• but has_received_map is set to false, so the new data will never be used.

The pattern is exactly the same with the route_ptr_.

[purewater0901]

@VRichardJP Thank you. Then how about creating a new mutex to protect has_received_map_ from written over by other threads? The code will be

void BehaviorPathPlannerNode::updatePlannerData()
{
  // update planner data (pose, map and route)
  mutex_map_.lock();
  planner_data_->self_pose = self_pose_listener_.getCurrentPose();
  if (has_received_map_) {
    planner_data_->route_handler->setMap(*map_ptr_);
    has_received_map_ = false;
  }
mutex_map_.unlock();
  //...

[VRichardJP]

Yes, I think you cannot get rid of a mutex.
As for your code you could also use a lock_guard instead of manually locking/unlocking.

[purewater0901]

@VRichardJP Thank you so much! I'll add the mutex in my code.

[purewater0901]

@VRichardJP I just added the mutex, could you please check my modification?