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.

Sign up for GitHub

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

fix(mrm_handler): fix bugprone-branch-clone #9729

Closed
wants to merge 5 commits into from
Closed

fix(mrm_handler): fix bugprone-branch-clone #9729

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
74fc864
fix: bugprone-error
kobayu858 Dec 23, 2024
cc46392
refactor: if else
kobayu858 Dec 25, 2024
f06639e
refactor: if
kobayu858 Dec 26, 2024
5f417a9
fix:bugprone-error
kobayu858 Dec 26, 2024
7b59903
revert
kobayu858 Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,11 @@
HazardLightsCommand msg;

msg.stamp = this->now();
if (is_emergency_holding_) {
// turn hazard on during emergency holding
msg.command = HazardLightsCommand::ENABLE;
} else if (isEmergency() && param_.turning_hazard_on.emergency) {
// turn hazard on if vehicle is in emergency state and
// turning hazard on if emergency flag is true
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

@mkuri @TetsuKawa
The original implementation looks wierd: Is the following correct?

if (param_.turning_hazard_on.emergency) {
  if (is_emergency_holding_ || isEmergency()) {
    msg.command = HazardLightsCommand::ENABLE;
  } else {
    msg.command = HazardLightsCommand::NO_COMMAND;
  }
} else {
  msg.command = HazardLightsCommand::NO_COMMAND;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@mkuri
I think that kambe san’ s code is better.
Is there a problem？

Copy link
Contributor

@veqcc veqcc Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In additiion to that, isEmergerncy() is implemented as follows:
https://github.com/autowarefoundation/autoware.universe/blob/d86c46a35882e75051a0abcba55853da78c1d21f/system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp#L540C1-L544C2

bool MrmHandler::isEmergency() const
{
  return !operation_mode_availability_->autonomous || is_emergency_holding_ ||
         is_operation_mode_availability_timeout;
}

So, if (is_emergency_holding_ || isEmergency()) is still redundant and just if (isEmergency()) seems enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

@veqcc
I agree. I think the following is fine too.

if (param_.turning_hazard_on.emergency) {
  if (isEmergency()) {
    msg.command = HazardLightsCommand::ENABLE;
  } else {
    msg.command = HazardLightsCommand::NO_COMMAND;
  }
} else {
  msg.command = HazardLightsCommand::NO_COMMAND;
}

is_emergency_holding_ || // turn hazard on during emergency holding
(isEmergency() && // turn hazard on if emergency flag is true

Check warning on line 120 in system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ New issue: Complex Conditional 

MrmHandler::publishHazardCmd has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
param_.turning_hazard_on.emergency) // turn hazard on if vehicle is in emergency state

Check warning on line 121 in system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp

View check run for this annotation

Codecov / codecov/patch

system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp#L119-L121 

Added lines #L119 - L121 were not covered by tests
) {
msg.command = HazardLightsCommand::ENABLE;
} else {
msg.command = HazardLightsCommand::NO_COMMAND;
Expand Down Expand Up @@ -185,12 +184,12 @@
if (current_mrm_behavior == mrm_state_.behavior) {
return;
}
if (!requestMrmBehavior(mrm_state_.behavior, RequestType::CANCEL)) {
if (
veqcc marked this conversation as resolved.
Show resolved Hide resolved
!requestMrmBehavior(mrm_state_.behavior, RequestType::CANCEL) ||
!requestMrmBehavior(current_mrm_behavior, RequestType::CALL)) {

Check warning on line 189 in system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp

View check run for this annotation

Codecov / codecov/patch

system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp#L188-L189 

Added lines #L188 - L189 were not covered by tests
handleFailedRequest();
} else if (requestMrmBehavior(current_mrm_behavior, RequestType::CALL)) {
mrm_state_.behavior = current_mrm_behavior;
} else {
handleFailedRequest();
mrm_state_.behavior = current_mrm_behavior;

Check warning on line 192 in system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp

View check run for this annotation

Codecov / codecov/patch

system/mrm_handler/src/mrm_handler/mrm_handler_core.cpp#L192 

Added line #L192 was not covered by tests
}
return;
}
Expand Down
Loading