fastcat icon indicating copy to clipboard operation
fastcat copied to clipboard

Bug in new brake disengage logic

Open d-loret opened this issue 2 years ago • 4 comments

Suggestion to simplify handling of brake disengage in Actuator's state machine

I might be missing something, but I also wonder if we considered just adding one additional state before any of the profile mode states. Something analogous to the HOLDING state but before you enter any of the profile states, say WAIT_BRAKE_DISENGAGE. In HandleNewProfX() you could store the command and what state you should transition after the wait, and transition to WAIT_BRAKE_DISENGAGE. In ProcessWaitBrakeDisengage you transition to the stored prof state immediately if servo_enabled is true. Otherwise, you wait for the max possible brake time (the current ~1s). And then just generate the trapezoid in ProcessProfX() if it is executing for the first time (e.g. maybe indicated by a flag that HandleNewProfX() sets).

I say this because right now you could go from, for example, PROF_POS -> PROF_POS_DISENGAGING -> PROF_POS, which is a little confusing.

d-loret avatar Dec 10 '22 03:12 d-loret

@alex-brinkman

d-loret avatar Dec 10 '22 03:12 d-loret

Mind breaking these into distinct issues so they can be closed independently

alex-brinkman avatar Dec 13 '22 20:12 alex-brinkman

Updated issue and split printing issue into separatae ticket: #75

d-loret avatar Jan 30 '23 21:01 d-loret

I believe this is solved now since the EPD actuator class refactor was merged:

 // Only transition to disengaging if it is needed (i.e. brakes are still
  // engaged)
  if (!state_->gold_actuator_state.servo_enabled) {
    TransitionToState(ACTUATOR_SMS_PROF_POS_DISENGAGING);
    last_cmd_ = cmd;
    return true;
  }
  ...

  TransitionToState(ACTUATOR_SMS_PROF_POS);

@d-loret can you confirm and close?

alex-brinkman avatar Jun 09 '23 18:06 alex-brinkman