rationalize boost::optional dereference in behavior_velocity_planner

[VRichardJP]

Checklist

• I've read the contribution guidelines.
• I've searched other issues and no duplicate issues were found.
• I've agreed with the maintainers that I can plan this task.

Description

boost::optional is used quite often across the code base. In most of the code, it is dereferenced carefully, like so:

if (optional_value) {
  const auto val = optional_value.get();
}

// or
if (!optional_value) 
  return err_value;
const auto val = optional_value.get();

However in the behavior_velocity_planner, boost::optional<double> current_accel is dereferenced at many places without any prior check. Or sometimes even with a check after the dereference:

autoware.universe/planning/behavior_velocity_planner/src/scene_module/no_stopping_area/scene_no_stopping_area.cpp

Lines 374 to 383 in 6247671

	const auto current_acceleration = planner_data_->current_accel.get();
	const double max_acc = planner_data_->max_stop_acceleration_threshold;
	const double max_jerk = planner_data_->max_stop_jerk_threshold;
	const double delay_response_time = planner_data_->delay_response_time;
	if (!planner_data_->current_accel) {
	RCLCPP_WARN_THROTTLE(
	logger_, *clock_, 1000,
	"[no stopping area] empty current acc! check current vel has been received.");
	return false;
	}

In the code above, the if clause is unreachable since an exception will be thrown at planner_data_->current_accel.get() if planner_data_->current_accel is not set. current_accel is only set after 2 messages have been received from ~/input/vehicle_odometry

Here is a list of all unchecked dereference in behavior_velocity_planner:

src/universe/autoware.universe/planning/behavior_velocity_planner/include/utilization/trajectory_utils.hpp
136:  const double a0 = planner_data->current_accel.get(); // smoothPath()

src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/intersection/util.cpp
172:  const double current_acc = planner_data->current_accel.get(); // generateStopLine()

src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/run_out/scene.cpp
55:  const auto current_acc = planner_data_->current_accel.get(); // modifyPathVelocity()
144:  const float initial_acc = planner_data_->current_accel.get(); // visualizeDetectionArea()

src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/no_stopping_area/scene_no_stopping_area.cpp
374:  const auto current_acceleration = planner_data_->current_accel.get(); // isStoppable()

src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/occlusion_spot/scene_occlusion_spot.cpp
94:    param_.v.a_ego = planner_data_->current_accel.get(); // modifyPathVelocity()

Purpose

Since planner_data_->current_accel may be unset, it would make sense to consistently check its state before dereferencing it (otherwise there is no point using boost::optional)

Possible approaches

add early return in related functions when planner_data_->current_accel is not set

Definition of done

no more unchecked dereference in:

• src/universe/autoware.universe/planning/behavior_velocity_planner/include/utilization/trajectory_utils.hpp
• src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/intersection/util.cpp
• src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/run_out/scene.cpp
• src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/no_stopping_area/scene_no_stopping_area.cpp
• src/universe/autoware.universe/planning/behavior_velocity_planner/src/scene_module/occlusion_spot/scene_occlusion_spot.cpp

[taikitanaka3]

@VRichardJP
thank you for isssue
I think these values are not used until current accel is not set commonly

autoware.universe/planning/behavior_velocity_planner/src/node.cpp

Line 219 in 32c2e41

if (!d.current_accel) {

, and will be repalced by /localization/accleration
#1213

[VRichardJP]

I get it will be solved once #1213 will be merged then.

[taikitanaka3]

@VRichardJP
Do you have any problem with current implementation?
I think as long as isDataReady() returns false
scene modules above you mentioned will not run

[VRichardJP]

@taikitanaka3 I did not have any runtime issue with these. But since there was a recent case of crash from unchecked boost::optional dereference (#1245) I looked for similar pattern ;)

[stale]

This pull request has been automatically marked as stale because it has not had recent activity.

[takayuki5168]

I agree with @VRichardJP 's idea.

When creating planner_data, the optional should be removed. If the optional cannot be removed since it is null, isDataready should return false.